This is an archive of the discontinued LLVM Phabricator instance.

[Clang][CodeGen] Fix this argument type for certain destructors
ClosedPublic

Authored by jacobly on Feb 2 2023, 5:05 PM.

Details

Summary

With the Microsoft ABI, some destructors need to offset a parameter to
get the derived this pointer, in which case the type of that parameter
should not be a pointer to the derived type.

Fixes #60465

Diff Detail

Event Timeline

jacobly created this revision.Feb 2 2023, 5:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 2 2023, 5:05 PM
jacobly requested review of this revision.Feb 2 2023, 5:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 2 2023, 5:05 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
jacobly added a reviewer: rnk.Feb 2 2023, 5:17 PM
jacobly updated this revision to Diff 495543.Feb 7 2023, 7:55 AM

Added regression test specific to the issue.

jacobly updated this revision to Diff 495645.Feb 7 2023, 2:43 PM

Cleanup and minor fixes.

jacobly updated this revision to Diff 495648.Feb 7 2023, 2:48 PM
aaron.ballman added a subscriber: aaron.ballman.

Please be sure to add release notes to clang/docs/ReleaseNotes.rst

Things look reasonable to me, but it'd be nice if someone with more Windows ABI experience could take a look as well.

clang/lib/CodeGen/MicrosoftCXXABI.cpp
243
jacobly updated this revision to Diff 497052.Feb 13 2023, 11:17 AM

Address review comments.

jacobly marked an inline comment as done.Feb 13 2023, 11:19 AM

If the parameter attributes are relevant, should we change the CHECK lines in the tests to check for them? At least for some of the tests. Once we fix these tests to enable opaque pointer types, you won't actually be checking for anything.

jacobly updated this revision to Diff 497097.Feb 13 2023, 1:46 PM

Fix bad diff update.

This revision is now accepted and ready to land.Feb 13 2023, 2:07 PM

I was hoping @rnk was available for review, but otherwise this is ready to land and I don't have commit access.

How do you want to be listed as author in the git commit (Name <email>)?

Jacob Young <jacobly0@users.noreply.github.com>

Maybe worth cherry-picking to 16 branch? I think someone will need to rebase onto the branch for that, though; there was merge conflict on the microsoft-abi-eh-cleanups.cpp change.

Maybe worth cherry-picking to 16 branch? I think someone will need to rebase onto the branch for that, though; there was merge conflict on the microsoft-abi-eh-cleanups.cpp change.

I wouldn't be opposed to picking this onto 16 given that it's fixing an ABI issue, if someone wants to do the work to cherry-pick it.

jacobly added a comment.EditedMar 2 2023, 1:40 AM

Maybe worth cherry-picking to 16 branch? I think someone will need to rebase onto the branch for that, though; there was merge conflict on the microsoft-abi-eh-cleanups.cpp change.

I wouldn't be opposed to picking this onto 16 given that it's fixing an ABI issue, if someone wants to do the work to cherry-pick it.

https://github.com/llvm/llvm-project/issues/61124 https://github.com/llvm/llvm-project-release-prs/pull/336