[llvm] r209015 - Add comdat key field to llvm.global_ctors and llvm.global_dtors

[Rafael Espíndola]

>> So having
>>
>>   %ini { i32 65535, void ()* @b_global_ctor, i8* bitcast (i32* @b to i8*)
>> }
>>
>> in @llvm.global_ctors is effectively a way of declaring that
>> b_global_cto should have the same comdat as @b?
>
>
> I didn't implement the CodeGen part of this for ELF, but it should be pretty
> trivial.  It doesn't change the comdat of the initializer function.  It
> changes the comdat of the *pointer to the initializer in .init_array or
> .CRT$XCU* to be the same as @b.
>

Oh, ok, I misunderstood the COFF assembly in the example.

How would this work in ELF? In ELF all the pointer are in a single
section, .init_array, so they cannot be in multiple comdats.

> It's a long term solution because we want to keep the representation of
> initializers high-level and platform neutral so that GlobalOpt can hack on
> it.  At least, that's the direction that Nick pushed for.
>
> Once we have explicit sections with comdats, as an alternative, we could
> simply emit function pointer globals with an explicit section and comdat.
> The downside is that GlobalOpt would have to be taught how to understand
> .init_array, .ctors, .CRT$XCU, and whatever MachO uses.

Yes, my comment was just because I thought it was changing the section
of the function itself.

> While the code as is doesn't put the initializer function into a comdat, why
> do you think we can't do this?  It seems like it'd be a nice code size
> optimization.  Right now the initializer function has internal linkage
> because there is no mangling for it in Itanium, and we end up with lots of
> unmerged duplicate code.
>
> My plan for the future is to put both the initializer function and the guard
> variable into the comdat group of the data being initialized using section
> IR.  Then either GlobalOpt or Clang can replace the global with a constant,
> so long as the initializer of the guard variable is updated.

If the function currently is not in a comdat, it should be safe.

Now that I think of it, what will not work is putting the two
variables in a comdat with the same name as the variable (as suggested
in the ABI). It would break linking with old objects. For example, a
new .o file would look like

----------------------------------------------------------------
.section .var, "G", @nobits, comdat_sym,comdat
var:
        .long 42
.section .guard, "G", @nobits, comdat_sym,comdat
guard:
        .long 42

.data
        .quad guard
----------------------------------------------------------------

and an old one looks like

----------------------------------------------------------------
.section .var, "G", @nobits, comdat_sym,comdat
var:
        .long 42

.section .guard, "G", @nobits, comdat_sym2,comdat
guard:
        .long 42
------------------------------------------------------------------

Depending on on the link order we get an warning and invalid output
from the linker:

warning: relocation refers to discarded section

What is needed is a new comdat symbol name for both variable and
guard, something like

----------------------------------------------------------------------------
.section .var, "G", @nobits, new_comdat_sym,comdat
var:
        .long 42
.section .guard, "G", @nobits, new_comdat_sym,comdat
guard:
        .long 42
.data
        .quad guard
-----------------------------------------------------------------------

This is equivalent to using a *C5* comdat symbol to hold the *C1* and
*C2* constructors.

Should we report  a bug on the itanium abi?

Cheers,
Rafael