[Bug target/93934] Unnecessary fld of uninitialized float stack variable results in ub of valid C++ code

[Bug target/93934] Unnecessary fld of uninitialized float stack variable results in ub of valid C++ code

[vajdaz at protonmail dot com]

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

--- Comment #9 from Zoltan Vajda <vajdaz at protonmail dot com> ---
As I understand it, it is acknowledged, that this is a bug. However, the issue
is in state NEW for a quite long time. The issue is still present in GCC 11.2.
Do you see any chances for some progress in this case?

[Bug target/93934] Unnecessary fld of uninitialized float stack variable results in ub of valid C++ code

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |uros at gcc dot gnu.org

--- Comment #10 from Richard Biener <rguenth at gcc dot gnu.org> ---
Btw, it seems to require -march=x86-64 to produce the fcmov.  It's RTL that
creates that so this might be an angle of attack for a fix.

We expand from

;;   basic block 9, loop depth 0
;;    pred:       8
  if (b_12(D) != 0)
    goto <bb 11>; [56.18%]
  else
    goto <bb 10>; [43.82%]
;;    succ:       11
;;                10

;;   basic block 10, loop depth 0
;;    pred:       9
;;                2
;;    succ:       11

;;   basic block 11, loop depth 0
;;    pred:       9
;;                10
  # iftmp.0_6 = PHI <result_3(9), 7.0e+0(10)>
  return iftmp.0_6;

and in split3 we seem to have jump threaded the exit in BB reorder
which should be fine but then we somehow if-converted the other check.

Disabling both CE1 and CE2 avoids that.  CE1 does

IF-THEN-JOIN block found, pass 1, test 5, then 7, join 9
scanning new insn with uid = 69.
scanning new insn with uid = 70.
if-conversion succeeded through noce_try_cmove
Removing jump 25.
deleting insn with uid = 25.
deleting insn with uid = 8.
deleting block 7
Conversion succeeded on pass 1.

IF-THEN-JOIN block found, pass 1, test 6, then 8, join 9

IF-CASE-2 found, start 6, else 8


maybe the backend should not advertise conditional moves with DF/SFmode and x87
math ...

[Bug target/93934] Unnecessary fld of uninitialized float stack variable results in ub of valid C++ code

[ubizjak at gmail dot com]

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

