Message ID | 816ce216-bf6a-75ca-4241-4861ec43ab27@gmail.com |
---|---|
State | New |
Headers | show |
Series | [libatomic/arm] avoid warning on constant addresses (PR 101379) | expand |
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 >
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
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
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
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
> -----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
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
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