[PATCH libatomic/arm] avoid warning on constant addresses (PR 101379)

[PATCH libatomic/arm] avoid warning on constant addresses (PR 101379)

[Martin Sebor]

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

The attached tweak avoids the new -Warray-bounds instances when
building libatomic for arm. Christophe confirms it resolves
the problem (thank you!)

As we have discussed, the main goal of this class of warnings
is to detect accesses at addresses derived from null pointers
(e.g., to struct members or array elements at a nonzero offset).
Diagnosing accesses at hardcoded addresses is incidental because
at the stage they are detected the two are not distinguishable
from each another.

I'm planning (hoping) to implement detection of invalid pointer
arithmetic involving null for GCC 12, so this patch is a stopgap
solution to unblock the arm libatomic build without compromising
the warning.  Once the new detection is in place these workarounds
can be removed or replaced with something more appropriate (e.g.,
declaring the objects at the hardwired addresses with an attribute
like AVR's address or io; that would enable bounds checking at
those addresses as well).

Martin

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

PR bootstrap/101379 - libatomic arm build failure after r12-2132 due to -Warray-bounds on a constant address

libatomic/ChangeLog:
	* /config/linux/arm/host-config.h (__kernel_helper_version): New
	function.  Adjust shadow macro.

diff --git a/libatomic/config/linux/arm/host-config.h b/libatomic/config/linux/arm/host-config.h
index 1520f237d73..777d08a2b85 100644
--- a/libatomic/config/linux/arm/host-config.h
+++ b/libatomic/config/linux/arm/host-config.h
@@ -39,8 +39,14 @@ typedef void (__kernel_dmb_t) (void);
 #define __kernel_dmb (*(__kernel_dmb_t *) 0xffff0fa0)
 
 /* Kernel helper page version number.  */
-#define __kernel_helper_version (*(unsigned int *)0xffff0ffc)
+static inline unsigned*
+__kernel_helper_version ()
+{
+  unsigned *volatile addr = (unsigned int *)0xffff0ffc;
+  return addr;
+}
 
+#define __kernel_helper_version (*__kernel_helper_version())
 
 #ifndef HAVE_STREX
 static inline bool

Re: [PATCH libatomic/arm] avoid warning on constant addresses (PR 101379)

[Christophe Lyon]

Hi,


On Sat, Jul 10, 2021 at 1:11 AM Martin Sebor via Gcc-patches <
gcc-patches@gcc.gnu.org> wrote:

> The attached tweak avoids the new -Warray-bounds instances when
> building libatomic for arm. Christophe confirms it resolves
> the problem (thank you!)
>
> As we have discussed, the main goal of this class of warnings
> is to detect accesses at addresses derived from null pointers
> (e.g., to struct members or array elements at a nonzero offset).
> Diagnosing accesses at hardcoded addresses is incidental because
> at the stage they are detected the two are not distinguishable
> from each another.
>
> I'm planning (hoping) to implement detection of invalid pointer
> arithmetic involving null for GCC 12, so this patch is a stopgap
> solution to unblock the arm libatomic build without compromising
> the warning.  Once the new detection is in place these workarounds
> can be removed or replaced with something more appropriate (e.g.,
> declaring the objects at the hardwired addresses with an attribute
> like AVR's address or io; that would enable bounds checking at
> those addresses as well).
>
>
May I ping this patch?
ARM toolchain build (cross & bootstrap) has been  broken for more
than a week, preventing regression detection.

Thanks,

Christophe

Martin
>

Re: [PATCH libatomic/arm] avoid warning on constant addresses (PR 101379)

[Thomas Schwinge]

Hi Martin!

On 2021-07-09T17:11:25-0600, Martin Sebor via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
> The attached tweak avoids the new -Warray-bounds instances when
> building libatomic for arm. Christophe confirms it resolves
> the problem (thank you!)

As Abid has just reported in
<https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101374#c16>, similar
problem with GCN target libgomp build:

    In function ‘gcn_thrs’,
        inlined from ‘gomp_thread’ at [...]/source-gcc/libgomp/libgomp.h:803:10,
        inlined from ‘GOMP_barrier’ at [...]/source-gcc/libgomp/barrier.c:34:29:
    [...]/source-gcc/libgomp/libgomp.h:792:10: error: array subscript 0 is outside array bounds of ‘__lds struct gomp_thread * __lds[0]’ [-Werror=array-bounds]
      792 |   return *thrs;
          |          ^~~~~

    gcc/config/gcn/gcn.h:  c_register_addr_space ("__lds", ADDR_SPACE_LDS);                   \

    libgomp/libgomp.h-static inline struct gomp_thread *gcn_thrs (void)
    libgomp/libgomp.h-{
    libgomp/libgomp.h-  /* The value is at the bottom of LDS.  */
    libgomp/libgomp.h:  struct gomp_thread * __lds *thrs = (struct gomp_thread * __lds *)4;
    libgomp/libgomp.h-  return *thrs;
    libgomp/libgomp.h-}

..., plus a few more.  Work-around:

       struct gomp_thread * __lds *thrs = (struct gomp_thread * __lds *)4;
    +# pragma GCC diagnostic push
    +# pragma GCC diagnostic ignored "-Warray-bounds"
       return *thrs;
    +# pragma GCC diagnostic pop

..., but it's a bit tedious to add that in all that the other places,
too.  (So I'll consider some GCN-specific '-Wno-array-bounds' if we don't
get to resolve this otherwise, soon.)

> As we have discussed, the main goal of this class of warnings
> is to detect accesses at addresses derived from null pointers
> (e.g., to struct members or array elements at a nonzero offset).

(ACK, and thanks for that work!)

> Diagnosing accesses at hardcoded addresses is incidental because
> at the stage they are detected the two are not distinguishable
> from each another.
>
> I'm planning (hoping) to implement detection of invalid pointer
> arithmetic involving null for GCC 12, so this patch is a stopgap
> solution to unblock the arm libatomic build without compromising
> the warning.  Once the new detection is in place these workarounds
> can be removed or replaced with something more appropriate (e.g.,
> declaring the objects at the hardwired addresses with an attribute
> like AVR's address or io; that would enable bounds checking at
> those addresses as well).

Of course, we may simply re-work the libgomp/GCN code -- but don't we
first need to answer the question whether the current code is actually
"bad"?  Aren't we going to get a lot of similar reports from
kernel/embedded/other low-level software developers, once this is out in
the wild?  I mean:

> PR bootstrap/101379 - libatomic arm build failure after r12-2132 due to -Warray-bounds on a constant address
>
> libatomic/ChangeLog:
>       * /config/linux/arm/host-config.h (__kernel_helper_version): New
>       function.  Adjust shadow macro.
>
> diff --git a/libatomic/config/linux/arm/host-config.h b/libatomic/config/linux/arm/host-config.h
> index 1520f237d73..777d08a2b85 100644
> --- a/libatomic/config/linux/arm/host-config.h
> +++ b/libatomic/config/linux/arm/host-config.h
> @@ -39,8 +39,14 @@ typedef void (__kernel_dmb_t) (void);
>  #define __kernel_dmb (*(__kernel_dmb_t *) 0xffff0fa0)
>
>  /* Kernel helper page version number.  */
> -#define __kernel_helper_version (*(unsigned int *)0xffff0ffc)

Are such (not un-common) '#define's actually "bad", and anyhow ought to
be replaced by something like the following?

> +static inline unsigned*
> +__kernel_helper_version ()
> +{
> +  unsigned *volatile addr = (unsigned int *)0xffff0ffc;
> +  return addr;
> +}
>
> +#define __kernel_helper_version (*__kernel_helper_version())

