Skip to content

[clang] Handle trivial_abi attribute for Microsoft ABI. #88857

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+41 −0
Open

[clang] Handle trivial_abi attribute for Microsoft ABI. #88857

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions clang/docs/ReleaseNotes.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,11 @@ ABI Changes in This Version
MSVC uses a different mangling for these objects, compatibility is not affected.
(#GH85423).

- Records carrying the trivial_abi attribute are now returned directly in registers
in more cases when using the Microsoft ABI. It is not possible to pass trivial_abi
records between MSVC and Clang, so there is no ABI compatibility requirement. This
is an ABI break with old versions of Clang. (#GH87993)

AST Dumping Potentially Breaking Changes
----------------------------------------

Expand Down
5 changes: 5 additions & 0 deletions clang/lib/CodeGen/MicrosoftCXXABI.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>())
Copy link
Collaborator

@rnk rnk Apr 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. :(

Copy link
Collaborator

@efriedma-quic efriedma-quic Apr 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;}

Copy link
Collaborator

@rnk rnk Apr 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@efriedma-quic efriedma-quic Apr 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@tru tru Apr 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

@rnk rnk Apr 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@tru tru Apr 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@rnk rnk Apr 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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().)
Expand Down
31 changes: 31 additions & 0 deletions clang/test/CodeGenCXX/trivial_abi_msvc.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
// 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;
}

Copy link
Collaborator

@rnk rnk Apr 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

@tru tru Apr 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
}

Copy link
Collaborator

@rnk rnk Apr 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I stand corrected, but I think it makes the point that this logic can be confusing, and it's worth testing. :)

struct TrivialInstance {
Trivial instanceMethod();
static Trivial staticMethod();
};

// We need to make sure that instanceMethod has a sret return value since `this` will always go in the register.
// CHECK-LABEL: define{{.*}} void @"?instanceMethod@TrivialInstance@@QEAA?AUTrivial@@XZ"({{.*}} sret(%struct.Trivial{{.*}}
Trivial TrivialInstance::instanceMethod() { return {}; }
// CHECK-LABEL: define{{.*}} i64 @"?staticMethod@TrivialInstance@@SA?AUTrivial@@XZ"(
Trivial TrivialInstance::staticMethod() { return {}; }