--- Comment #11 from Uroš Bizjak <ubizjak at gmail dot com> ---
(In reply to Zoltan Vajda from comment #9)
> As I understand it, it is acknowledged, that this is a bug. However, the
> issue is in state NEW for a quite long time. The issue is still present in
> GCC 11.2. Do you see any chances for some progress in this case?

The Comment #5 summarises all the troubles with trapping FLDL/FLDS. I'm afraid
that trapping FLDL/FLDS renders -fsignaling-nans on x87 practically impossible
to implement.

Even the documentation advises against usage of -fsignaling-nans:

     This option is experimental and does not currently guarantee to
     disable all GCC optimizations that affect signaling NaN behavior.

You should really use -mfpmath=sse here.

[Bug target/93934] Unnecessary fld of uninitialized float stack variable results in ub of valid C++ code

[vajdaz at protonmail dot com]

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

--- Comment #12 from Zoltan Vajda <vajdaz at protonmail dot com> ---
Using -mfpmath=sse here does not help on a 32 bit platfrom.
https://gcc.godbolt.org/z/hs1Ef6aj4
At line 31 the assembly code performs the speculative load.

[Bug target/93934] Unnecessary fld of uninitialized float stack variable results in ub of valid C++ code

[ubizjak at gmail dot com]

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

--- Comment #13 from Uroš Bizjak <ubizjak at gmail dot com> ---
(In reply to Zoltan Vajda from comment #12)
> Using -mfpmath=sse here does not help on a 32 bit platfrom.
> https://gcc.godbolt.org/z/hs1Ef6aj4
> At line 31 the assembly code performs the speculative load.

Yeah, -msse2 is also needed.

[Bug target/93934] Unnecessary fld of uninitialized float stack variable results in ub of valid C++ code

[amonakov at gcc dot gnu.org]

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

--- Comment #14 from Alexander Monakov <amonakov at gcc dot gnu.org> ---
Zoltan, excuse me, could you please clarify what specifically you are worried
about? Your bug title says "results in UB" and the opening comment said "the
behavior [..] is unpredictable", but as far as I see that is not the case here,
it's (as you also said) only raising an FPU exception where it shouldn't.
Unless the program inspects exception flags or enables signal delivery on FPU
exceptions, it won't notice. What is your main concern?

[Bug target/93934] Unnecessary fld of uninitialized float stack variable results in ub of valid C++ code

[vajdaz at protonmail dot com]

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

--- Comment #15 from Zoltan Vajda <vajdaz at protonmail dot com> ---
In my special case, I have an embedded realtime application with a lot of FP
atithmetic on Intel 32 bit architecture (huge and complex legacy codebase). FPU
exceptions are enabled, so loading an SNaN results in an exception. This is
intended, and we will don't want to change this configuration. In this context
the generated ASM code does result in an fld of an uninitialized local
variable, where looking on the C++ code such an access should not be possible.
If the content of the uninitialized local variable happens to be a SNaN by
accident (chances are very small, but not zero), an FPU exception happens. And
again, based on the C++ code no FPU exception should be possible (assuming d is
not an SNaN).

Here is a synthetic example that triggers the exception by "placing a bomb" on
the stack.

https://gcc.godbolt.org/z/aooex6dcT

Function place_bomb() has an effect on what happens in func(). That should not
be the case! This is all valid C++ code.

This may now accidentally happen in our application. The behavior is
unpredictable, because it depends on what previous function calls left on the
stack.

If you change "-march=i686" to "-march=i386" in the example linked above,
everything goes fine.

[Bug target/93934] Unnecessary fld of uninitialized float stack variable results in ub of valid C++ code

[joseph at codesourcery dot com]

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

--- Comment #16 from joseph at codesourcery dot com <joseph at codesourcery dot com> ---
I don't think this bug is anything to do with -fsignaling-nans, for the 
same reason as applies to bug 58416 and bug 71460.

The option -fsignaling-nans is only about correctly handling programs 
that, in the abstract machine, interpret the bit pattern of a signaling 
NaN as a floating-point value (i.e. do lvalue-to-rvalue conversion on it 
in the relevant floating-point type).  Thus bug, and those other two, and 
maybe others, involves a different case: a bit pattern of a signaling NaN 
occurs somewhere, but is never converted to an rvalue in the abstract 
machine.

In such cases, the bit pattern should be handled correctly even without 
-fsignaling-nans (and, thus, on x87, it's incorrect to do single-precision 
or double-precision loads on any bit-pattern where an lvalue-to-rvalue 
conversion doesn't occur in the abstract machine, even without 
-fsignaling-nans).

[Bug target/93934] Unnecessary fld of uninitialized float stack variable results in ub of valid C++ code

[rguenth at gcc dot gnu.org]

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

--- Comment #17 from Richard Biener <rguenth at gcc dot gnu.org> ---
It might help to provide a option or tunable to disable the use of fcmov which
should reduce the attack surface a bit.  I don't see any way to avoid using
fld for loading x87 float or double values (well, load into GPR, do softfp
convert to long double, spill and fld as long double ...).

For the reporter using -fno-if-conversion should also reliably avoid the
situation in the testcase.

[Bug target/93934] Unnecessary fld of uninitialized float stack variable results in ub of valid C++ code

[ubizjak at gmail dot com]

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

--- Comment #18 from Uroš Bizjak <ubizjak at gmail dot com> ---
The following patch fixes the PR, see the comment inline:

--cut here--
diff --git a/gcc/config/i386/i386-expand.c b/gcc/config/i386/i386-expand.c
index 6e2b7920d2b..b87490fe544 100644
--- a/gcc/config/i386/i386-expand.c
+++ b/gcc/config/i386/i386-expand.c
@@ -4094,6 +4094,15 @@ ix86_expand_fp_movcc (rtx operands[])
          && !TARGET_64BIT))
     return false;

+  /* Disable SFmode and DFmode x87 FCMOV with trapping math
+     to prevent speculative load using FLDL/FLDS from uninitialized
+     memory location, which can contain sNaN value.  FLDL/FLDS traps
+     on sNaN, see PR93934.  */
+
+  if ((mode == SFmode || mode == DFmode)
+      && flag_trapping_math)
+    return false;
+
   /* The floating point conditional move instructions don't directly
      support conditions resulting from a signed integer comparison.  */

