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

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

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.

Sent from my iPhone
> On Jul 31, 2014, at 13:45, "Duncan P. N. Exon Smith"
<dexonsmith at apple.com> wrote:
> 
> 
>>> 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.
>

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