[cfe-dev] weak_odr constant versus weak_odr global

[Richard Smith]

On Wed, November 2, 2011 00:19, Richard Smith wrote:
> On Tue, November 1, 2011 23:19, Richard Smith wrote:
>> We recently narrowed down a clang regression to the following testcase:
>>
>>
>> struct S { static const int x; }; template<typename T> struct U { static
>> const int k; }; template<typename T> const int U<T>::k = T::x;
>>
>> #ifdef TU1
>> extern int f(); const int S::x = 42; int main() { return f() + U<S>::k; }
>> #endif
>>
>>
>> #ifdef TU2
>> int f() { return U<S>::k; } #endif
>>
>>
>> /* Steps to repro:
>>
>>
>> clang repro.cpp -DTU1 -c -o tu1.o clang repro.cpp -DTU2 -c -o tu2.o clang
>> tu1.o tu2.o ./a.out
>>
>>
>> */
>>
>>
>> This segfaults, because... in TU1, we get:
>>
>>
>> @_ZN1UI1SE1kE = weak_odr constant i32 42, align 4
>>
>>
>> and in TU2, we get:
>>
>> @_ZN1UI1SE1kE = weak_odr global i32 0, align 4
>>
>>
>> plus a global constructor which writes to @_ZN1UI1SE1kE. The linker then
>> selects the symbol from TU1, which ends up in a read-only section,
>> resulting in the binary crashing when TU2 tries to write to it.
>>
>> Is this a clang bug (should we be generating a weak_odr global, and losing
>> optimization opportunities in TU1), or is this an llvm bug (should weak_odr
>> constants be banned from read-only sections, since another module might
>> write to them)?
>
> I think this is best fixed in llvm, since that gives maximum optimization
> opportunities: it's fine to propagate weak_odr constant values, even if some
> other module has the same object as a non-constant. Plus, with LTO, we'd want
> to merge the weak_odr constant and weak_odr global symbols into a weak_odr
> constant (and simultaneously remove any writes into the global).

The attached patch fixes this issue by treating weak constants as non-constant
when choosing their section kind. This also fixes the LTO case, although we
generate a merged module containing a store to a weak_odr constant, and would
have to be careful not to optimize that to unreachable in the future (though
simply erasing such stores would be fine if the variable is marked odr).

Is this the right solution (and the right implementation of it)?

Thanks,
Richard
-------------- next part --------------
A non-text attachment was scrubbed...
Name: weak-const.diff
Type: text/x-patch
Size: 1956 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20111102/449e3e35/attachment.bin>