[Bug sanitizer/110799] [tsan] False positive due to -fhoist-adjacent-loads

[rguenther at suse dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110799

--- Comment #11 from rguenther at suse dot de <rguenther at suse dot de> ---
On Wed, 26 Jul 2023, amonakov at gcc dot gnu.org wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110799
> 
> --- Comment #9 from Alexander Monakov <amonakov at gcc dot gnu.org> ---
> (In reply to Tom de Vries from comment #7)
> > Can you elaborate on what you consider a correct approach?
> 
> I think this optimization is incorrect and should be active only under -Ofast.
> 
> I can offer two arguments. First, even without considering correctness,
> breaking TSan and Helgrind is a substantial QoI issue and we should consider
> shielding -O2 users from that (otherwise they'll discover it the hard way,
> curse at us, stick -fno-hoist-adjacent-loads in their build system and consider
> switching to another compiler).
> 
> Second, I can upgrade the initial example to an actual miscompilation. The
> upgrade is based on two considerations: the optimization works on
> possibly-trapping accesses, and relies on types of memory references to decide
> if it's safe, but it runs late where the types are not what they were in the C
> source. Hence, the following example:
> 
> struct S {
>         int a;
> };
> struct M {
>         int a, b;
> };
> 
> int f(struct S *p, int c, int d)
> {
>         int r;
>         if (c)
>                 if (d)
>                         r = p->a;
>                 else
>                         r = ((struct M*)p)->a;
>         else
>                 r = ((struct M*)p)->b;
>         return r;
> }
> 
> is miscompiled to
> 
> f:
>         mov     eax, DWORD PTR [rdi+4]
>         test    esi, esi
>         cmovne  eax, DWORD PTR [rdi]
>         ret
> 
> even though the original program never accesses beyond struct S if 'c && d'.
> Phi-opt incorrectly performs hoisting after PRE collapses 'if (d) ... else
> ...'.

It looks like a bug in PRE (as well) though.  It shouldn't be an
argument to ditch the adjacent load hoisting alltogether.