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

[Duncan P. N. Exon Smith]

+reid, +llvmdev, -llvm-commits

> On 2014-Jul-30, at 11:56, Rafael Avila de Espindola <rafael.espindola at gmail.com> wrote:
> 
> 
>>   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?