disable -Warray-bounds in libgo (PR 101374)

disable -Warray-bounds in libgo (PR 101374)

[Martin Sebor]

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

Hi Ian,

Yesterday's enhancement to -Warray-bounds has exposed a couple of
issues in libgo where the code writes into an invalid constant
address that the warning is designed to flag.

On the assumption that those invalid addresses are deliberate,
the attached patch suppresses these instances by using #pragma
GCC diagnostic but I don't think I'm supposed to commit it (at
least Git won't let me).  To avoid Go bootstrap failures please
either apply the patch or otherwise suppress the warning (e.g.,
by using a volatile pointer temporary).

Thanks
Martin

[-- Attachment #2: libgo-pr101374.diff --]
[-- Type: text/x-patch, Size: 1646 bytes --]

Use Object Size Type zero for -Warray-bounds [PR101374].

PR bootstrap/101374 - -Warray-bounds accessing a member subobject as derived

libgo/ChangeLog:
	PR bootstrap/101374
	* runtime/proc.c (runtime_mstart): Suppress -Warray-bounds.
	* runtime/runtime_c.c (runtime_signalstack): Same.

diff --git a/libgo/runtime/proc.c b/libgo/runtime/proc.c
index 38bf7a6b255..61635e6c1ea 100644
--- a/libgo/runtime/proc.c
+++ b/libgo/runtime/proc.c
@@ -594,7 +594,14 @@ runtime_mstart(void *arg)
 		gp->entry = nil;
 		gp->param = nil;
 		__builtin_call_with_static_chain(pfn(gp1), fv);
+
+		/* Writing to an invalid address is detected.  */
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Warray-bounds"
+
 		*(int*)0x21 = 0x21;
+
+#pragma GCC diagnostic push
 	}
 
 	if(mp->exiting) {
@@ -662,7 +669,12 @@ setGContext(void)
 		gp->entry = nil;
 		gp->param = nil;
 		__builtin_call_with_static_chain(pfn(gp1), fv);
+
+		/* Writing to an invalid address is detected.  */
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Warray-bounds"
 		*(int*)0x22 = 0x22;
+#pragma GCC diagnostic pop
 	}
 }
 
diff --git a/libgo/runtime/runtime_c.c b/libgo/runtime/runtime_c.c
index 18222c14465..53feaa075c7 100644
--- a/libgo/runtime/runtime_c.c
+++ b/libgo/runtime/runtime_c.c
@@ -116,7 +116,11 @@ runtime_signalstack(byte *p, uintptr n)
 	if(p == nil)
 		st.ss_flags = SS_DISABLE;
 	if(sigaltstack(&st, nil) < 0)
+	  /* Writing to an invalid address is detected.  */
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Warray-bounds"
 		*(int *)0xf1 = 0xf1;
+#pragma GCC diagnostic push
 }
 
 int32 go_open(char *, int32, int32)

Re: disable -Warray-bounds in libgo (PR 101374)

[Rainer Orth]

Hi Martin,

