diff mbox series

asan: Handle poly-int sizes in ASAN_MARK [PR97696]

Message ID mptplw8eete.fsf@arm.com
State New
Headers show
Series asan: Handle poly-int sizes in ASAN_MARK [PR97696] | expand

Commit Message

Richard Sandiford March 5, 2024, 6:03 p.m. UTC 
This patch makes the expansion of IFN_ASAN_MARK let through
poly-int-sized objects.  The expansion itself was already generic
enough, but the tests for the fast path were too strict.

Bootstrapped & regression tested on aarch64-linux-gnu.  Is this OK
for trunk now, or should it wait for GCC 15?  I'm not sure that it's
technically a regression, in the sense that we previously accepted the
testcase, but rejecting with an ICE is arguably worse than "sorry, can't
do that".  And as noted in the PR, this bug is breaking numpy builds.

Richard


gcc/
	PR sanitizer/97696
	* asan.cc (asan_expand_mark_ifn): Allow the length to be a poly_int.

gcc/testsuite/
	PR sanitizer/97696
	* gcc.target/aarch64/sve/pr97696.c: New test.
---
 gcc/asan.cc                                   |  9 +++---
 .../gcc.target/aarch64/sve/pr97696.c          | 28 +++++++++++++++++++
 2 files changed, 32 insertions(+), 5 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/aarch64/sve/pr97696.c

Comments

Jakub Jelinek March 5, 2024, 6:14 p.m. UTC | #1 
On Tue, Mar 05, 2024 at 06:03:41PM +0000, Richard Sandiford wrote:
> This patch makes the expansion of IFN_ASAN_MARK let through
> poly-int-sized objects.  The expansion itself was already generic
> enough, but the tests for the fast path were too strict.
> 
> Bootstrapped & regression tested on aarch64-linux-gnu.  Is this OK
> for trunk now, or should it wait for GCC 15?  I'm not sure that it's
> technically a regression, in the sense that we previously accepted the
> testcase, but rejecting with an ICE is arguably worse than "sorry, can't
> do that".  And as noted in the PR, this bug is breaking numpy builds.
> 
> Richard
> 
> 
> gcc/
> 	PR sanitizer/97696
> 	* asan.cc (asan_expand_mark_ifn): Allow the length to be a poly_int.
> 
> gcc/testsuite/
> 	PR sanitizer/97696
> 	* gcc.target/aarch64/sve/pr97696.c: New test.

Ok for trunk now.

> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/sve/pr97696.c
> @@ -0,0 +1,28 @@
> +/* { dg-options "-fsanitize=address -fsanitize-address-use-after-scope" } */

Though I'd say this test should require sanitize_address affective target.
E.g. libsanitizer (sure, not actually used by the test) is only supported
on aarch64*-*-linux*, not e.g. on darwin nor freebsd nor fuchsia etc.

	Jakub
Richard Sandiford March 5, 2024, 6:30 p.m. UTC | #2 
Jakub Jelinek <jakub@redhat.com> writes:
> On Tue, Mar 05, 2024 at 06:03:41PM +0000, Richard Sandiford wrote:
>> This patch makes the expansion of IFN_ASAN_MARK let through
>> poly-int-sized objects.  The expansion itself was already generic
>> enough, but the tests for the fast path were too strict.
>> 
>> Bootstrapped & regression tested on aarch64-linux-gnu.  Is this OK
>> for trunk now, or should it wait for GCC 15?  I'm not sure that it's
>> technically a regression, in the sense that we previously accepted the
>> testcase, but rejecting with an ICE is arguably worse than "sorry, can't
>> do that".  And as noted in the PR, this bug is breaking numpy builds.
>> 
>> Richard
>> 
>> 
>> gcc/
>> 	PR sanitizer/97696
>> 	* asan.cc (asan_expand_mark_ifn): Allow the length to be a poly_int.
>> 
>> gcc/testsuite/
>> 	PR sanitizer/97696
>> 	* gcc.target/aarch64/sve/pr97696.c: New test.
>
> Ok for trunk now.

Thanks.  (And thanks for the quick review.)

>
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.target/aarch64/sve/pr97696.c
>> @@ -0,0 +1,28 @@
>> +/* { dg-options "-fsanitize=address -fsanitize-address-use-after-scope" } */
>
> Though I'd say this test should require sanitize_address affective target.
> E.g. libsanitizer (sure, not actually used by the test) is only supported
> on aarch64*-*-linux*, not e.g. on darwin nor freebsd nor fuchsia etc.

Yeah, I'd wondered about that.  But fsanitize_address is only available
in the asan.exp framework (or something else that includes asan-dg.exp).
And like you say, the test doesn't specifically need the library to
be available.

I guess the options are:

(1) Keep the test where it is, taking advantage of the current SVE
    handling in aarch64-sve.exp, and add:

      /* { dg-skip-if "" { no_fsanitize_address } } */

