Skip to content

Conversation

@vortex73
Copy link
Contributor

@vortex73 vortex73 commented May 15, 2025

Project report

This PR details the implementation details of the proposed ABI lowering library.

  • Implementing a Shadow TypeSystem
  • Implement a bridge between Clang Qualtype to the shadow Typesystem
  • Integrate into Clang
  • Implement BPF ABIInfo
  • Implement SysV ABIInfo

@github-actions
Copy link

github-actions bot commented May 26, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

@vortex73 vortex73 force-pushed the abi branch 2 times, most recently from 43855e6 to 114cab7 Compare May 26, 2025 21:51
@vortex73 vortex73 force-pushed the abi branch 2 times, most recently from 4410289 to 2832642 Compare June 1, 2025 16:47
Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

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

One thing still missing here is handling for C++ structs with base classes.

@makslevental makslevental self-requested a review June 2, 2025 17:22
@vortex73 vortex73 force-pushed the abi branch 2 times, most recently from 329556a to 0f045da Compare June 12, 2025 18:20
Copy link
Contributor

Choose a reason for hiding this comment

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

Having both FunctionABIInfo and ABIFunctionInfo is pretty confusing... do these need to be separate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

um, I've renamed it for the time being - if I feel it doesn't serve a purpose in a few days I'll put it somewhere else

@vortex73 vortex73 force-pushed the abi branch 2 times, most recently from 268c470 to 690068f Compare June 21, 2025 13:44
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm... this is tricky. I'm not entirely sure what to do here. For the purpose of mapping ABIArgInfo, we should never encounter unions. For the struct case, things are tricky. Mostly, I believe we just want LLVM structs as a means of holding multiple values. E.g. a StructType for Direct arguments will normally get unpacked into individual arguments. In this case, we really don't want any kind of padding fillers in it. I think the only case that cares about that is CoerceAndExpand, which has two struct types, one just with the elements (which will be passed as separate arguments) and one that has the actual layout (with correct alignment etc).

This kind of makes me wonder whether it would make sense to start by storing llvm types in ABIArgInfo to match what clang currently does. abi::Type is conceptually nicer, but also not quite the right representation for everything (e.g. because it can't really represent the "struct without layout" case well).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we should never encounter unions

Oh yeah! My bad...

Also I'm assuming both times you referred to ABIArgInfo you were talking about abi::ABIArgInfo and not codegen::ABIArgInfo (should probably name that something slightly different sorry).

Given that assumption - I really don't think we need to bring llvm::Type into the library as the struct without layout case can be added to the ABI typesystem itself (add another type of abi::StructPacking or decide whether we need padding while converting). But agreed a lot of cleanup to be done here, I'll keep it in mind while writing the x86 target.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also I'm assuming both times you referred to ABIArgInfo you were talking about abi::ABIArgInfo and not codegen::ABIArgInfo (should probably name that something slightly different sorry).

Yeah, I mean abi::ABIArgInfo. Btw, I wonder whether we should call these abi::ArgInfo etc, given that we already have abi in the namespace name...

Given that assumption - I really don't think we need to bring llvm::Type into the library as the struct without layout case can be added to the ABI typesystem itself (add another type of abi::StructPacking or decide whether we need padding while converting). But agreed a lot of cleanup to be done here, I'll keep it in mind while writing the x86 target.

Yeah, this is an option. I was mostly thinking about using llvm::Type as the path of least resistance to clang integration...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

um, I've renamed it for the time being - if I feel it doesn't serve a purpose in a few days I'll put it somewhere else

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we should never encounter unions

Oh yeah! My bad...

Also I'm assuming both times you referred to ABIArgInfo you were talking about abi::ABIArgInfo and not codegen::ABIArgInfo (should probably name that something slightly different sorry).

Given that assumption - I really don't think we need to bring llvm::Type into the library as the struct without layout case can be added to the ABI typesystem itself (add another type of abi::StructPacking or decide whether we need padding while converting). But agreed a lot of cleanup to be done here, I'll keep it in mind while writing the x86 target.

Copy link
Contributor

Choose a reason for hiding this comment

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

A note for the future: We probably need to do something to avoid leaking memory from these temporary types. We can probably checkpoint the type arena before computing ABIArgInfo and then restore the checkpoint once it's no longer needed.

@vortex73 vortex73 force-pushed the abi branch 3 times, most recently from b949795 to 565bf3b Compare August 28, 2025 21:42
@vortex73 vortex73 force-pushed the abi branch 2 times, most recently from 90500f7 to 7097478 Compare August 29, 2025 05:43
bool CanPassInRegisters;
bool IsCoercedRecord;
bool IsUnion;
bool IsTransparent;
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use something like LLVM_PREFERRED_TYPE(bool) unsigned IsTransparent : 1; etc to pack these into bits.

bool HasNonTrivialCopyConstructor;
bool HasNonTrivialDestructor;
bool HasFlexibleArrayMember;
bool HasUnalignedFields;
Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I can tell, this HasUnalignedFields property is unused.

if (RT->isPolymorphic() || RT->hasNonTrivialCopyConstructor() ||
RT->hasNonTrivialDestructor() || RT->hasFlexibleArrayMember() ||
RT->getNumVirtualBaseClasses() != 0)
return nullptr;
Copy link
Contributor

Choose a reason for hiding this comment

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

Where does this condition come from? Comparing to

const Type *CodeGen::isSingleElementStruct(QualType T, ASTContext &Context) {
it doesn't seem to be present there.

Copy link
Contributor Author

@vortex73 vortex73 Sep 11, 2025

Choose a reason for hiding this comment

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

Doesn't it need to account for VTable Pointers, and other C++ ABI affecting factors like dtors. Like:

struct test {
    virtual void foo();
    int x;
};

Was just a hunch, I could remove it if its trivial and get rid of those flags from the typesystem in that case...

Copy link
Contributor

Choose a reason for hiding this comment

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

That does make sense, but what I don't really get is why these things aren't checked in the existing Clang code. I think maybe the assumption is that this has already been handled previously by getRecordArgABI(), so records like that won't reach to this point? (In which case I guess the main needed property is CanPassInRegisters?)

Copy link
Contributor Author

@vortex73 vortex73 Sep 11, 2025

Choose a reason for hiding this comment

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

Hmm maybe... I'll give it a try

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet