llvm dev - Nov 2011 - [LLVMdev] weak_odr constant versus weak_odr global

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

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 4
here 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