[U-Boot] Data Abort with gcc 7.1

Wed Jul 12 14:41:52 UTC 2017

> On 12 Jul 2017, at 16:34, Tom Rini <trini at konsulko.com> wrote:
> 
> On Wed, Jul 12, 2017 at 04:20:52PM +0200, Maxime Ripard wrote:
>> On Tue, Jul 11, 2017 at 07:59:21PM +0200, Dr. Philipp Tomsich wrote:
>>> Maxime,
>>> 
>>>> On 11 Jul 2017, at 18:59, Tom Rini <trini at konsulko.com> wrote:
>>>> 
>>>> On Tue, Jul 11, 2017 at 06:54:55PM +0200, Maxime Ripard wrote:
>>>>> Hi,
>>>>> 
>>>>> I recently got a gcc 7.1 based toolchain, and it seems like it
>>>>> generates unaligned code, specifically in the net_set_ip_header
>>>>> function in my case.
>>>>> 
>>>>> Whenever some packet is sent, this data abort is triggered:
>>>>> 
>>>>> => setenv ipaddr 10.42.0.1; ping 10.42.0.254
>>>>> using musb-hdrc, OUT ep1out IN ep1in STATUS ep2in
>>>>> MAC de:ad:be:ef:00:01
>>>>> HOST MAC de:ad:be:af:00:00
>>>>> RNDIS ready
>>>>> musb-hdrc: peripheral reset irq lost!
>>>>> high speed config #2: 2 mA, Ethernet Gadget, using RNDIS
>>>>> USB RNDIS network up!
>>>>> Using usb_ether device
>>>>> data abort
>>>>> pc : [<7ff9db10>]	   lr : [<7ff9f00c>]
>>>>> reloc pc : [<4a043b10>]	   lr : [<4a04500c>]
>>>>> sp : 7bf37cc8  ip : 00000000	 fp : 7ff6236c
>>>>> r10: 7ffed2b8  r9 : 7bf39ee8	 r8 : 7ffed2b8
>>>>> r7 : 00000001  r6 : 00000000	 r5 : 0000002a  r4 : 7ffed30e
>>>>> r3 : 14000045  r2 : 01002a0a	 r1 : fe002a0a  r0 : 7ffed30e
>>>>> Flags: nZCv  IRQs off  FIQs off  Mode SVC_32
>>>>> Resetting CPU ...
>>>>> 
>>>>> 
>>>>> Running objdump on it gives us this:
>>>>> 
>>>>> 4a043b04 <net_set_ip_header>:
>>>>> 
>>>>> 	/*
>>>>> 	 *	Construct an IP header.
>>>>> 	 */
>>>>> 	/* IP_HDR_SIZE / 4 (not including UDP) */
>>>>> 	ip->ip_hl_v  = 0x45;
>>>>> 4a043b04:	e59f3074 	ldr	r3, [pc, #116]	; 4a043b80 <net_set_ip_header+0x7c>
>>>>> {
>>>>> 4a043b08:	e92d4013 	push	{r0, r1, r4, lr}
>>>>> 4a043b0c:	e1a04000 	mov	r4, r0
>>>>> 	ip->ip_hl_v  = 0x45;
>>>>> 4a043b10:	e5803000 	str	r3, [r0] <---- Abort
>>>>> 	ip->ip_tos   = 0;
>>>>> 	ip->ip_len   = htons(IP_HDR_SIZE);
>>>>> 	ip->ip_id    = htons(net_ip_id++);
>>>>> 4a043b14:	e59f3068 	ldr	r3, [pc, #104]	; 4a043b84 <net_set_ip_header+0x80>
>>>>> 
>>>>> It seems like r0 is indeed set to an unaligned address (0x7ffed30e)
>>>>> for some reason.
>>>>> 
>>>>> Using a Linaro 6.3 toolchain works on the same commit with the same
>>>>> config, so it really seems to be a compiler-related issue.
>>>>> 
>>>>> It generates this code:
>>>>> 
>>>>> 4a043ec4 <net_set_ip_header>:
>>>>> 
>>>>> 	/*
>>>>> 	 *	Construct an IP header.
>>>>> 	 */
>>>>> 	/* IP_HDR_SIZE / 4 (not including UDP) */
>>>>> 	ip->ip_hl_v  = 0x45;
>>>>> 4a043ec4:	e3a03045 	mov	r3, #69	; 0x45
>>>>> {
>>>>> 4a043ec8:	e92d4013 	push	{r0, r1, r4, lr}
>>>>> 4a043ecc:	e1a04000 	mov	r4, r0
>>>>> 	ip->ip_hl_v  = 0x45;
>>>>> 4a043ed0:	e5c03000 	strb	r3, [r0]
>>>>> 	ip->ip_tos   = 0;
>>>>> 	ip->ip_len   = htons(IP_HDR_SIZE);
>>>>> 4a043ed4:	e3a03b05 	mov	r3, #5120	; 0x1400
>>>>> 	ip->ip_tos   = 0;
>>>>> 4a043ed8:	e3a00000 	mov	r0, #0
>>>>> 	ip->ip_len   = htons(IP_HDR_SIZE);
>>>>> 4a043edc:	e1c430b2 	strh	r3, [r4, #2]
>>>>> 	ip->ip_id    = htons(net_ip_id++);
>>>>> 4a043ee0:	e59f3064 	ldr	r3, [pc, #100]	; 4a043f4c <net_set_ip_header+0x88>
>>>>> 
>>>>> And it seems like it's using an strb instruction to avoid the
>>>>> unaligned access.
>>>>> 
>>>>> As far as I know, we are passing --wno-unaligned-access, so the broken
>>>>> situation should not arise, and yet it does, so I'm a bit confused,
>>>>> and not really sure what to do from there.
>>>> 
>>>> Can you reduce the code into a testcase?  I think the first step is
>>>> filing a bug with gcc and seeing where it goes from there as yes, we
>>>> should be passing -mno-unaligned-access.
>>> 
>>> I don’t think that this is a GCC bug, as “-mno-unaligned-access”
>>> will change the behaviour for packed data-structures only. Here’s
>>> from the GCC docs:
>>> 
>>>> -munaligned-access
>>>> -mno-unaligned-access
>>>> 
>>>> Enables (or disables) reading and writing of 16- and 32- bit
>>>> values from addresses that are not 16- or 32- bit aligned. By
>>>> default unaligned access is disabled for all pre-ARMv6, all
>>>> ARMv6-M and for ARMv8-M Baseline architectures, and enabled for
>>>> all other architectures. If unaligned access is not enabled then
>>>> words in packed data structures are accessed a byte at a time.
>>> 
>>> The key word seems to be “in packed data structures”.
>>> However, I don’t see an attribute “packed” for the 'struct ip_udp_hdr’
>>> (in include/net.h).
>>> 
>>> Could you try to verify that the error reproduces with a packed variant
>>> of the ‘struct ip_udp_hdr’?
>> 
>> It indeed fixed the issue. There might just have been a subtle change
>> of behaviour in GCC, and this is probably going to bite us in other
>> areas.
>> 
>> I'll send a patch to add the packed attribute.
> 
> Please bear in mind that packed should be used carefully.  We've had
> some discussions about this before and have
> doc/README.unaligned-memory-access.txt which may need a little more
> updating now as well, depending on what the final resolution here is as

Note that the requirement regarding ‘packed’ has already been in place
on GCC-4.9, GCC-5 and GCC-6:
	https://gcc.gnu.org/onlinedocs/gcc-4.9.3/gcc/ARM-Options.html#ARM-Options
	https://gcc.gnu.org/onlinedocs/gcc-5.4.0/gcc/ARM-Options.html#ARM-Options
	https://gcc.gnu.org/onlinedocs/gcc-6.3.0/gcc/ARM-Options.html#ARM-Options

It’s just that it might not have been enforced before (i.e. that the implementation
only considered the flag and didn’t take into account if a structure was declared
packed or not).

> I seem to recall some other problem reports with gcc-7.x, but I've not
> personally been able to hit these just yet.  But I need to get on that
> soon.  http://toolchains.free-electrons.com/ is awesome and I eagerly
> await gcc-7.x toolchains there if I can't get something else spun up in
> a chroot soon :)

I have been using a gcc-7.1 for AArch64 for a couple of wweks now and
haven’t had any issues… however, we always build using crosstools and
it’s configures as an aarch64-elf (and not an aarch64-linux), which can
change behaviour.

Phil.