Re: [Xen-devel] [PATCH] trace: fix build with gcc9

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 07.05.19 at 19:33, <george.dunlap@xxxxxxxxxx> wrote:
> On 3/8/19 12:14 PM, Jan Beulich wrote:
>>>>> On 08.03.19 at 12:58, <George.Dunlap@xxxxxxxxxx> wrote:
>>> 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?

To be honest - I don't know without seeing a concrete example, which
would (hopefully) allow me to understand the whys. But I clearly would
aim at finding a solution ...

> 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.

... without the use of __packed.

> 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.

Switching op to uint64_t seems undesirable (beyond the fact of then
also needing to change the consumer), as this would then cost us an
array element (afaict). In turn (with the alignment of the array in this
example being 8 instead of the expected 4) it would cost us a second
array element unless we can allow sizeof(d) > 7 * 4, as compound
types get padded to a multiple of their alignment.

So my two goals here are
- don't use __packed (and I actually mean this in a wider sense, i.e.
  we shouldn't use it anywhere other than to declare structures for
  interfacing with hardware / firmware),
- cross the bridge of addressing the theoretical issue only once we
  actually have to, due to the code getting ported to a respective
  architecture (and I suppose we'd have to deal with more problems
  of this kind in that case, as the public interface may also make
  similar assumptions of there not being any padding between two
  fields of the same base type, but the latter of which being an
  array).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel