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

+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?

> 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

On Wed, Jul 30, 2014 at 3:46 PM, Rafael Espíndola <
rafael.espindola at gmail.com> wrote:
> > 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.
>
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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140730/12f2f73d/attachment.html>