(2) Move the test to gcc.dg/asan/, make it conditional on aarch64*-*-*,
    and add:

      #pragma GCC target "+sve"

Any preference?

Actually running the test would require both libsanitizer support and
aarch64_sve_hw.  Assembling it would need an assembler that understands SVE.

Richard
Jakub Jelinek March 5, 2024, 6:38 p.m. UTC | #3 
On Tue, Mar 05, 2024 at 06:30:40PM +0000, Richard Sandiford wrote:
> (1) Keep the test where it is, taking advantage of the current SVE
>     handling in aarch64-sve.exp, and add:
> 
>       /* { dg-skip-if "" { no_fsanitize_address } } */

I'd go with this.  asan/ directory for test would be needed for dg-do run
tests obviously, because then we need the test driver to add appropriate
options to find the library etc.

	Jakub
Richard Sandiford March 5, 2024, 7:49 p.m. UTC | #4 
Jakub Jelinek <jakub@redhat.com> writes:
> On Tue, Mar 05, 2024 at 06:30:40PM +0000, Richard Sandiford wrote:
>> (1) Keep the test where it is, taking advantage of the current SVE
>>     handling in aarch64-sve.exp, and add:
>> 
>>       /* { dg-skip-if "" { no_fsanitize_address } } */
>
> I'd go with this.  asan/ directory for test would be needed for dg-do run
> tests obviously, because then we need the test driver to add appropriate
> options to find the library etc.

Thanks, now pushed with that change.

What do you think about backports, after a baking-in period?

Richard
Jakub Jelinek March 5, 2024, 8:05 p.m. UTC | #5 
On Tue, Mar 05, 2024 at 07:49:21PM +0000, Richard Sandiford wrote:
> Jakub Jelinek <jakub@redhat.com> writes:
> > On Tue, Mar 05, 2024 at 06:30:40PM +0000, Richard Sandiford wrote:
> >> (1) Keep the test where it is, taking advantage of the current SVE
> >>     handling in aarch64-sve.exp, and add:
> >> 
> >>       /* { dg-skip-if "" { no_fsanitize_address } } */
> >
> > I'd go with this.  asan/ directory for test would be needed for dg-do run
> > tests obviously, because then we need the test driver to add appropriate
> > options to find the library etc.
> 
> Thanks, now pushed with that change.
> 
> What do you think about backports, after a baking-in period?

Looks backportable to me.

	Jakub
diff mbox series

Patch

diff --git a/gcc/asan.cc b/gcc/asan.cc
index 0fd7dd1f3ed..d621ec9c323 100644
--- a/gcc/asan.cc
+++ b/gcc/asan.cc
@@ -3795,9 +3795,7 @@  asan_expand_mark_ifn (gimple_stmt_iterator *iter)
     }
   tree len = gimple_call_arg (g, 2);
 
-  gcc_assert (tree_fits_shwi_p (len));
-  unsigned HOST_WIDE_INT size_in_bytes = tree_to_shwi (len);
-  gcc_assert (size_in_bytes);
+  gcc_assert (poly_int_tree_p (len));
 
   g = gimple_build_assign (make_ssa_name (pointer_sized_int_node),
 			   NOP_EXPR, base);
@@ -3806,9 +3804,10 @@  asan_expand_mark_ifn (gimple_stmt_iterator *iter)
   tree base_addr = gimple_assign_lhs (g);
 
   /* Generate direct emission if size_in_bytes is small.  */
-  if (size_in_bytes
-      <= (unsigned)param_use_after_scope_direct_emission_threshold)
+  unsigned threshold = param_use_after_scope_direct_emission_threshold;
+  if (tree_fits_uhwi_p (len) && tree_to_uhwi (len) <= threshold)
     {
+      unsigned HOST_WIDE_INT size_in_bytes = tree_to_uhwi (len);
       const unsigned HOST_WIDE_INT shadow_size
 	= shadow_mem_size (size_in_bytes);
       const unsigned int shadow_align
diff --git a/gcc/testsuite/gcc.target/aarch64/sve/pr97696.c b/gcc/testsuite/gcc.target/aarch64/sve/pr97696.c
new file mode 100644
index 00000000000..f533d9efc02
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/sve/pr97696.c
@@ -0,0 +1,28 @@ 
+/* { dg-options "-fsanitize=address -fsanitize-address-use-after-scope" } */
+
+#include <arm_sve.h>
+
+__attribute__((noinline, noclone)) int
+foo (char *a)
+{
+  int i, j = 0;
+  asm volatile ("" : "+r" (a) : : "memory");
+  for (i = 0; i < 12; i++)
+    j += a[i];
+  return j;
+}
+
+int
+main ()
+{
+  int i, j = 0;
+  for (i = 0; i < 4; i++)
+    {
+      char a[12];
+      __SVInt8_t freq;
+      __builtin_bcmp (&freq, a, 10);
+      __builtin_memset (a, 0, sizeof (a));
+      j += foo (a);
+    }
+  return j;
+}