(No 'volatile' in the original code, by the way.)


Grüße
 Thomas
-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955

Re: [PATCH libatomic/arm] avoid warning on constant addresses (PR 101379)

[Martin Sebor]

On 7/16/21 11:42 AM, Thomas Schwinge wrote:
> Hi Martin!
> 
> On 2021-07-09T17:11:25-0600, Martin Sebor via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
>> The attached tweak avoids the new -Warray-bounds instances when
>> building libatomic for arm. Christophe confirms it resolves
>> the problem (thank you!)
> 
> As Abid has just reported in
> <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101374#c16>, similar
> problem with GCN target libgomp build:
> 
>      In function ‘gcn_thrs’,
>          inlined from ‘gomp_thread’ at [...]/source-gcc/libgomp/libgomp.h:803:10,
>          inlined from ‘GOMP_barrier’ at [...]/source-gcc/libgomp/barrier.c:34:29:
>      [...]/source-gcc/libgomp/libgomp.h:792:10: error: array subscript 0 is outside array bounds of ‘__lds struct gomp_thread * __lds[0]’ [-Werror=array-bounds]
>        792 |   return *thrs;
>            |          ^~~~~
> 
>      gcc/config/gcn/gcn.h:  c_register_addr_space ("__lds", ADDR_SPACE_LDS);                   \
> 
>      libgomp/libgomp.h-static inline struct gomp_thread *gcn_thrs (void)
>      libgomp/libgomp.h-{
>      libgomp/libgomp.h-  /* The value is at the bottom of LDS.  */
>      libgomp/libgomp.h:  struct gomp_thread * __lds *thrs = (struct gomp_thread * __lds *)4;
>      libgomp/libgomp.h-  return *thrs;
>      libgomp/libgomp.h-}
> 
> ..., plus a few more.  Work-around:
> 
>         struct gomp_thread * __lds *thrs = (struct gomp_thread * __lds *)4;
>      +# pragma GCC diagnostic push
>      +# pragma GCC diagnostic ignored "-Warray-bounds"
>         return *thrs;
>      +# pragma GCC diagnostic pop
> 
> ..., but it's a bit tedious to add that in all that the other places,
> too.  (So I'll consider some GCN-specific '-Wno-array-bounds' if we don't
> get to resolve this otherwise, soon.)
> 
>> As we have discussed, the main goal of this class of warnings
>> is to detect accesses at addresses derived from null pointers
>> (e.g., to struct members or array elements at a nonzero offset).
> 
> (ACK, and thanks for that work!)
> 
>> Diagnosing accesses at hardcoded addresses is incidental because
>> at the stage they are detected the two are not distinguishable
>> from each another.
>>
>> I'm planning (hoping) to implement detection of invalid pointer
>> arithmetic involving null for GCC 12, so this patch is a stopgap
>> solution to unblock the arm libatomic build without compromising
>> the warning.  Once the new detection is in place these workarounds
>> can be removed or replaced with something more appropriate (e.g.,
>> declaring the objects at the hardwired addresses with an attribute
>> like AVR's address or io; that would enable bounds checking at
>> those addresses as well).
> 
> Of course, we may simply re-work the libgomp/GCN code -- but don't we
> first need to answer the question whether the current code is actually
> "bad"?  Aren't we going to get a lot of similar reports from
> kernel/embedded/other low-level software developers, once this is out in
> the wild?  I mean:
> 
>> PR bootstrap/101379 - libatomic arm build failure after r12-2132 due to -Warray-bounds on a constant address
>>
>> libatomic/ChangeLog:
>>        * /config/linux/arm/host-config.h (__kernel_helper_version): New
>>        function.  Adjust shadow macro.
>>
>> diff --git a/libatomic/config/linux/arm/host-config.h b/libatomic/config/linux/arm/host-config.h
>> index 1520f237d73..777d08a2b85 100644
>> --- a/libatomic/config/linux/arm/host-config.h
>> +++ b/libatomic/config/linux/arm/host-config.h
>> @@ -39,8 +39,14 @@ typedef void (__kernel_dmb_t) (void);
>>   #define __kernel_dmb (*(__kernel_dmb_t *) 0xffff0fa0)
>>
>>   /* Kernel helper page version number.  */
>> -#define __kernel_helper_version (*(unsigned int *)0xffff0ffc)
> 
> Are such (not un-common) '#define's actually "bad", and anyhow ought to
> be replaced by something like the following?

Like all warnings (and especially flow-based ones that depend on
optimization), this one too involves a trade-off between noise and
real bugs.  There clearly is some low-level code that intentionally
accesses memory at hardcoded addresses.  But because null pointers
are pervasive, there's a lot more code that could end up accessing
data at some offset from zero by accident (e.g., by writing to
an array element or a member of a struct).  This affects all code,
but is an especially big concern for privileged code that can access
all memory.  So in my view, the trade-off is worthwhile.

The logic the warning relies on isn't new: it was introduced in GCC
11.  There have been a handful of reports of this issue (some from
the kernel) but far fewer than in other warnings.  The recent change
expose more code to the logic so the numbers of both false and true
positives are bound to go up, in proportion.  Hopefully, before GCC
12 is released, I will have a more robust solution to the null+offset
problem.

> 
>> +static inline unsigned*
>> +__kernel_helper_version ()
>> +{
>> +  unsigned *volatile addr = (unsigned int *)0xffff0ffc;
>> +  return addr;
>> +}
>>
>> +#define __kernel_helper_version (*__kernel_helper_version())
> 
> (No 'volatile' in the original code, by the way.)

The volatile is what prevents the warning.  But I think a better
solution than the hack above is to introduce a named extern const
variable for the address.  It avoids the issue without the penalty
of multiple volatile accesses and if/when an attribute like AVR
address is introduced it can be more easily adapted to it.  Real
object declarations with an attribute is also a more appropriate
mechanism than using hardcoded address in pointers.

Martin

Re: [PATCH libatomic/arm] avoid warning on constant addresses (PR 101379)

[Andrew Stubbs]

On 16/07/2021 18:42, Thomas Schwinge wrote:
> Of course, we may simply re-work the libgomp/GCN code -- but don't we
> first need to answer the question whether the current code is actually
> "bad"?  Aren't we going to get a lot of similar reports from
> kernel/embedded/other low-level software developers, once this is out in
> the wild?  I mean:

GCN already uses address 4 for this value because address 0 caused 
problems with null-pointer checks.

