Inconsistent third field in global_ctors (was Re: [llvm] r214321 - UseListOrder: Visit global values)

+reid, +llvmdev, -llvm-commits

  I think the fix is to upgrade old-style global arrays (or reject
  them?) in the .ll parser.

The .ll format is not stable, so you should be able to just reject it.

Looking a little deeper, it's not "old-style" exactly; rejecting this
isn't trivial.

  - According to LangRef, the third field is optional [1].

[1]: http://llvm.org/docs/LangRef.html#the-llvm-global-ctors-global-variable

  - It was added in r209015 to support comdat keys.

  - The helper functions `llvm::appendToGlobalCtors()` and
    `llvm::appendToGlobalDtors()` (in ModuleUtils.cpp) will by default
    set up the two-field version.

  - Despite the field being optional (and the API defaulting to the
    short version), the BitcodeReader auto-upgrades to the 3-element
    version.

It looks to me like `@llvm.global_ctors` and `@llvm.global_dtors` are in
an inconsistent state here. In particular, with the same version of the
tool, if you generate the arrays you get one type, then if you write to
bitcode and read it back you get another type.

There are a few ways to move forward:

1. Remove the auto-upgrade from the BitcodeReader, making the third
    field truly optional. Why create them one way, and round-trip to
    bitcode another way?

2. Make the third field *required*. This primarily means:

    - changing `appendToGlobal?tors()` (etc.?) to add the third field
      immediately instead of waiting for a bitcode rount-trip and
    - rejecting the two-field version in .ll.

    This is another way of removing the inconsistency.

3. Straw man: upgrade to the three-field version in BitcodeWriter when
    preserving use-list order (or predict how the use-lists of `i8*
    null` will be affected by the upgrade).
    
    However, this actually modifies the use-lists of `i8* null`, far
    from preserving them.

I have strong preference for (1) or (2) -- I'd rather make it consistent
than work around it. I don't know why this field was made optional,
though, so I'm not sure which way to go.

Any guidance?

Looking a little deeper, it's not "old-style" exactly; rejecting this
isn't trivial.

  - According to LangRef, the third field is optional [1].

[1]: http://llvm.org/docs/LangRef.html#the-llvm-global-ctors-global-variable

  - It was added in r209015 to support comdat keys.

  - The helper functions `llvm::appendToGlobalCtors()` and
    `llvm::appendToGlobalDtors()` (in ModuleUtils.cpp) will by default
    set up the two-field version.

  - Despite the field being optional (and the API defaulting to the
    short version), the BitcodeReader auto-upgrades to the 3-element
    version.

It looks to me like `@llvm.global_ctors` and `@llvm.global_dtors` are in
an inconsistent state here. In particular, with the same version of the
tool, if you generate the arrays you get one type, then if you write to
bitcode and read it back you get another type.

There are a few ways to move forward:

1. Remove the auto-upgrade from the BitcodeReader, making the third
    field truly optional. Why create them one way, and round-trip to
    bitcode another way?

2. Make the third field *required*. This primarily means:

    - changing `appendToGlobal?tors()` (etc.?) to add the third field
      immediately instead of waiting for a bitcode rount-trip and
    - rejecting the two-field version in .ll.

    This is another way of removing the inconsistency.

I have a small preference for 2, but just because it makes the current
IR definition simpler. Reid implemented the auto upgrade, so he
probably has a better insight on which option is best.

Cheers,
Rafael

Nick was in favor of 2 as well, which is what pushed me to do the
auto-upgrade, even though I initially intended for this to be optional. I
didn't want to force the complexity of the 3 operand form on non-C++
frontends, basically. Obviously, having one form is simpler from LLVM's
perspective.

I was selfishly hoping (1) was better (deleting code is easier), but I
guess (2) is cleaner so if no one objects I'll move forward with that.

I should be able to throw a patch together tomorrow.

Hold on, I'm still not sure what kind of guarantees (restrictions) the C
API imposes.

If I implement (2), then -verify will fail if someone (e.g.) adds
`@llvm.global_ctors` with 2 fields. Does C API stability preclude
making that change?

Assuming it does, are there serious concerns with me implementing (1)
instead?

Hold on, I'm still not sure what kind of guarantees (restrictions) the C
API imposes.

If I implement (2), then -verify will fail if someone (e.g.) adds
`@llvm.global_ctors` with 2 fields. Does C API stability preclude
making that change?

Now you have reached the crux of it. Nobody really agrees. The verifier
doesn't reject the 2-field form yet, because I thought that would be a C
API break.

Assuming it does, are there serious concerns with me implementing (1)
instead?

That was my original intention: this would be an optional field
indefinitely, but it means globalopt, asan, and other consumers of
global_ctors have to dispatch on the size of the struct forever.

Well, indefinitely, not forever. And that's no different from the
current status, since it's not like they can rely on having been
serialized to/from bitcode; in particular, I don't think the
auto-upgrade is actually buying us anything.

I guess the "right" fix might be to make these global arrays first-class
IR objects, but even if we had the right design, it's not clear how to
upgrade to *that*.

The main complication with (1) is getting ModuleLinker to do the right
thing -- I think I can just move the canonicalization code there, and
only trigger it if the constructor styles don't match between the two
modules. AFAICT, it's doing the wrong thing right now if the modules
aren't read from bitcode, so this is probably necessary anyway.

What we guarantee for the C API is really fuzzy. Given just the "don't break it" rule, adding any assert would be invalid.

If you are really motivated, start a thread on llvm dev about it. IMHO, we need something on the lines of "don't break an existing user". What this would mean is that we *can* change it, but we have to have good reasons to think that no user will break because of it or we coordinate a transition, like what is going on with removal of the old jit (which *is* breaking the C API)

In this particular case, what is the expected way of creating it with the C API? Manually construct the global? If so, it does look like we have to make the third field optional. If wanted, we could also create an c API specific for the constructors, tell people to use it instead and eventually remove support for the old format.

Yup, that's the problem. That upgrade path makes sense to me, so I
filed PR20506. I'll send a patch for option #1 (making the third field
truly option) in a moment.