rs6000: Fix type size checks for __builtin_ppc_atomic_cas_local [PR126513]

Message ID 20260804090221.352728-1-avinashd@linux.ibm.com
State New
Headers
Series rs6000: Fix type size checks for __builtin_ppc_atomic_cas_local [PR126513] |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gcc_build--master-arm success Build passed

Commit Message

Avinash Jayakar Aug. 4, 2026, 9:02 a.m. UTC
  Hi Michael/Jakub,

Below patch is to fix PR126513. Bootstrapped and regtested on
powerpc64le-linux-gnu. Ok for trunk and affected branches (16 and 15)?

I would need some advice on whether we need to fix it for 16.2 release as well.
Although the issue reported in PR126513 is a negative test where compiler must
report error, but currently ICE happens. While testing it I see that case c4
mentioned in the commit message was also not handled. This is a positive test
where compiler must produce output, but ICEs and I think this is a bit more
higher severity issue. So do you think this should go in 16.2?

Thanks and regards,
Avinash Jayakar

Type checks for the first 3 operands of the
__builtin_ppc_atomic_cas_local were missed with its initial
implementation. This patch adds the following checks
c1. Make sure the size of first 3 operands is the same.
c2. Do not allow function pointers as arguments.
c3. Report error when -mno-quad-memory-atomic is used but 128 bit
operands are used as the first 3 operands.
c4. A separate issue was when no lhs was assigned to this builtin, ICE would
happen as a result of emit_cmp_and_jump_insns function dereferencing the
target rtx which would be null. Added the check in rs6000_expand_builtin
to allocate a reg_rtx if target is null or const0_rtx.

Tests have been added to check all the implemented checks described
above.
1. pr126513-acmp-tst-1.c: Checks c1 and c2.
2. pr126513-acmp-tst-2.c: Checks c3.
3. pr126513-acmp-tst-3.c: Checks the ICE in c4.

2026-08-04  Avinash Jayakar  <avinashd@linux.ibm.com>

gcc/ChangeLog:
        PR target/126513
        * config/rs6000/rs6000-builtin.cc (rs6000_expand_builtin): Allocate reg
	if target is null or const0_rtx.
        * config/rs6000/rs6000-c.cc (altivec_resolve_overloaded_builtin): Add
	error handling for c1, c2 and c3 described in this commit.

gcc/testsuite/ChangeLog:
        PR target/126513
        * gcc.target/powerpc/pr126513-acmp-tst-1.c: New test.
        * gcc.target/powerpc/pr126513-acmp-tst-2.c: New test.
        * gcc.target/powerpc/pr126513-acmp-tst-3.c: New test.
---
 gcc/config/rs6000/rs6000-builtin.cc           |   4 +
 gcc/config/rs6000/rs6000-c.cc                 |  79 +++++---
 .../gcc.target/powerpc/pr126513-acmp-tst-1.c  |  25 +++
 .../gcc.target/powerpc/pr126513-acmp-tst-2.c  |  15 ++
 .../gcc.target/powerpc/pr126513-acmp-tst-3.c  | 176 ++++++++++++++++++
 5 files changed, 270 insertions(+), 29 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/powerpc/pr126513-acmp-tst-1.c
 create mode 100644 gcc/testsuite/gcc.target/powerpc/pr126513-acmp-tst-2.c
 create mode 100644 gcc/testsuite/gcc.target/powerpc/pr126513-acmp-tst-3.c
  

Comments

Jakub Jelinek Aug. 5, 2026, 11:22 a.m. UTC | #1
On Tue, Aug 04, 2026 at 02:32:21PM +0530, Avinash Jayakar wrote:
> Below patch is to fix PR126513. Bootstrapped and regtested on
> powerpc64le-linux-gnu. Ok for trunk and affected branches (16 and 15)?
> 
> I would need some advice on whether we need to fix it for 16.2 release as well.
> Although the issue reported in PR126513 is a negative test where compiler must
> report error, but currently ICE happens. While testing it I see that case c4
> mentioned in the commit message was also not handled. This is a positive test
> where compiler must produce output, but ICEs and I think this is a bit more
> higher severity issue. So do you think this should go in 16.2?

Neither of that feels like a blocker for the 16.2 release, but otherwise
ice-on-valid-code and ice-on-invalid-code regressions are generally ok to
be exported (unlike e.g. accepts-invalid), so if Michael approves it (I'd
prefer if it is approved by PowerPC person) for trunk, it is ok for 16.2.1
(i.e. after 16.2 is released) and 16.x (but for regressions of the right
categories if safe one actually doesn't need extra approval for the
backports or whomever approves it for trunk can approve it for branches as
well).

	Jakub
  
