Re: [v3] Fix Werror breakage (maintainer-mode)

["Richard Guenther" <richard dot guenther at gmail dot com>]

On Sun, Jul 6, 2008 at 9:34 PM, Gabriel Dos Reis
<gdr@integrable-solutions.net> wrote:
> On Sun, Jul 6, 2008 at 7:51 AM, Richard Guenther
> <richard.guenther@gmail.com> wrote:
>> On Sun, Jul 6, 2008 at 12:08 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
>>>> Can somebody enlighten me as to a proper locale_init.cc fix?
>>>
>>> Yes, the warning is new and I'd like to understand whether the warning is worrisome or not, or even bogus. That's not clear to me, at the moment. As a LAST resort, the warning could be suppressed via the "usual" trick, something like:
>>>
>>>    const char* __p = reinterpret_cast<const char*>(c_locale);
>>>    return *reinterpret_cast<const locale*>(__p);
>>>
>>> which I would consider a tad better than disabling -Werror selectively.
>>
>> The code does
>>
>>  typedef char fake_locale[sizeof(locale)]
>>  __attribute__ ((aligned(__alignof__(locale))));
>>  fake_locale c_locale;
>>
>>  const locale&
>>  locale::classic()
>>  {
>>    _S_initialize();
>>    return reinterpret_cast<const locale&>(c_locale);
>>  }
>
> This idiom -- which I opposed when it was first mentioned, but now I think
> is better than alternatives -- is to designed to work around the unordered
> initialization of globals accross translation units.
>
> The idea behind it is that, we first allocate (static) storare for the object
> (fake_locale type), which we also name c_locale.  Then later on, we actually
> give that storage a definite type (the dynamic type) by using the new-placement
> operator.  From there on, the object acquires the definite dynamic type till
> destruction.   So, the reinterpret_cast really is not mucking around
> -- it does not
> change the dynamic type of the object.  So, everything is fine.
>
> Notice that this idiom may become common place, now that C++0x has alignas
> (the moral equivalent of GNU C++'s attribute above), along with the library
> aligned_storage<> template.
>
>>
>> which violates C aliasing rules.  But - I understand that possibly
>>
>>  void
>>  locale::_S_initialize_once()
>>  {
>>    // 2 references.
>>    // One reference for _S_classic, one for _S_global
>>    _S_classic = new (&c_locale_impl) _Impl(2);
>>    _S_global = _S_classic;
>>    new (&c_locale) locale(_S_classic);
>>  }
>>
>> is always called before the above, so the dynamic type of c_locale
>> will be locale, so this _doesn't_ violate the C++ aliasing rules.
>
> That is correct.
>
>>
>> (of course some of us don't agree on the interpretation of the C++
>> standard wording with regards to whether it is allowed to change
>> the dynamic type of storage with a declared type, c_locale in this case)
>>
>> A miscompilation is very unlikely (even though GCC still may have bugs
>> with honoring the C++ memory model), as reads/writes of type c_locale
>> always conflict with reads/writes of type char, which is the declared type
>> of c_clocale.
>
> We need to make sure that the compiler guarantees the semantics -- e.g.
> its getting `smarter' does not inadvertently produce miscompilation.

Well, it's easy to generate a testcase that shows wrong points-to / vops.
You just have to hide the change of the dynamic type from the compiler.

int i;

void changeme(void);

float foo(void)
{
  float *p;
  i = 0;
  changeme();
  p = (float *)&i;
  *p = 1.0;
  return *p;
}

suppose that changeme invokes a placement new of type float on i.

float foo() ()
{
  float * p;
  float D.2055;

<bb 2>:
  # i_5 = VDEF <i_4(D)>
  i = 0;
  # i_7 = VDEF <i_5>
  # SMT.9_8 = VDEF <SMT.9_6(D)>
  changeme ();
  p_1 = (float *) &i;
  # SMT.9_9 = VDEF <SMT.9_8>
  *p_1 = 1.0e+0;
  # VUSE <i_7>
  D.2055_2 = VIEW_CONVERT_EXPR<float>(i);
  return D.2055_2;

}

then you see we have done type-based pruning of i from the store to
*p_1.  Only because the V_C_E hides the load from i it is not optimized
to 0.0.

As I said back in the C++ placement new alias bugs the only way to
honor the C++ memory model is by properly making _all_ stores
conflict and to disallow hoisting of loads over stores.  For some common
cases Ian made it work with CHANGE_DYNAMIC_TYPE_EXPR and
DECL_NO_TBAA, but obviously this isn't enough once a declared type
is visible to the compiler.

Richard.