[PATCH] D118511: Add a warning for not packing non-POD members in packed structs
David Blaikie via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Feb 7 15:09:17 PST 2022
dblaikie marked an inline comment as done.
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;
> 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.
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:2140-2141
+ if (Packed && !FieldPacked && PackedFieldAlign < FieldAlign)
+ Diag(D->getLocation(), diag::warn_unpacked_field) << D;
> Hm. Doing this from here will mean we only warn if we actually compute the layout of the class. But I suppose that's the same as what we do a few lines above, and for other `-Wpacked` warnings, and it seems necessary if we want to suppress the warning if packing wouldn't have made any difference anyway.
Yeah, the test has that workaround `f` function that references all the types for that reason - but could be nice to avoid that, though would mean moving all this layout stuff somewhere else/more reusable, I guess? Probably out of scope for this patch, but I'm open to ideas if it's worth addressing as a follow-up.
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
More information about the cfe-commits