[U-Boot] Data Abort with gcc 7.1
Tom Rini
trini at konsulko.com
Wed Jul 12 17:49:33 UTC 2017
On Wed, Jul 12, 2017 at 05:48:16PM +0200, Dr. Philipp Tomsich 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
OK. And are there other structures that we should mark as packed as
well in include/net.h? I suspect so.
> — 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.
So, this is partly why I think we need to perhaps add / update / change
some wording in the current file. The overall desire is to avoid
"doesn't work, packed fixes it, packed must be the right fix!". We want
to emphasize that packed should be used when it's appropriate, ie the
cases you mention above. Thanks!
--
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170712/e5ebe486/attachment.sig>
More information about the U-Boot
mailing list