It really ought not be this hard. :-(

Andrew

[gcn] Work-around libgomp 'error: array subscript 0 is outside array bounds of ‘__lds struct gomp_thread * __lds[0]’ [-Werror=array-bounds]' (was: [PATCH libatomic/arm] avoid warning on constant addresses (PR 101379))

[Thomas Schwinge]

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

Hi!

| On 7/16/21 11:42 AM, Thomas Schwinge wrote:
|> On 2021-07-09T17:11:25-0600, Martin Sebor via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
|>> The attached tweak avoids the new -Warray-bounds instances when
|>> building libatomic for arm. Christophe confirms it resolves
|>> the problem (thank you!)
|>
|> As Abid has just reported in
|> <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101374#c16>, similar
|> problem with GCN target libgomp build:
|>
|>      In function ‘gcn_thrs’,
|>          inlined from ‘gomp_thread’ at [...]/source-gcc/libgomp/libgomp.h:803:10,
|>          inlined from ‘GOMP_barrier’ at [...]/source-gcc/libgomp/barrier.c:34:29:
|>      [...]/source-gcc/libgomp/libgomp.h:792:10: error: array subscript 0 is outside array bounds of ‘__lds struct gomp_thread * __lds[0]’ [-Werror=array-bounds]
|>        792 |   return *thrs;
|>            |          ^~~~~
|>
|>      gcc/config/gcn/gcn.h:  c_register_addr_space ("__lds", ADDR_SPACE_LDS);                   \
|>
|>      libgomp/libgomp.h-static inline struct gomp_thread *gcn_thrs (void)
|>      libgomp/libgomp.h-{
|>      libgomp/libgomp.h-  /* The value is at the bottom of LDS.  */
|>      libgomp/libgomp.h:  struct gomp_thread * __lds *thrs = (struct gomp_thread * __lds *)4;
|>      libgomp/libgomp.h-  return *thrs;
|>      libgomp/libgomp.h-}
|>
|> ..., plus a few more.  Work-around:
|>
|>         struct gomp_thread * __lds *thrs = (struct gomp_thread * __lds *)4;
|>      +# pragma GCC diagnostic push
|>      +# pragma GCC diagnostic ignored "-Warray-bounds"
|>         return *thrs;
|>      +# pragma GCC diagnostic pop
|>
|> ..., but it's a bit tedious to add that in all that the other places,
|> too.

Wasn't so bad after all; a lot of duplicates due to 'libgomp.h'.  I've
thus pushed "[gcn] Work-around libgomp 'error: array subscript 0 is
outside array bounds of ‘__lds struct gomp_thread * __lds[0]’
[-Werror=array-bounds]' [PR101484]" to master branch in commit
9f2bc5077debef2b046b6c10d38591ac324ad8b5, see attached.

|> (So I'll consider some GCN-specific '-Wno-array-bounds' if we don't
|> get to resolve this otherwise, soon.)

Now: "Awaiting a different solution, of course."  ;-)


On 2021-07-17T23:28:45+0100, Andrew Stubbs <ams@codesourcery.com> wrote:
> On 16/07/2021 18:42, Thomas Schwinge wrote:
>> Of course, we may simply re-work the libgomp/GCN code -- but don't we
>> first need to answer the question whether the current code is actually
>> "bad"?  Aren't we going to get a lot of similar reports from
>> kernel/embedded/other low-level software developers, once this is out in
>> the wild?  I mean:
>
> GCN already uses address 4 for this value because address 0 caused
> problems with null-pointer checks.

Ugh.  How much wasted bytes per what is that?  (I haven't looked yet;
hopefully not per GPU thread?)  Because:

> It really ought not be this hard. :-(

I'm sure we can avoid that.


Grüße
 Thomas


-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-gcn-Work-around-libgomp-error-array-subscript-0-is-o.patch --]
[-- Type: text/x-diff, Size: 3543 bytes --]

From 9f2bc5077debef2b046b6c10d38591ac324ad8b5 Mon Sep 17 00:00:00 2001
From: Thomas Schwinge <thomas@codesourcery.com>
Date: Fri, 16 Jul 2021 19:12:02 +0200
Subject: [PATCH] =?UTF-8?q?[gcn]=20Work-around=20libgomp=20'error:=20array?=
 =?UTF-8?q?=20subscript=200=20is=20outside=20array=20bounds=20of=20?=
 =?UTF-8?q?=E2=80=98=5F=5Flds=20struct=20gomp=5Fthread=20*=20=5F=5Flds[0]?=
 =?UTF-8?q?=E2=80=99=20[-Werror=3Darray-bounds]'=20[PR101484]?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

... seen as of commit a110855667782dac7b674d3e328b253b3b3c919b "Correct
handling of variable offset minus constant in -Warray-bounds [PR100137]".

Awaiting a different solution, of course.

	libgomp/
	PR target/101484
	* config/gcn/team.c: Apply '-Werror=array-bounds' work-around.
	* libgomp.h [__AMDGCN__]: Likewise.
---
 libgomp/config/gcn/team.c |  3 +++
 libgomp/libgomp.h         | 12 ++++++++++++
 2 files changed, 15 insertions(+)

diff --git a/libgomp/config/gcn/team.c b/libgomp/config/gcn/team.c
index 627210ea407..94ce2f2dfeb 100644
--- a/libgomp/config/gcn/team.c
+++ b/libgomp/config/gcn/team.c
@@ -65,9 +65,12 @@ gomp_gcn_enter_kernel (void)
       void * __lds *arena_start = (void * __lds *)TEAM_ARENA_START;
       void * __lds *arena_free = (void * __lds *)TEAM_ARENA_FREE;
       void * __lds *arena_end = (void * __lds *)TEAM_ARENA_END;
+# pragma GCC diagnostic push
+# pragma GCC diagnostic ignored "-Warray-bounds" /*TODO PR101484 */
       *arena_start = team_arena;
       *arena_free = team_arena;
       *arena_end = team_arena + TEAM_ARENA_SIZE;
+# pragma GCC diagnostic pop
 
       /* Allocate and initialize the team-local-storage data.  */
       struct gomp_thread *thrs = team_malloc_cleared (sizeof (*thrs)
diff --git a/libgomp/libgomp.h b/libgomp/libgomp.h
index 8d25dc8e2a8..4159cbe3334 100644
--- a/libgomp/libgomp.h
+++ b/libgomp/libgomp.h
@@ -128,7 +128,10 @@ team_malloc (size_t size)
        : "=v"(result) : "v"(TEAM_ARENA_FREE), "v"(size), "e"(1L) : "memory");
 
   /* Handle OOM.  */
+# pragma GCC diagnostic push
+# pragma GCC diagnostic ignored "-Warray-bounds" /*TODO PR101484 */
   if (result + size > *(void * __lds *)TEAM_ARENA_END)
+# pragma GCC diagnostic pop
     {
       /* While this is experimental, let's make sure we know when OOM
 	 happens.  */
@@ -159,8 +162,11 @@ team_free (void *ptr)
      However, if we fell back to using heap then we should free it.
      It would be better if this function could be a no-op, but at least
      LDS loads are cheap.  */
+# pragma GCC diagnostic push
+# pragma GCC diagnostic ignored "-Warray-bounds" /*TODO PR101484 */
   if (ptr < *(void * __lds *)TEAM_ARENA_START
       || ptr >= *(void * __lds *)TEAM_ARENA_END)
+# pragma GCC diagnostic pop
     free (ptr);
 }
 #else
