Bug 45070 - Miscompiled c++ class with packed attribute on ARM with -Os optimizations (Qt 4.6.2)
Summary: Miscompiled c++ class with packed attribute on ARM with -Os optimizations (Qt...
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: target (show other bugs)
Version: 4.5.0
: P3 normal
Target Milestone: 4.4.5
Assignee: Ian Bolton
URL:
Keywords: wrong-code
: 54414 (view as bug list)
Depends on:
Blocks:
 
Reported: 2010-07-25 23:23 UTC by Siarhei Siamashka
Modified: 2012-09-07 11:01 UTC (History)
7 users (show)

See Also:
Host:
Target: arm-unknown-linux-gnueabi
Build:
Known to work: 4.3.5
Known to fail: 4.4.4 4.5.0
Last reconfirmed: 2010-09-07 09:26:10


Attachments
packed-testcase.cpp (593 bytes, text/plain)
2010-07-25 23:25 UTC, Siarhei Siamashka
Details
reduced test case in C (365 bytes, text/plain)
2010-07-26 09:33 UTC, Mikael Pettersson
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Siarhei Siamashka 2010-07-25 23:23:35 UTC
Compilation:
   arm-unknown-linux-gnueabi-g++ -Os -mcpu=cortex-a8 -o test test.cpp

Expected results:
   ./test
   65534 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0

Real results (some garbage data):
   ./test
   544 544 544 544 544 544 544 544 544 544 544 544 544 544 544 544

Note: This is not a big practical issue because Qt 4.7 does not use packed attribute for QChar anymore (a good idea because using this packed attribute results in a horribly slow code): http://qt.gitorious.org/qt/qt/commit/1ec8acd77b6c048f5a68887ac7750b0764ade598
Comment 1 Siarhei Siamashka 2010-07-25 23:25:43 UTC
Created attachment 21308 [details]
packed-testcase.cpp
Comment 2 Mikael Pettersson 2010-07-26 08:49:45 UTC
With -Os on armv5tel I see a random number repeated 16 times, with -O2 I see the expected output.  gcc-4.4 and gcc-4.5 are affected.  (Can't test 4.6 or 4.3 right now.)
Comment 3 Mikael Pettersson 2010-07-26 09:33:00 UTC
Created attachment 21312 [details]
reduced test case in C

You don't need C++ to trigger the bug.  Proper C with a function that may recurse before returning a packed struct suffices.
Comment 4 Siarhei Siamashka 2010-07-28 07:16:05 UTC
Could not reproduce the problem with gcc 4.3.5

Disassembly of pr45070.o:

0000000c <next>:
   c:   e92d401f        push    {r0, r1, r2, r3, r4, lr}
  10:   e890000c        ldm     r0, {r2, r3}
  14:   e1a04000        mov     r4, r0
  18:   e1520003        cmp     r2, r3
  1c:   b3a03000        movlt   r3, #0
  20:   ba000014        blt     78 <next+0x6c>
  24:   e5903008        ldr     r3, [r0, #8]
  28:   e3530000        cmp     r3, #0
  2c:   0a00000e        beq     6c <next+0x60>
  30:   e3a03000        mov     r3, #0
  34:   e5803008        str     r3, [r0, #8]
  38:   e2800004        add     r0, r0, #4
  3c:   ebffffef        bl      0 <fetch>
  40:   e1a00004        mov     r0, r4
  44:   ebfffff0        bl      c <next>
  48:   e1a00800        lsl     r0, r0, #16
  4c:   e1a00840        asr     r0, r0, #16
  50:   e5cd0000        strb    r0, [sp]
  54:   e1a00420        lsr     r0, r0, #8
  58:   e5cd0001        strb    r0, [sp, #1]
  5c:   e1dd30b0        ldrh    r3, [sp]
  60:   e1cd30bc        strh    r3, [sp, #12]
  64:   e1dd30bc        ldrh    r3, [sp, #12]
  68:   ea000002        b       78 <next+0x6c>
  6c:   e3a03001        mov     r3, #1
  70:   e5803008        str     r3, [r0, #8]
  74:   e59f3010        ldr     r3, [pc, #16]   ; 8c <next+0x80>
  78:   e1cd30bc        strh    r3, [sp, #12]
  7c:   e5dd300c        ldrb    r3, [sp, #12]
  80:   e5dd000d        ldrb    r0, [sp, #13]
  84:   e1830400        orr     r0, r3, r0, lsl #8
  88:   e8bd801f        pop     {r0, r1, r2, r3, r4, pc}

^^^ POP instruction just overwrites return value in r0 register here

  8c:   0000ffff        .word   0x0000ffff

Looks like the function gets treated as if it were returning 'void'.
Comment 5 Siarhei Siamashka 2010-07-28 07:18:11 UTC
The disassembly chunk from the comment above was from gcc 4.5.0, using '-Os -match=armv5te' options.
Comment 6 Siarhei Siamashka 2010-07-28 08:37:03 UTC
'arm_size_return_regs()' returns 2 when generating epilogue for 'next' function here. And as a result, return value not registered in the mask, causing it to be clobbered.

Would the following patch be the right fix?

Index: gcc/config/arm/arm.c
===================================================================
--- gcc/config/arm/arm.c        (revision 162411)
+++ gcc/config/arm/arm.c        (working copy)
@@ -13705,7 +13705,7 @@
                  && !crtl->tail_call_emit)
                {
                  unsigned long mask;
-                 mask = (1 << (arm_size_return_regs() / 4)) - 1;
+                 mask = (1 << ((arm_size_return_regs() + 3) / 4)) - 1;
                  mask ^= 0xf;
                  mask &= ~saved_regs_mask;
                  reg = 0;
Comment 7 Ramana Radhakrishnan 2010-07-28 09:01:18 UTC
Thanks for the analysis, yes that appears to be the nub of the problem with the result being removed . I see the same problem on trunk - 

Patches should however be submitted to gcc-patches@gcc.gnu.org after appropriate regression testing.

cheers
Ramana
Comment 8 Ramana Radhakrishnan 2010-07-28 09:22:02 UTC
(In reply to comment #7)
> Thanks for the analysis, yes that appears to be the nub of the problem with the
> result being removed . I see the same problem on trunk - 
> 


I just realized that this is a packed structure and probably need to look up the semantics of this in the AAPCS. IIRC the AAPCS states that it doesn't support packed structures or bitfields at exported interfaces. 

Adding Richard for some ABI commentary.

cheers
Ramana

Comment 9 Richard Earnshaw 2010-07-28 09:34:58 UTC
(In reply to comment #8)

> I just realized that this is a packed structure and probably need to look up
> the semantics of this in the AAPCS. IIRC the AAPCS states that it doesn't
> support packed structures or bitfields at exported interfaces. 

That just means the ABI doesn't specify the behaviour here.  Portable code shouldn't use this feature, but that doesn't mean that a compiler can't support it as an extension.
Comment 10 Mikael Pettersson 2010-07-28 09:45:39 UTC
(In reply to comment #8)
> I just realized that this is a packed structure and probably need to look up
> the semantics of this in the AAPCS. IIRC the AAPCS states that it doesn't
> support packed structures or bitfields at exported interfaces. 

In the C test case the function returning a packed struct is `static' and called only by name from within the same translation unit.  Surely that's not an "exported interface".
Comment 11 Ian Bolton 2010-08-03 17:13:38 UTC
I have regression-tested the patch for gcc and g++ with target=arm-eabi and created a regression testsuite case for it, so I expect to be able to submit both to the mailing list for review in the next day or so.

Comment 12 Raúl Porcel 2010-08-14 15:33:02 UTC
Any news? :)
Comment 13 Siarhei Siamashka 2010-08-14 16:28:28 UTC
(In reply to comment #12)
> Any news? :)

http://gcc.gnu.org/ml/gcc-patches/2010-08/msg00894.html
Comment 14 Ramana Radhakrishnan 2010-08-19 08:28:30 UTC
Subject: Bug 45070

Author: ramana
Date: Thu Aug 19 08:27:59 2010
New Revision: 163367

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=163367
Log:
For Ian Bolton <ian.bolton@arm.com>

2010-08-19  Ian Bolton  <ian.bolton@arm.com>

	PR target/45070
	* gcc.c-torture/execute/pr45070.c: New.
	* config/arm/arm.c (arm_output_epilogue): Ensure that return
	 value of size 1-3 is handled correctly.

Added:
    trunk/gcc/testsuite/gcc.c-torture/execute/pr45070.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/arm/arm.c
    trunk/gcc/testsuite/ChangeLog

Comment 15 Ian Bolton 2010-09-02 13:05:53 UTC
Subject: Bug 45070

Author: ibolton
Date: Thu Sep  2 13:05:30 2010
New Revision: 163774

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=163774
Log:
2010-09-02  Ian Bolton  <ian.bolton@arm.com>

	Backport from mainline
	2010-09-01  Ian Bolton  <ian.bolton@arm.com>

	* Makefile.in (tree-switch-conversion.o): Update dependencies.
	
	2010-08-19  Ian Bolton  <ian.bolton@arm.com>
	
	PR target/45070
	* config/arm/arm.c (arm_output_epilogue): Ensure that return
	value of size 1-3 is handled correctly.

	* gcc.c-torture/execute/pr45070.c: New.

	2010-08-19  Ian Bolton  <ian.bolton@arm.com>

	* tree-switch-conversion.c (gen_inbound_check): Ensure that the
	type for the conditional has wide enough range.

	* g++.dg/pr44328.C: New test.

	2010-08-07  Marcus Shawcroft  <marcus.shawcroft@arm.com>
	
	* config/arm/linux-atomic.c (SUBWORD_VAL_CAS): Instantiate with
	'unsigned short' and 'unsigned char' instead of 'short' and 'char'.
	(SUBWORD_BOOL_CAS): Likewise.
	(SUBWORD_SYNC_OP): Likewise.
	(SUBWORD_TEST_AND_SET): Likewise.
	(FETCH_AND_OP_WORD): Parenthesise INF_OP
	(SUBWORD_SYNC_OP): Likewise.
	(OP_AND_FETCH_WORD): Likewise.

	* lib/target-supports.exp: (check_effective_target_sync_int_long):
	Add arm*-*-linux-gnueabi.
	(check_effective_target_sync_char_short): Likewise.

Added:
    branches/gcc-4_5-branch/gcc/testsuite/g++.dg/pr44328.C
      - copied unchanged from r163367, trunk/gcc/testsuite/g++.dg/pr44328.C
    branches/gcc-4_5-branch/gcc/testsuite/gcc.c-torture/execute/pr45070.c
      - copied unchanged from r163367, trunk/gcc/testsuite/gcc.c-torture/execute/pr45070.c
Modified:
    branches/gcc-4_5-branch/gcc/ChangeLog
    branches/gcc-4_5-branch/gcc/Makefile.in
    branches/gcc-4_5-branch/gcc/config/arm/arm.c
    branches/gcc-4_5-branch/gcc/config/arm/linux-atomic.c
    branches/gcc-4_5-branch/gcc/testsuite/ChangeLog
    branches/gcc-4_5-branch/gcc/testsuite/lib/target-supports.exp
    branches/gcc-4_5-branch/gcc/tree-switch-conversion.c

Comment 16 Raúl Porcel 2010-09-04 16:41:50 UTC
I'd like it backported to 4.4 if possible, thanks
Comment 17 Ian Bolton 2010-09-07 09:26:10 UTC
(In reply to comment #16)
> I'd like it backported to 4.4 if possible, thanks
> 

Just awaiting approval on the mailing list.  It's ready to go.
Comment 18 Ian Bolton 2010-09-07 11:07:54 UTC
Subject: Bug 45070

Author: ibolton
Date: Tue Sep  7 11:07:31 2010
New Revision: 163945

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=163945
Log:
2010-09-07  Ian Bolton  <ian.bolton@arm.com>

	Backport from mainline
	2010-09-01  Ian Bolton  <ian.bolton@arm.com>

	* Makefile.in (tree-switch-conversion.o): Update dependencies.
	
	2010-08-19  Ian Bolton  <ian.bolton@arm.com>
	
	PR target/45070
	* config/arm/arm.c (arm_output_epilogue): Ensure that return
	value of size 1-3 is handled correctly.

	* gcc.c-torture/execute/pr45070.c: New.

	2010-08-19  Ian Bolton  <ian.bolton@arm.com>

	* tree-switch-conversion.c (gen_inbound_check): Ensure that the
	type for the conditional has wide enough range.

	* g++.dg/pr44328.C: New test.

	2010-08-07  Marcus Shawcroft  <marcus.shawcroft@arm.com>
	
	* config/arm/linux-atomic.c (SUBWORD_VAL_CAS): Instantiate with
	'unsigned short' and 'unsigned char' instead of 'short' and 'char'.
	(SUBWORD_BOOL_CAS): Likewise.
	(SUBWORD_SYNC_OP): Likewise.
	(SUBWORD_TEST_AND_SET): Likewise.
	(FETCH_AND_OP_WORD): Parenthesise INF_OP
	(SUBWORD_SYNC_OP): Likewise.
	(OP_AND_FETCH_WORD): Likewise.

	* lib/target-supports.exp: (check_effective_target_sync_int_long):
	Add arm*-*-linux-gnueabi.
	(check_effective_target_sync_char_short): Likewise.


Added:
    branches/gcc-4_4-branch/gcc/testsuite/g++.dg/pr44328.C
      - copied unchanged from r163774, branches/gcc-4_5-branch/gcc/testsuite/g++.dg/pr44328.C
    branches/gcc-4_4-branch/gcc/testsuite/gcc.c-torture/execute/pr45070.c
      - copied unchanged from r163774, branches/gcc-4_5-branch/gcc/testsuite/gcc.c-torture/execute/pr45070.c
Modified:
    branches/gcc-4_4-branch/gcc/ChangeLog
    branches/gcc-4_4-branch/gcc/Makefile.in
    branches/gcc-4_4-branch/gcc/config/arm/arm.c
    branches/gcc-4_4-branch/gcc/config/arm/linux-atomic.c
    branches/gcc-4_4-branch/gcc/testsuite/ChangeLog
    branches/gcc-4_4-branch/gcc/testsuite/lib/target-supports.exp
    branches/gcc-4_4-branch/gcc/tree-switch-conversion.c

Comment 19 Richard Earnshaw 2010-09-14 14:05:52 UTC
Fixed in all maintained releases.
Comment 20 amker 2012-09-04 09:36:48 UTC
Author: amker
Date: Tue Sep  4 09:36:44 2012
New Revision: 190919

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=190919
Log:

	PR target/45070
	* config/arm/arm.c (thumb1_extra_regs_pushed): Handle return value of size
	less than 4 bytes by using macro ARM_NUM_INTS.
	(thumb1_unexpanded_epilogue): Use macro ARM_NUM_INTS.


Modified:
    trunk/gcc/config/arm/arm.c
Comment 21 Ramana Radhakrishnan 2012-09-04 19:42:16 UTC
*** Bug 54414 has been marked as a duplicate of this bug. ***
Comment 22 amker 2012-09-05 10:50:00 UTC
Author: amker
Date: Wed Sep  5 10:49:56 2012
New Revision: 190970

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=190970
Log:
	Backport from 2012-09-04 mainline r190919

	PR target/45070
	* config/arm/arm.c (thumb1_extra_regs_pushed): Handle return value of size
	less than 4 bytes by using macro ARM_NUM_INTS.
	(thumb1_unexpanded_epilogue): Use macro ARM_NUM_INTS.

Modified:
    branches/gcc-4_7-branch/gcc/ChangeLog
    branches/gcc-4_7-branch/gcc/config/arm/arm.c
Comment 23 amker 2012-09-05 10:54:11 UTC
Author: amker
Date: Wed Sep  5 10:54:08 2012
New Revision: 190971

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=190971
Log:
	Backport from 2012-09-04 mainline r190919

	PR target/45070
	* config/arm/arm.c (thumb1_extra_regs_pushed): Handle return value of
	size less than 4 bytes by using macro ARM_NUM_INTS.
	(thumb1_unexpanded_epilogue): Use macro ARM_NUM_INTS.

Modified:
    branches/gcc-4_6-branch/gcc/ChangeLog
    branches/gcc-4_6-branch/gcc/config/arm/arm.c
Comment 24 amker 2012-09-07 10:50:40 UTC
Author: amker
Date: Fri Sep  7 10:50:35 2012
New Revision: 191067

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=191067
Log:
	Backport from 2012-09-04 mainline r190919

	PR target/45070
	* config/arm/arm.c (thumb1_extra_regs_pushed): Handle return value of
	size less than 4 bytes by using macro ARM_NUM_INTS.
	(thumb1_unexpanded_epilogue): Use macro ARM_NUM_INTS.

Modified:
    branches/ARM/embedded-4_7-branch/gcc/ChangeLog.arm
    branches/ARM/embedded-4_7-branch/gcc/config/arm/arm.c
Comment 25 amker 2012-09-07 11:01:02 UTC
Author: amker
Date: Fri Sep  7 11:00:52 2012
New Revision: 191068

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=191068
Log:
	Backport from 2012-09-04 mainline r190919

	PR target/45070
	* config/arm/arm.c (thumb1_extra_regs_pushed): Handle return value of
	size less than 4 bytes by using macro ARM_NUM_INTS.
	(thumb1_unexpanded_epilogue): Use macro ARM_NUM_INTS.

Modified:
    branches/ARM/embedded-4_6-branch/gcc/ChangeLog.arm
    branches/ARM/embedded-4_6-branch/gcc/config/arm/arm.c