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-repl] Fix RuntimeInterfaceBuilder for 32-bit systems #97071

Merged
merged 3 commits into from
Jul 3, 2024

Conversation

weliveindetail
Copy link
Contributor

@weliveindetail weliveindetail commented Jun 28, 2024

When generating runtime interface bindings, extend integral types to the native register size rather than 64-bit per se

Fixes #94994

When generating runtime interface bindings, extend integral types to the native register size rather than 64-bit per se
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jun 28, 2024
@llvmbot
Copy link
Member

llvmbot commented Jun 28, 2024

@llvm/pr-subscribers-clang

Author: Stefan Gränitz (weliveindetail)

Changes

When generating runtime interface bindings, extend integral types to the native register size rather than 64-bit per se


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

1 Files Affected:

  • (modified) clang/lib/Interpreter/Interpreter.cpp (+5-3)
diff --git a/clang/lib/Interpreter/Interpreter.cpp b/clang/lib/Interpreter/Interpreter.cpp
index 7a95278914276..e6d6ab6024bb8 100644
--- a/clang/lib/Interpreter/Interpreter.cpp
+++ b/clang/lib/Interpreter/Interpreter.cpp
@@ -673,10 +673,12 @@ class InterfaceKindVisitor
   }
 
 private:
-  // Force cast these types to uint64 to reduce the number of overloads of
-  // `__clang_Interpreter_SetValueNoAlloc`.
+  // Force cast these types to the uint that fits the register size. That way we
+  // reduce the number of overloads of `__clang_Interpreter_SetValueNoAlloc`.
   void HandleIntegralOrEnumType(const Type *Ty) {
-    TypeSourceInfo *TSI = Ctx.getTrivialTypeSourceInfo(Ctx.UnsignedLongLongTy);
+    uint64_t PtrBits = Ctx.getTypeSize(Ctx.VoidPtrTy);
+    QualType UIntTy = Ctx.getBitIntType(true, PtrBits);
+    TypeSourceInfo *TSI = Ctx.getTrivialTypeSourceInfo(UIntTy);
     ExprResult CastedExpr =
         S.BuildCStyleCastExpr(SourceLocation(), TSI, SourceLocation(), E);
     assert(!CastedExpr.isInvalid() && "Cannot create cstyle cast expr");

Copy link
Contributor

@vgvassilev vgvassilev left a comment

Choose a reason for hiding this comment

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

Oh, that's a good catch. Any chance of adding a test case? Maybe something like:

enum X : short {...}?

clang/lib/Interpreter/Interpreter.cpp Outdated Show resolved Hide resolved
Co-authored-by: Vassil Vassilev <v.g.vassilev@gmail.com>
@weliveindetail
Copy link
Contributor Author

Any chance of adding a test case?

We could add a lot more tests in general, but I don't think this specific detail requires a dedicated one. It's a fix for an existing bug, so we have coverage already.

Copy link
Contributor

@vgvassilev vgvassilev left a comment

Choose a reason for hiding this comment

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

LGTM!

@eymay
Copy link
Contributor

eymay commented Jun 29, 2024

Great work, I can imagine how hard it would be to uncover it!

Small note: I think you may also want to remove the Arm macro guards that mask the unittest in the same commit, as you did in the previous PR

@DavidSpickett
Copy link
Collaborator

DavidSpickett commented Jul 1, 2024

Also remove the comment that links to #94994 and add Fixes #94994 to the commit message.

I checked locally, the test now passes.

@weliveindetail
Copy link
Contributor Author

Thanks for your feedback everyone!

@weliveindetail weliveindetail merged commit 1787d4b into llvm:main Jul 3, 2024
7 checks passed
lravenclaw pushed a commit to lravenclaw/llvm-project that referenced this pull request Jul 3, 2024
When generating runtime interface bindings, extend integral types to the
native register size rather than 64-bit per se

Fixes llvm#94994
kbluck pushed a commit to kbluck/llvm-project that referenced this pull request Jul 6, 2024
When generating runtime interface bindings, extend integral types to the
native register size rather than 64-bit per se

Fixes llvm#94994
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 Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

clang-repl value unit test fails on 32 bit Arm
5 participants