@@ -789,13 +795,19 @@ static inline struct gomp_thread *gcn_thrs (void)
 {
   /* The value is at the bottom of LDS.  */
   struct gomp_thread * __lds *thrs = (struct gomp_thread * __lds *)4;
+# pragma GCC diagnostic push
+# pragma GCC diagnostic ignored "-Warray-bounds" /*TODO PR101484 */
   return *thrs;
+# pragma GCC diagnostic pop
 }
 static inline void set_gcn_thrs (struct gomp_thread *val)
 {
   /* The value is at the bottom of LDS.  */
   struct gomp_thread * __lds *thrs = (struct gomp_thread * __lds *)4;
+# pragma GCC diagnostic push
+# pragma GCC diagnostic ignored "-Warray-bounds" /*TODO PR101484 */
   *thrs = val;
+# pragma GCC diagnostic pop
 }
 static inline struct gomp_thread *gomp_thread (void)
 {
-- 
2.30.2

Re: [PATCH libatomic/arm] avoid warning on constant addresses (PR 101379)

[Thomas Schwinge]

Hi!

On 2021-07-16T15:11:24-0600, Martin Sebor via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
> On 7/16/21 11:42 AM, Thomas Schwinge wrote:
>> On 2021-07-09T17:11:25-0600, Martin Sebor via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
>>> The attached tweak avoids the new -Warray-bounds instances when
>>> building libatomic for arm. Christophe confirms it resolves
>>> the problem (thank you!)

>>> As we have discussed, the main goal of this class of warnings
>>> is to detect accesses at addresses derived from null pointers
>>> (e.g., to struct members or array elements at a nonzero offset).
>>
>> (ACK, and thanks for that work!)
>>
>>> Diagnosing accesses at hardcoded addresses is incidental because
>>> at the stage they are detected the two are not distinguishable
>>> from each another.
>>>
>>> I'm planning (hoping) to implement detection of invalid pointer
>>> arithmetic involving null for GCC 12, so this patch is a stopgap
>>> solution to unblock the arm libatomic build without compromising
>>> the warning.  Once the new detection is in place these workarounds
>>> can be removed or replaced with something more appropriate (e.g.,
>>> declaring the objects at the hardwired addresses with an attribute
>>> like AVR's address or io; that would enable bounds checking at
>>> those addresses as well).
>>
>> Of course, we may simply re-work the libgomp/GCN code -- but don't we
>> first need to answer the question whether the current code is actually
>> "bad"?  Aren't we going to get a lot of similar reports from
>> kernel/embedded/other low-level software developers, once this is out in
>> the wild?  I mean:
>>
>>> PR bootstrap/101379 - libatomic arm build failure after r12-2132 due to -Warray-bounds on a constant address
>>>
>>> libatomic/ChangeLog:
>>>        * /config/linux/arm/host-config.h (__kernel_helper_version): New
>>>        function.  Adjust shadow macro.
>>>
>>> diff --git a/libatomic/config/linux/arm/host-config.h b/libatomic/config/linux/arm/host-config.h
>>> index 1520f237d73..777d08a2b85 100644
>>> --- a/libatomic/config/linux/arm/host-config.h
>>> +++ b/libatomic/config/linux/arm/host-config.h
>>> @@ -39,8 +39,14 @@ typedef void (__kernel_dmb_t) (void);
>>>   #define __kernel_dmb (*(__kernel_dmb_t *) 0xffff0fa0)
>>>
>>>   /* Kernel helper page version number.  */
>>> -#define __kernel_helper_version (*(unsigned int *)0xffff0ffc)
>>
>> Are such (not un-common) '#define's actually "bad", and anyhow ought to
>> be replaced by something like the following?
>
> Like all warnings (and especially flow-based ones that depend on
> optimization), this one too involves a trade-off between noise and
> real bugs.  There clearly is some low-level code that intentionally
> accesses memory at hardcoded addresses.  But because null pointers
> are pervasive, there's a lot more code that could end up accessing
> data at some offset from zero by accident (e.g., by writing to
> an array element or a member of a struct).  This affects all code,
> but is an especially big concern for privileged code that can access
> all memory.  So in my view, the trade-off is worthwhile.
>
> The logic the warning relies on isn't new: it was introduced in GCC
> 11.  There have been a handful of reports of this issue (some from
> the kernel) but far fewer than in other warnings.  The recent change
> expose more code to the logic so the numbers of both false and true
> positives are bound to go up, in proportion.  Hopefully, before GCC
> 12 is released, I will have a more robust solution to the null+offset
> problem.

Understood.  And I'll certainly be happy to see all these instances of
hard-coded-address-with-pointer-punning be re-written into "something
proper".  I was just wary of the many instances out there.  (But of
course, a lot of people don't pay attention to compiler diagnostics
anyway, so...)  ;-|

>>> +static inline unsigned*
>>> +__kernel_helper_version ()
>>> +{
>>> +  unsigned *volatile addr = (unsigned int *)0xffff0ffc;
>>> +  return addr;
>>> +}
>>>
>>> +#define __kernel_helper_version (*__kernel_helper_version())
>>
>> (No 'volatile' in the original code, by the way.)
>
> The volatile is what prevents the warning.

Uhm, but isn't that then a behavioral change (potentially performance
impact)?  That wasn't obvious from the patch posted.  (Not my area of
interest, though, just noting.)

> But I think a better
> solution than the hack above is to introduce a named extern const
> variable for the address.  It avoids the issue without the penalty
> of multiple volatile accesses and if/when an attribute like AVR
> address is introduced it can be more easily adapted to it.  Real
> object declarations with an attribute is also a more appropriate
> mechanism than using hardcoded address in pointers.

Sounds plausible, yes.


Grüße
 Thomas
-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955

Re: [gcn] Work-around libgomp 'error: array subscript 0 is outside array bounds of ‘__lds struct gomp_thread * __lds[0]’ [-Werror=array-bounds]' (was: [PATCH libatomic/arm] avoid warning on constant addresses (PR 101379))

[Jakub Jelinek]

On Mon, Jul 19, 2021 at 10:46:35AM +0200, Thomas Schwinge wrote:
> |> (So I'll consider some GCN-specific '-Wno-array-bounds' if we don't
> |> get to resolve this otherwise, soon.)
> 
> Now: "Awaiting a different solution, of course."  ;-)

The current state of the warning is unacceptable.  IMNSHO it should stop
warning for non-generic address spaces and for generic address space for
constants larger or equal to typical page size with
-fdelete-null-pointer-checks and not at all with -fno-delete-null-pointer-checks.

Large structures are rare and using *(type *)0x12340000 is very common
mostly in the embedded world, but with mmap MAP_FIXED also used in
non-embedded world.

We don't want all the people out there to add too many workarounds for badly
designed warning.

	Jakub

Re: [gcn] Work-around libgomp 'error: array subscript 0 is outside array bounds of ‘__lds struct gomp_thread * __lds[0]’ [-Werror=array-bounds]' (was: [PATCH libatomic/arm] avoid warning on constant addresses (PR 101379))

[Andrew Stubbs]

On 19/07/2021 09:46, Thomas Schwinge wrote:
>> GCN already uses address 4 for this value because address 0 caused
>> problems with null-pointer checks.
> 
> Ugh.  How much wasted bytes per what is that?  (I haven't looked yet;
> hopefully not per GPU thread?)  Because:

It's 4 bytes per gang. And that pointer is the only 8 bytes in the whole 
of LDS (OpenMP mostly uses stack and heap), so it's not so bad, but still.

I did investigate the target macro that lets you control null pointer 
behaviour, but it didn't just work, and it wasn't important enough for 
me to spend more time on it so I let it go.

Andrew

Re: [gcn] Work-around libgomp 'error: array subscript 0 is outside array bounds of ‘__lds struct gomp_thread * __lds[0]’ [-Werror=array-bounds]'

[Thomas Schwinge]

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

Hi!

