Skip to content

[PAC][clang] Correct handling of ptrauth queries of incomplete types #164528

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

Merged
merged 2 commits into from
Oct 23, 2025
Merged

[PAC][clang] Correct handling of ptrauth queries of incomplete types #164528

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cf5921c
[PAC][clang] Correct handling of ptrauth queries of incomplete types
ojhunt Oct 22, 2025
c267422
Simplified the guards, and added an additional test path
ojhunt Oct 23, 2025
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
25 changes: 23 additions & 2 deletions clang/lib/AST/ASTContext.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1618,8 +1618,26 @@ void ASTContext::setRelocationInfoForCXXRecord(
RelocatableClasses.insert({D, Info});
}

// In future we may want to distinguish the presence or absence of address
// discrimination, from the inability to determine the presence. For now we rely
// on all source facing interfaces (type trait queries, etc) diagnosing and
// reporting an error before reaching these paths.
static bool canDeterminePointerAuthContent(QualType Type) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cor3ntin is there a better way to do this? It seems like there should be

if (Type->isIncompleteType() || Type->isDependentType())
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it guaranteed at this point to have already required this type to be complete?

Otherwise, this will not make sense for template specializations, since they can be lazily instantiated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mizvekov do you mean in a future "the answer is {yes, no, unknown}" world?

You can only hit this in the case where we have reached an error state that has meant we have a type that claims to be complete but contains an incomplete type (by value, not a pointer/reference).

In the case of template types, we already ignore template decls and don't produce true results until the point of instantiation.

I spent a bunch of time trying to find a state where I could create a "complete" type, containing an incomplete member, but it seems to only be achievable via the original report: if you exceed the max errors limit we stop instantiation of templated types inside classes.

What this code is dealing with is

// many many many errors

template <class> struct SomeCorrectTemplatedType {};

struct SomeOtherStruct {
   SomeCorrectTemplatedType<int> field;
};

// perform some type trait query
__is_trivially_relocatable(SomeOtherStruct);

Clang constructs SomeOtherStruct, and includes the CorrectTemplatedType<int> field, but does not actually synthesize CorrectTemplatedType<int>, leaving it as an incomplete type, despite being a value typed field of SomeOtherStruct. The various type trait queries generally get culled early on by either excluding incomplete types - which SomeOtherStruct is not - or querying bits from the type definition, which is present (that's why we believe the type is complete). When we try to compute the pointer auth properties we can't rely on precomputed bits in the DefinitionData and similar, so we have to duplicate the isCompleteDefinition check that is performed when adding a field to the type originally in our own code paths.

If I could find a way to get down this code path without requiring the "too many errors" mechanism for prevent template instantiation.

Copy link
Contributor

@cor3ntin cor3ntin Oct 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

maybe there is a bug in Sema::RequireCompleteType

 static bool primaryBaseHaseAddressDiscriminatedVTableAuthentication(
     const ASTContext &Context, const CXXRecordDecl *Class) {
-  if (!Class->isPolymorphic())
+  if (Class->isInvalidDecl() || !Class->isPolymorphic())
     return false;
   const CXXRecordDecl *BaseType = Context.baseForVTableAuthentication(Class);
   using AuthAttr = VTablePointerAuthenticationAttr;
diff --git a/clang/lib/Sema/SemaType.cpp b/clang/lib/Sema/SemaType.cpp
index 280b3c92cce1..9c9a071abf18 100644
--- a/clang/lib/Sema/SemaType.cpp
+++ b/clang/lib/Sema/SemaType.cpp
@@ -9231,7 +9231,7 @@ bool Sema::RequireCompleteType(SourceLocation Loc, QualType T,
     TD->setCompleteDefinitionRequired();
     Consumer.HandleTagDeclRequiredDefinition(TD);
   }
-  return false;
+  return T->isIncompleteType();
 }
 
 bool Sema::hasStructuralCompatLayout(Decl *D, Decl *Suggested) {

Try to see if that change helps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah ha, that does sound like it might be a better option :D

Assuming this works I will be much happier - I was certain that there must be a centralized location to test this.

I'm also convinced that there must be some other path that ends up assuming a complete type cannot include an incomplete one but I haven't been able to find one - my only reason for wanting such a path is to verify that if it does exist, this would fix it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nah, this does not work at all!
But I think there is probably a bug in RequireCompleteTypeImpl or something nearby

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does it break existing tests, or simply not fix this issue?

return false;
const TagDecl *Decl = Type->getAsTagDecl();
return !Decl || !Decl->getDefinition()->isInvalidDecl();
Copy link
Contributor

@mizvekov mizvekov Oct 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Did you try modifying isIncompleteType so it doesn't consider a Tag with invalid definition as complete?

That would give us more data to answer if this a good approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was worried about making more widespread changes, but I'll give that a shot - certainly it seems reasonable to consider invalid as being incomplete.

I had a comment for Corentin asking if there's an existing function that basically answers the "is this a complete, valid, and fully instantiated type" (rather than these local helpers), but I'm not sure if such a query has any value beyond this specific case. Certainly if we were to use some bits in the type/decl and evaluate this logic eagerly in future this logic disappears and the query would become unnecessary so if there's no other need for such a general there would be no reason to add it globally.

Anyway, that last paragraph is independent of isIncomplete also testing for isInvalid. I have a suspicion that we may get some issues with diagnostics - at least the diagnostic messages themselves changing, I can't imagine it impacting actual correctness, but lets find out :D

Copy link
Contributor

Choose a reason for hiding this comment

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

If the isIncompleteType change has larger consequences and ends up being a positive, we can tease it out into another PR.

That would certainly be less arbitrary than if we did that just for this specific case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, it will just take time to run all the tests (and I have a few other builds running atm), will hopefully get to testing that in the next hour or so

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mizvekov Changing the behavior of isIncompleteType, or changing RequireCompleteType as Corentin tried break all kinds of things everywhere. A lot of it seems to be diagnostics (significant regressions), but there are so many failures I can't be sure there are also actual correctness errors.

}
static bool canDeterminePointerAuthContent(const ASTContext &Ctx,
const TagDecl *Decl) {
CanQualType DeclType = Ctx.getCanonicalTagType(Decl);
return canDeterminePointerAuthContent(DeclType);
}

static bool primaryBaseHaseAddressDiscriminatedVTableAuthentication(
const ASTContext &Context, const CXXRecordDecl *Class) {
if (!canDeterminePointerAuthContent(Context, Class))
return false;
if (!Class->isPolymorphic())
return false;
const CXXRecordDecl *BaseType = Context.baseForVTableAuthentication(Class);
Expand All @@ -1639,7 +1657,7 @@ ASTContext::findPointerAuthContent(QualType T) const {
assert(isPointerAuthenticationAvailable());

T = T.getCanonicalType();
if (T->isDependentType())
if (!canDeterminePointerAuthContent(T))
return PointerAuthContent::None;

if (T.hasAddressDiscriminatedPointerAuth())
Expand Down Expand Up @@ -3230,6 +3248,7 @@ QualType ASTContext::removeAddrSpaceQualType(QualType T) const {

uint16_t
ASTContext::getPointerAuthVTablePointerDiscriminator(const CXXRecordDecl *RD) {
assert(canDeterminePointerAuthContent(*this, RD));
assert(RD->isPolymorphic() &&
"Attempted to get vtable pointer discriminator on a monomorphic type");
std::unique_ptr<MangleContext> MC(createMangleContext());
Expand Down Expand Up @@ -3517,7 +3536,9 @@ static void encodeTypeForFunctionPointerAuth(const ASTContext &Ctx,
uint16_t ASTContext::getPointerAuthTypeDiscriminator(QualType T) {
assert(!T->isDependentType() &&
"cannot compute type discriminator of a dependent type");

assert(canDeterminePointerAuthContent(T) &&
"cannot compute type discriminator of an incomplete or otherwise "
"invalid type");
SmallString<256> Str;
llvm::raw_svector_ostream Out(Str);

Expand Down
39 changes: 39 additions & 0 deletions clang/test/SemaCXX/ptrauth-nested-incomplete-types.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
// RUN: %clang_cc1 -fptrauth-intrinsics -fsyntax-only -ferror-limit 1 -verify -std=c++26 %s
// RUN: %clang_cc1 -fptrauth-intrinsics -fsyntax-only -ferror-limit 1 -verify -std=c++03 %s
// RUN: %clang_cc1 -fsyntax-only -ferror-limit 1 -verify -std=c++03 %s

Comment on lines +1 to +4
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you test without -fsyntax-only

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

have added, is there any difference in behavior between -fsyntax-only and not if there are errors? (i.e. do further passes running prior to the abort)

/// Force two errors so we hit the error limit leading to skip of template instantiation
# "" // expected-error {{invalid preprocessing directive}}
# ""
// expected-error@* {{too many errors emitted}}

template <typename>
struct a {};
struct test_polymorphic {
virtual ~test_polymorphic();
a<int> field;
};
static_assert(__is_trivially_relocatable(test_polymorphic));

struct test_struct {
test_struct(int) {}
void test_instantiate() {
test_struct d(0);
}
void test_type_trait_query() {
__is_trivially_relocatable(test_struct);
}
a<int> e;
};

struct test_subclass : test_struct {
test_subclass() : test_struct(0) {
}

void test_subclass_instantiation() {
test_subclass subclass{};
}
void test_subclass_type_trait_query() {
__is_trivially_relocatable(test_subclass);
}
};