--cut here--

[Bug target/93934] Unnecessary fld of uninitialized float stack variable results in ub of valid C++ code

[vajdaz at protonmail dot com]

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

--- Comment #19 from Zoltan Vajda <vajdaz at protonmail dot com> ---
(In reply to Uroš Bizjak from comment #18)
> The following patch fixes the PR, see the comment inline:
> 
> --cut here--
> diff --git a/gcc/config/i386/i386-expand.c b/gcc/config/i386/i386-expand.c
> index 6e2b7920d2b..b87490fe544 100644
> --- a/gcc/config/i386/i386-expand.c
> +++ b/gcc/config/i386/i386-expand.c
> @@ -4094,6 +4094,15 @@ ix86_expand_fp_movcc (rtx operands[])
>           && !TARGET_64BIT))
>      return false;
>  
> +  /* Disable SFmode and DFmode x87 FCMOV with trapping math
> +     to prevent speculative load using FLDL/FLDS from uninitialized
> +     memory location, which can contain sNaN value.  FLDL/FLDS traps
> +     on sNaN, see PR93934.  */
> +
> +  if ((mode == SFmode || mode == DFmode)
> +      && flag_trapping_math)
> +    return false;
> +
>    /* The floating point conditional move instructions don't directly
>       support conditions resulting from a signed integer comparison.  */
>  
> --cut here--

The problem does not only apply for conditional moves! I can turn on sse, for
example.

https://gcc.godbolt.org/z/jP3Kne8T5

Then the problematic code with the conditional move disappears, but I have a
similar speculative fld problem in another situation.

.L10:
        inc     esi
        cmp     edi, esi
        jne     .L11
        test    bl, bl                <= test input variable 'b'
        fld     QWORD PTR [ebp-24]    <= load of (maybe) uninitialized 'result'
        je      .L24                  <= jump based on value of 'b'
        add     esp, 20
        pop     ebx
        pop     esi
        pop     edi
        pop     ebp
        ret
.L24:
        fstp    st(0)
.L8:
        fld     QWORD PTR .LC1
        add     esp, 20
        pop     ebx
        pop     esi
        pop     edi
        pop     ebp
        ret

[Bug target/93934] Unnecessary fld of uninitialized float stack variable results in ub of valid C++ code

[ubizjak at gmail dot com]

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

