Re: "Wire" definitions and __packed
On Thu, Oct 06, 2016 at 06:08:30PM +, paul.kon...@dell.com wrote: > Still, though, the original comment is largely valid: you can't do > meaningful testing of changes that affect alignment on an x86 system, > because for the most part it doesn't care. (The same goes for various > other CISC machines such as VAX.) Also, structure padding is different > for x86 than for most RISC machines. Can we please the keep the amount of generic wisdom injection down, please? Roy is quite aware that x86 is not strict alignment. As I said in my initial mail, the proposed changes have a chance of exposing incorrect alignment assumptions in the kernel. Some of them will be properly detected at build time, some will only be found due to testing. Padding differences are irrelevant since I explicitly said that size assertions are going to be kept. Random general remarks do nothing to improve the situation. Joerg
Re: "Wire" definitions and __packed
> On Oct 6, 2016, at 2:01 PM, Joerg Sonnenberger wrote: > > On Fri, Oct 07, 2016 at 04:59:30AM +1100, matthew green wrote: >> John Nemeth writes: >>> On Oct 6, 3:01pm, matthew green wrote: >>> } >>> } > X86 doesn't have alignment restrictions. The platform >>> } > practically lets you get away with murder, and thus is not useful >>> } > as a test platform. >>> } >>> } FWIW, this hasn't been true since at least 1999 (SSE.) also, >>> >>> That only counts if somebody is using SSE, and I highly doubt >>> that dhcpcd does. >> >> GCC will emit SSE code even if you don't explicitly use them. > > Like for inlined memset or memcpy... Still, though, the original comment is largely valid: you can't do meaningful testing of changes that affect alignment on an x86 system, because for the most part it doesn't care. (The same goes for various other CISC machines such as VAX.) Also, structure padding is different for x86 than for most RISC machines. The trouble with making a change in fundamental machinery and then doing a "test it to see if it breaks" is that this only exposes issues in the code paths that happened to be touched by the particular test. paul
Re: "Wire" definitions and __packed
On Fri, Oct 07, 2016 at 04:59:30AM +1100, matthew green wrote: > John Nemeth writes: > > On Oct 6, 3:01pm, matthew green wrote: > > } > > } > X86 doesn't have alignment restrictions. The platform > > } > practically lets you get away with murder, and thus is not useful > > } > as a test platform. > > } > > } FWIW, this hasn't been true since at least 1999 (SSE.) also, > > > > That only counts if somebody is using SSE, and I highly doubt > > that dhcpcd does. > > GCC will emit SSE code even if you don't explicitly use them. Like for inlined memset or memcpy... Joerg
re: "Wire" definitions and __packed
John Nemeth writes: > On Oct 6, 3:01pm, matthew green wrote: > } > } > X86 doesn't have alignment restrictions. The platform > } > practically lets you get away with murder, and thus is not useful > } > as a test platform. > } > } FWIW, this hasn't been true since at least 1999 (SSE.) also, > > That only counts if somebody is using SSE, and I highly doubt > that dhcpcd does. GCC will emit SSE code even if you don't explicitly use them. that's why we build kernels etc. with -mno-sse -mno-mmx, etc. it could also be using an SSE enhanced functionality in libc. you can never really be sure unless you control the entire environment (like a kernel.) .mrg.
re: "Wire" definitions and __packed
On Oct 6, 3:01pm, matthew green wrote: } } > X86 doesn't have alignment restrictions. The platform } > practically lets you get away with murder, and thus is not useful } > as a test platform. } } FWIW, this hasn't been true since at least 1999 (SSE.) also, That only counts if somebody is using SSE, and I highly doubt that dhcpcd does. } while no one uses them, x86 has "alignment checking" options. I am aware of the flag, but as you noted nobody uses it, thus it might as well not be there. }-- End of excerpt from matthew green
re: "Wire" definitions and __packed
> X86 doesn't have alignment restrictions. The platform > practically lets you get away with murder, and thus is not useful > as a test platform. FWIW, this hasn't been true since at least 1999 (SSE.) also, while no one uses them, x86 has "alignment checking" options. .mrg.
Re: "Wire" definitions and __packed
On Oct 5, 10:15pm, Roy Marples wrote: } On Wednesday 05 October 2016 17:10:28 Eduardo Horvath wrote: } > On Wed, 5 Oct 2016, Roy Marples wrote: } > > On 04/10/2016 23:06, Joerg Sonnenberger wrote: } > > > I'd like to addressing this by cutting down on the first set. For this } > > > purpose, I want to replace many of the __packed attributes in the } > > > current network headers with CTASSERT of the proper size, especially for } > > > those structs that are clearly not wire definitions by themselve. } > > } > > I tested the following structs without packed with the latest dhcpcd } > > trunk (not yet in NetBSD). } > > } > > ip } > > udphdr } > > arphdr } > > in_addr } > > nd_router_advert } > > nd_opt_hdr } > > nd_opt_prefix_info } > > nd_opt_mtu } > > nd_opt_rdnss } > > nd_opt_dnssl } > > } > > Works fine so far. } > } > What platforms did you test it on? } > } > I recommend trying it on sparc64. That's one of the worst cases, being } > big-endian 64-bit with alignment constraints. And I recall some ABI (was } > it ARM?) has strange alignment restrictions on byte values. } } i386/amd64 only right now. X86 doesn't have alignment restrictions. The platform practically lets you get away with murder, and thus is not useful as a test platform. } I'll test on mips64-eb tomorrow. } Sadly my sparc64 is dead, the network card reports an unspecified hardware } address. Traditionally, sparc boxes got their network MAC address programmed by a value specified in CMOS RAM. This likely means that the CMOS battery is dead. }-- End of excerpt from Roy Marples
Re: "Wire" definitions and __packed
On Wednesday 05 October 2016 17:10:28 Eduardo Horvath wrote: > On Wed, 5 Oct 2016, Roy Marples wrote: > > On 04/10/2016 23:06, Joerg Sonnenberger wrote: > > > I'd like to addressing this by cutting down on the first set. For this > > > purpose, I want to replace many of the __packed attributes in the > > > current network headers with CTASSERT of the proper size, especially for > > > those structs that are clearly not wire definitions by themselve. > > > > I tested the following structs without packed with the latest dhcpcd > > trunk (not yet in NetBSD). > > > > ip > > udphdr > > arphdr > > in_addr > > nd_router_advert > > nd_opt_hdr > > nd_opt_prefix_info > > nd_opt_mtu > > nd_opt_rdnss > > nd_opt_dnssl > > > > Works fine so far. > > What platforms did you test it on? > > I recommend trying it on sparc64. That's one of the worst cases, being > big-endian 64-bit with alignment constraints. And I recall some ABI (was > it ARM?) has strange alignment restrictions on byte values. i386/amd64 only right now. I'll test on mips64-eb tomorrow. Sadly my sparc64 is dead, the network card reports an unspecified hardware address. Roy
Re: "Wire" definitions and __packed
On Wed, Oct 05, 2016 at 05:10:28PM +, Eduardo Horvath wrote: > I recommend trying it on sparc64. That's one of the worst cases, being > big-endian 64-bit with alignment constraints. And I recall some ABI (was > it ARM?) has strange alignment restrictions on byte values. Old (ancient?) ARM ABIs had a minimal struct size. Thankfully, noone is brain dead to do something like that and in fact, we had disabled that part from what I dimly remember. We do assume that power-of-two sized types require no more than that in terms of alignment. While not strictly guaranteed by the standard, I believe a "don't care" attitude is justified for platforms don't follow that. Joerg
Re: "Wire" definitions and __packed
On Wed, 5 Oct 2016, Roy Marples wrote: > On 04/10/2016 23:06, Joerg Sonnenberger wrote: > > I'd like to addressing this by cutting down on the first set. For this > > purpose, I want to replace many of the __packed attributes in the > > current network headers with CTASSERT of the proper size, especially for > > those structs that are clearly not wire definitions by themselve. > > I tested the following structs without packed with the latest dhcpcd > trunk (not yet in NetBSD). > > ip > udphdr > arphdr > in_addr > nd_router_advert > nd_opt_hdr > nd_opt_prefix_info > nd_opt_mtu > nd_opt_rdnss > nd_opt_dnssl > > Works fine so far. What platforms did you test it on? I recommend trying it on sparc64. That's one of the worst cases, being big-endian 64-bit with alignment constraints. And I recall some ABI (was it ARM?) has strange alignment restrictions on byte values. Eduardo
Re: "Wire" definitions and __packed
On 04/10/2016 23:06, Joerg Sonnenberger wrote: > I'd like to addressing this by cutting down on the first set. For this > purpose, I want to replace many of the __packed attributes in the > current network headers with CTASSERT of the proper size, especially for > those structs that are clearly not wire definitions by themselve. I tested the following structs without packed with the latest dhcpcd trunk (not yet in NetBSD). ip udphdr arphdr in_addr nd_router_advert nd_opt_hdr nd_opt_prefix_info nd_opt_mtu nd_opt_rdnss nd_opt_dnssl Works fine so far. Roy
"Wire" definitions and __packed
Hello all, a long time ago, Jason Thorpe added the packed attribute to a lot of "wire" format definitions. Some of those like in_addr are part of a lot of other interfaces that have nothing to do with on-wire representations. The attribute has two functions: (1) It removes any implicit padding as expected from the name. (2) It also (unspectedly for many) removes any alignment constraints from the structure. The second item is problematic as can result in rather inefficient byte-wise access. More important and the reason why I am bringing this up: it impacts addresses to individual struct members. The current clang development version has a new warning to trigger for situations like struct foo { int x; } __packed; int *f(struct foo *f) { return &f.x; } The return value of f() is not guarenteed to be aligned correctly, so this is effectively an implicit cast that raises the alignment. The clang warning is still being refined to avoid triggering in situation where alignment is explicitly not relevant (cast to intptr_t, cast to char *, cast to void *) or where other alignment constraints are still in place. Nevertheless quite a few cases arround the network stack remain, triggering this warning. While investigating them, I have found two different situations. Some places are safe and there should not have been a __packed in first place. in_addr is a good example where the attribute servers no real purpose. For other places it is not easy to see where the alignment guarantees come from, so whether they work on strict alignment platforms is difficult to tell. I'd like to addressing this by cutting down on the first set. For this purpose, I want to replace many of the __packed attributes in the current network headers with CTASSERT of the proper size, especially for those structs that are clearly not wire definitions by themselve. There is a certain chance that some code path in the kernel currently does depend on the implicit byte access to work, so this is not without risk. On the other hand, it can result in more efficient code and better diagnostic of mismatches, so IMO it will be an over all win. It should also make it easier to identify cases where we do mishandled unaligned access. Comments? Joerg