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

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

On Thu, Jul 31, 2014 at 9:49 AM, Duncan P. N. Exon Smith <
dexonsmith at apple.com> wrote:
>
> 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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140731/c2e93fb0/attachment.html>

> On 2014-Jul-31, at 10:07, Reid Kleckner <rnk at google.com> wrote:
> 
>> On Thu, Jul 31, 2014 at 9:49 AM, Duncan P. N. Exon Smith <dexonsmith
at apple.com> wrote:
>> 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.