Message ID | 806cc630-86ef-2e3f-f72b-68bab2cd3e86@gmail.com |
---|---|
State | New |
Headers | show |
Series | check to see if null pointer is dereferenceable [PR102630] | expand |
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.
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
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
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
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" } +}