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

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>

> On 2014-Jul-30, at 17:19, Reid Kleckner <rnk at google.com> wrote:
> 
>> 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.
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.

> On 2014-Jul-30, at 18:05, Duncan P. N. Exon Smith <dexonsmith at
apple.com> wrote:
> 
>> On 2014-Jul-30, at 17:19, Reid Kleckner <rnk at google.com>
wrote:
>> 
>>> 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.
> 
> 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?