Hi, 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)? Thanks, Richard
Hi [Adding cfe-dev to widen the net] 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). Thanks, Richard
Eli Friedman
2011-Nov-02 00:58 UTC
[LLVMdev] [cfe-dev] weak_odr constant versus weak_odr global
On Tue, Nov 1, 2011 at 5:19 PM, Richard Smith <richard at metafoo.co.uk> wrote:> Hi > > [Adding cfe-dev to widen the net] > > 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).Another solution you haven't mentioned is that we could make clang generate a guard variable initialized to 1 for all relevant static data members of class templates. The solutions you mention will require that we can't put static data members of templates into a read-only section unless we can prove every translation unit will statically initialize the definition. -Eli
Hi Richard,> 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 4here one version has an initial value of 42, and the other an initial value of 0. Doesn't this break the One Definition Rule (the odr in weak_odr)? I realize that the constantness is the real issue here, but I thought I should mention this.> 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)?Dragonegg has the following comment in the analogous spot: // Allow loads from constants to be folded even if the constant has weak // linkage. Do this by giving the constant weak_odr linkage rather than // weak linkage. It is not clear whether this optimization is valid (see // gcc bug 36685), but mainline gcc chooses to do it, and fold may already // have done it, so we might as well join in with gusto. Ciao, Duncan.
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/llvm-dev/attachments/20111102/449e3e35/attachment.bin>
I tried a small variation: 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() + reinterpret_cast<long>(&U<S>::k); } #endif #ifdef TU2 int f() { return U<S>::k; } #endif and it crashes with gcc too :-( The original testcase works because gcc folds the value into main even at -O0. If we are going to support it for real, I think Richard patch is going on the right direction, but we should completely drop the idea of a "weak_odr constant". If we cannot use it for instructing the linker to put it an o ro section, the "constant" gives us nothing that the "odr" doesn't. We could always convert a "weak_odr global" into a plain constant in LTO if the linker tells us we have all the parts. Cheers, Rafael
2011/11/7 Rafael Espíndola <rafael.espindola at gmail.com>:> I tried a small variation: > > 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() + reinterpret_cast<long>(&U<S>::k); > } > #endif > #ifdef TU2 > int f() { return U<S>::k; } > #endif > > and it crashes with gcc too :-( The original testcase works because > gcc folds the value into main even at -O0. > > If we are going to support it for real, I think Richard patch is going > on the right direction, but we should completely drop the idea of a > "weak_odr constant". If we cannot use it for instructing the linker to > put it an o ro section, the "constant" gives us nothing that the "odr" > doesn't.In cases where the C++ standard requires static initialization, introducing a write violates the guarantees of the C++ standard for static initialization. Therefore, I'm not sure the whole "make the constant writable" approach is actually viable. -Eli
Apparently Analagous Threads
- [LLVMdev] weak_odr constant versus weak_odr global
- [LLVMdev] weak_odr constant versus weak_odr global
- [LLVMdev] [cfe-dev] weak_odr constant versus weak_odr global
- [LLVMdev] weak_odr constant versus weak_odr global
- [LLVMdev] [cfe-dev] weak_odr constant versus weak_odr global