match: Disable `(type)zero_one_valuep*CST` for 1bit signed types [PR115154]

Message ID 20240520220207.1977085-1-quic_apinski@quicinc.com
State Committed
Commit 49c87d22535ac4f8aacf088b3f462861c26cacb4
Headers
Series match: Disable `(type)zero_one_valuep*CST` for 1bit signed types [PR115154] |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gcc_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_gcc_check--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_gcc_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gcc_check--master-arm success Testing passed

Commit Message

Andrew Pinski May 20, 2024, 10:02 p.m. UTC
  The problem here is the pattern added in r13-1162-g9991d84d2a8435
assumes that it is well defined to multiply zero_one_valuep by the truncated
converted integer constant. It is well defined for all types except for signed 1bit types.
Where `a * -1` is produced which is undefined/
So disable this pattern for 1bit signed types.

Note the pattern added in r14-3432-gddd64a6ec3b38e is able to workaround the undefinedness except when
`-fsanitize=undefined` is turned on, this is why I added a testcase for that.

OK for trunk and gcc-14 and gcc-13 branches? Bootstrapped and tested on x86_64-linux-gnu with no regressions.

	PR tree-optimization/115154

gcc/ChangeLog:

	* match.pd (convert (mult zero_one_valued_p@1 INTEGER_CST@2)): Disable
	for 1bit signed types.

gcc/testsuite/ChangeLog:

	* c-c++-common/ubsan/signed1bitfield-1.c: New test.
	* gcc.c-torture/execute/signed1bitfield-1.c: New test.

Signed-off-by: Andrew Pinski <quic_apinski@quicinc.com>
---
 gcc/match.pd                                  |  6 +++--
 .../c-c++-common/ubsan/signed1bitfield-1.c    | 25 +++++++++++++++++++
 .../gcc.c-torture/execute/signed1bitfield-1.c | 23 +++++++++++++++++
 3 files changed, 52 insertions(+), 2 deletions(-)
 create mode 100644 gcc/testsuite/c-c++-common/ubsan/signed1bitfield-1.c
 create mode 100644 gcc/testsuite/gcc.c-torture/execute/signed1bitfield-1.c
  

Comments

Richard Biener May 21, 2024, 6:09 a.m. UTC | #1
On Tue, May 21, 2024 at 12:02 AM Andrew Pinski <quic_apinski@quicinc.com> wrote:
>
> The problem here is the pattern added in r13-1162-g9991d84d2a8435
> assumes that it is well defined to multiply zero_one_valuep by the truncated
> converted integer constant. It is well defined for all types except for signed 1bit types.
> Where `a * -1` is produced which is undefined/
> So disable this pattern for 1bit signed types.
>
> Note the pattern added in r14-3432-gddd64a6ec3b38e is able to workaround the undefinedness except when
> `-fsanitize=undefined` is turned on, this is why I added a testcase for that.
>
> OK for trunk and gcc-14 and gcc-13 branches? Bootstrapped and tested on x86_64-linux-gnu with no regressions.

OK for trunk and branches.  Please wait until after 13.3.

Richard.

>         PR tree-optimization/115154
>
> gcc/ChangeLog:
>
>         * match.pd (convert (mult zero_one_valued_p@1 INTEGER_CST@2)): Disable
>         for 1bit signed types.
>
> gcc/testsuite/ChangeLog:
>
>         * c-c++-common/ubsan/signed1bitfield-1.c: New test.
>         * gcc.c-torture/execute/signed1bitfield-1.c: New test.
>
> Signed-off-by: Andrew Pinski <quic_apinski@quicinc.com>
> ---
>  gcc/match.pd                                  |  6 +++--
>  .../c-c++-common/ubsan/signed1bitfield-1.c    | 25 +++++++++++++++++++
>  .../gcc.c-torture/execute/signed1bitfield-1.c | 23 +++++++++++++++++
>  3 files changed, 52 insertions(+), 2 deletions(-)
>  create mode 100644 gcc/testsuite/c-c++-common/ubsan/signed1bitfield-1.c
>  create mode 100644 gcc/testsuite/gcc.c-torture/execute/signed1bitfield-1.c
>
> diff --git a/gcc/match.pd b/gcc/match.pd
> index 0f9c34fa897..35e3d82b131 100644
> --- a/gcc/match.pd
> +++ b/gcc/match.pd
> @@ -2395,12 +2395,14 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
>    (mult (convert @0) @1)))
>
>  /* Narrow integer multiplication by a zero_one_valued_p operand.
> -   Multiplication by [0,1] is guaranteed not to overflow.  */
> +   Multiplication by [0,1] is guaranteed not to overflow except for
> +   1bit signed types.  */
>  (simplify
>   (convert (mult@0 zero_one_valued_p@1 INTEGER_CST@2))
>   (if (INTEGRAL_TYPE_P (type)
>        && INTEGRAL_TYPE_P (TREE_TYPE (@0))
> -      && TYPE_PRECISION (type) < TYPE_PRECISION (TREE_TYPE (@0)))
> +      && TYPE_PRECISION (type) < TYPE_PRECISION (TREE_TYPE (@0))
> +      && (TYPE_UNSIGNED (type) || TYPE_PRECISION (type) > 1))
>    (mult (convert @1) (convert @2))))
>
>  /* (X << C) != 0 can be simplified to X, when C is zero_one_valued_p.
> diff --git a/gcc/testsuite/c-c++-common/ubsan/signed1bitfield-1.c b/gcc/testsuite/c-c++-common/ubsan/signed1bitfield-1.c
> new file mode 100644
> index 00000000000..2ba8cf4dab0
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/ubsan/signed1bitfield-1.c
> @@ -0,0 +1,25 @@
> +/* { dg-do run } */
> +/* { dg-options "-O2 -fsanitize=undefined" } */
> +
> +/* PR tree-optimization/115154 */
> +/* This was being miscompiled with -fsanitize=undefined due to
> +   `(signed:1)(t*5)` being transformed into `-((signed:1)t)` which
> +   is undefined. */
> +
> +struct s {
> +  signed b : 1;
> +} f;
> +int i = 55;
> +__attribute__((noinline))
> +void check(int a)
> +{
> +        if (!a)
> +        __builtin_abort();
> +}
> +int main() {
> +    int t = i != 5;
> +    t = t*5;
> +    f.b = t;
> +    int tt = f.b;
> +    check(f.b);
> +}
> diff --git a/gcc/testsuite/gcc.c-torture/execute/signed1bitfield-1.c b/gcc/testsuite/gcc.c-torture/execute/signed1bitfield-1.c
> new file mode 100644
> index 00000000000..ab888ca3a04
> --- /dev/null
> +++ b/gcc/testsuite/gcc.c-torture/execute/signed1bitfield-1.c
> @@ -0,0 +1,23 @@
> +/* PR tree-optimization/115154 */
> +/* This was being miscompiled to `(signed:1)(t*5)`
> +   being transformed into `-((signed:1)t)` which is undefined.
> +   Note there is a pattern which removes the negative in some cases
> +   which works around the issue.  */
> +
> +struct {
> +  signed b : 1;
> +} f;
> +int i = 55;
> +__attribute__((noinline))
> +void check(int a)
> +{
> +        if (!a)
> +        __builtin_abort();
> +}
> +int main() {
> +    int t = i != 5;
> +    t = t*5;
> +    f.b = t;
> +    int tt = f.b;
> +    check(f.b);
> +}
> --
> 2.43.0
>
  

