[PATCH] D118511: Add a warning for not packing non-POD members in packed structs

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.


================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:2029-2036
   // The align if the field is not packed. This is to check if the attribute
   // was unnecessary (-Wpacked).
   CharUnits UnpackedFieldAlign =
       !DefaultsToAIXPowerAlignment ? FieldAlign : PreferredAlign;
   CharUnits UnpackedFieldOffset = FieldOffset;
   CharUnits OriginalFieldAlign = UnpackedFieldAlign;
 
----------------
dblaikie wrote:
> rsmith wrote:
> > It seems a little wasteful and error-prone that we're now computing the actual alignment, the alignment if the field were not packed, and the alignment if the field were packed. Is there any way we can reduce this down to computing just the alignment if the field were packed plus the alignment if the field were not packed, then picking one of those two as the actual field alignment? Or does that end up being messier?
> I had a go at that refactor - we can't pull the `FieldPacked` computation lower (it'd be great if it could move down to after the packed/unpacked computation, so it was clear that those values were computed independently of the `FieldPacked` value, and that `FieldPacked` just determined which one to pick) because of the `alignedAttrCanDecreaseAIXAlignment`, by the looks of it.
> 
> And also the AIX alignment stuff seems to do some weird things around the preferred alignment that caused the carefully constructed 3 `if`s below (`!FieldPacked`, `DefaultsToAIXPowerAlignment`, and `FieldPacked`) which I spent more time than I'd like to admit figuring out why anything else/less/more streamlined was inadequate.
> 
> But I also don't understand why `DefaultsToAIXAlignment` causes the `AlignTo` value to be the `PreferredAlign`, but the `FieldAlign` stays as it is? (like why doesn't `DefaultsToAIXPowerAlignment` cause `FieldAlign` to /be/ `PreferredAlign` - I think that'd simplify things, but tests (maybe the tests are incorrect/) seemed to break when I tried that) - I would've thought not doing that (as the code currently doesn't) would cause problems for the `UnadjustedAlignment`, `UpdateAlignment`, and `warn_unaligned_access` issues later on that depend on `FieldAlign`, but feel like they should probably depend on the alignment that actually got used (the `PreferredAlign`) instead? It's pretty confusing to me, so... yeah.
Ping on this discussion. @rsmith 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118511/new/

https://reviews.llvm.org/D118511