[clang] Handle trivial_abi attribute for Microsoft ABI. #88857
Conversation
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-codegen Author: Tobias Hieta (tru) ChangesPreviously the trivial_abi was ignored for records when targetting the microsoft abi, the MSVC rules where always enforced to ensure compatibility with MSVC. This commit changes it to be closer to the itanium abi when a record is marked with the trivial_abi attribute. Fixes #87993 Full diff: https://github.com/llvm/llvm-project/pull/88857.diff 3 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 76701dc723b6c3..c5241403711726 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -63,6 +63,10 @@ ABI Changes in This Version
MSVC uses a different mangling for these objects, compatibility is not affected.
(#GH85423).
+- The attribute ``trivial_abi`` now works when targetting the Microsoft ABI. Marking
+ a struct with this attribute will now cause clang to emit ABI similar to the Itanium
+ ABI. This is not compatible with the Microsoft ABI. (#GH87993).
+
AST Dumping Potentially Breaking Changes
----------------------------------------
diff --git a/clang/lib/CodeGen/MicrosoftCXXABI.cpp b/clang/lib/CodeGen/MicrosoftCXXABI.cpp
index d38a26940a3cb6..b930913badcd3f 100644
--- a/clang/lib/CodeGen/MicrosoftCXXABI.cpp
+++ b/clang/lib/CodeGen/MicrosoftCXXABI.cpp
@@ -1105,6 +1105,11 @@ bool MicrosoftCXXABI::hasMostDerivedReturn(GlobalDecl GD) const {
static bool isTrivialForMSVC(const CXXRecordDecl *RD, QualType Ty,
CodeGenModule &CGM) {
+ // If the record is marked with the trivial_abi attribute, we don't
+ // have to conform to the standard MSVC ABI.
+ if (RD->hasAttr<TrivialABIAttr>())
+ return true;
+
// On AArch64, HVAs that can be passed in registers can also be returned
// in registers. (Note this is using the MSVC definition of an HVA; see
// isPermittedToBeHomogeneousAggregate().)
diff --git a/clang/test/CodeGenCXX/trivial_abi_msvc.cpp b/clang/test/CodeGenCXX/trivial_abi_msvc.cpp
new file mode 100644
index 00000000000000..8826a8224c6671
--- /dev/null
+++ b/clang/test/CodeGenCXX/trivial_abi_msvc.cpp
@@ -0,0 +1,21 @@
+// RUN: %clang_cc1 -triple x86_64-pc-windows-msvc -std=c++11 -fcxx-exceptions -fexceptions -emit-llvm -o - %s | FileCheck %s
+
+// CHECK: %[[STRUCT_TRIVIAL:.*]] = type { ptr }
+struct __attribute__((trivial_abi)) Trivial {
+ int *p;
+ Trivial() : p(0) {}
+ Trivial(const Trivial &) noexcept = default;
+};
+
+// CHECK-LABEL: define{{.*}} i64 @"?retTrivial@@YA?AUTrivial@@XZ"(
+// CHECK: %retval = alloca %[[STRUCT_TRIVIAL]], align 8
+// CHECK: %call = call noundef ptr @"??0Trivial@@QEAA@XZ"(ptr noundef nonnull align 8 dereferenceable(8) %retval)
+// CHECK: %coerce.dive = getelementptr inbounds %[[STRUCT_TRIVIAL]], ptr %retval, i32 0, i32 0
+// CHECK: %0 = load ptr, ptr %coerce.dive, align 8
+// CHECK: %coerce.val.pi = ptrtoint ptr %0 to i64
+// CHECK: ret i64 %coerce.val.pi
+Trivial retTrivial() {
+ Trivial s;
+ return s;
+}
+
|
clang/docs/ReleaseNotes.rst
Outdated
| @@ -63,6 +63,10 @@ ABI Changes in This Version | |||
| MSVC uses a different mangling for these objects, compatibility is not affected. | |||
| (#GH85423). | |||
|
|
|||
| - The attribute ``trivial_abi`` now works when targetting the Microsoft ABI. Marking | |||
There was a problem hiding this comment.
It's overly broad to say it "works", and I suggest narrowing the scope of the release note. For example, there is a major known issue with __is_trivially_relocatable, which I think means that trivial_abi won't work the way people want it to in containers. Maybe try:
Records carrying the
trivial_abiattribute are now returned directly in registers in more cases when using the Microsoft ABI. It is not possible to passtrivial_abirecords between MSVC and Clang, so there is no ABI compatibility requirement. This is an ABI break with old versions of Clang. (#GH87993)
| Trivial s; | ||
| return s; | ||
| } | ||
|
|
There was a problem hiding this comment.
For completeness, please add the instance and static method return cases, since those are indirect for other reasons. For me, it's sufficient to check the function prototype, both of which should have an sret pointer:
https://godbolt.org/z/fn7vrGrKh
There was a problem hiding this comment.
I checked your example there and the instanceMethod is no problem, but I noticed that the staticMethod doesn't contain a sret return value with this patch. Since we are not passing this to it - I think this is expected, but just want to make sure.
; Function Attrs: mustprogress noinline optnone uwtable
define dso_local i64 @"?staticMethod@TrivialInstance@@SA?AUTrivial@@XZ"() #0 align 2 {
entry:
%retval = alloca %struct.Trivial, align 8
%call = call noundef ptr @"??0Trivial@@QEAA@XZ"(ptr noundef nonnull align 8 dereferenceable(8) %retval)
%coerce.dive = getelementptr inbounds %struct.Trivial, ptr %retval, i32 0, i32 0
%0 = load ptr, ptr %coerce.dive, align 8
%coerce.val.pi = ptrtoint ptr %0 to i64
ret i64 %coerce.val.pi
}
There was a problem hiding this comment.
I stand corrected, but I think it makes the point that this logic can be confusing, and it's worth testing. :)
Previously the trivial_abi was ignored for records when targetting the microsoft abi, the MSVC rules where always enforced to ensure compatibility with MSVC. This commit changes it to be closer to the itanium abi when a record is marked with the trivial_abi attribute. Fixes llvm#87993
tru
force-pushed
the
tru/trivial_abi_msvc
branch
from
22f627c
to
08214d8
Compare
|
@efriedma-quic are you ok with this change? |
| @@ -1105,6 +1105,11 @@ bool MicrosoftCXXABI::hasMostDerivedReturn(GlobalDecl GD) const { | |||
|
|
|||
| static bool isTrivialForMSVC(const CXXRecordDecl *RD, QualType Ty, | |||
| CodeGenModule &CGM) { | |||
| // If the record is marked with the trivial_abi attribute, we don't | |||
| // have to conform to the standard MSVC ABI. | |||
| if (RD->hasAttr<TrivialABIAttr>()) | |||
There was a problem hiding this comment.
I've realized this misses a case, consider this a wrapper struct that contains a trivial_abi subobject: https://godbolt.org/z/bK6aGv4d7
I spent 15 minutes looking through the things we track for trivial_abi, and unfortunately, we don't track this anywhere already, so the best solution I can come up with is to do a recursive walk of all the subobjects and look for the attribute. :(
There was a problem hiding this comment.
That's not how trivial_abi normally works? You'll never even reach this code for anything with a non-trivial destructor that isn't explicitly marked trivial_abi.
I guess if you're abusing trivial_abi just to get the MSVC-specific behavior in this patch, you might want that recursive behavior, but that seems like it's stretching it a bit. We might want to consider a dedicated attribute.
On a related note, if we're going to mess with this anyway, we might want to mess with the canPassInRegisters() in clang/lib/Sema/SemaDeclCXX.cpp also, so structs with a deleted copy constructor work, for example:
struct __attribute__((trivial_abi)) Trivial {
int *p = nullptr;
Trivial(const Trivial &) noexcept = delete;
Trivial(Trivial &&) noexcept;
~Trivial();
};
Trivial f(Trivial x) { return x;}
There was a problem hiding this comment.
That's not how trivial_abi normally works?
I think it is how it works, based on re-reading the code just now and the test case I linked.
To your point, the MSVC-specific code in canPassInRegisters is full of bugs, but I don't want to ask @tru to deal with that.
There was a problem hiding this comment.
Oh, I see, whether the constructors/destructor/etc. are trivial for calls is determined recursively; we don't record whether that was because of an explicit trivial_abi attribute, or because there just weren't any non-trivial fields. So we can't tell the difference here.
It feels sort of weird to treat a struct with a trivial_abi member as somehow more trivial than a struct that just contains plain data.
There was a problem hiding this comment.
So if I understand correctly:
- There are additional places in the microsoft code path where we don't handle trivial cases without the attribute.
- The glaring issue here with this PR is that if you put a trivial_abi struct in a struct it won't be marked as trivial itself since it doesn't search recursively.
Seems like there are many more places where we can do better, do we want to block this PR on the recursive search? What would the rules for that be? It can't just be that if you have a struct with a trivial_abi attribute inside any struct the parent should also be considered trivial. Would it be that isTrivialForMicrosoft would return true if the current struct passes the checks there and only contains a struct with a trivial_abi attribute?
There was a problem hiding this comment.
Yes, I think I do want to block this on the recursive search. Even if it's ugly and we ultimately replace it with an explicitly tracked and type trait (containsTrivialAbiSubobject?), it reduces the number of externally visible ABI changes. I know we're saying this is ABI unstable already, but I still think fewer breaks is better.
There was a problem hiding this comment.
Happy to add this. Can you point me to an example where we search recursively or give some pseudo code example and I should be able to take it from there pretty soon.
There was a problem hiding this comment.
The pseudo code version is something like:
bool containsTrivialAbiSubobject(CXXRecordDecl RD) {
if (RD->hasAttr()) return true;
for (auto B : RD->bases()) {
if (containsTrivialAbiSubobject(B->getAsCXXRecordDecl()))
return true;
}
for (auto F : RD->fields()) {
const CXXRecordDecl FRD = F->getType()->getAsCXXRecordDecl();
if (FRD && containsTrivialAbiSubobject(FRD))
return true;
}
return false;
}
A bit of a gross inefficient DFS, with some CXXBaseSpecifier details omitted. It might also be worth tracking Visited records to handle fields of the same record type without recursing over all subobjects again.
|
ping on this. |
|
Sorry for the delay, work life does its best to intervene. |
Previously the trivial_abi was ignored for records when targetting the microsoft abi, the MSVC rules where always enforced to ensure compatibility with MSVC. This commit changes it to be closer to the itanium abi when a record is marked with the trivial_abi attribute.
Fixes #87993