--- Comment #20 from Uroš Bizjak <ubizjak at gmail dot com> ---
(In reply to joseph@codesourcery.com from comment #16)
> I don't think this bug is anything to do with -fsignaling-nans, for the 
> same reason as applies to bug 58416 and bug 71460.

The situation is hopeless from the beginning. Please consider this testcase:

--cut here--
#include <cpuid.h>
#include <fenv.h>

double
__attribute__((noinline,noipa))
foo (double a, double b, char c)
{
  return c ? a : b;
}

int main ()
{
  double a = __builtin_nans ("");
  double b = 42.0;

  feclearexcept (FE_INVALID);
  foo (a, b, 0);
  if (fetestexcept (FE_INVALID))
    __builtin_abort ();

  return 0;
}
--cut here--

$ gcc -O2 -m32 -march=i686 -lm fcmov.c
$ ./a.out 
Aborted (core dumped)
$ gcc -O2 -m32 -march=i386 -lm fcmov.c
$ ./a.out 
Aborted (core dumped)

Because the compiler generates:

foo:
        cmpb    $0, 20(%esp)
        fldl    12(%esp)
        fldl    4(%esp)
        fcmove  %st(1), %st
        fstp    %st(1)
        ret

in the former case and:

foo:
        fldl    4(%esp)
        fldl    12(%esp)
        cmpb    $0, 20(%esp)
        jne     .L4
        fstp    %st(1)
        jmp     .L2
.L4:
        fstp    %st(0)
.L2:
        ret

in the later.

Since the ABI specifies the operand size on the stack, the above code will
always trap.

[Bug target/93934] Unnecessary fld of uninitialized float stack variable results in ub of valid C++ code

[ubizjak at gmail dot com]

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

--- Comment #21 from Uroš Bizjak <ubizjak at gmail dot com> ---
(In reply to Zoltan Vajda from comment #19)
> The problem does not only apply for conditional moves! I can turn on sse,
> for example.
> 
> https://gcc.godbolt.org/z/jP3Kne8T5
> 
> Then the problematic code with the conditional move disappears, but I have a
> similar speculative fld problem in another situation.
> 
> .L10:
>         inc     esi
>         cmp     edi, esi
>         jne     .L11
>         test    bl, bl                <= test input variable 'b'
>         fld     QWORD PTR [ebp-24]    <= load of (maybe) uninitialized
> 'result'
>         je      .L24                  <= jump based on value of 'b'

This one is fixed at least in gcc-10.

.L18:
        testb   %bl, %bl
        je      .L8
        fldl    -24(%ebp)
        addl    $20, %esp
        popl    %ebx

[Bug target/93934] Unnecessary fld of uninitialized float stack variable results in ub of valid C++ code

[rguenther at suse dot de]

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

--- Comment #22 from rguenther at suse dot de <rguenther at suse dot de> ---
On Thu, 14 Oct 2021, ubizjak at gmail dot com wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93934
> 
> --- Comment #20 from Uroš Bizjak <ubizjak at gmail dot com> ---
> (In reply to joseph@codesourcery.com from comment #16)
> > I don't think this bug is anything to do with -fsignaling-nans, for the 
> > same reason as applies to bug 58416 and bug 71460.
> 
> The situation is hopeless from the beginning. Please consider this testcase:
> 
> --cut here--
> #include <cpuid.h>
> #include <fenv.h>
> 
> double
> __attribute__((noinline,noipa))
> foo (double a, double b, char c)
> {
>   return c ? a : b;
> }
> 
> int main ()
> {
>   double a = __builtin_nans ("");
>   double b = 42.0;
> 
>   feclearexcept (FE_INVALID);
>   foo (a, b, 0);
>   if (fetestexcept (FE_INVALID))
>     __builtin_abort ();
> 
>   return 0;
> }
> --cut here--
> 
> $ gcc -O2 -m32 -march=i686 -lm fcmov.c
> $ ./a.out 
> Aborted (core dumped)
> $ gcc -O2 -m32 -march=i386 -lm fcmov.c
> $ ./a.out 
> Aborted (core dumped)
> 
> Because the compiler generates:
> 
> foo:
>         cmpb    $0, 20(%esp)
>         fldl    12(%esp)
>         fldl    4(%esp)
>         fcmove  %st(1), %st
>         fstp    %st(1)
>         ret
> 
> in the former case and:
> 
> foo:
>         fldl    4(%esp)
>         fldl    12(%esp)
>         cmpb    $0, 20(%esp)
>         jne     .L4
>         fstp    %st(1)
>         jmp     .L2
> .L4:
>         fstp    %st(0)
> .L2:
>         ret
> 
> in the later.
> 
> Since the ABI specifies the operand size on the stack, the above code will
> always trap.

Indeed and since those loads from the argument space appear as registers
in GIMPLE there's nothing avoiding "speculative" accesses to those so
the issue for argument slots are much harder to mitigate.  I also think
that RTL expansion happily puts those loads in the prologue rather
than next to the first use.

[Bug target/93934] Unnecessary fld of uninitialized float stack variable results in ub of valid C++ code

[vajdaz at protonmail dot com]

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

--- Comment #23 from Zoltan Vajda <vajdaz at protonmail dot com> ---
(In reply to Uroš Bizjak from comment #20)
> (In reply to joseph@codesourcery.com from comment #16)
> > I don't think this bug is anything to do with -fsignaling-nans, for the 
> > same reason as applies to bug 58416 and bug 71460.
> 
> The situation is hopeless from the beginning. Please consider this testcase:
> 
> --cut here--
> #include <cpuid.h>
> #include <fenv.h>
> 
> double
> __attribute__((noinline,noipa))
> foo (double a, double b, char c)
> {
>   return c ? a : b;
> }
> 
> int main ()
> {
>   double a = __builtin_nans ("");
>   double b = 42.0;
> 
>   feclearexcept (FE_INVALID);
>   foo (a, b, 0);
>   if (fetestexcept (FE_INVALID))
>     __builtin_abort ();
> 
>   return 0;
> }
> --cut here--
> 
> $ gcc -O2 -m32 -march=i686 -lm fcmov.c
> $ ./a.out 
> Aborted (core dumped)
> $ gcc -O2 -m32 -march=i386 -lm fcmov.c
> $ ./a.out 
> Aborted (core dumped)
> 
> Because the compiler generates:
> 
> foo:
>         cmpb    $0, 20(%esp)
>         fldl    12(%esp)
>         fldl    4(%esp)
>         fcmove  %st(1), %st
>         fstp    %st(1)
>         ret
> 
> in the former case and:
> 
> foo:
>         fldl    4(%esp)
>         fldl    12(%esp)
>         cmpb    $0, 20(%esp)
>         jne     .L4
>         fstp    %st(1)
>         jmp     .L2
> .L4:
>         fstp    %st(0)
> .L2:
>         ret
> 
> in the later.
> 
> Since the ABI specifies the operand size on the stack, the above code will
> always trap.

There is a small but very important difference between this example and my
example.

In this example the compiler may assume that objects 'a' and 'b' both are
initialized at the beginning of the function execution (calling a function with
uninitialized input by value would be UB). Therefore you may argue that
accessing both of them is fine. If you feed foo() with SNaN through 'a', it
will always abort, independent of 'c'.

In my example there is an uninitialized object when the function execution
starts (local variable 'result') , and at C++ level there is no execution path
that would result in accessing this object before initialization. In spite of
this, at ASM level, the object is accessed before initialization. I don't see
any argument that makes this access valid. My example function aborts randomly,
independent of the input. The behavior depends on some random data on the
stack.

[Bug target/93934] Unnecessary fld of uninitialized float stack variable results in ub of valid C++ code

[joseph at codesourcery dot com]

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

--- Comment #24 from joseph at codesourcery dot com <joseph at codesourcery dot com> ---
On Thu, 14 Oct 2021, ubizjak at gmail dot com via Gcc-bugs wrote:

> The situation is hopeless from the beginning. Please consider this testcase:
> 
> --cut here--
> #include <cpuid.h>
> #include <fenv.h>
> 
> double
> __attribute__((noinline,noipa))
> foo (double a, double b, char c)
> {
>   return c ? a : b;
> }
> 
> int main ()
> {
>   double a = __builtin_nans ("");
>   double b = 42.0;
> 
>   feclearexcept (FE_INVALID);
>   foo (a, b, 0);

This is a fundamentally different test, because it involves (in the 
abstract machine) lvalue-to-rvalue conversion of a sNaN representation.  
That means that, unlike the present bug, and the two others I referenced, 
(a) it's only valid with -fsignaling-nans, (b) it's at most a 
quality-of-implementation issue because of the rule that assignment to the 
same format may be a convertFormat operation rather a copy operation, and 
(c) the ABI means the exception can't be avoided when an sNaN is returned.  
Effectively, this test is bug 56831, whereas the present bug is more like 
bug 58416 and bug 71460.

[Bug target/93934] Unnecessary fld of uninitialized float stack variable results in ub of valid C++ code

[ubizjak at gmail dot com]

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

--- Comment #25 from Uroš Bizjak <ubizjak at gmail dot com> ---
(In reply to joseph@codesourcery.com from comment #24)
> This is a fundamentally different test, because it involves (in the 
> abstract machine) lvalue-to-rvalue conversion of a sNaN representation.  
> That means that, unlike the present bug, and the two others I referenced, 
> (a) it's only valid with -fsignaling-nans, (b) it's at most a 
> quality-of-implementation issue because of the rule that assignment to the 
> same format may be a convertFormat operation rather a copy operation, and 
> (c) the ABI means the exception can't be avoided when an sNaN is returned.  
> Effectively, this test is bug 56831, whereas the present bug is more like 
> bug 58416 and bug 71460.

I would like to point out (in the context of trapping behavior of x87) that I
don't think we should disable FP conditional moves with the patch in Comment
#18 to fix the presented corner case, as this will effect the runtime
performance of the vast majority of applications that don't care about traps.
Looking at the above referred bugs, it is IMO time to throw in the towel and
declare x87 math as kind-of-broken w.r.t trapping. The compiler will generate
code that speculatively loads values at various places (as shown by the
testcase in Comment #20), not only at the FCMOV site.

I'm not a language lawyer, but IMO access to a non-volatile object is not
forbidden *if the value is not used*. Unfortunately, some accesses have
secondary effects that are not considered by the compiler, and the trapping
behavior of FLDS/FLDL is certainly one of them.