On 3/8/19 12:14 PM, Jan Beulich wrote: >>>> On 08.03.19 at 12:58, <george.dun...@citrix.com> wrote: > >> >>> On Mar 8, 2019, at 7:11 AM, Jan Beulich <jbeul...@suse.com> wrote: >>> >>>>>> George Dunlap <george.dun...@citrix.com> 03/07/19 1:02 PM >>> >>>> On 3/7/19 10:34 AM, Jan Beulich wrote: >>>>> While I've not observed this myself, gcc 9 (imo validly) reportedly may >>>>> complain >>>>> >>>>> trace.c: In function '__trace_hypercall': >>>>> trace.c:826:19: error: taking address of packed member of 'struct >>>>> <anonymous>' >> may result in an unaligned pointer value >>>>> [-Werror=address-of-packed-member] >>>>> 826 | uint32_t *a = d.args; >>>> >>>> Wait, is this saying that in this case (i.e., with a single uint32_t >>>> before args), you *do* get an unaligned pointer value, or just that if >>>> the struct changes in the future that the pointer value may become >>>> un-aligned? >>> >>> With the __packed attribute, the compiler _could_ place the structure at >>> a misaligned slot on the stack. It has got nothing to do with struct layout >>> afaict. Note how the diagnostic says "may", not "does”. >> >> That’s interesting — so is there no way to specify that the struct should be >> aligned but the individual elements not? > > I think there is (marking the fields __packed and the struct __aligned()), > but that wouldn't necessarily help (depending on how the checking gets > don internal to the compiler). > >> If that’s the case, what about doing something like the attached instead? >> It avoids introducing a not-very-obvious BUILD_BUG_ON(), and also I think >> makes the algorithm a (tiny) bit easier to follow. (And if the >> BUILD_BUG_ON() ever triggered, we’d probably end up having to do something >> like this anyway.) > > That's an option. Yet don't forget that the compiler noticing the issue > (and spitting out the warning) likely means that it would still spot the > issue, but just have no reason to warn anymore. It spotting the issue > would mean though that on architectures where mis-aligned accesses > may fault, it may then produce pretty inefficient code for a case where > simple aligned accesses would be fine. > > IOW I prefer my variant of the workaround, but you're the maintainer > of this code, so you've got to decide.
Sorry, coming back to this quite late. So my main question with the BUG_ON() is, suppose it triggers: what will the fix be? My original idea was that we'd end up doing a fix like one I sent anyway; i.e., putting back the __packed attribute but avoiding taking a pointer for it. In that case, we might as well do the final fix immediately and save people the hassle. But of course, if that would cause inefficient mis-aligned accesses, then maybe what we'd do is change the layout of the structure such that it was naturally packed again (for instance, by changing `op` to being uint64_t instead). In that case, then maybe a BUG_ON() would be better, for the reasons you describe. Thoughts? -George _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
