Issue 5434084: [google] Backport r171347 and r181549 from trunk (strict volatile bitfield)
|
Created:
13 years, 4 months ago by dougkwan Modified:
13 years, 4 months ago Reviewers:
jakub, mikestump, rearnsha, richard.guenther, Diego Novillo, joseph CC:
jingyu, gcc-patches_gcc.gnu.org Base URL:
svn+ssh://gcc.gnu.org/svn/gcc/branches/google/gcc-4_6/ Visibility:
Public. |
Patch Set 1 #
MessagesTotal messages: 10
Hi Diego,
This is a backport for two upstream patches into our 4.6 branch.
I submitted the first patch by Julian a while ago for backport but
Richard Earnshaw pointed out a problem with the first patch. The second
patch from Joey fixes that problem. This was tested on x86 and ARM.
-Doug
2011-11-22 Doug Kwan <dougkwan@google.com>
Backport r171347 and r181549 from trunk.
gcc/
2011-03-23 Julian Brown <julian@codesourcery.com>
* expr.c (expand_expr_real_1): Only use BLKmode for volatile
accesses which are not naturally aligned.
2011-11-20 Joey Ye <joey.ye@arm.com>
* expr.c (expand_expr_real_1): Correctly handle strict volatile
bitfield loads smaller than mode size.
gcc/testsuite/
2011-11-20 Joey Ye <joey.ye@arm.com>
* gcc.dg/volatile-bitfields-1.c: New.
Index: gcc/testsuite/gcc.dg/volatile-bitfields-1.c
===================================================================
--- gcc/testsuite/gcc.dg/volatile-bitfields-1.c (revision 0)
+++ gcc/testsuite/gcc.dg/volatile-bitfields-1.c (revision 0)
@@ -0,0 +1,23 @@
+/* { dg-options "-fstrict-volatile-bitfields" } */
+/* { dg-do run } */
+
+extern int puts(const char *);
+extern void abort(void) __attribute__((noreturn));
+
+typedef struct {
+ volatile unsigned short a:8, b:8;
+} BitStruct;
+
+BitStruct bits = {1, 2};
+
+void check(int i, int j)
+{
+ if (i != 1 || j != 2) puts("FAIL"), abort();
+}
+
+int main ()
+{
+ check(bits.a, bits.b);
+
+ return 0;
+}
Index: gcc/expr.c
===================================================================
--- gcc/expr.c (revision 181550)
+++ gcc/expr.c (working copy)
@@ -9200,8 +9200,16 @@
&& modifier != EXPAND_CONST_ADDRESS
&& modifier != EXPAND_INITIALIZER)
/* If the field is volatile, we always want an aligned
- access. */
- || (volatilep && flag_strict_volatile_bitfields > 0)
+ access. Do this in following two situations:
+ 1. the access is not already naturally
+ aligned, otherwise "normal" (non-bitfield) volatile fields
+ become non-addressable.
+ 2. the bitsize is narrower than the access size. Need
+ to extract bitfields from the access. */
+ || (volatilep && flag_strict_volatile_bitfields > 0
+ && (bitpos % GET_MODE_ALIGNMENT (mode) != 0
+ || (mode1 != BLKmode
+ && bitsize < GET_MODE_SIZE (mode1) * BITS_PER_UNIT)))
/* If the field isn't aligned enough to fetch as a memref,
fetch it as a bit field. */
|| (mode1 != BLKmode
--
This patch is available for review at http://codereview.appspot.com/5434084
Sign in to reply to this message.
On 11/29/11 20:59, Doug Kwan wrote: > Hi Diego, > > This is a backport for two upstream patches into our 4.6 branch. > I submitted the first patch by Julian a while ago for backport but > Richard Earnshaw pointed out a problem with the first patch. The second > patch from Joey fixes that problem. This was tested on x86 and ARM. > > -Doug > > 2011-11-22 Doug Kwan<dougkwan@google.com> > > Backport r171347 and r181549 from trunk. > > gcc/ > 2011-03-23 Julian Brown<julian@codesourcery.com> > > * expr.c (expand_expr_real_1): Only use BLKmode for volatile > accesses which are not naturally aligned. > > 2011-11-20 Joey Ye<joey.ye@arm.com> > > * expr.c (expand_expr_real_1): Correctly handle strict volatile > bitfield loads smaller than mode size. > > gcc/testsuite/ > 2011-11-20 Joey Ye<joey.ye@arm.com> > > * gcc.dg/volatile-bitfields-1.c: New. OK. Diego.
Sign in to reply to this message.
On Tue, Nov 29, 2011 at 05:59:53PM -0800, Doug Kwan wrote: > This is a backport for two upstream patches into our 4.6 branch. > I submitted the first patch by Julian a while ago for backport but > Richard Earnshaw pointed out a problem with the first patch. The second > patch from Joey fixes that problem. This was tested on x86 and ARM. Why hasn't this been proposed for upstream 4.6 instead? See PR51442. > 2011-11-22 Doug Kwan <dougkwan@google.com> > > Backport r171347 and r181549 from trunk. > > gcc/ > 2011-03-23 Julian Brown <julian@codesourcery.com> > > * expr.c (expand_expr_real_1): Only use BLKmode for volatile > accesses which are not naturally aligned. > > 2011-11-20 Joey Ye <joey.ye@arm.com> > > * expr.c (expand_expr_real_1): Correctly handle strict volatile > bitfield loads smaller than mode size. > > gcc/testsuite/ > 2011-11-20 Joey Ye <joey.ye@arm.com> > > * gcc.dg/volatile-bitfields-1.c: New. Jakub
Sign in to reply to this message.
On Wed, Dec 7, 2011 at 1:34 PM, Jakub Jelinek <jakub@redhat.com> wrote: > On Tue, Nov 29, 2011 at 05:59:53PM -0800, Doug Kwan wrote: >> This is a backport for two upstream patches into our 4.6 branch. >> I submitted the first patch by Julian a while ago for backport but >> Richard Earnshaw pointed out a problem with the first patch. The second >> patch from Joey fixes that problem. This was tested on x86 and ARM. > > Why hasn't this been proposed for upstream 4.6 instead? > See PR51442. It's indeed annoying to see arm related backports only going to vendor branches, not the last officially maintained GCC release branch (4.6). I keep getting local requests to include random patches to our 4.6 build to make "arm work at all". It seriously seems like having arm-linux-gnueabi as a primary target is a lie to our users. Arm maintainers - please consider maintaining at least the current release series and shift focus away from your vendor branches. Thanks, Richard. >> 2011-11-22 Doug Kwan <dougkwan@google.com> >> >> Backport r171347 and r181549 from trunk. >> >> gcc/ >> 2011-03-23 Julian Brown <julian@codesourcery.com> >> >> * expr.c (expand_expr_real_1): Only use BLKmode for volatile >> accesses which are not naturally aligned. >> >> 2011-11-20 Joey Ye <joey.ye@arm.com> >> >> * expr.c (expand_expr_real_1): Correctly handle strict volatile >> bitfield loads smaller than mode size. >> >> gcc/testsuite/ >> 2011-11-20 Joey Ye <joey.ye@arm.com> >> >> * gcc.dg/volatile-bitfields-1.c: New. > > Jakub
Sign in to reply to this message.
On 07/12/11 13:02, Richard Guenther wrote: > On Wed, Dec 7, 2011 at 1:34 PM, Jakub Jelinek <jakub@redhat.com> wrote: >> On Tue, Nov 29, 2011 at 05:59:53PM -0800, Doug Kwan wrote: >>> This is a backport for two upstream patches into our 4.6 branch. >>> I submitted the first patch by Julian a while ago for backport but >>> Richard Earnshaw pointed out a problem with the first patch. The second >>> patch from Joey fixes that problem. This was tested on x86 and ARM. >> >> Why hasn't this been proposed for upstream 4.6 instead? >> See PR51442. > > It's indeed annoying to see arm related backports only going to > vendor branches, not the last officially maintained GCC release > branch (4.6). I keep getting local requests to include random > patches to our 4.6 build to make "arm work at all". It seriously > seems like having arm-linux-gnueabi as a primary target is a lie to our users. > > Arm maintainers - please consider maintaining at least the current > release series and shift focus away from your vendor branches. > So this, to some extent seems to conflict with your rules for only fixing regressions. This code has always been broken in one way or another, so technically this doesn't qualify for the 4.6 branch. I think we need clearer rules (on the web site, not in a mailing list post) that describes which patches are considered acceptable on the branch and which are not. R.
Sign in to reply to this message.
On Wed, Dec 7, 2011 at 2:32 PM, Richard Earnshaw <rearnsha@arm.com> wrote: > On 07/12/11 13:02, Richard Guenther wrote: >> On Wed, Dec 7, 2011 at 1:34 PM, Jakub Jelinek <jakub@redhat.com> wrote: >>> On Tue, Nov 29, 2011 at 05:59:53PM -0800, Doug Kwan wrote: >>>> This is a backport for two upstream patches into our 4.6 branch. >>>> I submitted the first patch by Julian a while ago for backport but >>>> Richard Earnshaw pointed out a problem with the first patch. The second >>>> patch from Joey fixes that problem. This was tested on x86 and ARM. >>> >>> Why hasn't this been proposed for upstream 4.6 instead? >>> See PR51442. >> >> It's indeed annoying to see arm related backports only going to >> vendor branches, not the last officially maintained GCC release >> branch (4.6). I keep getting local requests to include random >> patches to our 4.6 build to make "arm work at all". It seriously >> seems like having arm-linux-gnueabi as a primary target is a lie to our users. >> >> Arm maintainers - please consider maintaining at least the current >> release series and shift focus away from your vendor branches. >> > > So this, to some extent seems to conflict with your rules for only fixing > regressions. This code has always been broken in one way or another, > so technically this doesn't qualify for the 4.6 branch. > > I think we need clearer rules (on the web site, not in a mailing list post) > that describes which patches are considered acceptable on the branch and > which are not. We generally accept wrong-code fixes (or rejects-valid) that are low-risk. Target maintainers have complete freedom for their targets provided a fix is really target-only (we even accept new ports or new ISAs on branches, well - in theory at least). If a maintainer thinks backporting a fix is important he can always defer to a release manager for a final decision. What we generally do not want is new middle-end functionality. And we generally raise the barrier for "low-risk" the more mature a branch is. As a general rule, if you'd point users to "use the arm-embedded branch" for bugreports you get, then you are doing sth wrong. If you say, "use the arm-embedded branch" to get smaller/faster code - well, that's ok. Pointing people to the latest official release series (in this case 4.6.x) is also ok, we're keeping too many branches active IMNSHO. Richard. > R. >
Sign in to reply to this message.
On Wed, 7 Dec 2011, Richard Guenther wrote: > code - well, that's ok. Pointing people to the latest official > release series (in this case 4.6.x) is also ok, we're keeping too > many branches active IMNSHO. As I recall we agreed in London that both 4.3 and 4.4 should be closed (after a final release from each branch) - and in general two active release branches was enough - but so far have only closed 4.3. (Closed = no datestamp updates, no snapshots, no releases, no tracking regression status in Bugzilla, not absolutely disallowing commits if someone feels it's still useful to put some fix on an old branch.) -- Joseph S. Myers joseph@codesourcery.com
Sign in to reply to this message.
On Wed, Dec 07, 2011 at 04:16:25PM +0000, Joseph S. Myers wrote: > On Wed, 7 Dec 2011, Richard Guenther wrote: > > > code - well, that's ok. Pointing people to the latest official > > release series (in this case 4.6.x) is also ok, we're keeping too > > many branches active IMNSHO. > > As I recall we agreed in London that both 4.3 and 4.4 should be closed > (after a final release from each branch) - and in general two active > release branches was enough - but so far have only closed 4.3. > > (Closed = no datestamp updates, no snapshots, no releases, no tracking > regression status in Bugzilla, not absolutely disallowing commits if > someone feels it's still useful to put some fix on an old branch.) I'll handle 4.4.7 and closing the branch in the coming weeks. Jakub
Sign in to reply to this message.
On Dec 7, 2011, at 5:32 AM, Richard Earnshaw <rearnsha@arm.com> wrote: > So this, to some extent seems to conflict with your rules for only fixing > regressions. This code has always been broken in one way or another, > so technically this doesn't qualify for the 4.6 branch. My take, does this fix improve the quality enough to be work the risk the patch brings... Having bad quality because the quality has been bad in the past, is well, a bad idea.
Sign in to reply to this message.
Sorry about my oversight. I am on vacation now until Dec 19. I don't have good internet access now and I will backport this to upstream 4.6 after I come back if the 4.6 maintainers agree to take this. -Doug On Wed, Dec 7, 2011 at 9:02 PM, Richard Guenther <richard.guenther@gmail.com> wrote: > On Wed, Dec 7, 2011 at 1:34 PM, Jakub Jelinek <jakub@redhat.com> wrote: >> On Tue, Nov 29, 2011 at 05:59:53PM -0800, Doug Kwan wrote: >>> This is a backport for two upstream patches into our 4.6 branch. >>> I submitted the first patch by Julian a while ago for backport but >>> Richard Earnshaw pointed out a problem with the first patch. The second >>> patch from Joey fixes that problem. This was tested on x86 and ARM. >> >> Why hasn't this been proposed for upstream 4.6 instead? >> See PR51442. > > It's indeed annoying to see arm related backports only going to > vendor branches, not the last officially maintained GCC release > branch (4.6). I keep getting local requests to include random > patches to our 4.6 build to make "arm work at all". It seriously > seems like having arm-linux-gnueabi as a primary target is a lie to our users. > > Arm maintainers - please consider maintaining at least the current > release series and shift focus away from your vendor branches. > > Thanks, > Richard. > >>> 2011-11-22 Doug Kwan <dougkwan@google.com> >>> >>> Backport r171347 and r181549 from trunk. >>> >>> gcc/ >>> 2011-03-23 Julian Brown <julian@codesourcery.com> >>> >>> * expr.c (expand_expr_real_1): Only use BLKmode for volatile >>> accesses which are not naturally aligned. >>> >>> 2011-11-20 Joey Ye <joey.ye@arm.com> >>> >>> * expr.c (expand_expr_real_1): Correctly handle strict volatile >>> bitfield loads smaller than mode size. >>> >>> gcc/testsuite/ >>> 2011-11-20 Joey Ye <joey.ye@arm.com> >>> >>> * gcc.dg/volatile-bitfields-1.c: New. >> >> Jakub
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