Michael Meissner Aug. 6, 2026, 2:37 a.m. UTC | #2
On Tue, Aug 04, 2026 at 02:32:21PM +0530, Avinash Jayakar wrote:
> Hi Michael/Jakub,
> 
> Below patch is to fix PR126513. Bootstrapped and regtested on
> powerpc64le-linux-gnu. Ok for trunk and affected branches (16 and 15)?
> 
> I would need some advice on whether we need to fix it for 16.2 release as well.
> Although the issue reported in PR126513 is a negative test where compiler must
> report error, but currently ICE happens. While testing it I see that case c4
> mentioned in the commit message was also not handled. This is a positive test
> where compiler must produce output, but ICEs and I think this is a bit more
> higher severity issue. So do you think this should go in 16.2?
> 
> Thanks and regards,
> Avinash Jayakar
> 
> Type checks for the first 3 operands of the
> __builtin_ppc_atomic_cas_local were missed with its initial
> implementation. This patch adds the following checks
> c1. Make sure the size of first 3 operands is the same.
> c2. Do not allow function pointers as arguments.
> c3. Report error when -mno-quad-memory-atomic is used but 128 bit
> operands are used as the first 3 operands.
> c4. A separate issue was when no lhs was assigned to this builtin, ICE would
> happen as a result of emit_cmp_and_jump_insns function dereferencing the
> target rtx which would be null. Added the check in rs6000_expand_builtin
> to allocate a reg_rtx if target is null or const0_rtx.

This looks ok for the trunk and for GCC 16.2.1 or GCC 16.3 when they
open up for general patches.
  

Patch

