From d19b0682f18f9f5217aee8002e3d04f8ded04ae8 Mon Sep 17 00:00:00 2001
From: Jakub Jelinek <jakub@redhat.com>
Date: Thu, 23 Jan 2025 11:11:23 +0100
Subject: [PATCH] builtins: Store unspecified value to *exp for inf/nan
[PR114877]
The fold_builtin_frexp folding for NaN/Inf just returned the first argument
with evaluating second arguments side-effects, rather than storing something
to what the second argument points to.
The PR argues that the C standard requires the function to store something
there but what exactly is stored is unspecified, so not storing there
anything can result in UB if the value isn't initialized and is read later.
glibc and newlib store there 0, musl apparently doesn't store anything.
The following patch stores there zero (or would you prefer storing there
some other value, 42, INT_MAX, INT_MIN, etc.?; zero is cheapest to form
in assembly though) and adjusts the test so that it
doesn't rely on not storing there anything but instead checks for
-Wmaybe-uninitialized warning to find out that something has been stored
there.
Unfortunately I had to disable the NaN tests for -O0, while we can fold
__builtin_isnan (__builtin_nan ("")) at compile time, we can't fold
__builtin_isnan ((i = 0, __builtin_nan (""))) at compile time.
fold_builtin_classify uses just tree_expr_nan_p and if that isn't true
(because expr is a COMPOUND_EXPR with tree_expr_nan_p on the second arg),
it does
arg = builtin_save_expr (arg);
return fold_build2_loc (loc, UNORDERED_EXPR, type, arg, arg);
and that isn't folded at -O0 further, as we wrap it into SAVE_EXPR and
nothing propagates the NAN to the comparison.
I think perhaps tree_expr_nan_p etc. could have case COMPOUND_EXPR:
added and recurse on the second argument, but that feels like stage1
material to me if we want to do that at all.
2025-01-23 Jakub Jelinek <jakub@redhat.com>
PR middle-end/114877
* builtins.cc (fold_builtin_frexp): Handle rvc_nan and rvc_inf cases
like rvc_zero, return passed in arg and set *exp = 0.
* gcc.dg/torture/builtin-frexp-1.c: Add -Wmaybe-uninitialized as
dg-additional-options.
(bar): New function.
(TESTIT_FREXP2): Rework the macro so that it doesn't test whether
nothing has been stored to what the second argument points to, but
instead that something has been stored there, whatever it is.
(main): Temporarily don't enable the nan tests for -O0.
---
gcc/builtins.cc | 10 +++---
.../gcc.dg/torture/builtin-frexp-1.c | 33 ++++++++++++++-----
2 files changed, 31 insertions(+), 12 deletions(-)
diff --git a/gcc/builtins.cc b/gcc/builtins.cc
index 8bb91263f5f7..468bd65bc42a 100644
--- a/gcc/builtins.cc
+++ b/gcc/builtins.cc
@@ -9574,14 +9574,16 @@ fold_builtin_frexp (location_t loc, tree arg0, tree arg1, tree rettype)
switch (value->cl)
{
case rvc_zero:
+ case rvc_nan:
+ case rvc_inf:
/* For +-0, return (*exp = 0, +-0). */
+ /* For +-NaN or +-Inf, *exp is unspecified, but something should
+ be stored there so that it isn't read from uninitialized object.
+ As glibc and newlib store *exp = 0 for +-Inf/NaN, storing
+ 0 here as well is easiest. */
exp = integer_zero_node;
frac = arg0;
break;
- case rvc_nan:
- case rvc_inf:
- /* For +-NaN or +-Inf, *exp is unspecified, return arg0. */
- return omit_one_operand_loc (loc, rettype, arg0, arg1);
case rvc_normal:
{
/* Since the frexp function always expects base 2, and in
diff --git a/gcc/testsuite/gcc.dg/torture/builtin-frexp-1.c b/gcc/testsuite/gcc.dg/torture/builtin-frexp-1.c
index 2d1c1847b267..328b803e9a1e 100644
--- a/gcc/testsuite/gcc.dg/torture/builtin-frexp-1.c
+++ b/gcc/testsuite/gcc.dg/torture/builtin-frexp-1.c
@@ -11,6 +11,7 @@
floating point formats need -funsafe-math-optimizations. */
/* { dg-require-effective-target inf } */
/* { dg-options "-funsafe-math-optimizations" { target powerpc*-*-* } } */
+/* { dg-additional-options "-Wmaybe-uninitialized" } */
extern void link_error(int);
@@ -52,22 +53,36 @@ extern void link_error(int);
link_error(__LINE__); \
} while (0)
+int __attribute__ ((__noipa__))
+bar (int x)
+{
+ (void) x;
+ return 42;
+}
+
/* Test that FUNCRES(frexp(NEG FUNCARG(ARGARG),&i)) is false. Check
- the sign as well. Ensure side-effects are evaluated in i. */
+ the sign as well. Ensure side-effects are evaluated in the second
+ frexp argument. */
#define TESTIT_FREXP2(NEG,FUNCARG,ARGARG,FUNCRES) do { \
- int i=5; \
+ int i, j = 5; \
if (!__builtin_##FUNCRES##f(__builtin_frexpf(NEG __builtin_##FUNCARG##f(ARGARG),&i)) \
- || CKSGN_F(__builtin_frexpf(NEG __builtin_##FUNCARG##f(ARGARG),(i++,&i)), NEG __builtin_##FUNCARG##f(ARGARG)) \
- || CKEXP(i,6)) \
+ || CKSGN_F(__builtin_frexpf(NEG __builtin_##FUNCARG##f(ARGARG),(j++,&i)), NEG __builtin_##FUNCARG##f(ARGARG)) \
+ || CKEXP(j,6)) \
link_error(__LINE__); \
+ if (CKEXP(bar(i),42)) \
+ __builtin_abort(); \
if (!__builtin_##FUNCRES(__builtin_frexp(NEG __builtin_##FUNCARG(ARGARG),&i)) \
- || CKSGN(__builtin_frexp(NEG __builtin_##FUNCARG(ARGARG),(i++,&i)), NEG __builtin_##FUNCARG(ARGARG)) \
- || CKEXP(i,7)) \
+ || CKSGN(__builtin_frexp(NEG __builtin_##FUNCARG(ARGARG),(j++,&i)), NEG __builtin_##FUNCARG(ARGARG)) \
+ || CKEXP(j,7)) \
link_error(__LINE__); \
+ if (CKEXP(bar(i),42)) \
+ __builtin_abort(); \
if (!__builtin_##FUNCRES##l(__builtin_frexpl(NEG __builtin_##FUNCARG##l(ARGARG),&i)) \
- || CKSGN_L(__builtin_frexpl(NEG __builtin_##FUNCARG##l(ARGARG),(i++,&i)), NEG __builtin_##FUNCARG##l(ARGARG)) \
- || CKEXP(i,8)) \
+ || CKSGN_L(__builtin_frexpl(NEG __builtin_##FUNCARG##l(ARGARG),(j++,&i)), NEG __builtin_##FUNCARG##l(ARGARG)) \
+ || CKEXP(j,8)) \
link_error(__LINE__); \
+ if (CKEXP(bar(i),42)) \
+ __builtin_abort(); \
} while (0)
void __attribute__ ((__noinline__))
@@ -111,8 +126,10 @@ foo(void)
Exponent is left unspecified, but we test for side-effects. */
TESTIT_FREXP2 ( ,inf, , isinf);
TESTIT_FREXP2 (- ,inf, , isinf);
+#ifdef __OPTIMIZE__
TESTIT_FREXP2 ( ,nan, "", isnan);
TESTIT_FREXP2 (- ,nan, "", isnan);
+#endif
}
int main()
--
GitLab