Bug 56341 - GCC produces unaligned data access
GCC produces unaligned data access
Status: RESOLVED FIXED
Product: gcc
Classification: Unclassified
Component: middle-end
4.6.3
: P3 normal
: ---
Assigned To: Not yet assigned to anyone
: wrong-code
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-02-15 13:08 UTC by Bernd Edlinger
Modified: 2014-02-27 07:28 UTC (History)
4 users (show)

See Also:
Host:
Target: arm*-*-*
Build:
Known to work:
Known to fail: 4.7.3, 4.8.0, 4.8.1, 4.9.0
Last reconfirmed: 2013-10-30 00:00:00


Attachments
test program produces alignment faults (575 bytes, text/plain)
2013-02-15 13:08 UTC, Bernd Edlinger
Details
proposed patch (16.28 KB, patch)
2013-02-15 13:12 UTC, Bernd Edlinger
Details | Diff
proposed patch (16.36 KB, patch)
2013-02-20 01:38 UTC, Bernd Edlinger
Details | Diff
proposed patch, cleaned up (12.57 KB, patch)
2013-02-26 18:24 UTC, Bernd Edlinger
Details | Diff
another example of the alignment faults (350 bytes, text/plain)
2013-06-03 13:01 UTC, Bernd Edlinger
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Bernd Edlinger 2013-02-15 13:08:42 UTC
Created attachment 29464 [details]
test program produces alignment faults

Hello,

The attached test program causes two problems when compiled with GCC 4.6.3 for ARM:

1. test() fails to write the high word of an unaligned volatile struct member.

2. test1() crashes because it uses an unaligned word access.

This code did compile and execute correctly with GCC 4.3.2

As a workaround, the bug goes away if the code is compiled with -fno-strict-volatile-bitfields,
but this is probably less efficient code.


The attached patch is a backport of the following patch, and seems to resolve this issue:

http://gcc.gnu.org/ml/gcc-patches/2012-08/msg01449.html

the original patch ignored the alignment of the target, and fixed only the
first test case, but not the crash in the second test case.

Regards
Bernd Edlinger
Comment 1 Bernd Edlinger 2013-02-15 13:12:56 UTC
Created attachment 29465 [details]
proposed patch

attached is a patch for gcc-4.6.3 that should resolve this issue.
volatile packed struct members are accessed in words if structure
is aligned by 2 and in bytes if structure is aligned by 1.
Comment 2 Mikael Pettersson 2013-02-15 14:19:28 UTC
The test case causes alignment exceptions for me on armv5tel-linux-gnueabi, when compiled with any one of gcc 4.8, 4.7, or 4.6.  Was Sandra's patch ever applied?
Comment 3 Bernd Edlinger 2013-02-15 14:46:39 UTC
(In reply to comment #2)
> The test case causes alignment exceptions for me on armv5tel-linux-gnueabi,
> when compiled with any one of gcc 4.8, 4.7, or 4.6.  Was Sandra's patch ever
> applied?

apparently not. not in 4.6.x not in 4.7.2.

When I used the original patch the assignment in test() was fixed,
but the crash in test1() was still there, because the patch
did not pay attention to the alignment of the structure.

Therefore I added a check for the alignment in both read and
write instructions.

Regards,
Bernd Edlinger.
Comment 4 Richard Biener 2013-02-18 11:20:43 UTC
There is now better ways of implementing -fstrict-volatile-bitfields which
I repeatedly told the arm people.  Not for 4.6, but for 4.7 and trunk.
Comment 5 Sandra Loosemore 2013-02-18 15:30:16 UTC
The patch linked from the initial message was rejected.  I did not (and still do not) have the time to rewrite it; if someone else can figure out how to fix this in a way that's acceptable to the maintainers, that would be great.
Comment 6 Bernd Edlinger 2013-02-18 18:41:55 UTC
hhmm...

could some one give an example where packedp would be false but the value
is packed or unaligned?
Comment 7 Bernd Edlinger 2013-02-20 01:38:04 UTC
Created attachment 29506 [details]
proposed patch

this patch uses packedp only for the warn_misaligned_bitfield()
but does always use multiple load or stores. So even if the
packedp may be unreliable it will only have influence on the warning text.

reason:
if packedp == false the code will always use a single but mis-aligned
instruction which is known to abort at runtime. So that is always wrong.

note: there are two almost identical formula used for packedp.

packedp as it is used in extract_bit_field (old code):
        if (TYPE_PACKED (TREE_TYPE (TREE_OPERAND (exp, 0)))
            || (TREE_CODE (TREE_OPERAND (exp, 1)) == FIELD_DECL
                && DECL_PACKED (TREE_OPERAND (exp, 1))))
          packedp = true;

packedp as it is used in store_field (new code):
        if (TREE_CODE(to) == COMPONENT_REF
            && (TYPE_PACKED (TREE_TYPE (TREE_OPERAND (to, 0)))
                || (TREE_CODE (TREE_OPERAND (to, 1)) == FIELD_DECL
                    && DECL_PACKED (TREE_OPERAND (to, 1)))))
          packedp = true;

However if we can not trust the second one why should we trust the first one?

Therefore the packedp should not have influence on the code generation at all.
That would only take unnecessary risks.

Well, I think that should resolve your objections... Right?
Comment 8 Bernd Edlinger 2013-02-26 18:24:58 UTC
Created attachment 29546 [details]
proposed patch, cleaned up

in the last version of this patch the packedp parameter had only an
impact on the generated warning.

if packedp==true the warning is:
"multiple accesses to volatile structure member/bitfield
 because of packed attribute"

if packedp==false the warning is:
"mis-aligned access used for structure member/bitfield
 when a volatile object spans multiple type-sized locations,
 the compiler must choose between using a single mis-aligned access to
 preserve the volatility, or using multiple aligned accesses to avoid
 runtime faults; this code may fail at runtime if the hardware does
 not allow this access"

The second warning says in short:
"I am going to generate mis-aligned code, and I know it will fail at runtime."

However this patch is supposed to avoid mis-aligned code, at least for ARM.
Therefore it is only natural that the second warning is no longer needed.

Now I removed all packedp code in extract_bit_field and store_bit_field,
including the second warning. Fortunately that makes the patch much smaller.

I did boot-strap the patched compiler several times, and everything looks good.

TODO: remove translations of the obsolete warnings. (I dont know how to)
Comment 9 Bernd Edlinger 2013-03-27 10:36:48 UTC
Hello GCC-Maintainers,

what do you think? Should'nt this patch be in the 4.6.4 release?
Comment 10 Sandra Loosemore 2013-06-03 04:11:41 UTC
I'm working on a new patch that addresses the first problem, the failure in test().

I think the second failure is not in test1() at all, and has nothing to do with -fstrict-volatile-bitfields.  Looks to me like problem is that the expression "&x1->t1" is returning an unaligned pointer due to the packed attribute on struct test2.  It should probably not be allowed to take the address of a packed struct field, at least on targets that require strict alignment.

Hmmmm, that bug is already filed as PR 41809.

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=41809
Comment 11 Bernd Edlinger 2013-06-03 13:01:34 UTC
Created attachment 30248 [details]
another example of the alignment faults

Hello Sandra,

good that you continue to work on that bug again.

I agree that there may be two completely different aspects of this bug.
Attached you'll find a new test program that came in my mind when I
looked at PR 41809, the structure s is aligned 2 and packed.
If you make it an array of size 10, each second call of f is given
an odd pointer. But the compiler should know that because of the
aligned(2) attribute.

What is the difference to PR 41809 is this:
1. PR 41809 is not a correct C-program at all, and has never been.
   While this attached new test program is correct C program.
   previous GCC versions did compile that correctly, current GCC does not even
   emit a warning.

2. PR 41809 is not about volatile at all.
   However if you remove the "volatile" in the test program(s),
   the code is correct and does no longer use unaligned addresses.

On the other hand, "volatile" might mean that the compiler
should try not to optimize the read instructions, for instance in loops.
But of course not to an extent that the generated code is no longer valid.
Comment 12 Sandra Loosemore 2013-06-14 03:01:04 UTC
Patch for the first problem posted here:

http://gcc.gnu.org/ml/gcc-patches/2013-06/msg00750.html
Comment 13 Sandra Loosemore 2013-10-07 15:41:28 UTC
Updated patch series:

http://gcc.gnu.org/ml/gcc-patches/2013-09/msg02057.html
http://gcc.gnu.org/ml/gcc-patches/2013-09/msg02058.html
http://gcc.gnu.org/ml/gcc-patches/2013-09/msg02059.html

Unfortunately, it seems that fixing bugs with -fstrict-volatile-bitfields has been blocked by disagreement between global reviewers and target maintainers who can't agree on whether the C/C++11 memory model should take precedence over target-specific ABIs by default.  :-(
Comment 14 Ramana Radhakrishnan 2013-10-30 17:08:12 UTC
well, confirmed.
Comment 15 Bernd Edlinger 2013-12-11 16:50:07 UTC
Author: edlinger
Date: Wed Dec 11 16:50:05 2013
New Revision: 205896

URL: http://gcc.gnu.org/viewcvs?rev=205896&root=gcc&view=rev
Log:
2013-12-11  Sandra Loosemore  <sandra@codesourcery.com>

        PR middle-end/23623
        PR middle-end/48784
        PR middle-end/56341
        PR middle-end/56997

        gcc/
        * expmed.c (strict_volatile_bitfield_p): New function.
        (store_bit_field_1): Don't special-case strict volatile
        bitfields here.
        (store_bit_field): Handle strict volatile bitfields here instead.
        (store_fixed_bit_field): Don't special-case strict volatile
        bitfields here.
        (extract_bit_field_1): Don't special-case strict volatile
        bitfields here.
        (extract_bit_field): Handle strict volatile bitfields here instead.
        (extract_fixed_bit_field): Don't special-case strict volatile
        bitfields here.  Simplify surrounding code to resemble that in
        store_fixed_bit_field.
        * doc/invoke.texi (Code Gen Options): Update
        -fstrict-volatile-bitfields description.

        gcc/testsuite/
        * gcc.dg/pr23623.c: New test.
        * gcc.dg/pr48784-1.c: New test.
        * gcc.dg/pr48784-2.c: New test.
        * gcc.dg/pr56341-1.c: New test.
        * gcc.dg/pr56341-2.c: New test.
        * gcc.dg/pr56997-1.c: New test.
        * gcc.dg/pr56997-2.c: New test.
        * gcc.dg/pr56997-3.c: New test.

Added:
    trunk/gcc/testsuite/gcc.dg/pr23623.c
    trunk/gcc/testsuite/gcc.dg/pr48784-1.c
    trunk/gcc/testsuite/gcc.dg/pr48784-2.c
    trunk/gcc/testsuite/gcc.dg/pr56341-1.c
    trunk/gcc/testsuite/gcc.dg/pr56341-2.c
    trunk/gcc/testsuite/gcc.dg/pr56997-1.c
    trunk/gcc/testsuite/gcc.dg/pr56997-2.c
    trunk/gcc/testsuite/gcc.dg/pr56997-3.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/doc/invoke.texi
    trunk/gcc/expmed.c
    trunk/gcc/testsuite/ChangeLog
Comment 16 Bernd Edlinger 2013-12-11 16:59:27 UTC
Author: edlinger
Date: Wed Dec 11 16:59:24 2013
New Revision: 205897

URL: http://gcc.gnu.org/viewcvs?rev=205897&root=gcc&view=rev
Log:
2013-12-11  Bernd Edlinger  <bernd.edlinger@hotmail.de>
            Sandra Loosemore  <sandra@codesourcery.com>

        PR middle-end/23623
        PR middle-end/48784
        PR middle-end/56341
        PR middle-end/56997
        * expmed.c (strict_volatile_bitfield_p): Add bitregion_start
        and bitregion_end parameters.  Test for compliance with C++
        memory model.
        (store_bit_field): Adjust call to strict_volatile_bitfield_p.
        Add fallback logic for cases where -fstrict-volatile-bitfields
        is supposed to apply, but cannot.
        (extract_bit_field): Likewise. Use narrow_bit_field_mem and
        extract_fixed_bit_field_1 to do the extraction.
        (extract_fixed_bit_field): Revert to previous mode selection algorithm.
        Call extract_fixed_bit_field_1 to do the real work.
        (extract_fixed_bit_field_1): New function.

testsuite:        
        * gcc.dg/pr23623.c: Update to test interaction with C++
        memory model.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/expmed.c
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/gcc.dg/pr23623.c
Comment 17 Bernd Edlinger 2014-01-07 13:43:12 UTC
fixed on trunk
Comment 18 jye2 2014-02-27 07:28:39 UTC
Author: jye2
Date: Thu Feb 27 07:28:06 2014
New Revision: 208195

URL: http://gcc.gnu.org/viewcvs?rev=208195&root=gcc&view=rev
Log:
2014-02-27  Joey Ye  <joey.ye@arm.com>

        Backport mainline strict-volatile-bitfields fixes
	2013-09-28  Sandra Loosemore  <sandra@codesourcery.com>

        gcc/
        * expr.h (extract_bit_field): Remove packedp parameter.
        * expmed.c (extract_fixed_bit_field): Remove packedp parameter
        from forward declaration.
        (store_split_bit_field): Remove packedp arg from calls to
        extract_fixed_bit_field.
        (extract_bit_field_1): Remove packedp parameter and packedp
        argument from recursive calls and calls to extract_fixed_bit_field.
        (extract_bit_field): Remove packedp parameter and corresponding
        arg to extract_bit_field_1.
        (extract_fixed_bit_field): Remove packedp parameter.  Remove code
        to issue warnings.
        (extract_split_bit_field): Remove packedp arg from call to
        extract_fixed_bit_field.
        * expr.c (emit_group_load_1): Adjust calls to extract_bit_field.
        (copy_blkmode_from_reg): Likewise.
        (copy_blkmode_to_reg): Likewise.
        (read_complex_part): Likewise.
        (store_field): Likewise.
        (expand_expr_real_1): Likewise.
        * calls.c (store_unaligned_arguments_into_pseudos): Adjust call
        to extract_bit_field.
        * config/tilegx/tilegx.c (tilegx_expand_unaligned_load): Adjust
        call to extract_bit_field.
        * config/tilepro/tilepro.c (tilepro_expand_unaligned_load): Adjust
        call to extract_bit_field.
        * doc/invoke.texi (Code Gen Options): Remove mention of warnings
        and special packedp behavior from -fstrict-volatile-bitfields
        documentation.

2013-12-11  Bernd Edlinger  <bernd.edlinger@hotmail.de>

        * expr.c (expand_assignment): Remove dependency on 
        flag_strict_volatile_bitfields. Always set the memory
        access mode.
        (expand_expr_real_1): Likewise.

2013-12-11  Sandra Loosemore  <sandra@codesourcery.com>

        PR middle-end/23623
        PR middle-end/48784
        PR middle-end/56341
        PR middle-end/56997

        gcc/
        * expmed.c (strict_volatile_bitfield_p): New function.
        (store_bit_field_1): Don't special-case strict volatile
        bitfields here.
        (store_bit_field): Handle strict volatile bitfields here instead.
        (store_fixed_bit_field): Don't special-case strict volatile
        bitfields here.
        (extract_bit_field_1): Don't special-case strict volatile
        bitfields here.
        (extract_bit_field): Handle strict volatile bitfields here instead.
        (extract_fixed_bit_field): Don't special-case strict volatile
        bitfields here.  Simplify surrounding code to resemble that in
        store_fixed_bit_field.
        * doc/invoke.texi (Code Gen Options): Update
        -fstrict-volatile-bitfields description.

        gcc/testsuite/
        * gcc.dg/pr23623.c: New test.
        * gcc.dg/pr48784-1.c: New test.
        * gcc.dg/pr48784-2.c: New test.
        * gcc.dg/pr56341-1.c: New test.
        * gcc.dg/pr56341-2.c: New test.
        * gcc.dg/pr56997-1.c: New test.
        * gcc.dg/pr56997-2.c: New test.
        * gcc.dg/pr56997-3.c: New test.

2013-12-11  Bernd Edlinger  <bernd.edlinger@hotmail.de>
             Sandra Loosemore  <sandra@codesourcery.com>
        
        PR middle-end/23623
        PR middle-end/48784
        PR middle-end/56341
        PR middle-end/56997
        * expmed.c (strict_volatile_bitfield_p): Add bitregion_start
        and bitregion_end parameters.  Test for compliance with C++
        memory model.
        (store_bit_field): Adjust call to strict_volatile_bitfield_p.
        Add fallback logic for cases where -fstrict-volatile-bitfields
        is supposed to apply, but cannot.
        (extract_bit_field): Likewise. Use narrow_bit_field_mem and
        extract_fixed_bit_field_1 to do the extraction.
        (extract_fixed_bit_field): Revert to previous mode selection
algorithm.
        Call extract_fixed_bit_field_1 to do the real work.
        (extract_fixed_bit_field_1): New function.
        
testsuite:
        * gcc.dg/pr23623.c: Update to test interaction with C++
        memory model.

2013-12-11  Bernd Edlinger  <bernd.edlinger@hotmail.de>
             
        PR middle-end/59134
        * expmed.c (store_bit_field): Use narrow_bit_field_mem and
        store_fixed_bit_field_1 for -fstrict-volatile-bitfields.
        (store_fixed_bit_field): Split up.  Call store_fixed_bit_field_1
        to do the real work.
        (store_fixed_bit_field_1): New function. 
        (store_split_bit_field): Limit the unit size to the memory mode
size,
        to prevent recursion.
        
testsuite:
        * gcc.c-torture/compile/pr59134.c: New test.
        * gnat.dg/misaligned_volatile.adb: New test.

Added:
    branches/ARM/embedded-4_8-branch/gcc/testsuite/gcc.c-torture/compile/pr59134.c
    branches/ARM/embedded-4_8-branch/gcc/testsuite/gcc.dg/pr23623.c
    branches/ARM/embedded-4_8-branch/gcc/testsuite/gcc.dg/pr48784-1.c
    branches/ARM/embedded-4_8-branch/gcc/testsuite/gcc.dg/pr48784-2.c
    branches/ARM/embedded-4_8-branch/gcc/testsuite/gcc.dg/pr56341-1.c
    branches/ARM/embedded-4_8-branch/gcc/testsuite/gcc.dg/pr56341-2.c
    branches/ARM/embedded-4_8-branch/gcc/testsuite/gcc.dg/pr56997-1.c
    branches/ARM/embedded-4_8-branch/gcc/testsuite/gcc.dg/pr56997-2.c
    branches/ARM/embedded-4_8-branch/gcc/testsuite/gcc.dg/pr56997-3.c
    branches/ARM/embedded-4_8-branch/gcc/testsuite/gnat.dg/misaligned_volatile.adb
Modified:
    branches/ARM/embedded-4_8-branch/gcc/ChangeLog.arm
    branches/ARM/embedded-4_8-branch/gcc/calls.c
    branches/ARM/embedded-4_8-branch/gcc/config/tilegx/tilegx.c
    branches/ARM/embedded-4_8-branch/gcc/config/tilepro/tilepro.c
    branches/ARM/embedded-4_8-branch/gcc/doc/invoke.texi
    branches/ARM/embedded-4_8-branch/gcc/expmed.c
    branches/ARM/embedded-4_8-branch/gcc/expr.c
    branches/ARM/embedded-4_8-branch/gcc/expr.h
    branches/ARM/embedded-4_8-branch/gcc/testsuite/ChangeLog.arm