[PATCH] check to see if null pointer is dereferenceable [PR102630]

[PATCH] check to see if null pointer is dereferenceable [PR102630]

[Martin Sebor]

[-- Attachment #1: Type: text/plain, Size: 472 bytes --]

When determining the size of an object compute_objsize_r() assumes
that addresses derived from null pointers are not derefernceable
because null pointers themselves are not, without calling
targetm.addr_space.zero_address_valid() to see if that assumption
is supported for the pointer on the target.  The attached change
adds the missing call to avoid a spurious warning in Glibc that
uses named address spaces to implement TLS on x86_64.

Tested on x86_64-linux.

Martin

[-- Attachment #2: gcc-102630.diff --]
[-- Type: text/x-patch, Size: 2825 bytes --]

Check to see if null pointer is dereferenceable [PR102630].

Resolves:
PR middle-end/102630 - Spurious -Warray-bounds with named address space

gcc/ChangeLog:

	PR middle-end/102630
	* pointer-query.cc (compute_objsize_r): Handle named address spaces.

gcc/testsuite/ChangeLog:

	PR middle-end/102630
	* gcc.dg/Warray-bounds-90.c: New test.

diff --git a/gcc/pointer-query.cc b/gcc/pointer-query.cc
index 83b1f0fc866..910f452868e 100644
--- a/gcc/pointer-query.cc
+++ b/gcc/pointer-query.cc
@@ -41,6 +41,7 @@
 #include "pointer-query.h"
 #include "tree-pretty-print.h"
 #include "tree-ssanames.h"
+#include "target.h"
 
 static bool compute_objsize_r (tree, int, access_ref *, ssa_name_limit_t &,
 			       pointer_query *);
@@ -1869,13 +1870,24 @@ compute_objsize_r (tree ptr, int ostype, access_ref *pref,
   if (code == INTEGER_CST)
     {
       /* Pointer constants other than null are most likely the result
-	 of erroneous null pointer addition/subtraction.  Set size to
-	 zero.  For null pointers, set size to the maximum for now
-	 since those may be the result of jump threading.  */
+	 of erroneous null pointer addition/subtraction.  Unless zero
+	 is a valid address set size to zero.  For null pointers, set
+	 size to the maximum for now since those may be the result of
+	 jump threading.  */
       if (integer_zerop (ptr))
 	pref->set_max_size_range ();
+      else if (POINTER_TYPE_P (TREE_TYPE (ptr)))
+	{
+	  tree deref_type = TREE_TYPE (TREE_TYPE (ptr));
+	  addr_space_t as = TYPE_ADDR_SPACE (deref_type);
+	  if (targetm.addr_space.zero_address_valid (as))
+	    pref->set_max_size_range ();
+	  else
+	    pref->sizrng[0] = pref->sizrng[1] = 0;
+	}
       else
 	pref->sizrng[0] = pref->sizrng[1] = 0;
+
       pref->ref = ptr;
 
       return true;
diff --git a/gcc/testsuite/gcc.dg/Warray-bounds-90.c b/gcc/testsuite/gcc.dg/Warray-bounds-90.c
new file mode 100644
index 00000000000..93461d4b238
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Warray-bounds-90.c
@@ -0,0 +1,31 @@
+/* PR middle-end/102630 - spurious -Warray-bounds with named address space
+   { dg-do compile }
+   { dg-options "-O2 -Wall" } */
+
+void sink (void*, ...);
+#define sink(...) sink (0, __VA_ARGS__)
+
+
+void test_generic_null (void)
+{
+  // We want some warning here but -Warray-bounds may not be it.
+  *(char*)0 = 0;              // { dg-warning "" "pr??????" { xfail *-*-* } }
+}
+
+void test_generic_cstaddr (void)
+{
+  /* We get -Warray-bounds (-Wstringop-overflow in GCC 11) but some
+     other warning might be preferable.  */
+  *(char*)1234 = 1;           // { dg-warning "" }
+}
+
+
+void test_named_address_space_null (void)
+{
+  *(char __seg_fs*)0 = 0;     // no warning (intentional)
+}
+
+void test_named_address_space_cstaddr (void)
+{
+  *(char __seg_fs*)1234 = 0;  // { dg-bogus "-Warray-bounds" }
+}

Re: [PATCH] check to see if null pointer is dereferenceable [PR102630]

[Joseph Myers]

The testcase uses the __seg_fs address space, which is x86-specific, but 
it isn't in an x86-specific directory or otherwise restricted to x86 
targets; thus, I'd expect it to fail for other architectures.

This is not a review of the rest of the patch.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] check to see if null pointer is dereferenceable [PR102630]

[Martin Sebor]

On 10/11/21 6:26 PM, Joseph Myers wrote:
> The testcase uses the __seg_fs address space, which is x86-specific, but
> it isn't in an x86-specific directory or otherwise restricted to x86
> targets; thus, I'd expect it to fail for other architectures.
> 
> This is not a review of the rest of the patch.
> 

Good point!  I thought I might make the test target-independent
(via macros) but it looks like just i386 defines the hook to
something other than false so I should probably move it under
i386.

Thanks
Martin

Re: [PATCH] check to see if null pointer is dereferenceable [PR102630]

[Richard Biener]

On Wed, Oct 13, 2021 at 3:32 AM Martin Sebor via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> On 10/11/21 6:26 PM, Joseph Myers wrote:
> > The testcase uses the __seg_fs address space, which is x86-specific, but
> > it isn't in an x86-specific directory or otherwise restricted to x86
> > targets; thus, I'd expect it to fail for other architectures.
> >
> > This is not a review of the rest of the patch.
> >
>
> Good point!  I thought I might make the test target-independent
> (via macros) but it looks like just i386 defines the hook to
> something other than false so I should probably move it under
> i386.

The patch is OK with the testcase moved.

Note I don't think we should warn about *(int *)0xdeadbee0,

       /* Pointer constants other than null are most likely the result
-        of erroneous null pointer addition/subtraction.  Set size to
-        zero.  For null pointers, set size to the maximum for now
-        since those may be the result of jump threading.  */

there's too much "may be" and "most likely" for my taste.  How can
the user mark a deliberate valid constant address?

Maybe we can use better (target dependent?) heuristic based on
what virtual addresses are likely unmapped (the zero page, the
page "before" the zero page)?

Richard.


> Thanks
> Martin

Re: [PATCH] check to see if null pointer is dereferenceable [PR102630]

[Martin Sebor]

On 10/13/21 2:25 AM, Richard Biener wrote:
> On Wed, Oct 13, 2021 at 3:32 AM Martin Sebor via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>>
>> On 10/11/21 6:26 PM, Joseph Myers wrote:
>>> The testcase uses the __seg_fs address space, which is x86-specific, but
>>> it isn't in an x86-specific directory or otherwise restricted to x86
>>> targets; thus, I'd expect it to fail for other architectures.
>>>
>>> This is not a review of the rest of the patch.
>>>
>>
>> Good point!  I thought I might make the test target-independent
>> (via macros) but it looks like just i386 defines the hook to
>> something other than false so I should probably move it under
>> i386.
> 
> The patch is OK with the testcase moved.

I changed the test, moved it under the i386 directory, and also
added -Wall to the existing addr-space-2.c, and committed
the result in r12-4376.

> 
> Note I don't think we should warn about *(int *)0xdeadbee0,
> 
>         /* Pointer constants other than null are most likely the result
> -        of erroneous null pointer addition/subtraction.  Set size to
> -        zero.  For null pointers, set size to the maximum for now
> -        since those may be the result of jump threading.  */
> 
> there's too much "may be" and "most likely" for my taste.  How can
> the user mark a deliberate valid constant address?

Using a volatile pointer works

   int* volatile p = (int*)0xdeadbee0;
   *p = 0;

but is not very elegant.  The other common solution is to make
it a variable and assign it an address in a linker script, but
that's too heavy-weight for some.

I would prefer to make AVR's attribute address generic and
encourage programmers to switch to using it to declare these
things as objects.  The major advantage is that what's at such
an address becomes a first class citizen in the type system
(and beyond), with a type and size.

> 
> Maybe we can use better (target dependent?) heuristic based on
> what virtual addresses are likely unmapped (the zero page, the
> page "before" the zero page)?

I agree we need something better: ideally, detect the null
pointer arithmetic before it's folded into a constant pointer
address.  I've opened pr102731 as a reminder.

Martin

> 
> Richard.
> 
> 
>> Thanks
>> Martin