Patch

diff --git a/gcc/match.pd b/gcc/match.pd
index 0f9c34fa897..35e3d82b131 100644
--- a/gcc/match.pd
+++ b/gcc/match.pd
@@ -2395,12 +2395,14 @@  DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
   (mult (convert @0) @1)))
 
 /* Narrow integer multiplication by a zero_one_valued_p operand.
-   Multiplication by [0,1] is guaranteed not to overflow.  */
+   Multiplication by [0,1] is guaranteed not to overflow except for
+   1bit signed types.  */
 (simplify
  (convert (mult@0 zero_one_valued_p@1 INTEGER_CST@2))
  (if (INTEGRAL_TYPE_P (type)
       && INTEGRAL_TYPE_P (TREE_TYPE (@0))
-      && TYPE_PRECISION (type) < TYPE_PRECISION (TREE_TYPE (@0)))
+      && TYPE_PRECISION (type) < TYPE_PRECISION (TREE_TYPE (@0))
+      && (TYPE_UNSIGNED (type) || TYPE_PRECISION (type) > 1))
   (mult (convert @1) (convert @2))))
 
 /* (X << C) != 0 can be simplified to X, when C is zero_one_valued_p.
diff --git a/gcc/testsuite/c-c++-common/ubsan/signed1bitfield-1.c b/gcc/testsuite/c-c++-common/ubsan/signed1bitfield-1.c
new file mode 100644
index 00000000000..2ba8cf4dab0
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/ubsan/signed1bitfield-1.c
@@ -0,0 +1,25 @@ 
+/* { dg-do run } */
+/* { dg-options "-O2 -fsanitize=undefined" } */
+
+/* PR tree-optimization/115154 */
+/* This was being miscompiled with -fsanitize=undefined due to
+   `(signed:1)(t*5)` being transformed into `-((signed:1)t)` which
+   is undefined. */
+
+struct s {
+  signed b : 1;
+} f;
+int i = 55;
+__attribute__((noinline))
+void check(int a)
+{
+        if (!a)
+        __builtin_abort();
+}
+int main() {
+    int t = i != 5;
+    t = t*5;
+    f.b = t;
+    int tt = f.b;
+    check(f.b);
+}
diff --git a/gcc/testsuite/gcc.c-torture/execute/signed1bitfield-1.c b/gcc/testsuite/gcc.c-torture/execute/signed1bitfield-1.c
new file mode 100644
index 00000000000..ab888ca3a04
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/execute/signed1bitfield-1.c
@@ -0,0 +1,23 @@ 
+/* PR tree-optimization/115154 */
+/* This was being miscompiled to `(signed:1)(t*5)`
+   being transformed into `-((signed:1)t)` which is undefined.
+   Note there is a pattern which removes the negative in some cases
+   which works around the issue.  */
+
+struct {
+  signed b : 1;
+} f;
+int i = 55;
+__attribute__((noinline))
+void check(int a)
+{
+        if (!a)
+        __builtin_abort();
+}
+int main() {
+    int t = i != 5;
+    t = t*5;
+    f.b = t;
+    int tt = f.b;
+    check(f.b);
+}