[U-Boot] Data Abort with gcc 7.1

Dr. Philipp Tomsich philipp.tomsich at theobroma-systems.com
Wed Jul 12 18:26:13 UTC 2017


> On 12 Jul 2017, at 20:07, Siarhei Siamashka <siarhei.siamashka at gmail.com> wrote:
> 
> On Wed, 12 Jul 2017 17:48:16 +0200
> "Dr. Philipp Tomsich" <philipp.tomsich at theobroma-systems.com> wrote:
> 
>> Tom & Maxime,
>> 
>>> 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
>>> 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 :)  
>> 
>> So there’s the remaining question of how to fix this permanently:
>> — with my compiler engineering hat on, I’d recommend to mark this
>> as a packed struct, as that’s what it is: the compiler needs to keep it
>> packed, because that is how it is received/sent on the wire
>> — rereading the doc/README.unaligned-memory-access.txt, the
>> preferred solution in U-Boot would be to use put/get_unaligned to
>> access these fields (although I have concerns with this—see below).
>> 
>> I honestly wonder if the recommendation (to avoid ‘packed’) from the
>> README is appropriate for many of the data structure declarations in
>> U-Boot which deal with  the external representation of data (i.e. DMA
>> descriptors, memory-mapped register files and data sent on a wire):
>> the C language does not offer any protection against a compiler adding
>> patting between structure members, as it sees fit.  The original wording
>> from the standard is:
>> 14	Each non-bit-field member of a structure or union object is aligned in an implementation-defined manner appropriate to its type.
>> 15	Within a structure object, the non-bit-field members and the units in which bit-fields reside have addresses that increase in the order in which they are declared. A pointer to a structure object, suitably converted, points to its initial member (or if that member is a bit-field, then to the unit in which it resides), and vice versa. There may be unnamed padding within a structure object, but not at its beginning.
>> 
>> In other words: there’s nothing in the standard from stopping a compiler
>> to insert additional padding within structures, unless the ‘packed’ attribute
>> is added. 
> 
> I would strongly advise against adding the "packed" attribute
> everywhere unnecessarily. This just makes the code bigger and
> slower.

The intent was not to add it everywhere, but only to choose this solution
when encountering cases like this one: i.e. cases where the source code
was risking future trouble.

Clearly we can rely on ABI documents when we have code that targets
a specific architecture (e.g the arch/arm), but should not do so in the
architecture-independent code.

And clearly there’s also the aspect of needing the ‘packed’ to enable GCC
to process the unaligned data elements with “-mno-unaligned-accesses”.

> The ANSI/ISO C language standard indeed leaves a lot up to the
> implementation. But we also have ABI documents for each platform,
> which cover all of these details. We just need to use them.
> 
> In the case of 32-bit ARM, it is
>    http://infocenter.arm.com/help/topic/com.arm.doc.ihi0042e/IHI0042E_aapcs.pdf
> 
> In the case of 64-bit ARM, it is 
>    http://infocenter.arm.com/help/topic/com.arm.doc.ihi0055b/IHI0055B_aapcs64.pdf

Given that the common header files should be used across architectures
(and also future architectures, once someone decides to port U-Boot there),
we shouldn’t assume that the ABI documents for each platform will define
this in such a way that we can rely on.

Cheers,
Phil.


More information about the U-Boot mailing list