On 2021-07-19T10:46:35+0200, I wrote:
> | On 7/16/21 11:42 AM, Thomas Schwinge wrote:
> |> On 2021-07-09T17:11:25-0600, Martin Sebor via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
> |>> The attached tweak avoids the new -Warray-bounds instances when
> |>> building libatomic for arm. Christophe confirms it resolves
> |>> the problem (thank you!)
> |>
> |> As Abid has just reported in
> |> <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101374#c16>, similar
> |> problem with GCN target libgomp build:
> |>
> |>      In function ‘gcn_thrs’,
> |>          inlined from ‘gomp_thread’ at [...]/source-gcc/libgomp/libgomp.h:803:10,
> |>          inlined from ‘GOMP_barrier’ at [...]/source-gcc/libgomp/barrier.c:34:29:
> |>      [...]/source-gcc/libgomp/libgomp.h:792:10: error: array subscript 0 is outside array bounds of ‘__lds struct gomp_thread * __lds[0]’ [-Werror=array-bounds]
> |>        792 |   return *thrs;
> |>            |          ^~~~~
> |>
> |>      gcc/config/gcn/gcn.h:  c_register_addr_space ("__lds", ADDR_SPACE_LDS);                   \
> |>
> |>      libgomp/libgomp.h-static inline struct gomp_thread *gcn_thrs (void)
> |>      libgomp/libgomp.h-{
> |>      libgomp/libgomp.h-  /* The value is at the bottom of LDS.  */
> |>      libgomp/libgomp.h:  struct gomp_thread * __lds *thrs = (struct gomp_thread * __lds *)4;
> |>      libgomp/libgomp.h-  return *thrs;
> |>      libgomp/libgomp.h-}
> |>
> |> ..., plus a few more.  Work-around:
> |>
> |>         struct gomp_thread * __lds *thrs = (struct gomp_thread * __lds *)4;
> |>      +# pragma GCC diagnostic push
> |>      +# pragma GCC diagnostic ignored "-Warray-bounds"
> |>         return *thrs;
> |>      +# pragma GCC diagnostic pop
> |>
> |> ..., but it's a bit tedious to add that in all that the other places,
> |> too.
>
> Wasn't so bad after all; a lot of duplicates due to 'libgomp.h'.  I've
> thus pushed "[gcn] Work-around libgomp 'error: array subscript 0 is
> outside array bounds of ‘__lds struct gomp_thread * __lds[0]’
> [-Werror=array-bounds]' [PR101484]" to master branch in commit
> 9f2bc5077debef2b046b6c10d38591ac324ad8b5, see attached.

As I should find, these '#pragma GCC diagnostic [...]' directives cause
some code generation changes (that seems unexpected, problematic!).
(Martin, any idea?  Might be a pre-existing problem, of course.)  This
results in a lot (ten thousands) of 'GCN team arena exhausted' run-time
diagnostics, also leading to a few FAILs:

    PASS: libgomp.c/../libgomp.c-c++-common/for-11.c (test for excess errors)
    [-PASS:-]{+FAIL:+} libgomp.c/../libgomp.c-c++-common/for-11.c execution test

    PASS: libgomp.c/../libgomp.c-c++-common/for-12.c (test for excess errors)
    [-PASS:-]{+FAIL:+} libgomp.c/../libgomp.c-c++-common/for-12.c execution test

    PASS: libgomp.c/../libgomp.c-c++-common/for-3.c (test for excess errors)
    [-PASS:-]{+FAIL:+} libgomp.c/../libgomp.c-c++-common/for-3.c execution test

    PASS: libgomp.c/../libgomp.c-c++-common/for-5.c (test for excess errors)
    [-PASS:-]{+FAIL:+} libgomp.c/../libgomp.c-c++-common/for-5.c execution test

    PASS: libgomp.c/../libgomp.c-c++-common/for-6.c (test for excess errors)
    [-PASS:-]{+FAIL:+} libgomp.c/../libgomp.c-c++-common/for-6.c execution test

    PASS: libgomp.c/../libgomp.c-c++-common/for-9.c (test for excess errors)
    [-PASS:-]{+FAIL:+} libgomp.c/../libgomp.c-c++-common/for-9.c execution test

Same for 'libgomp.c++'.

It remains to be analyzed how '#pragma GCC diagnostic [...]' directives
can cause code generation changes; for now I'm working around the
"unexpected" '-Werror=array-bounds' diagnostics differently:

> |> (So I'll consider some GCN-specific '-Wno-array-bounds' if we don't
> |> get to resolve this otherwise, soon.)

'-Wno-error=array-bounds', precisely.  I've now pushed "[gcn]
Work-around libgomp 'error: array subscript 0 is outside array bounds of
‘__lds struct gomp_thread * __lds[0]’ [-Werror=array-bounds]' some more
[PR101484]" to master branch in commit
8168338684fc2bed576bb09202c63b3e9e678d92, see attached.


Grüße
 Thomas


-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955

[-- Attachment #2: 0001-gcn-Work-around-libgomp-error-array-subscript-0-is-o.patch --]
[-- Type: text/x-diff, Size: 5890 bytes --]

From 8168338684fc2bed576bb09202c63b3e9e678d92 Mon Sep 17 00:00:00 2001
From: Thomas Schwinge <thomas@codesourcery.com>
Date: Mon, 19 Jul 2021 23:11:38 +0200
Subject: [PATCH] =?UTF-8?q?[gcn]=20Work-around=20libgomp=20'error:=20array?=
 =?UTF-8?q?=20subscript=200=20is=20outside=20array=20bounds=20of=20?=
 =?UTF-8?q?=E2=80=98=5F=5Flds=20struct=20gomp=5Fthread=20*=20=5F=5Flds[0]?=
 =?UTF-8?q?=E2=80=99=20[-Werror=3Darray-bounds]'=20some=20more=20[PR101484?=
 =?UTF-8?q?]?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

With yesterday's commit 9f2bc5077debef2b046b6c10d38591ac324ad8b5 "[gcn]
Work-around libgomp 'error: array subscript 0 is outside array bounds of
‘__lds struct gomp_thread * __lds[0]’ [-Werror=array-bounds]' [PR101484]",
I did defuse the "unexpected" '-Werror=array-bounds' diagnostics that we see
as of commit a110855667782dac7b674d3e328b253b3b3c919b "Correct handling of
variable offset minus constant in -Warray-bounds [PR100137]".  However, these
'#pragma GCC diagnostic [...]' directives cause some code generation changes
(that seems unexpected, problematic!), which results in a lot (ten thousands)
of 'GCN team arena exhausted' run-time diagnostics, also leading to a few
FAILs:

    PASS: libgomp.c/../libgomp.c-c++-common/for-11.c (test for excess errors)
    [-PASS:-]{+FAIL:+} libgomp.c/../libgomp.c-c++-common/for-11.c execution test

    PASS: libgomp.c/../libgomp.c-c++-common/for-12.c (test for excess errors)
    [-PASS:-]{+FAIL:+} libgomp.c/../libgomp.c-c++-common/for-12.c execution test

    PASS: libgomp.c/../libgomp.c-c++-common/for-3.c (test for excess errors)
    [-PASS:-]{+FAIL:+} libgomp.c/../libgomp.c-c++-common/for-3.c execution test

    PASS: libgomp.c/../libgomp.c-c++-common/for-5.c (test for excess errors)
    [-PASS:-]{+FAIL:+} libgomp.c/../libgomp.c-c++-common/for-5.c execution test

    PASS: libgomp.c/../libgomp.c-c++-common/for-6.c (test for excess errors)
    [-PASS:-]{+FAIL:+} libgomp.c/../libgomp.c-c++-common/for-6.c execution test

    PASS: libgomp.c/../libgomp.c-c++-common/for-9.c (test for excess errors)
    [-PASS:-]{+FAIL:+} libgomp.c/../libgomp.c-c++-common/for-9.c execution test

Same for 'libgomp.c++'.

It remains to be analyzed how '#pragma GCC diagnostic [...]' directives can
cause code generation changes; for now I'm working around the "unexpected"
'-Werror=array-bounds' diagnostics differently.

Overall, still awaiting a different solution, of course.

	libgomp/
	PR target/101484
	* configure.tgt [amdgcn*-*-*] (XCFLAGS): Add
	'-Wno-error=array-bounds'.
	* config/gcn/team.c: Remove '-Werror=array-bounds' work-around.
	* libgomp.h [__AMDGCN__]: Likewise.
