From patchwork Mon Feb 3 21:08:39 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Bernd Edlinger X-Patchwork-Id: 316329 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id D2E522C0086 for ; Tue, 4 Feb 2014 08:09:38 +1100 (EST) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :message-id:from:to:cc:subject:date:in-reply-to:references :content-type:content-transfer-encoding:mime-version; q=dns; s= default; b=s5olOXDrRy8xcPgAr93UdE2aKVyJulGldjClOWCJPlqxhMbdGf3Go +LM+iS0hhFQ5wJ+oTd0BTQ6R71T4AcxLdB/scbOBWiuTJqtLxscfhtsjQ8KO9+C3 cpuQFQEIzjdnU68jg+Dw1h4YNc8p5nraBFLPq1BWePTinoMtEQLkh8= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :message-id:from:to:cc:subject:date:in-reply-to:references :content-type:content-transfer-encoding:mime-version; s=default; bh=ibwJxLZMB6xNRoGDGINQXqSeB18=; b=jT42p4ZP5ZxxLHG2Ke9He5Z0b/Qf i6g1A6nokOjQlrakXS5wuXGdxUQ6B5IMwmrRDZXeE2hMSpMBvgJ4TnWWFrPzAkoy S4DdFpOe78e6KyAKzFpYXcwnKHqB2GcF1gxlyMaQ/oLGuqawy5VD7O8KFO7I/+Xg /p0ykSuMA3rL1GY= Received: (qmail 23824 invoked by alias); 3 Feb 2014 21:08:46 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 23807 invoked by uid 89); 3 Feb 2014 21:08:45 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.0 required=5.0 tests=AWL, BAYES_00, FREEMAIL_FROM, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.2 X-Spam-User: qpsmtpd, 2 recipients X-HELO: dub0-omc4-s15.dub0.hotmail.com Received: from dub0-omc4-s15.dub0.hotmail.com (HELO dub0-omc4-s15.dub0.hotmail.com) (157.55.2.90) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 03 Feb 2014 21:08:42 +0000 Received: from DUB129-W37 ([157.55.2.71]) by dub0-omc4-s15.dub0.hotmail.com with Microsoft SMTPSVC(6.0.3790.4675); Mon, 3 Feb 2014 13:08:39 -0800 X-TMN: [NMfKmHDydlajY1n3Jrk9L3iQ453O6Xxb] Message-ID: From: Bernd Edlinger To: Joey Ye CC: Richard Biener , Richard Earnshaw , "gcc@gcc.gnu.org" , Sandra Loosemore , "gcc-patches@gcc.gnu.org" Subject: RE: Still fails with strict-volatile-bitfields Date: Mon, 3 Feb 2014 22:08:39 +0100 In-Reply-To: References: , , <52CECCC9.1080007@arm.com>, , <52CFC032.8070604@arm.com>, , , , MIME-Version: 1.0 Hi, On Mon, 13 Jan 2014 10:26:03, Joey Ye wrote: > > Bernd, > > If that's the case, can you please firstly fix invoke.texi where the > behavior of strict-volatile-bitfields is described? At least my > interpretation of current doc doesn't explain the behavior of the case > we are discussing. Also it should be a generic definition rather than > target specific one. > Yes, if nobody objects I'd add a note like this to the documentation regarding the -fstrict-volatile-bitfields: Thanks Bernd. > A related issue is that how would we expect users to build their > original code with volatile bitfields usage? From the approach I > understand they have to explicitly add -fstrict-volatile-bitfields for > 4.9 (by default it is disabled now), and probably > -fstrict-volatile-bitfields=aapcs (to allow C++ memory model conflict) > later. Neither of them are obvious to users. How about a configure > option to set default? > > Thanks, > Joey > > On Fri, Jan 10, 2014 at 10:20 PM, Bernd Edlinger > wrote: >> On Fri, 10 Jan 2014 14:45:12, Richard Biener wrote: >>> >>> On Fri, Jan 10, 2014 at 2:30 PM, Bernd Edlinger >>> wrote: >>>> On, Fri, 10 Jan 2014 09:41:06, Richard Earnshaw wrote: >>>>> >>>>> On 10/01/14 08:49, Bernd Edlinger wrote: >>>>>> On Thu, 9 Jan 2014 16:22:33, Richard Earnshaw wrote: >>>>>>> >>>>>>> On 09/01/14 08:26, Bernd Edlinger wrote: >>>>>>>> Hi, >>>>>>>> >>>>>>>> On Thu, 9 Jan 2014 15:01:54, Yoey Ye wrote: >>>>>>>>> >>>>>>>>> Sandra, Bernd, >>>>>>>>> >>>>>>>>> Can you take a look at >>>>>>>>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59734 >>>>>>>>> >>>>>>>>> It seems a siimple case still doesn't work as expected. Did I miss anything? >>>>>>>>> >>>>>>>>> Thanks, >>>>>>>>> Joey >>>>>>>> >>>>>>>> Yes, >>>>>>>> >>>>>>>> this is a major case where the C++ memory model is >>>>>>>> in conflict with AAPCS. >>>>>>>> >>>>>>> >>>>>>> Does the compiler warn about this? And if so, is the warning on by >>>>>>> default? I think it's essential that we don't have quiet changes in >>>>>>> behaviour without such warnings. >>>>>>> >>>>>>> R. >>>>>> >>>>>> No. This example was working in 4.6 and broken in 4.7 and 4.8. >>>>>> Well, 4.7 should have warned about that. >>>>>> >>>>>> 4.9 does simply not change that, because it would violate the C++ >>>>>> memory model. Maybe that should go into release notes. >>>>> >>>>> That's no excuse for not generating a warning at compile time when the >>>>> situation is encountered. Users of this feature are experiencing a >>>>> quiet change of behaviour and that's unacceptable, even if the compiler >>>>> is doing what it was intended to do; that doesn't necessarily match the >>>>> user's expectations. There should always be a way to rewrite the C11 >>>>> intended semantics in a way that doesn't violate the strict volatile >>>>> bitfields semantics. >>>>> >>>> >>>> Hmm... >>>> You mean we should have a (ugly) warning enabled by default in 4.9 (at expmed.c) >>>> if a bit-field access would be perfectly aligned and so, but is in conflict with the >>>> C++ memory model, and -fstrict-volatile-bitfields is in effect. >>>> Maybe only once per compilation? >>> >>> I'd say you want a warning for the structure declaration instead >>> "Accesses to XYZ will not follow AACPS due to C++ memory model >>> constraints". >>> >>> Richard. >>> >> >> Yes, that would be the way how we wanted to implement the >> -Wportable-volatility warning, except that it would be on by default >> if -fstrict-volatile-bitfields is in effect. >> And it would not only warn if the member is declared volatile, >> because the structure can be declared volatile later. >> >> Except that I did not even start to implement it that way, that >> would be quite late for 4.9 now? >> >> Bernd. >>>> >>>>>> >>>>>> At the same time we had all kinds of invalid code generation, >>>>>> starting at 4.6, especially with packed structures in C and Ada(!), >>>>>> (writing not all bits, and using unaligned accesses) >>>>>> and that is fixed in 4.9. >>>>>> >>>>> >>>>> I'm not worried about packed structures. You can't sensibly implement >>>>> the strict volatile bitfields rules when things aren't aligned. >>>>> >>>>> R. >>>>> >>>>>> >>>>>> Bernd. >>>>>> >>>>>>> >>>>>>>> You can get the expected code, by changing the struct >>>>>>>> like this: >>>>>>>> >>>>>>>> struct str { >>>>>>>> volatile unsigned f1: 8; >>>>>>>> unsigned dummy:24; >>>>>>>> }; >>>>>>>> >>>>>>>> If it is written this way the C++ memory model allows >>>>>>>> QImode, HImode, SImode. And -fstrict-volatile-bitfields >>>>>>>> demands SImode, so the conflict is resolved. This dummy >>>>>>>> member makes only a difference on the C level, and is >>>>>>>> completely invisible in the generated code. >>>>>>>> >>>>>>>> If -fstrict-volatile-bitfields is now given, we use SImode, >>>>>>>> if -fno-strict-volatile-bitfields is given, we give GCC the >>>>>>>> freedom to choose the access mode, maybe QImode if that is >>>>>>>> faster. >>>>>>>> >>>>>>>> In the _very_ difficult process to find an solution >>>>>>>> that seems to be acceptable to all maintainers, we came to >>>>>>>> the solution, that we need to adhere to the C++ memory >>>>>>>> model by default. And we may not change the default >>>>>>>> setting of -fstruct-volatile-bitfields at the same time! >>>>>>>> >>>>>>>> As a future extension we discussed the possibility >>>>>>>> to add a new option -fstrict-volatile-bitfields=aapcs >>>>>>>> that explicitly allows us to break the C++ memory model. >>>>>>>> >>>>>>>> But I did not yet try to implement this, as I feel that >>>>>>>> would certainly not be accepted as we are in Phase3 now. >>>>>>>> >>>>>>>> As another future extension there was the discussion >>>>>>>> about the -Wportable-volatility warning, which I see now >>>>>>>> as a warning that analyzes the structure layout and >>>>>>>> warns about any structures that are not "well-formed", >>>>>>>> in the sense, that a bit-field fails to define all >>>>>>>> bits of the container. >>>>>>>> >>>>>>>> Those people that do use bit-fields to access device-registers >>>>>>>> do always define all bits, and of course in the same mode. >>>>>>>> >>>>>>>> It would be good to have a warning, when some bits are missing. >>>>>>>> They currently have to use great care to check their structures >>>>>>>> manually. >>>>>>>> >>>>>>>> I had a proposal for that warning but that concentrated >>>>>>>> only on the volatile attribute, but I will have to re-write >>>>>>>> that completely so that it can be done in stor-layout.c: >>>>>>>> >>>>>>>> It should warn independent of optimization levels or actual >>>>>>>> bitfield member references, thus, be implemented entirely at >>>>>>>> the time we lay out the structure. The well-formed-ness of >>>>>>>> a bit-field makes that possible. >>>>>>>> >>>>>>>> But that will come too late for Phase3 as well. >>>>>>>> >>>>>>>> >>>>>>>> Regards >>>>>>>> Bernd. >>>>>>>> >>>>>>>> >>>>>>> >>>>>>> >>>>>> >>>>> >>>>> Index: invoke.texi =================================================================== --- invoke.texi (revision 207223) +++ invoke.texi (working copy) @@ -22456,6 +22456,10 @@  case GCC falls back to generating multiple accesses rather than code that  will fault or truncate the result at run time. +Note:  Due to restrictions of the C/C++11 memory model, write accesses are +not allowed to touch non bit-field members.  It is therefore recommended +to define all bits of the field's type as bit-field members. +  The default value of this option is determined by the application binary  interface for the target processor.