Page MenuHomePhabricator

Add a warning for not packing non-POD members in packed structs
AcceptedPublic

Authored by dblaikie on Jan 28 2022, 2:22 PM.

Details

Reviewers
rsmith

Diff Detail

Event Timeline

dblaikie requested review of this revision.Jan 28 2022, 2:22 PM
dblaikie created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJan 28 2022, 2:22 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
dblaikie updated this revision to Diff 405468.Feb 2 2022, 3:25 PM
  • Create a separate sub-warning flag of -Wpacked (-Wpacked-non-pod), in case you're just interested in the ABI breaks here
  • Warn only when this produces a different alignment requirement for the field
rsmith accepted this revision.Feb 2 2022, 5:19 PM
rsmith added inline comments.
clang/lib/AST/RecordLayoutBuilder.cpp
2027–2034

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?

2140–2141

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.

clang/test/CodeGenCXX/warn-padded-packed.cpp
157

Maybe consider adding another test showing that we don't warn if packing would have made no difference anyway (for example if the alignment of the POD struct was already 1).

This revision is now accepted and ready to land.Feb 2 2022, 5:19 PM
dblaikie updated this revision to Diff 406626.Feb 7 2022, 3:02 PM

Refactor the max/min/preferred alignment calculations.

dblaikie marked an inline comment as done.Feb 7 2022, 3:09 PM
dblaikie added inline comments.
clang/lib/AST/RecordLayoutBuilder.cpp
2027–2034

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

2140–2141

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.

Should we backport this to the release/14.x branch?

Herald added a project: Restricted Project. · View Herald TranscriptMar 8 2022, 12:05 PM
tbaeder added a subscriber: tbaeder.Mar 8 2022, 9:49 PM

Hey @dblaikie, seems like this has never been pushed?

Hey @dblaikie, seems like this has never been pushed?

Yeah, was holding off on this because it looked like maybe there was still outstanding work on the nuance/precise nature of the ABI change - turned out to be a known/outstanding issue: https://reviews.llvm.org/D119051 though it does have some impact on the warning and the layout change: If we get that POD ABI fix in, then the layout change/warning will fire on fewer places. So it seems sort of unfortunate to add the warning & end up having folks cleaning up things that don't need cleaning up once the POD ABI change happens.

But equally, bad to ship the layout change without teh associated warning.

Thoughts? Revert the layout change for now/in the release until the broader POD ABI issue is addressed? Then apply the layout change+warning?

I'm fine with reverting if you think this is the best solution. I just would like to conclude soon so I can make the final release candidate.

I'm fine with reverting if you think this is the best solution. I just would like to conclude soon so I can make the final release candidate.

ISTM that reverting the ABI change in the 14.x branch makes sense, to avoid ping-ponging the ABI for packed structs which would become non-packed (breaking ABI) in 14.x and packed again (breaking ABI) in https://reviews.llvm.org/D119051.

I'm fine with reverting if you think this is the best solution. I just would like to conclude soon so I can make the final release candidate.

ISTM that reverting the ABI change in the 14.x branch makes sense, to avoid ping-ponging the ABI for packed structs which would become non-packed (breaking ABI) in 14.x and packed again (breaking ABI) in https://reviews.llvm.org/D119051.

Yeah - I think it'd be a pretty niche amount of code that'd churn like that, but doesn't seem super important to rush this either.

