llvm dev - Sep 2014 - [LLVMdev] [cfe-dev] weak_odr constant versus weak_odr global

> I see. Using two comdats would still cause the same problem for us,
> no? So the solution in the end is to emit:
>
> TU1:
> --------------------------------
> @_ZN1UI1SE1kE = weak_odr constant i32 42, align 4, comdat _ZN1UI1SE1kE
> @_ZGVN1UI1SE1kE = weak_odr global i64 1, comdat _ZN1UI1SE1kE
> --------------------------------
>
> TU2:
> -----------------------------------
> @_ZN1UI1SE1kE = weak_odr global i32 0, align 4, comdat _ZN1UI1SE1kE
> @_ZGVN1UI1SE1kE = weak_odr global i64 0, comdat _ZN1UI1SE1kE
> ...
> @llvm.global_ctors = ....
> define internal void @_GLOBAL__I_a() nounwind section
".text.startup" ....
> -----------------------------------
Restarting a really old thread now that we have comdat support in the IR.

While the above idea would work, there are two problems with it

* Existing compilers (clang and gcc) produce a comdat with just the
constant in TU1. Linking one of those with TU2 can still cause a crash
since the guard variable would be undefined.
* It requires always outputting the guard variable.

Since neither gcc nor clang implement this part of the ABI, I was
thinking if there was a better way to do it. One interesting option is
putting the .init_array of TU2 in the comdat. That is exactly what we
do for windows. In fact, just passing -Xclang -mllvm -Xclang
-enable-structor-comdat will avoids the crash in the above example.

Given that this has been broken since forever, waiting a bit more for
https://sourceware.org/bugzilla/show_bug.cgi?id=17350 to be fixed and
then flipping -enable-structor-comdat might be the best way to fix
this. With that done we can add the function and the guard variable in
TU2 to the comdat to remove a bit of bloat (see attached patch).

What is the discussion list for the itanium abi? Should I propose a patch?

Cheers,
Rafael
-------------- next part --------------
A non-text attachment was scrubbed...
Name: t.patch
Type: text/x-patch
Size: 1372 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140904/f457599a/attachment.bin>

On Thu, Sep 4, 2014 at 7:48 PM, Rafael Espíndola <rafael.espindola at
gmail.com
> wrote:
> > I see. Using two comdats would still cause the same problem for us,
> > no? So the solution in the end is to emit:
> >
> > TU1:
> > --------------------------------
> > @_ZN1UI1SE1kE = weak_odr constant i32 42, align 4, comdat _ZN1UI1SE1kE
> > @_ZGVN1UI1SE1kE = weak_odr global i64 1, comdat _ZN1UI1SE1kE
> > --------------------------------
> >
> > TU2:
> > -----------------------------------
> > @_ZN1UI1SE1kE = weak_odr global i32 0, align 4, comdat _ZN1UI1SE1kE
> > @_ZGVN1UI1SE1kE = weak_odr global i64 0, comdat _ZN1UI1SE1kE
> > ...
> > @llvm.global_ctors = ....
> > define internal void @_GLOBAL__I_a() nounwind section
".text.startup"
> ....
> > -----------------------------------
>
> Restarting a really old thread now that we have comdat support in the IR.
>
> While the above idea would work, there are two problems with it
>
> * Existing compilers (clang and gcc) produce a comdat with just the
> constant in TU1. Linking one of those with TU2 can still cause a crash
> since the guard variable would be undefined.
> * It requires always outputting the guard variable.
>
> Since neither gcc nor clang implement this part of the ABI, I was
> thinking if there was a better way to do it. One interesting option is
> putting the .init_array of TU2 in the comdat. That is exactly what we
> do for windows. In fact, just passing -Xclang -mllvm -Xclang
> -enable-structor-comdat will avoids the crash in the above example.
>
> Given that this has been broken since forever, waiting a bit more for
> https://sourceware.org/bugzilla/show_bug.cgi?id=17350 to be fixed and
> then flipping -enable-structor-comdat might be the best way to fix
> this. With that done we can add the function and the guard variable in
> TU2 to the comdat to remove a bit of bloat (see attached patch).
>
Even if this doesn't address the constant vs global crash issues, I think
adding the .init_array entry to the comdat is a great startup time
optimization. Everything continues to work as it does today, but less code
is run during dynamic initialization because all the new TU initializers
get deduplicated by the linker.

It sounds like you are proposing to emit this sort of IR, after the linker
bug is fixed:

// some header:
int f();
template <typename T> struct A { static int var; };
template <typename T> int A<T>::var = f();
template struct A<int>;

New TU1:
-------------
$var = comdat
@var = weak_odr global i32 0, comdat $var
@guard = weak_odr global i32 0, comdat $var
define weak_odr void @initializer() comdat $var {
  ; check @guard
  store i32 42, i32* @var
}
@llvm.global_ctors = [ { i32, void ()*, i8* } x 1 ] [ { i32 65535, void ()*
@initializer, i8* bitcast (i32* @var to i8*) } ]

Old TU2:
--------------
@var = weak_odr global i32 0
@guard = weak_odr global i32 0
define weak_odr void @initializer() {
  ; check @guard
  store i32 42, i32* @var
}
@llvm.global_ctors = [ { i32, void ()*, i8* } x 1 ] [ { i32 65535, void ()*
@initializer, i8* null } ]

It sounds like we still can't make @var into a constant if we want ABI
compatibility with old object files, though.

However, if we wait until we don't care about old object file
compatibility, we can transition to linking new TU1 with this new TU2:
-------
$var = comdat
@var = weak_odr constant i32 42, comdat $var
; no guard, no initializer, no global_ctors entry

Is that what you're thinking?

What is the discussion list for the itanium abi? Should I propose a
patch?
>
cxx-abi-dev at codesourcery.com
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140905/8a01b9e3/attachment.html>

>> Given that this has been broken since forever, waiting a bit more for
>> https://sourceware.org/bugzilla/show_bug.cgi?id=17350 to be fixed and
>> then flipping -enable-structor-comdat might be the best way to fix
>> this. With that done we can add the function and the guard variable in
>> TU2 to the comdat to remove a bit of bloat (see attached patch).
>
>
> Even if this doesn't address the constant vs global crash issues, I
think
> adding the .init_array entry to the comdat is a great startup time
> optimization. Everything continues to work as it does today, but less code
> is run during dynamic initialization because all the new TU initializers
get
> deduplicated by the linker.
Excellent news: the output from bfd ld was valid. I got confused
because X86_64 uses a Rela relocation and the value in the relocated
location is not used. I reverted the patch disabling the fix and clang
now produces a testcase that doesn't crash  :-)

> However, if we wait until we don't care about old object file
compatibility,
> we can transition to linking new TU1 with this new TU2:
> -------
> $var = comdat
> @var = weak_odr constant i32 42, comdat $var
> ; no guard, no initializer, no global_ctors entry
>
> Is that what you're thinking?
Yes, in fact that what we get now. So, what do you think of the patch
attached to the previous email? With it we also put the guard variable
and __cxx_global_var_init in the _ZN1UI1SE1kE comdat, saving a bit of
space in the final binary.
>> What is the discussion list for the itanium abi? Should I propose a
patch?
>
>
> cxx-abi-dev at codesourcery.com
Cool, I will try to subscribe to it.

Cheers,
Rafael