diff --git a/gcc/config/rs6000/rs6000-builtin.cc b/gcc/config/rs6000/rs6000-builtin.cc
index d8eef2b65fe..ad6f1fc09f4 100644
--- a/gcc/config/rs6000/rs6000-builtin.cc
+++ b/gcc/config/rs6000/rs6000-builtin.cc
@@ -3504,6 +3504,10 @@  rs6000_expand_builtin (tree exp, rtx target, rtx /* subtarget */,
 
       // 0: Boolean return (Output)
       struct expand_operand ops[8];
+      // Make sure we always have a place for bool operand.
+      if (target == const0_rtx || !target)
+	target = gen_reg_rtx (SImode);
+
       create_output_operand (&ops[0], target, SImode);
 
       // 1: Old value return (Output)
diff --git a/gcc/config/rs6000/rs6000-c.cc b/gcc/config/rs6000/rs6000-c.cc
index 3cbdb6fb2ba..4984ec1c35a 100644
--- a/gcc/config/rs6000/rs6000-c.cc
+++ b/gcc/config/rs6000/rs6000-c.cc
@@ -1734,41 +1734,55 @@  altivec_resolve_overloaded_builtin (location_t loc, tree fndecl,
 	    error_at (loc, "%qE requires 6 arguments", fndecl);
 	  return error_mark_node;
 	}
+      unsigned HOST_WIDE_INT size;
+      for (int i=0; i<3; i++) {
+	/* Get the first argument to determine the actual type.  */
+	tree arg0 = (*arglist)[i];
+	tree type0 = TREE_TYPE (arg0);
+
+	/* Must be a pointer.  */
+	if (!POINTER_TYPE_P (type0))
+	  {
+	    if (complain)
+	      error_at (loc, "argument %d to %qE must be a pointer",
+			i + 1, fndecl);
+	    return error_mark_node;
+	  }
 
-      /* Get the first argument to determine the actual type.  */
-      tree arg0 = (*arglist)[0];
-      tree type0 = TREE_TYPE (arg0);
-
-      /* Must be a pointer.  */
-      if (!POINTER_TYPE_P (type0))
-	{
-	  if (complain)
-	    error_at (loc, "first argument to %qE must be a pointer", fndecl);
-	  return error_mark_node;
-	}
-
-      /* Get the pointee type.  */
-      tree pointee_type = TREE_TYPE (type0);
+	/* Get the pointee type.  */
+	tree pointee_type = TREE_TYPE (type0);
 
-      /* Must be a complete type.  */
-      if (!COMPLETE_TYPE_P (pointee_type))
-	{
-	  if (complain)
-	    error_at (loc, "first argument to %qE must point to a complete"
-		      " type", fndecl);
-	  return error_mark_node;
-	}
+	/* Must be a complete type.  */
+	if (!COMPLETE_TYPE_P (pointee_type))
+	  {
+	    if (complain)
+	      error_at (loc, "argument %d to %qE must point to a complete"
+			" type", i + 1, fndecl);
+	    return error_mark_node;
+	  }
+	if (FUNCTION_POINTER_TYPE_P (type0))
+	  {
+	    if (complain)
+	      error_at (loc, "argument %d to %qE must not be a pointer to a"
+			" function", i + 1, fndecl);
+	    return error_mark_node;
+	  }
 
-      /* Get size in bytes.  */
-      tree size_tree = TYPE_SIZE_UNIT (pointee_type);
-      if (!tree_fits_uhwi_p (size_tree))
-	{
+	/* Get size in bytes.  */
+	tree size_tree = TYPE_SIZE_UNIT (pointee_type);
+	if (!tree_fits_uhwi_p (size_tree))
+	  {
+		if (complain)
+		error_at (loc, "type size must be constant");
+		return error_mark_node;
+	  }
+	if (i > 0 && size != tree_to_uhwi (size_tree)) {
 	  if (complain)
-	    error_at (loc, "type size must be constant");
+	    error_at(loc, "size mismatch in argument %d", i + 1);
 	  return error_mark_node;
 	}
-
-      unsigned HOST_WIDE_INT size = tree_to_uhwi (size_tree);
+	size = tree_to_uhwi (size_tree);
+      }
 
       /* Determine which size-specific builtin to use.  */
       rs6000_gen_builtins target_fcode;
@@ -1793,6 +1807,13 @@  altivec_resolve_overloaded_builtin (location_t loc, tree fndecl,
 	  int_type = long_long_unsigned_type_node;
 	  break;
 	case 16:
+	  if (!TARGET_QUAD_MEMORY_ATOMIC)
+	    {
+	      if (complain)
+		error_at (loc, "%qE requires the %qs option for 16-byte operands",
+			  fndecl, "-mquad-memory-atomic");
+	      return error_mark_node;
+	    }
 	  target_fcode = RS6000_BIF_PPC_ATOMIC_CAS_TI;
 	  int_type = unsigned_intTI_type_node;
 	  break;
diff --git a/gcc/testsuite/gcc.target/powerpc/pr126513-acmp-tst-1.c b/gcc/testsuite/gcc.target/powerpc/pr126513-acmp-tst-1.c
new file mode 100644
index 00000000000..8d71152de77
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/pr126513-acmp-tst-1.c
@@ -0,0 +1,25 @@ 
+/* { dg-do compile } */
+/* { dg-require-effective-target lp64 } */
+
+// Need power8 for l<b,h,q>arx
+/* { dg-options "-O2 -mdejagnu-cpu=power8" } */
+
+__int128 word_exchange_uti_ptr;
+int word_exchange_uti_expected;
+unsigned word_exchange_uti_desired() {
+  __builtin_ppc_atomic_cas_local( /* { dg-error "argument 2 to '__builtin_ppc_atomic_cas_local' must be a pointer" } */
+      &word_exchange_uti_ptr, word_exchange_uti_expected,
+      word_exchange_uti_desired, 0, __ATOMIC_SEQ_CST, __ATOMIC_ACQUIRE);
+}
+
+unsigned word_exchange_uti_desired_fptr() {
+  __builtin_ppc_atomic_cas_local( /* { dg-error "argument 1 to '__builtin_ppc_atomic_cas_local' must not be a pointer to a function" } */
+      &word_exchange_uti_desired_fptr, &word_exchange_uti_desired,
+      &word_exchange_uti_desired_fptr, 0, __ATOMIC_SEQ_CST, __ATOMIC_ACQUIRE);
+}
+
+unsigned word_exchange_uti_desired_mismatch() {
+  __builtin_ppc_atomic_cas_local( /* { dg-error "size mismatch in argument 2" } */
+      &word_exchange_uti_ptr, &word_exchange_uti_expected,
+      &word_exchange_uti_ptr, 0, __ATOMIC_SEQ_CST, __ATOMIC_ACQUIRE);
+}
diff --git a/gcc/testsuite/gcc.target/powerpc/pr126513-acmp-tst-2.c b/gcc/testsuite/gcc.target/powerpc/pr126513-acmp-tst-2.c
new file mode 100644
index 00000000000..2c7c3c796c4
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/pr126513-acmp-tst-2.c
@@ -0,0 +1,15 @@ 
+/* { dg-do compile } */
+/* { dg-require-effective-target lp64 } */
+
+// Need power8 for l<b,h,q>arx
+/* { dg-options "-O2 -mdejagnu-cpu=power8 -mno-quad-memory-atomic" } */
+
+__int128 word_exchange_uti_ptr;
+__int128 word_exchange_uti_ptr_expected;
+__int128 word_exchange_uti_ptr_desired;
+
+unsigned word_exchange_uti_desired() {
+  __builtin_ppc_atomic_cas_local( /* { dg-error "'__builtin_ppc_atomic_cas_local' requires the '-mquad-memory-atomic' option for 16-byte operands" } */
+      &word_exchange_uti_ptr, &word_exchange_uti_ptr_expected,
+      &word_exchange_uti_ptr_desired, 0, __ATOMIC_SEQ_CST, __ATOMIC_ACQUIRE);
+}
diff --git a/gcc/testsuite/gcc.target/powerpc/pr126513-acmp-tst-3.c b/gcc/testsuite/gcc.target/powerpc/pr126513-acmp-tst-3.c
new file mode 100644
index 00000000000..da6e4756058
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/pr126513-acmp-tst-3.c
@@ -0,0 +1,176 @@ 
+/* { dg-do compile } */
+/* { dg-require-effective-target lp64 } */
+
+// Need power8 for l<b,h,q>arx
+/* { dg-options "-O2 -mdejagnu-cpu=power8" } */
+
+typedef struct udt_1
+{
+  char *a;
+} udt_1t;
+typedef struct udt_2
+{
+  char a;
+  char b;
+} udt_2t;
+typedef struct udt_4
+{
+  short a;
+  short b;
+} udt_4t;
+typedef struct udt_8
+{
+  int a;
+  int b;
+} udt_8t;
+typedef struct udt_16
+{
+  long long a;
+  long long b;
+} udt_16t;
+void
+word_exchange_nqi (char *ptr, char *expected, char *desired)
+{
+  __builtin_ppc_atomic_cas_local (ptr, expected, desired, 0,
+					 __ATOMIC_SEQ_CST, __ATOMIC_ACQUIRE);
+}
+void
+word_exchange_qi (signed char *ptr, signed char *expected, signed char *desired)
+{
+  __builtin_ppc_atomic_cas_local (ptr, expected, desired, 0,
+					 __ATOMIC_SEQ_CST, __ATOMIC_ACQUIRE);
+}
+void
+word_exchange_uqi (unsigned char *ptr, unsigned char *expected,
+                   unsigned char *desired)
+{
+  __builtin_ppc_atomic_cas_local (ptr, expected, desired, 0,
+					 __ATOMIC_SEQ_CST, __ATOMIC_ACQUIRE);
+}
+void
+word_exchange_hi (short *ptr, short *expected, short *desired)
+{
+  __builtin_ppc_atomic_cas_local (ptr, expected, desired, 0,
+					 __ATOMIC_SEQ_CST, __ATOMIC_ACQUIRE);
+}
+void
+word_exchange_shi (signed short *ptr, signed short *expected,
+                   signed short *desired)
+{
+  __builtin_ppc_atomic_cas_local (ptr, expected, desired, 0,
+					 __ATOMIC_SEQ_CST, __ATOMIC_ACQUIRE);
+}
+void
+word_exchange_uhi (unsigned short *ptr, unsigned short *expected,
+                   unsigned short *desired)
+{
+  __builtin_ppc_atomic_cas_local (ptr, expected, desired, 0,
+					 __ATOMIC_SEQ_CST, __ATOMIC_ACQUIRE);
+}
+void
+word_exchange_si (int *ptr, int *expected, int *desired)
+{
+  __builtin_ppc_atomic_cas_local (ptr, expected, desired, 0,
+					 __ATOMIC_SEQ_CST, __ATOMIC_ACQUIRE);
+}
+void
+word_exchange_ssi (signed int *ptr, signed int *expected, signed int *desired)
+{
+  __builtin_ppc_atomic_cas_local (ptr, expected, desired, 0,
+					 __ATOMIC_SEQ_CST, __ATOMIC_ACQUIRE);
+}
+void
+word_exchange_usi (unsigned int *ptr, unsigned int *expected,
+                   unsigned int *desired)
+{
+  __builtin_ppc_atomic_cas_local (ptr, expected, desired, 0,
+					 __ATOMIC_SEQ_CST, __ATOMIC_ACQUIRE);
+}
+void
+word_exchange_di (long long *ptr, long long *expected, long long *desired)
+{
+  __builtin_ppc_atomic_cas_local (ptr, expected, desired, 0,
+					 __ATOMIC_SEQ_CST, __ATOMIC_ACQUIRE);
+}
+void
+word_exchange_sdi (signed long long *ptr, signed long long *expected,
+                   signed long long *desired)
+{
+  __builtin_ppc_atomic_cas_local (ptr, expected, desired, 0,
+					 __ATOMIC_SEQ_CST, __ATOMIC_ACQUIRE);
+}
+void
+word_exchange_udi (unsigned long long *ptr, unsigned long long *expected,
+                   unsigned long long *desired)
+{
+  __builtin_ppc_atomic_cas_local (ptr, expected, desired, 0,
+					 __ATOMIC_SEQ_CST, __ATOMIC_ACQUIRE);
+}
+void
+word_exchange_sti (signed __int128 *ptr, signed __int128 *expected,
+                   signed __int128 *desired)
+{
+  __builtin_ppc_atomic_cas_local (ptr, expected, desired, 0,
+					 __ATOMIC_SEQ_CST, __ATOMIC_ACQUIRE);
+}
+void
+word_exchange_uti (unsigned __int128 *ptr, unsigned __int128 *expected,
+                   unsigned __int128 *desired)
+{
+  __builtin_ppc_atomic_cas_local (ptr, expected, desired, 0,
+					 __ATOMIC_SEQ_CST, __ATOMIC_ACQUIRE);
+}
+void
+word_exchange_f32 (float *ptr, float *expected, float *desired)
+{
+  __builtin_ppc_atomic_cas_local (ptr, expected, desired, 0,
+					 __ATOMIC_SEQ_CST, __ATOMIC_ACQUIRE);
+}
+void
+word_exchange_f64 (double *ptr, double *expected, double *desired)
+{
+  __builtin_ppc_atomic_cas_local (ptr, expected, desired, 0,
+					 __ATOMIC_SEQ_CST, __ATOMIC_ACQUIRE);
+}
+void
+word_exchange_f128 (__ieee128 *ptr, __ieee128 *expected, __ieee128 *desired)
+{
+  __builtin_ppc_atomic_cas_local (ptr, expected, desired, 0,
+					 __ATOMIC_SEQ_CST, __ATOMIC_ACQUIRE);
+}
+void
+word_exchange_udt_1 (udt_1t *ptr, udt_1t *expected, udt_1t *desired)
+{
+  __builtin_ppc_atomic_cas_local (ptr, expected, desired, 0,
+					 __ATOMIC_SEQ_CST, __ATOMIC_ACQUIRE);
+}
+void
+word_exchange_udt_2 (udt_2t *ptr, udt_2t *expected, udt_2t *desired)
+{
+  __builtin_ppc_atomic_cas_local (ptr, expected, desired, 0,
+					 __ATOMIC_SEQ_CST, __ATOMIC_ACQUIRE);
+}
+void
+word_exchange_udt_4 (udt_4t *ptr, udt_4t *expected, udt_4t *desired)
+{
+  __builtin_ppc_atomic_cas_local (ptr, expected, desired, 0,
+					 __ATOMIC_SEQ_CST, __ATOMIC_ACQUIRE);
+}
+void
+word_exchange_udt_8 (udt_8t *ptr, udt_8t *expected, udt_8t *desired)
+{
+  __builtin_ppc_atomic_cas_local (ptr, expected, desired, 0,
+					 __ATOMIC_SEQ_CST, __ATOMIC_ACQUIRE);
+}
+void
+word_exchange_udt_16 (udt_16t *ptr, udt_16t *expected, udt_16t *desired)
+{
+  __builtin_ppc_atomic_cas_local (ptr, expected, desired, 0,
+					 __ATOMIC_SEQ_CST, __ATOMIC_ACQUIRE);
+}
+
+/* { dg-final { scan-assembler-times {\mlbarx +[0-9]+,[0-9]+,[0-9]+,1} 3 } } */
+/* { dg-final { scan-assembler-times {\mlharx +[0-9]+,[0-9]+,[0-9]+,1} 4 } } */
+/* { dg-final { scan-assembler-times {\mlwarx +[0-9]+,[0-9]+,[0-9]+,1} 5 } } */
+/* { dg-final { scan-assembler-times {\mldarx +[0-9]+,[0-9]+,[0-9]+,1} 6 } } */
+/* { dg-final { scan-assembler-times {\mlqarx +[0-9]+,[0-9]+,[0-9]+,1} 4 } } */