@tstellar - can/do you want to revert this on the release branch yourself? Is that something I should do? Should I revert this on trunk (would be a bit awkward/more churny for users - maybe not a full revert, but one that leaves the new ABI version flag available as a no-op so users opting out don't need to remove the flag only to add it back in later) so it can be integrated to the release?

I'm fine with reverting if you think this is the best solution. I just would like to conclude soon so I can make the final release candidate.

ISTM that reverting the ABI change in the 14.x branch makes sense, to avoid ping-ponging the ABI for packed structs which would become non-packed (breaking ABI) in 14.x and packed again (breaking ABI) in https://reviews.llvm.org/D119051.

Yeah - I think it'd be a pretty niche amount of code that'd churn like that, but doesn't seem super important to rush this either.

@tstellar - can/do you want to revert this on the release branch yourself? Is that something I should do? Should I revert this on trunk (would be a bit awkward/more churny for users - maybe not a full revert, but one that leaves the new ABI version flag available as a no-op so users opting out don't need to remove the flag only to add it back in later) so it can be integrated to the release?

I can revert in the release branch. Is this the commit: https://reviews.llvm.org/rG277123376ce08c98b07c154bf83e4092a5d4d3c6?

It doesn't seem necessary to me to revert in the main branch, but I think that can be your call.

I'm fine with reverting if you think this is the best solution. I just would like to conclude soon so I can make the final release candidate.

ISTM that reverting the ABI change in the 14.x branch makes sense, to avoid ping-ponging the ABI for packed structs which would become non-packed (breaking ABI) in 14.x and packed again (breaking ABI) in https://reviews.llvm.org/D119051.

Yeah - I think it'd be a pretty niche amount of code that'd churn like that, but doesn't seem super important to rush this either.

@tstellar - can/do you want to revert this on the release branch yourself? Is that something I should do? Should I revert this on trunk (would be a bit awkward/more churny for users - maybe not a full revert, but one that leaves the new ABI version flag available as a no-op so users opting out don't need to remove the flag only to add it back in later) so it can be integrated to the release?

I can revert in the release branch. Is this the commit: https://reviews.llvm.org/rG277123376ce08c98b07c154bf83e4092a5d4d3c6?

Yep, that's the one!

It doesn't seem necessary to me to revert in the main branch, but I think that can be your call.

Yeah, I'm leaning towards not reverting on main & hoping we can sort out the POD ABI issue.

@rsmith I had a few responses to your last round of review here - could you have a look through them/see if this is the right way to go, or if more refactoring would be suitable?

I'm fine with reverting if you think this is the best solution. I just would like to conclude soon so I can make the final release candidate.

ISTM that reverting the ABI change in the 14.x branch makes sense, to avoid ping-ponging the ABI for packed structs which would become non-packed (breaking ABI) in 14.x and packed again (breaking ABI) in https://reviews.llvm.org/D119051.

Yeah - I think it'd be a pretty niche amount of code that'd churn like that, but doesn't seem super important to rush this either.

@tstellar - can/do you want to revert this on the release branch yourself? Is that something I should do? Should I revert this on trunk (would be a bit awkward/more churny for users - maybe not a full revert, but one that leaves the new ABI version flag available as a no-op so users opting out don't need to remove the flag only to add it back in later) so it can be integrated to the release?

I can revert in the release branch. Is this the commit: https://reviews.llvm.org/rG277123376ce08c98b07c154bf83e4092a5d4d3c6?

Yep, that's the one!

It doesn't seem necessary to me to revert in the main branch, but I think that can be your call.

Yeah, I'm leaning towards not reverting on main & hoping we can sort out the POD ABI issue.

@tstellar seems we didn't sort out the POD ABI issue in time for the 15 release branch - though we're making some progress on it now (see discussion here: D119051 ) - reckon we should again revert the packed ABI layout in 15, to try to put this off until the 3 issues (ignoring packed when packing non-pod (277123376ce08c98b07c154bf83e4092a5d4d3c6), pod with defaulted special members (D119051), warning for ignoring packed due to non-pod (this patch, D118511)) are put together in one release?

Sorry for the churn/slow-movement on these.

@dblaikie Can you file a bug and add the 15.0.0 Release Milestone, so we don't forget to fix this in the release branch.

thieta added a subscriber: thieta.Aug 18 2022, 11:00 AM

@dblaikie Can you file a bug and add the 15.0.0 Release Milestone, so we don't forget to fix this in the release branch.

Yeah, sorry about the delay on this, last week was a lot on my side. Filed https://github.com/llvm/llvm-project/issues/57346 - happy to edit/change/have that changed in whatever way you were thinking.