---
 libgomp/config/gcn/team.c |  3 ---
 libgomp/configure.tgt     |  3 +++
 libgomp/libgomp.h         | 12 ------------
 3 files changed, 3 insertions(+), 15 deletions(-)

diff --git a/libgomp/config/gcn/team.c b/libgomp/config/gcn/team.c
index 94ce2f2dfeb..627210ea407 100644
--- a/libgomp/config/gcn/team.c
+++ b/libgomp/config/gcn/team.c
@@ -65,12 +65,9 @@ gomp_gcn_enter_kernel (void)
       void * __lds *arena_start = (void * __lds *)TEAM_ARENA_START;
       void * __lds *arena_free = (void * __lds *)TEAM_ARENA_FREE;
       void * __lds *arena_end = (void * __lds *)TEAM_ARENA_END;
-# pragma GCC diagnostic push
-# pragma GCC diagnostic ignored "-Warray-bounds" /*TODO PR101484 */
       *arena_start = team_arena;
       *arena_free = team_arena;
       *arena_end = team_arena + TEAM_ARENA_SIZE;
-# pragma GCC diagnostic pop
 
       /* Allocate and initialize the team-local-storage data.  */
       struct gomp_thread *thrs = team_malloc_cleared (sizeof (*thrs)
diff --git a/libgomp/configure.tgt b/libgomp/configure.tgt
index fe2bf1dac51..d4f1e741b5a 100644
--- a/libgomp/configure.tgt
+++ b/libgomp/configure.tgt
@@ -173,6 +173,9 @@ case "${target}" in
 
   amdgcn*-*-*)
 	config_path="gcn accel"
+
+	#TODO PR101484
+	XCFLAGS="$XCFLAGS -Wno-error=array-bounds"
 	;;
 
   *)
diff --git a/libgomp/libgomp.h b/libgomp/libgomp.h
index 4159cbe3334..8d25dc8e2a8 100644
--- a/libgomp/libgomp.h
+++ b/libgomp/libgomp.h
@@ -128,10 +128,7 @@ team_malloc (size_t size)
        : "=v"(result) : "v"(TEAM_ARENA_FREE), "v"(size), "e"(1L) : "memory");
 
   /* Handle OOM.  */
-# pragma GCC diagnostic push
-# pragma GCC diagnostic ignored "-Warray-bounds" /*TODO PR101484 */
   if (result + size > *(void * __lds *)TEAM_ARENA_END)
-# pragma GCC diagnostic pop
     {
       /* While this is experimental, let's make sure we know when OOM
 	 happens.  */
@@ -162,11 +159,8 @@ team_free (void *ptr)
      However, if we fell back to using heap then we should free it.
      It would be better if this function could be a no-op, but at least
      LDS loads are cheap.  */
-# pragma GCC diagnostic push
-# pragma GCC diagnostic ignored "-Warray-bounds" /*TODO PR101484 */
   if (ptr < *(void * __lds *)TEAM_ARENA_START
       || ptr >= *(void * __lds *)TEAM_ARENA_END)
-# pragma GCC diagnostic pop
     free (ptr);
 }
 #else
@@ -795,19 +789,13 @@ static inline struct gomp_thread *gcn_thrs (void)
 {
   /* The value is at the bottom of LDS.  */
   struct gomp_thread * __lds *thrs = (struct gomp_thread * __lds *)4;
-# pragma GCC diagnostic push
-# pragma GCC diagnostic ignored "-Warray-bounds" /*TODO PR101484 */
   return *thrs;
-# pragma GCC diagnostic pop
 }
 static inline void set_gcn_thrs (struct gomp_thread *val)
 {
   /* The value is at the bottom of LDS.  */
   struct gomp_thread * __lds *thrs = (struct gomp_thread * __lds *)4;
-# pragma GCC diagnostic push
-# pragma GCC diagnostic ignored "-Warray-bounds" /*TODO PR101484 */
   *thrs = val;
-# pragma GCC diagnostic pop
 }
 static inline struct gomp_thread *gomp_thread (void)
 {
-- 
2.30.2

'#pragma GCC diagnostic' (mis-)use in 'statement' of 'if' (was: [gcn] Work-around libgomp 'error: array subscript 0 is outside array bounds of ‘__lds struct gomp_thread * __lds[0]’ [-Werror=array-bounds]')

[Thomas Schwinge]

Hi!

On 2021-07-20T09:23:24+0200, I wrote:
> On 2021-07-19T10:46:35+0200, I wrote:
>> | On 7/16/21 11:42 AM, Thomas Schwinge wrote:
>> |> On 2021-07-09T17:11:25-0600, Martin Sebor via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
>> |>> The attached tweak avoids the new -Warray-bounds instances when
>> |>> building libatomic for arm. Christophe confirms it resolves
>> |>> the problem (thank you!)
>> |>
>> |> As Abid has just reported in
>> |> <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101374#c16>, similar
>> |> problem with GCN target libgomp build:
>> |>
>> |>      In function ‘gcn_thrs’,
>> |>          inlined from ‘gomp_thread’ at [...]/source-gcc/libgomp/libgomp.h:803:10,
>> |>          inlined from ‘GOMP_barrier’ at [...]/source-gcc/libgomp/barrier.c:34:29:
>> |>      [...]/source-gcc/libgomp/libgomp.h:792:10: error: array subscript 0 is outside array bounds of ‘__lds struct gomp_thread * __lds[0]’ [-Werror=array-bounds]
>> |>        792 |   return *thrs;
>> |>            |          ^~~~~
>> |>
>> |>      gcc/config/gcn/gcn.h:  c_register_addr_space ("__lds", ADDR_SPACE_LDS);                   \
>> |>
>> |>      libgomp/libgomp.h-static inline struct gomp_thread *gcn_thrs (void)
>> |>      libgomp/libgomp.h-{
>> |>      libgomp/libgomp.h-  /* The value is at the bottom of LDS.  */
>> |>      libgomp/libgomp.h:  struct gomp_thread * __lds *thrs = (struct gomp_thread * __lds *)4;
>> |>      libgomp/libgomp.h-  return *thrs;
>> |>      libgomp/libgomp.h-}
>> |>
>> |> ..., plus a few more.  Work-around:
>> |>
>> |>         struct gomp_thread * __lds *thrs = (struct gomp_thread * __lds *)4;
>> |>      +# pragma GCC diagnostic push
>> |>      +# pragma GCC diagnostic ignored "-Warray-bounds"
>> |>         return *thrs;
>> |>      +# pragma GCC diagnostic pop
>> |>
>> |> ..., but it's a bit tedious to add that in all that the other places,
>> |> too.
>>
>> Wasn't so bad after all; a lot of duplicates due to 'libgomp.h'.  I've
>> thus pushed "[gcn] Work-around libgomp 'error: array subscript 0 is
>> outside array bounds of ‘__lds struct gomp_thread * __lds[0]’
>> [-Werror=array-bounds]' [PR101484]" to master branch in commit
>> 9f2bc5077debef2b046b6c10d38591ac324ad8b5, see attached.
>
> As I should find, these '#pragma GCC diagnostic [...]' directives cause
> some code generation changes (that seems unexpected, problematic!).
> (Martin, any idea?  Might be a pre-existing problem, of course.)

OK, phew.  Martin: your diagnostic changes are *not* to be blamed for
code generation changes -- it's my '#pragma GCC diagnostic pop'
placement that triggers:

> This
> results in a lot (ten thousands) of 'GCN team arena exhausted' run-time
> diagnostics, also leading to a few FAILs:
>
>     PASS: libgomp.c/../libgomp.c-c++-common/for-11.c (test for excess errors)
>     [-PASS:-]{+FAIL:+} libgomp.c/../libgomp.c-c++-common/for-11.c execution test
>
>     PASS: libgomp.c/../libgomp.c-c++-common/for-12.c (test for excess errors)
>     [-PASS:-]{+FAIL:+} libgomp.c/../libgomp.c-c++-common/for-12.c execution test
>
>     PASS: libgomp.c/../libgomp.c-c++-common/for-3.c (test for excess errors)
>     [-PASS:-]{+FAIL:+} libgomp.c/../libgomp.c-c++-common/for-3.c execution test
>
>     PASS: libgomp.c/../libgomp.c-c++-common/for-5.c (test for excess errors)
>     [-PASS:-]{+FAIL:+} libgomp.c/../libgomp.c-c++-common/for-5.c execution test
>
>     PASS: libgomp.c/../libgomp.c-c++-common/for-6.c (test for excess errors)
>     [-PASS:-]{+FAIL:+} libgomp.c/../libgomp.c-c++-common/for-6.c execution test
>
>     PASS: libgomp.c/../libgomp.c-c++-common/for-9.c (test for excess errors)
>     [-PASS:-]{+FAIL:+} libgomp.c/../libgomp.c-c++-common/for-9.c execution test
>
> Same for 'libgomp.c++'.
>
> It remains to be analyzed how '#pragma GCC diagnostic [...]' directives
> can cause code generation changes; for now I'm working around the
> "unexpected" '-Werror=array-bounds' diagnostics differently:

In addition to a few in straight-line code, I also had these two:

> --- a/libgomp/libgomp.h
> +++ b/libgomp/libgomp.h
> @@ -128,7 +128,10 @@ team_malloc (size_t size)
>         : "=v"(result) : "v"(TEAM_ARENA_FREE), "v"(size), "e"(1L) : "memory");
>
>    /* Handle OOM.  */
> +# pragma GCC diagnostic push
> +# pragma GCC diagnostic ignored "-Warray-bounds" /*TODO PR101484 */
>    if (result + size > *(void * __lds *)TEAM_ARENA_END)
> +# pragma GCC diagnostic pop
>      {
>        /* While this is experimental, let's make sure we know when OOM
>        happens.  */
> @@ -162,8 +159,11 @@ team_free (void *ptr)
>       However, if we fell back to using heap then we should free it.
>       It would be better if this function could be a no-op, but at least
>       LDS loads are cheap.  */
> +# pragma GCC diagnostic push
> +# pragma GCC diagnostic ignored "-Warray-bounds" /*TODO PR101484 */
>    if (ptr < *(void * __lds *)TEAM_ARENA_START
>        || ptr >= *(void * __lds *)TEAM_ARENA_END)
> +# pragma GCC diagnostic pop
>      free (ptr);
>  }
>  #else