> Yesterday's enhancement to -Warray-bounds has exposed a couple of
> issues in libgo where the code writes into an invalid constant
> address that the warning is designed to flag.
>
> On the assumption that those invalid addresses are deliberate,
> the attached patch suppresses these instances by using #pragma
> GCC diagnostic but I don't think I'm supposed to commit it (at
> least Git won't let me).  To avoid Go bootstrap failures please
> either apply the patch or otherwise suppress the warning (e.g.,
> by using a volatile pointer temporary).

while this patch does fix the libgo bootstrap failure, Go is completely
broken: almost 1000 go.test failures and all libgo tests FAIL as well.
Seen on both i386-pc-solaris2.11 and sparc-sun-solaris2.11.

Please fix.

	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: disable -Warray-bounds in libgo (PR 101374)

[Richard Biener]

On Thu, Jul 8, 2021 at 8:02 PM Martin Sebor via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> Hi Ian,
>
> Yesterday's enhancement to -Warray-bounds has exposed a couple of
> issues in libgo where the code writes into an invalid constant
> address that the warning is designed to flag.
>
> On the assumption that those invalid addresses are deliberate,
> the attached patch suppresses these instances by using #pragma
> GCC diagnostic but I don't think I'm supposed to commit it (at
> least Git won't let me).  To avoid Go bootstrap failures please
> either apply the patch or otherwise suppress the warning (e.g.,
> by using a volatile pointer temporary).

Btw, I don't think we should diagnose things like

                *(int*)0x21 = 0x21;

when somebody literally writes that he'll be just annoyed by diagnostics.

Of course the above might be able to use __builtin_trap (); - it looks
like it is placed where control flow should never end, kind of a
__builtin_unreachable (), which means abort () might do as well.

Richard.

> Thanks
> Martin

Re: disable -Warray-bounds in libgo (PR 101374)

[Maxim Kuvyrkov]

> On 9 Jul 2021, at 09:16, Richard Biener via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
> 
> On Thu, Jul 8, 2021 at 8:02 PM Martin Sebor via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>> 
>> Hi Ian,
>> 
>> Yesterday's enhancement to -Warray-bounds has exposed a couple of
>> issues in libgo where the code writes into an invalid constant
>> address that the warning is designed to flag.
>> 
>> On the assumption that those invalid addresses are deliberate,
>> the attached patch suppresses these instances by using #pragma
>> GCC diagnostic but I don't think I'm supposed to commit it (at
>> least Git won't let me).  To avoid Go bootstrap failures please
>> either apply the patch or otherwise suppress the warning (e.g.,
>> by using a volatile pointer temporary).
> 
> Btw, I don't think we should diagnose things like
> 
>                *(int*)0x21 = 0x21;
> 
> when somebody literally writes that he'll be just annoyed by diagnostics.

And we have an assortment of similar cases in 32-bit ARM kernel-page helpers.

At the moment building libatomic for arm-linux-gnueabihf fails with:
===
In function ‘select_test_and_set_8’,
    inlined from ‘select_test_and_set_8’ at /home/tcwg-buildslave/workspace/tcwg-dev-build/snapshots/gcc.git~master/libatomic/tas_n.c:115:1:
/home/tcwg-buildslave/workspace/tcwg-dev-build/snapshots/gcc.git~master/libatomic/config/linux/arm/host-config.h:42:34: error: array subscript 0 is outside array bounds of ‘unsigned int[0]’ [-Werror=array-bounds]
   42 | #define __kernel_helper_version (*(unsigned int *)0xffff0ffc)
      |                                 ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~
===

In libatomic/config/linux/arm/host-config.h we have:
===
/* Kernel helper for 32-bit compare-and-exchange.  */
typedef int (__kernel_cmpxchg_t) (UWORD oldval, UWORD newval, UWORD *ptr);
#define __kernel_cmpxchg (*(__kernel_cmpxchg_t *) 0xffff0fc0)

/* Kernel helper for 64-bit compare-and-exchange.  */
typedef int (__kernel_cmpxchg64_t) (const U_8 * oldval, const U_8 * newval,
				    U_8 *ptr);
#define __kernel_cmpxchg64 (*(__kernel_cmpxchg64_t *) 0xffff0f60)

/* Kernel helper for memory barrier.  */
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)
===



--
Maxim Kuvyrkov
https://www.linaro.org

Re: disable -Warray-bounds in libgo (PR 101374)

[Rainer Orth]

Hi Martin,

>> Yesterday's enhancement to -Warray-bounds has exposed a couple of
>> issues in libgo where the code writes into an invalid constant
>> address that the warning is designed to flag.
>>
>> On the assumption that those invalid addresses are deliberate,
>> the attached patch suppresses these instances by using #pragma
>> GCC diagnostic but I don't think I'm supposed to commit it (at
>> least Git won't let me).  To avoid Go bootstrap failures please
>> either apply the patch or otherwise suppress the warning (e.g.,
>> by using a volatile pointer temporary).
>
> while this patch does fix the libgo bootstrap failure, Go is completely
> broken: almost 1000 go.test failures and all libgo tests FAIL as well.
> Seen on both i386-pc-solaris2.11 and sparc-sun-solaris2.11.

FWIW, I see exactly the same failures on x86_64-pc-linux-gnu, so nothing
Solaris-specific here.

	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: disable -Warray-bounds in libgo (PR 101374)

[Martin Sebor]

On 7/9/21 12:16 AM, Richard Biener wrote:
> On Thu, Jul 8, 2021 at 8:02 PM Martin Sebor via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>>
>> Hi Ian,
>>
>> Yesterday's enhancement to -Warray-bounds has exposed a couple of
>> issues in libgo where the code writes into an invalid constant
>> address that the warning is designed to flag.
>>
>> On the assumption that those invalid addresses are deliberate,
>> the attached patch suppresses these instances by using #pragma
>> GCC diagnostic but I don't think I'm supposed to commit it (at
>> least Git won't let me).  To avoid Go bootstrap failures please
>> either apply the patch or otherwise suppress the warning (e.g.,
>> by using a volatile pointer temporary).
> 
> Btw, I don't think we should diagnose things like
> 
>                  *(int*)0x21 = 0x21;
> 
> when somebody literally writes that he'll be just annoyed by diagnostics.
> 
> Of course the above might be able to use __builtin_trap (); - it looks
> like it is placed where control flow should never end, kind of a
> __builtin_unreachable (), which means abort () might do as well.

I agree that the literal case isn't interesting.  At the time
the warnings run the distinction between a nonnull literal and
one derived from a null has been lost.  I'm hoping to replace
this with an early pass to detect null pointer arithmetic.

Martin

> 
> Richard.
> 
>> Thanks
>> Martin

Re: disable -Warray-bounds in libgo (PR 101374)

[Martin Sebor]

On 7/9/21 7:19 AM, Maxim Kuvyrkov wrote:
>> On 9 Jul 2021, at 09:16, Richard Biener via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
>>
>> On Thu, Jul 8, 2021 at 8:02 PM Martin Sebor via Gcc-patches
>> <gcc-patches@gcc.gnu.org> wrote:
>>>
>>> Hi Ian,
>>>
>>> Yesterday's enhancement to -Warray-bounds has exposed a couple of
>>> issues in libgo where the code writes into an invalid constant
>>> address that the warning is designed to flag.
>>>
>>> On the assumption that those invalid addresses are deliberate,
>>> the attached patch suppresses these instances by using #pragma
>>> GCC diagnostic but I don't think I'm supposed to commit it (at
>>> least Git won't let me).  To avoid Go bootstrap failures please
>>> either apply the patch or otherwise suppress the warning (e.g.,
>>> by using a volatile pointer temporary).
>>
>> Btw, I don't think we should diagnose things like
>>
>>                 *(int*)0x21 = 0x21;
>>
>> when somebody literally writes that he'll be just annoyed by diagnostics.
> 
> And we have an assortment of similar cases in 32-bit ARM kernel-page helpers.
> 
> At the moment building libatomic for arm-linux-gnueabihf fails with:
> ===
> In function ‘select_test_and_set_8’,
>      inlined from ‘select_test_and_set_8’ at /home/tcwg-buildslave/workspace/tcwg-dev-build/snapshots/gcc.git~master/libatomic/tas_n.c:115:1:
> /home/tcwg-buildslave/workspace/tcwg-dev-build/snapshots/gcc.git~master/libatomic/config/linux/arm/host-config.h:42:34: error: array subscript 0 is outside array bounds of ‘unsigned int[0]’ [-Werror=array-bounds]
>     42 | #define __kernel_helper_version (*(unsigned int *)0xffff0ffc)
>        |                                 ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ===
> 
> In libatomic/config/linux/arm/host-config.h we have:
> ===
> /* Kernel helper for 32-bit compare-and-exchange.  */
> typedef int (__kernel_cmpxchg_t) (UWORD oldval, UWORD newval, UWORD *ptr);
> #define __kernel_cmpxchg (*(__kernel_cmpxchg_t *) 0xffff0fc0)
> 
> /* Kernel helper for 64-bit compare-and-exchange.  */
> typedef int (__kernel_cmpxchg64_t) (const U_8 * oldval, const U_8 * newval,
> 				    U_8 *ptr);
> #define __kernel_cmpxchg64 (*(__kernel_cmpxchg64_t *) 0xffff0f60)
> 
> /* Kernel helper for memory barrier.  */
> 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)
> ===

This failure is tracked in pr101379.  I have added an untested POC
patch with a possible way to avoid the warning.  Other approaches
are possible (I mention some in my comment on the bug) but they
are limited by the exposure of the constant address using macros.
Hiding them behind APIs instead would make it possible to suppress
the warnings via #pragma GCC diagnostic.  Alternatively, making
the addresses extern const variables would hide the constants from
the warning altogether.

With a suitable attribute an API or variable could also describe
the size of the object.  The AVR back end, for example, has two
attributes for hardwired addresses: io and address.  One of them
(or another one like it) could be made the target-indendependent
way to declare global variables of any type at fixed addresses,
e.g., like so:

   extern __attribute__ ((address (0xffff0fc0))) __kernel_cmpxchg64_t
   __kernel_cmpxchg;

Martin

Re: disable -Warray-bounds in libgo (PR 101374)

[Ian Lance Taylor]

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

On Thu, Jul 8, 2021 at 11:16 PM Richard Biener
<richard.guenther@gmail.com> wrote:
>
> On Thu, Jul 8, 2021 at 8:02 PM Martin Sebor via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
> >
> > Hi Ian,
> >
> > Yesterday's enhancement to -Warray-bounds has exposed a couple of
> > issues in libgo where the code writes into an invalid constant
> > address that the warning is designed to flag.
> >
> > On the assumption that those invalid addresses are deliberate,
> > the attached patch suppresses these instances by using #pragma
> > GCC diagnostic but I don't think I'm supposed to commit it (at
> > least Git won't let me).  To avoid Go bootstrap failures please
> > either apply the patch or otherwise suppress the warning (e.g.,
> > by using a volatile pointer temporary).
>
> Btw, I don't think we should diagnose things like
>
>                 *(int*)0x21 = 0x21;
>
> when somebody literally writes that he'll be just annoyed by diagnostics.
>
> Of course the above might be able to use __builtin_trap (); - it looks
> like it is placed where control flow should never end, kind of a
> __builtin_unreachable (), which means abort () might do as well.


I agree.  While this code is certainly intentional, abort will work
just as well in practice.  I committed the following to change it.

Ian

[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 1322 bytes --]

a15210699cbc60bc9ed077549dcd5288a295f42c
diff --git a/gcc/go/gofrontend/MERGE b/gcc/go/gofrontend/MERGE
index ab1384d698b..4d0f44f2dd2 100644
--- a/gcc/go/gofrontend/MERGE
+++ b/gcc/go/gofrontend/MERGE
@@ -1,4 +1,4 @@
-01cb2b5e69a2d08ef3cc1ea023c22ed9b79f5114
+adcf10890833026437a94da54934ce50c0018309
 
 The first line of this file holds the git revision number of the last
 merge done from the gofrontend repository.
diff --git a/libgo/runtime/proc.c b/libgo/runtime/proc.c
index 38bf7a6b255..3a30748d329 100644
--- a/libgo/runtime/proc.c
+++ b/libgo/runtime/proc.c
@@ -594,7 +594,7 @@ runtime_mstart(void *arg)
 		gp->entry = nil;
 		gp->param = nil;
 		__builtin_call_with_static_chain(pfn(gp1), fv);
-		*(int*)0x21 = 0x21;
+		abort();
 	}
 
 	if(mp->exiting) {
@@ -662,7 +662,7 @@ setGContext(void)
 		gp->entry = nil;
 		gp->param = nil;
 		__builtin_call_with_static_chain(pfn(gp1), fv);
-		*(int*)0x22 = 0x22;
+		abort();
 	}
 }
 
diff --git a/libgo/runtime/runtime_c.c b/libgo/runtime/runtime_c.c
index 18222c14465..bc920a5d406 100644
--- a/libgo/runtime/runtime_c.c
+++ b/libgo/runtime/runtime_c.c
@@ -116,7 +116,7 @@ runtime_signalstack(byte *p, uintptr n)
 	if(p == nil)
 		st.ss_flags = SS_DISABLE;
 	if(sigaltstack(&st, nil) < 0)
-		*(int *)0xf1 = 0xf1;
+		abort();
 }
 
 int32 go_open(char *, int32, int32)

Re: disable -Warray-bounds in libgo (PR 101374)

[Martin Sebor]

On 7/9/21 7:26 AM, Rainer Orth wrote:
> Hi Martin,
> 
>>> Yesterday's enhancement to -Warray-bounds has exposed a couple of
>>> issues in libgo where the code writes into an invalid constant
>>> address that the warning is designed to flag.
>>>
>>> On the assumption that those invalid addresses are deliberate,
>>> the attached patch suppresses these instances by using #pragma
>>> GCC diagnostic but I don't think I'm supposed to commit it (at
>>> least Git won't let me).  To avoid Go bootstrap failures please
>>> either apply the patch or otherwise suppress the warning (e.g.,
>>> by using a volatile pointer temporary).
>>
>> while this patch does fix the libgo bootstrap failure, Go is completely
>> broken: almost 1000 go.test failures and all libgo tests FAIL as well.
>> Seen on both i386-pc-solaris2.11 and sparc-sun-solaris2.11.
> 
> FWIW, I see exactly the same failures on x86_64-pc-linux-gnu, so nothing
> Solaris-specific here.

I don't normally test Go because of PR 91992, but I see just
the three test failures below on x86_64-linux with the latest trunk:

FAIL: go.test/test/fixedbugs/issue10441.go   -O  (test for excess errors)
FAIL: ./index0-out.go execution,  -O0 -g -fno-var-tracking-assignments
FAIL: runtime/pprof

The excess errors don't look related to my changes:

FAIL: go.test/test/fixedbugs/issue10441.go   -O  (test for excess errors)
Excess errors:
/usr/bin/ld: 
/ssd/test/build/gcc-trunk/x86_64-pc-linux-gnu/./libgo/.libs/libgo.so: 
undefined reference to `__go_init_main'
/usr/bin/ld: 
/ssd/test/build/gcc-trunk/x86_64-pc-linux-gnu/./libgo/.libs/libgo.so: 
undefined reference to `main.main'

My libgo.log shows the FAILs below.  I don't know how to interpret
that but nothing in the file suggests that my change is the cause
of these failures

--- FAIL: ExampleFrames (0.00s)
FAIL
FAIL: runtime
--- FAIL: TestConvertCPUProfile (0.00s)
--- FAIL: TestConvertMemProfile (0.00s)
     --- FAIL: TestConvertMemProfile/heap (0.00s)
     --- FAIL: TestConvertMemProfile/allocs (0.00s)
FAIL
FAIL: runtime/pprof
--- FAIL: ExampleFrames (0.00s)
FAIL
FAIL: runtime
--- FAIL: TestDurationSeconds (0.00s)
--- FAIL: ExampleParseDuration (0.00s)
FAIL
FAIL: time

If you see different failures in your build that look like they
might be caused by them then please show what those are.

Martin

Re: disable -Warray-bounds in libgo (PR 101374)

[Rainer Orth]

Hi Martin,

>>> while this patch does fix the libgo bootstrap failure, Go is completely
>>> broken: almost 1000 go.test failures and all libgo tests FAIL as well.
>>> Seen on both i386-pc-solaris2.11 and sparc-sun-solaris2.11.
>> FWIW, I see exactly the same failures on x86_64-pc-linux-gnu, so nothing
>> Solaris-specific here.
>
> I don't normally test Go because of PR 91992, but I see just

I've never seen this myself, neither on Fedora 29 in the past or on
Ubuntu 20.04 right now.

> the three test failures below on x86_64-linux with the latest trunk:
>
> FAIL: go.test/test/fixedbugs/issue10441.go   -O  (test for excess errors)
> FAIL: ./index0-out.go execution,  -O0 -g -fno-var-tracking-assignments
> FAIL: runtime/pprof
>
> The excess errors don't look related to my changes:
>
> FAIL: go.test/test/fixedbugs/issue10441.go   -O  (test for excess errors)
> Excess errors:
> /usr/bin/ld:
> /ssd/test/build/gcc-trunk/x86_64-pc-linux-gnu/./libgo/.libs/libgo.so: 
> undefined reference to `__go_init_main'
> /usr/bin/ld:
> /ssd/test/build/gcc-trunk/x86_64-pc-linux-gnu/./libgo/.libs/libgo.so: 
> undefined reference to `main.main'

Indeed: this is a compile-only test that isn't marked as such and thus
fails to link.

> If you see different failures in your build that look like they
> might be caused by them then please show what those are.

I've started another round of bootstraps last night for the first time
since Friday and indeed the failures are gone.  Before, every single Go
execution test died with a SEGV.  No idea what fixed that, though.

	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: disable -Warray-bounds in libgo (PR 101374)

[Dimitar Dimitrov]

On Fri, Jul 09, 2021 at 08:16:24AM +0200, Richard Biener via Gcc-patches wrote:
> On Thu, Jul 8, 2021 at 8:02 PM Martin Sebor via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
> >
> > Hi Ian,
> >
> > Yesterday's enhancement to -Warray-bounds has exposed a couple of
> > issues in libgo where the code writes into an invalid constant
> > address that the warning is designed to flag.
> >
> > On the assumption that those invalid addresses are deliberate,
> > the attached patch suppresses these instances by using #pragma
> > GCC diagnostic but I don't think I'm supposed to commit it (at
> > least Git won't let me).  To avoid Go bootstrap failures please
> > either apply the patch or otherwise suppress the warning (e.g.,
> > by using a volatile pointer temporary).
> 
> Btw, I don't think we should diagnose things like
> 
>                 *(int*)0x21 = 0x21;
> 
> when somebody literally writes that he'll be just annoyed by diagnostics.
I agree. This will raise a lot of noise for embedded targets.

Similar constructs are used extensively in pretty much any microcontroller
project to define macros to access I/O special-function addresses.
A few random examples:

http://svn.savannah.gnu.org/viewvc/avr-libc/trunk/avr-libc/include/avr/sfr_defs.h?view=markup#l128
https://sourceforge.net/p/mspgcc/msp430mcu/ci/master/tree/upstream/cc430f5123.h#l2141
https://github.com/ARM-software/CMSIS_5/blob/develop/CMSIS/RTOS/RTX/SRC/rt_HAL_CM.h#L138

Regards,
Dimitar

> 
> Of course the above might be able to use __builtin_trap (); - it looks
> like it is placed where control flow should never end, kind of a
> __builtin_unreachable (), which means abort () might do as well.
> 
> Richard.
> 
> > Thanks
> > Martin