Tom & Maxime,
On 12 Jul 2017, at 16:34, Tom Rini trini@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@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.
Regards, Philipp.