Skip to content
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.

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

[clang] Add missing canonicalization in int literal profile #67822

Merged
merged 1 commit into from
Oct 2, 2023

Conversation

hnrklssn
Copy link
Member

The addition of the type kind to the profile ID of IntegerLiterals results in e.g. size_t and unsigned long literals mismatch even on platforms where they are canonically the same type. This patch checks the Canonical field to determine whether to canonicalize the type first.

rdar://116063468

The addition of the type kind to the profile ID of IntegerLiterals
results in e.g. size_t and unsigned long literals mismatch even on
platforms where they are canonically the same type. This patch checks
the Canonical field to determine whether to canonicalize the type first.

rdar://116063468
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:modules C++20 modules and Clang Header Modules labels Sep 29, 2023
@llvmbot
Copy link
Collaborator

llvmbot commented Sep 29, 2023

@llvm/pr-subscribers-clang-modules

@llvm/pr-subscribers-clang

Changes

The addition of the type kind to the profile ID of IntegerLiterals results in e.g. size_t and unsigned long literals mismatch even on platforms where they are canonically the same type. This patch checks the Canonical field to determine whether to canonicalize the type first.

rdar://116063468


Full diff: https://github.com/llvm/llvm-project/pull/67822.diff

1 Files Affected:

  • (modified) clang/lib/AST/StmtProfile.cpp (+2)
diff --git a/clang/lib/AST/StmtProfile.cpp b/clang/lib/AST/StmtProfile.cpp
index 2e4f15f83ac26ef..763d3d612698095 100644
--- a/clang/lib/AST/StmtProfile.cpp
+++ b/clang/lib/AST/StmtProfile.cpp
@@ -1335,6 +1335,8 @@ void StmtProfiler::VisitIntegerLiteral(const IntegerLiteral *S) {
   S->getValue().Profile(ID);
 
   QualType T = S->getType();
+  if (Canonical)
+    T = T.getCanonicalType();
   ID.AddInteger(T->getTypeClass());
   if (auto BitIntT = T->getAs<BitIntType>())
     BitIntT->Profile(ID);

Copy link
Contributor

@NagyDonat NagyDonat left a comment

Choose a reason for hiding this comment

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

LGTM.

By the way, as I searched for other applications of the flag Canonical, I noticed that StmtProfiler has a VisitType(QualType T) method which "cheats" and just stores T as an opaque pointer (after canonicalizing it if needed). If the pointer value is canonical enough (which I presume), then calling that (instead of the profiling that was custom-built for the Builtin and BitInt types) could provide a different way for implementing VisitIntegerLiteral -- but the current code is also OK after your correction.

@hnrklssn hnrklssn merged commit 5317912 into llvm:main Oct 2, 2023
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:modules C++20 modules and Clang Header Modules clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants