[Bug target/67246] New: MIPS: lw (load word) is generated for byte bitfield, leading to unaligned access

[Bug target/67246] New: MIPS: lw (load word) is generated for byte bitfield, leading to unaligned access

[kugel at rockbox dot org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67246

            Bug ID: 67246
           Summary: MIPS: lw (load word) is generated for byte bitfield,
                    leading to unaligned access
           Product: gcc
           Version: 5.1.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: kugel at rockbox dot org
  Target Milestone: ---

Created attachment 36194
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=36194&action=edit
test program

Hello,

I hit a problem with unaligned accesses in the Linux kernel. The kernel defines

struct iphdr {
#if defined(__LITTLE_ENDIAN_BITFIELD)
        __u8    ihl:4,
                version:4;
#elif defined (__BIG_ENDIAN_BITFIELD)
        __u8    version:4,
                ihl:4;
#endif
... more fields ...
};

Then, in a source file it does (net/core/flow_dissector.c)
   nhoff += iph->ihl * 4;


GCC generates a lw for that
 248:   8e220000        lw      v0,0(s1)
 24c:   8fa40044        lw      a0,68(sp)
 250:   7c421e00        ext     v0,v0,0x18,0x4
 254:   00021080        sll     v0,v0,0x2
 258:   00821021        addu    v0,a0,v0

The problem is that iphdr can be 16bit aligned (due to ethernet header being 14
bytes, and iphdr usually follows). If that happens, this code generates a
unaligned access which is really unwanted.

I would expect that GCC uses a lbu to load that value, avoiding the unaligned
access. I think GCCs behavior is buggy in this instance.

I attached a small test program that also shows the problem.

[Bug target/67246] MIPS: lw (load word) is generated for byte bitfield, leading to unaligned access

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67246

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
This testcase does not test what you think it tests.  Since t is on the stack,
the alignment can (and most likely will be) aligned to a 4 byte boundary.

Also the struct tester has an alignment of 2 bytes so using lh is fine.

We need a full testcase because your reduced one is fine.

[Bug target/67246] MIPS: lw (load word) is generated for byte bitfield, leading to unaligned access

[kugel at rockbox dot org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67246

--- Comment #2 from Thomas Martitz <kugel at rockbox dot org> ---
Thanks for your immediate reply.

In trying to provide a better test case I think I've found what the culprit is.

The full iphdr defintion is:

struct iphdr {
#if defined(__LITTLE_ENDIAN_BITFIELD)
        __u8    ihl:4,
                version:4;
#elif defined (__BIG_ENDIAN_BITFIELD)
        __u8    version:4,
                ihl:4;
#else
#error  "Please fix <asm/byteorder.h>"
#endif
        __u8    tos;
        __be16  tot_len;
        __be16  id;
        __be16  frag_off;
        __u8    ttl;
        __u8    protocol;
        __sum16 check;
        __be32  saddr;
        __be32  daddr;
        /*The options start here. */
};

Note that it ends with 32bit saddr and daddr fields.

If I change these to __be16, then the following code is generated:
 248:   96220000        lhu     v0,0(s1)
 24c:   8fa40044        lw      a0,68(sp)
 250:   7c421a00        ext     v0,v0,0x8,0x4
 254:   00021080        sll     v0,v0,0x2
 258:   00821021        addu    v0,a0,v0

So I guess that means the 32bit fields change the alignment of the whole struct
to 4 byte. And then the compiler assumes iph must be 4-byte aligned (in a
conformant program).

The linux code carefully avoids accessing saddr and daddr fields near this code
(not shown above) so I guess it's aware of the potentially unaligned access.

So I'm not sure anymore who's on fault now

[Bug target/67246] MIPS: lw (load word) is generated for byte bitfield, leading to unaligned access

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67246

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|---                         |INVALID

--- Comment #3 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Thomas Martitz from comment #2)
> Thanks for your immediate reply.
> 
> In trying to provide a better test case I think I've found what the culprit
> is.
> 
> The full iphdr defintion is:
> 
> struct iphdr {
> #if defined(__LITTLE_ENDIAN_BITFIELD)
> 	__u8	ihl:4,
> 		version:4;
> #elif defined (__BIG_ENDIAN_BITFIELD)
> 	__u8	version:4,
>   		ihl:4;
> #else
> #error	"Please fix <asm/byteorder.h>"
> #endif
> 	__u8	tos;
> 	__be16	tot_len;
> 	__be16	id;
> 	__be16	frag_off;
> 	__u8	ttl;
> 	__u8	protocol;
> 	__sum16	check;
> 	__be32	saddr;
> 	__be32	daddr;
> 	/*The options start here. */
> };
> 
> Note that it ends with 32bit saddr and daddr fields.
> 
> If I change these to __be16, then the following code is generated:
>  248:   96220000        lhu     v0,0(s1)
>  24c:   8fa40044        lw      a0,68(sp)
>  250:   7c421a00        ext     v0,v0,0x8,0x4
>  254:   00021080        sll     v0,v0,0x2
>  258:   00821021        addu    v0,a0,v0
> 
> So I guess that means the 32bit fields change the alignment of the whole
> struct to 4 byte. And then the compiler assumes iph must be 4-byte aligned
> (in a conformant program).

yes that is correct.  The struct has alignment of 4 byte when there is a field
of 4 bytes long (unless you mark the struct to have a different alignment).

> 
> The linux code carefully avoids accessing saddr and daddr fields near this
> code (not shown above) so I guess it's aware of the potentially unaligned
> access.
> 
> So I'm not sure anymore who's on fault now

The linux kernel fault.  Does not matter what about the code around it or not.
>From gcc-bugs-return-494969-listarch-gcc-bugs=gcc.gnu.org@gcc.gnu.org Mon Aug 17 09:55:18 2015
Return-Path: <gcc-bugs-return-494969-listarch-gcc-bugs=gcc.gnu.org@gcc.gnu.org>
Delivered-To: listarch-gcc-bugs@gcc.gnu.org
Received: (qmail 39223 invoked by alias); 17 Aug 2015 09:55:18 -0000
Mailing-List: contact gcc-bugs-help@gcc.gnu.org; run by ezmlm
Precedence: bulk
List-Id: <gcc-bugs.gcc.gnu.org>
List-Archive: <http://gcc.gnu.org/ml/gcc-bugs/>
List-Post: <mailto:gcc-bugs@gcc.gnu.org>
List-Help: <mailto:gcc-bugs-help@gcc.gnu.org>
Sender: gcc-bugs-owner@gcc.gnu.org
Delivered-To: mailing list gcc-bugs@gcc.gnu.org
Received: (qmail 39185 invoked by uid 48); 17 Aug 2015 09:55:13 -0000
From: "kugel at rockbox dot org" <gcc-bugzilla@gcc.gnu.org>
To: gcc-bugs@gcc.gnu.org
Subject: [Bug target/67246] MIPS: lw (load word) is generated for byte bitfield, leading to unaligned access
Date: Mon, 17 Aug 2015 09:55:00 -0000
X-Bugzilla-Reason: CC
X-Bugzilla-Type: changed
X-Bugzilla-Watch-Reason: None
X-Bugzilla-Product: gcc
X-Bugzilla-Component: target
X-Bugzilla-Version: 5.1.0
X-Bugzilla-Keywords:
X-Bugzilla-Severity: normal
X-Bugzilla-Who: kugel at rockbox dot org
X-Bugzilla-Status: UNCONFIRMED
X-Bugzilla-Resolution:
X-Bugzilla-Priority: P3
X-Bugzilla-Assigned-To: unassigned at gcc dot gnu.org
X-Bugzilla-Target-Milestone: ---
X-Bugzilla-Flags:
X-Bugzilla-Changed-Fields: bug_status resolution
Message-ID: <bug-67246-4-zq2vkFXFJj@http.gcc.gnu.org/bugzilla/>
In-Reply-To: <bug-67246-4@http.gcc.gnu.org/bugzilla/>
References: <bug-67246-4@http.gcc.gnu.org/bugzilla/>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
X-Bugzilla-URL: http://gcc.gnu.org/bugzilla/
Auto-Submitted: auto-generated
MIME-Version: 1.0
X-SW-Source: 2015-08/txt/msg01111.txt.bz2
Content-length: 536

https://gcc.gnu.org/bugzilla/show_bug.cgi?idg246

Thomas Martitz <kugel at rockbox dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |UNCONFIRMED
         Resolution|INVALID                     |---

--- Comment #4 from Thomas Martitz <kugel at rockbox dot org> ---
Well the question is still why it access this bitfield as 4-byte word, when the
bitfield is declared as a byte (__u8)

[Bug target/67246] MIPS: lw (load word) is generated for byte bitfield, leading to unaligned access

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67246

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|---                         |INVALID

--- Comment #5 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Because that is what GCC decides to use.  If you want something different then
don't use bitfields.

[Bug target/67246] MIPS: lw (load word) is generated for byte bitfield, leading to unaligned access

[kugel at rockbox dot org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67246

Thomas Martitz <kugel at rockbox dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |UNCONFIRMED
         Resolution|INVALID                     |---

--- Comment #6 from Thomas Martitz <kugel at rockbox dot org> ---
Sorry to annoy you with re-open it's not fully resolved for me.

You proposed solution of specifying a different alignment has no effect.

struct iphdr {
#if defined(__LITTLE_ENDIAN_BITFIELD)
        __u8    ihl:4,
                version:4;
#elif defined (__BIG_ENDIAN_BITFIELD)
        __u8    version:4,
                ihl:4;
#else
#error  "Please fix <asm/byteorder.h>"
#endif
        __u8    tos;
        __be16  tot_len;
        __be16  id;
        __be16  frag_off;
        __u8    ttl;
        __u8    protocol;
        __sum16 check;
        __be32  saddr;
        __be32  daddr;
        /*The options start here. */
} __attribute((aligned(2)));

Generated code:
                nhoff += iph->ihl * 4;
 248:   8e220000        lw      v0,0(s1)
 24c:   8fa40044        lw      a0,68(sp)
 250:   7c421e00        ext     v0,v0,0x18,0x4
 254:   00021080        sll     v0,v0,0x2
 258:   00821021        addu    v0,a0,v0

I understand you that this should work?

[Bug target/67246] MIPS: lw (load word) is generated for byte bitfield, leading to unaligned access

[schwab@linux-m68k.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67246

Andreas Schwab <schwab@linux-m68k.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|---                         |INVALID

--- Comment #7 from Andreas Schwab <schwab@linux-m68k.org> ---
You cannot decrease the alignment without packed.  Please refer to
gcc-help@gcc.gnu.org for any further help.