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