..., and it appears that the '#pragma GCC diagnostic pop' are considered
here to be the 'statement' of the 'if'!  That's (a) unexpected (to me, at
least) for this kind of "non-executable" '#pragma', and (b) certainly
would be worth a dignostic, like we have for OMP pragmas, for example:

    if (context == pragma_stmt)
      {
        error_at (loc, "%<#pragma %s%> may only be used in compound statements",
                  "[...]");

Addressing that is for another day.


Grüße
 Thomas
-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955

Re: '#pragma GCC diagnostic' (mis-)use in 'statement' of 'if'

[Martin Sebor]

On 7/20/21 2:40 AM, Thomas Schwinge wrote:
> Hi!
> 
> On 2021-07-20T09:23:24+0200, I wrote:
>> On 2021-07-19T10:46:35+0200, I wrote:
>>> | On 7/16/21 11:42 AM, Thomas Schwinge wrote:
>>> |> On 2021-07-09T17:11:25-0600, Martin Sebor via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
>>> |>> The attached tweak avoids the new -Warray-bounds instances when
>>> |>> building libatomic for arm. Christophe confirms it resolves
>>> |>> the problem (thank you!)
>>> |>
>>> |> As Abid has just reported in
>>> |> <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101374#c16>, similar
>>> |> problem with GCN target libgomp build:
>>> |>
>>> |>      In function ‘gcn_thrs’,
>>> |>          inlined from ‘gomp_thread’ at [...]/source-gcc/libgomp/libgomp.h:803:10,
>>> |>          inlined from ‘GOMP_barrier’ at [...]/source-gcc/libgomp/barrier.c:34:29:
>>> |>      [...]/source-gcc/libgomp/libgomp.h:792:10: error: array subscript 0 is outside array bounds of ‘__lds struct gomp_thread * __lds[0]’ [-Werror=array-bounds]
>>> |>        792 |   return *thrs;
>>> |>            |          ^~~~~
>>> |>
>>> |>      gcc/config/gcn/gcn.h:  c_register_addr_space ("__lds", ADDR_SPACE_LDS);                   \
>>> |>
>>> |>      libgomp/libgomp.h-static inline struct gomp_thread *gcn_thrs (void)
>>> |>      libgomp/libgomp.h-{
>>> |>      libgomp/libgomp.h-  /* The value is at the bottom of LDS.  */
>>> |>      libgomp/libgomp.h:  struct gomp_thread * __lds *thrs = (struct gomp_thread * __lds *)4;
>>> |>      libgomp/libgomp.h-  return *thrs;
>>> |>      libgomp/libgomp.h-}
>>> |>
>>> |> ..., plus a few more.  Work-around:
>>> |>
>>> |>         struct gomp_thread * __lds *thrs = (struct gomp_thread * __lds *)4;
>>> |>      +# pragma GCC diagnostic push
>>> |>      +# pragma GCC diagnostic ignored "-Warray-bounds"
>>> |>         return *thrs;
>>> |>      +# pragma GCC diagnostic pop
>>> |>
>>> |> ..., but it's a bit tedious to add that in all that the other places,
>>> |> too.
>>>
>>> Wasn't so bad after all; a lot of duplicates due to 'libgomp.h'.  I've
>>> thus pushed "[gcn] Work-around libgomp 'error: array subscript 0 is
>>> outside array bounds of ‘__lds struct gomp_thread * __lds[0]’
>>> [-Werror=array-bounds]' [PR101484]" to master branch in commit
>>> 9f2bc5077debef2b046b6c10d38591ac324ad8b5, see attached.
>>
>> As I should find, these '#pragma GCC diagnostic [...]' directives cause
>> some code generation changes (that seems unexpected, problematic!).
>> (Martin, any idea?  Might be a pre-existing problem, of course.)
> 
> OK, phew.  Martin: your diagnostic changes are *not* to be blamed for
> code generation changes -- it's my '#pragma GCC diagnostic pop'
> placement that triggers:
> 
>> This
>> results in a lot (ten thousands) of 'GCN team arena exhausted' run-time
>> diagnostics, also leading to a few FAILs:
>>
>>      PASS: libgomp.c/../libgomp.c-c++-common/for-11.c (test for excess errors)
>>      [-PASS:-]{+FAIL:+} libgomp.c/../libgomp.c-c++-common/for-11.c execution test
>>
>>      PASS: libgomp.c/../libgomp.c-c++-common/for-12.c (test for excess errors)
>>      [-PASS:-]{+FAIL:+} libgomp.c/../libgomp.c-c++-common/for-12.c execution test
>>
>>      PASS: libgomp.c/../libgomp.c-c++-common/for-3.c (test for excess errors)
>>      [-PASS:-]{+FAIL:+} libgomp.c/../libgomp.c-c++-common/for-3.c execution test
>>
>>      PASS: libgomp.c/../libgomp.c-c++-common/for-5.c (test for excess errors)
>>      [-PASS:-]{+FAIL:+} libgomp.c/../libgomp.c-c++-common/for-5.c execution test
>>
>>      PASS: libgomp.c/../libgomp.c-c++-common/for-6.c (test for excess errors)
>>      [-PASS:-]{+FAIL:+} libgomp.c/../libgomp.c-c++-common/for-6.c execution test
>>
>>      PASS: libgomp.c/../libgomp.c-c++-common/for-9.c (test for excess errors)
>>      [-PASS:-]{+FAIL:+} libgomp.c/../libgomp.c-c++-common/for-9.c execution test
>>
>> Same for 'libgomp.c++'.
>>
>> It remains to be analyzed how '#pragma GCC diagnostic [...]' directives
>> can cause code generation changes; for now I'm working around the
>> "unexpected" '-Werror=array-bounds' diagnostics differently:
> 
> In addition to a few in straight-line code, I also had these two:
> 
>> --- a/libgomp/libgomp.h
>> +++ b/libgomp/libgomp.h
>> @@ -128,7 +128,10 @@ team_malloc (size_t size)
>>          : "=v"(result) : "v"(TEAM_ARENA_FREE), "v"(size), "e"(1L) : "memory");
>>
>>     /* Handle OOM.  */
>> +# pragma GCC diagnostic push
>> +# pragma GCC diagnostic ignored "-Warray-bounds" /*TODO PR101484 */
>>     if (result + size > *(void * __lds *)TEAM_ARENA_END)
>> +# pragma GCC diagnostic pop
>>       {
>>         /* While this is experimental, let's make sure we know when OOM
>>         happens.  */
>> @@ -162,8 +159,11 @@ team_free (void *ptr)
>>        However, if we fell back to using heap then we should free it.
>>        It would be better if this function could be a no-op, but at least
>>        LDS loads are cheap.  */
>> +# pragma GCC diagnostic push
>> +# pragma GCC diagnostic ignored "-Warray-bounds" /*TODO PR101484 */
>>     if (ptr < *(void * __lds *)TEAM_ARENA_START
>>         || ptr >= *(void * __lds *)TEAM_ARENA_END)
>> +# pragma GCC diagnostic pop
>>       free (ptr);
>>   }
>>   #else
> 
> ..., and it appears that the '#pragma GCC diagnostic pop' are considered
> here to be the 'statement' of the 'if'!  That's (a) unexpected (to me, at
> least) for this kind of "non-executable" '#pragma', and (b) certainly
> would be worth a dignostic, like we have for OMP pragmas, for example:
> 
>      if (context == pragma_stmt)
>        {
>          error_at (loc, "%<#pragma %s%> may only be used in compound statements",
>                    "[...]");
> 

I agree, that does seem quite surprising.  I opened pr101538 only
to find that the problem is already tracked in pr63326.

> Addressing that is for another day.

David Malcolm (CC'd) has a patch attached to pr63326 to issue
a warning to point out that #pragmas are treated as statements
that would help prevent this type of a bug.  David, do you still
plan to submit it?

Martin

Re: '#pragma GCC diagnostic' (mis-)use in 'statement' of 'if'

[Jakub Jelinek]

On Tue, Jul 20, 2021 at 01:47:01PM -0600, Martin Sebor wrote:
> > Addressing that is for another day.
> 
> David Malcolm (CC'd) has a patch attached to pr63326 to issue
> a warning to point out that #pragmas are treated as statements
> that would help prevent this type of a bug.  David, do you still
> plan to submit it?

That patch doesn't look correct.
c_parser_pragma and cp_parser_pragma is already told if it appears
in a context for which treating the pragma as standalone statement changes
the behavior and in contexts where it doesn't - pragma_stmt stands
for the problematic ones, pragma_compound for the correct ones (there are
other values for namespace scope, class scope etc.).
OpenMP/OpenACC pragmas shouldn't be touched, those already do the right
thing the standard asks for, for the remaining ones there should be a
warning for the pragma_stmt cases.

	Jakub

RE: [PATCH libatomic/arm] avoid warning on constant addresses (PR 101379)

[Kyrylo Tkachov]

> -----Original Message-----
> From: Gcc-patches <gcc-patches-
> bounces+kyrylo.tkachov=arm.com@gcc.gnu.org> On Behalf Of Martin Sebor
> via Gcc-patches
> Sent: 10 July 2021 00:11
> To: gcc-patches <gcc-patches@gcc.gnu.org>; Christophe Lyon
> <christophe.lyon@linaro.org>
> Subject: [PATCH libatomic/arm] avoid warning on constant addresses (PR
> 101379)
> 
> The attached tweak avoids the new -Warray-bounds instances when
> building libatomic for arm. Christophe confirms it resolves
> the problem (thank you!)
> 
> As we have discussed, the main goal of this class of warnings
> is to detect accesses at addresses derived from null pointers
> (e.g., to struct members or array elements at a nonzero offset).
> Diagnosing accesses at hardcoded addresses is incidental because
> at the stage they are detected the two are not distinguishable
> from each another.
> 
> I'm planning (hoping) to implement detection of invalid pointer
> arithmetic involving null for GCC 12, so this patch is a stopgap
> solution to unblock the arm libatomic build without compromising
> the warning.  Once the new detection is in place these workarounds
> can be removed or replaced with something more appropriate (e.g.,
> declaring the objects at the hardwired addresses with an attribute
> like AVR's address or io; that would enable bounds checking at
> those addresses as well).

Let's get this patch in to unbreak bootstrap while the discussion on how to avoid these workarounds continues...
So ok.
Thanks,
Kyrill

> 
> Martin

Re: [PATCH libatomic/arm] avoid warning on constant addresses (PR 101379)

[Martin Sebor]

On 7/21/21 10:41 AM, Kyrylo Tkachov wrote:
> 
> 
>> -----Original Message-----
>> From: Gcc-patches <gcc-patches-
>> bounces+kyrylo.tkachov=arm.com@gcc.gnu.org> On Behalf Of Martin Sebor
>> via Gcc-patches
>> Sent: 10 July 2021 00:11
>> To: gcc-patches <gcc-patches@gcc.gnu.org>; Christophe Lyon
>> <christophe.lyon@linaro.org>
>> Subject: [PATCH libatomic/arm] avoid warning on constant addresses (PR
>> 101379)
>>
>> The attached tweak avoids the new -Warray-bounds instances when
>> building libatomic for arm. Christophe confirms it resolves
>> the problem (thank you!)
>>
>> As we have discussed, the main goal of this class of warnings
>> is to detect accesses at addresses derived from null pointers
>> (e.g., to struct members or array elements at a nonzero offset).
>> Diagnosing accesses at hardcoded addresses is incidental because
>> at the stage they are detected the two are not distinguishable
>> from each another.
>>
>> I'm planning (hoping) to implement detection of invalid pointer
>> arithmetic involving null for GCC 12, so this patch is a stopgap
>> solution to unblock the arm libatomic build without compromising
>> the warning.  Once the new detection is in place these workarounds
>> can be removed or replaced with something more appropriate (e.g.,
>> declaring the objects at the hardwired addresses with an attribute
>> like AVR's address or io; that would enable bounds checking at
>> those addresses as well).
> 
> Let's get this patch in to unbreak bootstrap while the discussion on how to avoid these workarounds continues...
> So ok.

I just pushed it in r12-2438.

Martin

> Thanks,
> Kyrill
> 
>>
>> Martin