From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from esa4.mentor.iphmx.com (esa4.mentor.iphmx.com [68.232.137.252]) by sourceware.org (Postfix) with ESMTPS id C08183858038 for ; Mon, 19 Jul 2021 08:49:32 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org C08183858038 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=codesourcery.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=mentor.com IronPort-SDR: z8ZOvL3p2bE/HkurzFPY2XbGXitWmNto7YecZ4xhBT0c9SmVTxpN1qLqV0hYViTIiN4Sfyku/B bwkaNKtFLkfUEKwG/wcRX8hr1410FFDDFCVVxqZMf1uTR2UrzUWMWAC6WF1EusIWOneh4t0PZr /GPeX8NQaaSHCHHIQdTErk9m3txb70lDDnJP34vVF57St134uGMPGz7pqa7FRL/X2oE9vNbrQh gOom1EJ9mhnZvco4LjzQ2r5vY4yUMfp6phMBIRHAVgNVSVVDnO3Zxj2L+kkOWMkmKxZmbRe3MR /jw2Bj6KAMgwYM/AweRM0dhD X-IronPort-AV: E=Sophos;i="5.84,251,1620720000"; d="scan'208";a="63842998" Received: from orw-gwy-02-in.mentorg.com ([192.94.38.167]) by esa4.mentor.iphmx.com with ESMTP; 19 Jul 2021 00:49:31 -0800 IronPort-SDR: DvI4mbAVbKTcJD4H2zcfhUqEqUByjm9mptwZ+49S0d8xlsrLm6mR2afAQIWaOUsrKLWBkXlFft XiO9wkN/qsViAe1jhQ3Vlp4j3J3X87obhyg3hZrHIcVA/EzbgIqkc6QiM+hZ0res8tFapvrnxv m9cOeAmA0lF3VnSbrKrQLtiTG+oC0bLdLvT2naCYPli9ejcscdkuwAJzrjTwPujxIjRByqJKvZ F/oEYtVAPt3aWkYhPE+r/yoqu8sn6RWIKKxzaNlmSGoIQhzcePMCeJXHSFDhbPlkeAipGrAVF6 88A= From: Thomas Schwinge To: Martin Sebor CC: , Christophe Lyon , Andrew Stubbs , Hafiz Abid Qadeer Subject: Re: [PATCH libatomic/arm] avoid warning on constant addresses (PR 101379) In-Reply-To: <1ab598da-6cac-c1bd-b54f-4f5c8b0683f6@gmail.com> References: <816ce216-bf6a-75ca-4241-4861ec43ab27@gmail.com> <87pmvii29w.fsf@euler.schwinge.homeip.net> <1ab598da-6cac-c1bd-b54f-4f5c8b0683f6@gmail.com> User-Agent: Notmuch/0.29.3+94~g74c3f1b (https://notmuchmail.org) Emacs/27.1 (x86_64-pc-linux-gnu) Date: Mon, 19 Jul 2021 10:49:23 +0200 Message-ID: <87a6mibsdo.fsf@euler.schwinge.homeip.net> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable X-Originating-IP: [137.202.0.90] X-ClientProxiedBy: svr-ies-mbx-05.mgc.mentorg.com (139.181.222.5) To svr-ies-mbx-01.mgc.mentorg.com (139.181.222.1) X-Spam-Status: No, score=-12.3 required=5.0 tests=BAYES_00, GIT_PATCH_0, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 19 Jul 2021 08:49:36 -0000 Hi! On 2021-07-16T15:11:24-0600, Martin Sebor via Gcc-patches wrote: > On 7/16/21 11:42 AM, Thomas Schwinge wrote: >> On 2021-07-09T17:11:25-0600, Martin Sebor via Gcc-patches 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/confi= g/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 =3D (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=C3=BC=C3=9Fe Thomas ----------------- Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstra=C3=9Fe 201= , 80634 M=C3=BCnchen; Gesellschaft mit beschr=C3=A4nkter Haftung; Gesch=C3= =A4ftsf=C3=BChrer: Thomas Heurung, Frank Th=C3=BCrauf; Sitz der Gesellschaf= t: M=C3=BCnchen; Registergericht M=C3=BCnchen, HRB 106955