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] Lay the foundation of pretty printing for C. #89811

Merged
merged 1 commit into from
Jun 6, 2024

Conversation

vgvassilev
Copy link
Contributor

Depends on #89804.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Apr 23, 2024
@vgvassilev vgvassilev requested a review from lhames April 23, 2024 19:37
@llvmbot
Copy link
Member

llvmbot commented Apr 23, 2024

@llvm/pr-subscribers-clang

Author: Vassil Vassilev (vgvassilev)

Changes

Depends on #89804.


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

6 Files Affected:

  • (modified) clang/lib/Interpreter/IncrementalParser.cpp (+11-2)
  • (modified) clang/lib/Interpreter/Interpreter.cpp (+92-76)
  • (modified) clang/lib/Parse/ParseStmt.cpp (+1-4)
  • (modified) clang/lib/Sema/SemaDecl.cpp (+2-1)
  • (added) clang/test/Interpreter/execute.c (+21)
  • (added) clang/test/Interpreter/pretty-print.c (+8)
diff --git a/clang/lib/Interpreter/IncrementalParser.cpp b/clang/lib/Interpreter/IncrementalParser.cpp
index ef90fe9e6f5451..f1cb5fc870eb94 100644
--- a/clang/lib/Interpreter/IncrementalParser.cpp
+++ b/clang/lib/Interpreter/IncrementalParser.cpp
@@ -387,8 +387,7 @@ std::unique_ptr<llvm::Module> IncrementalParser::GenModule() {
 
 void IncrementalParser::CleanUpPTU(PartialTranslationUnit &PTU) {
   TranslationUnitDecl *MostRecentTU = PTU.TUPart;
-  TranslationUnitDecl *FirstTU = MostRecentTU->getFirstDecl();
-  if (StoredDeclsMap *Map = FirstTU->getPrimaryContext()->getLookupPtr()) {
+  if (StoredDeclsMap *Map = MostRecentTU->getPrimaryContext()->getLookupPtr()) {
     for (auto &&[Key, List] : *Map) {
       DeclContextLookupResult R = List.getLookupResult();
       std::vector<NamedDecl *> NamedDeclsToRemove;
@@ -407,6 +406,16 @@ void IncrementalParser::CleanUpPTU(PartialTranslationUnit &PTU) {
       }
     }
   }
+
+  // FIXME: We should de-allocate MostRecentTU
+  for (Decl *D : MostRecentTU->decls()) {
+    if (!isa<NamedDecl>(D))
+      continue;
+    // Check if we need to clean up the IdResolver chain.
+    NamedDecl *ND = cast<NamedDecl>(D);
+    if (ND->getDeclName().getFETokenInfo())
+      getCI()->getSema().IdResolver.RemoveDecl(ND);
+  }
 }
 
 llvm::StringRef IncrementalParser::GetMangledName(GlobalDecl GD) const {
diff --git a/clang/lib/Interpreter/Interpreter.cpp b/clang/lib/Interpreter/Interpreter.cpp
index b20e6efcebfd10..96abf4bc53ef4b 100644
--- a/clang/lib/Interpreter/Interpreter.cpp
+++ b/clang/lib/Interpreter/Interpreter.cpp
@@ -42,6 +42,9 @@
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/TargetParser/Host.h"
+
+#include <stdarg.h>
+
 using namespace clang;
 
 // FIXME: Figure out how to unify with namespace init_convenience from
@@ -250,14 +253,9 @@ Interpreter::~Interpreter() {
 // can't find the precise resource directory in unittests so we have to hard
 // code them.
 const char *const Runtimes = R"(
+    #define __CLANG_REPL__ 1
 #ifdef __cplusplus
     void *__clang_Interpreter_SetValueWithAlloc(void*, void*, void*);
-    void __clang_Interpreter_SetValueNoAlloc(void*, void*, void*);
-    void __clang_Interpreter_SetValueNoAlloc(void*, void*, void*, void*);
-    void __clang_Interpreter_SetValueNoAlloc(void*, void*, void*, float);
-    void __clang_Interpreter_SetValueNoAlloc(void*, void*, void*, double);
-    void __clang_Interpreter_SetValueNoAlloc(void*, void*, void*, long double);
-    void __clang_Interpreter_SetValueNoAlloc(void*,void*,void*,unsigned long long);
     struct __clang_Interpreter_NewTag{} __ci_newtag;
     void* operator new(__SIZE_TYPE__, void* __p, __clang_Interpreter_NewTag) noexcept;
     template <class T, class = T (*)() /*disable for arrays*/>
@@ -269,7 +267,10 @@ const char *const Runtimes = R"(
     void __clang_Interpreter_SetValueCopyArr(const T (*Src)[N], void* Placement, unsigned long Size) {
       __clang_Interpreter_SetValueCopyArr(Src[0], Placement, Size);
     }
+    extern "C"
 #endif // __cplusplus
+
+  void __clang_Interpreter_SetValueNoAlloc(void *This, void *OutVal, void *OpaqueType, ...);
 )";
 
 llvm::Expected<std::unique_ptr<Interpreter>>
@@ -564,15 +565,17 @@ std::unique_ptr<RuntimeInterfaceBuilder> Interpreter::FindRuntimeInterface() {
   if (!LookupInterface(ValuePrintingInfo[NoAlloc],
                        MagicRuntimeInterface[NoAlloc]))
     return nullptr;
-  if (!LookupInterface(ValuePrintingInfo[WithAlloc],
-                       MagicRuntimeInterface[WithAlloc]))
-    return nullptr;
-  if (!LookupInterface(ValuePrintingInfo[CopyArray],
-                       MagicRuntimeInterface[CopyArray]))
-    return nullptr;
-  if (!LookupInterface(ValuePrintingInfo[NewTag],
-                       MagicRuntimeInterface[NewTag]))
-    return nullptr;
+  if (Ctx.getLangOpts().CPlusPlus) {
+    if (!LookupInterface(ValuePrintingInfo[WithAlloc],
+                         MagicRuntimeInterface[WithAlloc]))
+      return nullptr;
+    if (!LookupInterface(ValuePrintingInfo[CopyArray],
+                         MagicRuntimeInterface[CopyArray]))
+      return nullptr;
+    if (!LookupInterface(ValuePrintingInfo[NewTag],
+                         MagicRuntimeInterface[NewTag]))
+      return nullptr;
+  }
 
   return createInProcessRuntimeInterfaceBuilder(*this, Ctx, S);
 }
@@ -831,69 +834,82 @@ __clang_Interpreter_SetValueWithAlloc(void *This, void *OutVal,
   return VRef.getPtr();
 }
 
-// Pointers, lvalue struct that can take as a reference.
-REPL_EXTERNAL_VISIBILITY void
-__clang_Interpreter_SetValueNoAlloc(void *This, void *OutVal, void *OpaqueType,
-                                    void *Val) {
-  Value &VRef = *(Value *)OutVal;
-  VRef = Value(static_cast<Interpreter *>(This), OpaqueType);
-  VRef.setPtr(Val);
-}
-
-REPL_EXTERNAL_VISIBILITY void
-__clang_Interpreter_SetValueNoAlloc(void *This, void *OutVal,
-                                    void *OpaqueType) {
+REPL_EXTERNAL_VISIBILITY
+extern "C" void __clang_Interpreter_SetValueNoAlloc(void *This, void *OutVal,
+                                                    void *OpaqueType, ...) {
   Value &VRef = *(Value *)OutVal;
-  VRef = Value(static_cast<Interpreter *>(This), OpaqueType);
-}
-
-static void SetValueDataBasedOnQualType(Value &V, unsigned long long Data) {
-  QualType QT = V.getType();
-  if (const auto *ET = QT->getAs<EnumType>())
-    QT = ET->getDecl()->getIntegerType();
-
-  switch (QT->castAs<BuiltinType>()->getKind()) {
-  default:
-    llvm_unreachable("unknown type kind!");
-#define X(type, name)                                                          \
-  case BuiltinType::name:                                                      \
-    V.set##name(Data);                                                         \
-    break;
-    REPL_BUILTIN_TYPES
-#undef X
+  Interpreter *I = static_cast<Interpreter *>(This);
+  VRef = Value(I, OpaqueType);
+  if (VRef.isVoid())
+    return;
+
+  va_list args;
+  va_start(args, /*last named param*/ OpaqueType);
+
+  QualType QT = VRef.getType();
+  if (VRef.getKind() == Value::K_PtrOrObj) {
+    VRef.setPtr(va_arg(args, void *));
+  } else {
+    if (const auto *ET = QT->getAs<EnumType>())
+      QT = ET->getDecl()->getIntegerType();
+    switch (QT->castAs<BuiltinType>()->getKind()) {
+    default:
+      llvm_unreachable("unknown type kind!");
+      break;
+      // Types shorter than int are resolved as int, else va_arg has UB.
+    case BuiltinType::Bool:
+      VRef.setBool(va_arg(args, int));
+      break;
+    case BuiltinType::Char_S:
+      VRef.setChar_S(va_arg(args, int));
+      break;
+    case BuiltinType::SChar:
+      VRef.setSChar(va_arg(args, int));
+      break;
+    case BuiltinType::Char_U:
+      VRef.setChar_U(va_arg(args, unsigned));
+      break;
+    case BuiltinType::UChar:
+      VRef.setUChar(va_arg(args, unsigned));
+      break;
+    case BuiltinType::Short:
+      VRef.setShort(va_arg(args, int));
+      break;
+    case BuiltinType::UShort:
+      VRef.setUShort(va_arg(args, unsigned));
+      break;
+    case BuiltinType::Int:
+      VRef.setInt(va_arg(args, int));
+      break;
+    case BuiltinType::UInt:
+      VRef.setUInt(va_arg(args, unsigned));
+      break;
+    case BuiltinType::Long:
+      VRef.setLong(va_arg(args, long));
+      break;
+    case BuiltinType::ULong:
+      VRef.setULong(va_arg(args, unsigned long));
+      break;
+    case BuiltinType::LongLong:
+      VRef.setLongLong(va_arg(args, long long));
+      break;
+    case BuiltinType::ULongLong:
+      VRef.setULongLong(va_arg(args, unsigned long long));
+      break;
+      // Types shorter than double are resolved as double, else va_arg has UB.
+    case BuiltinType::Float:
+      VRef.setFloat(va_arg(args, double));
+      break;
+    case BuiltinType::Double:
+      VRef.setDouble(va_arg(args, double));
+      break;
+    case BuiltinType::LongDouble:
+      VRef.setLongDouble(va_arg(args, long double));
+      break;
+      // See REPL_BUILTIN_TYPES.
+    }
   }
-}
-
-REPL_EXTERNAL_VISIBILITY void
-__clang_Interpreter_SetValueNoAlloc(void *This, void *OutVal, void *OpaqueType,
-                                    unsigned long long Val) {
-  Value &VRef = *(Value *)OutVal;
-  VRef = Value(static_cast<Interpreter *>(This), OpaqueType);
-  SetValueDataBasedOnQualType(VRef, Val);
-}
-
-REPL_EXTERNAL_VISIBILITY void
-__clang_Interpreter_SetValueNoAlloc(void *This, void *OutVal, void *OpaqueType,
-                                    float Val) {
-  Value &VRef = *(Value *)OutVal;
-  VRef = Value(static_cast<Interpreter *>(This), OpaqueType);
-  VRef.setFloat(Val);
-}
-
-REPL_EXTERNAL_VISIBILITY void
-__clang_Interpreter_SetValueNoAlloc(void *This, void *OutVal, void *OpaqueType,
-                                    double Val) {
-  Value &VRef = *(Value *)OutVal;
-  VRef = Value(static_cast<Interpreter *>(This), OpaqueType);
-  VRef.setDouble(Val);
-}
-
-REPL_EXTERNAL_VISIBILITY void
-__clang_Interpreter_SetValueNoAlloc(void *This, void *OutVal, void *OpaqueType,
-                                    long double Val) {
-  Value &VRef = *(Value *)OutVal;
-  VRef = Value(static_cast<Interpreter *>(This), OpaqueType);
-  VRef.setLongDouble(Val);
+  va_end(args);
 }
 
 // A trampoline to work around the fact that operator placement new cannot
diff --git a/clang/lib/Parse/ParseStmt.cpp b/clang/lib/Parse/ParseStmt.cpp
index 629421c01d17d2..63bc83c1cfd506 100644
--- a/clang/lib/Parse/ParseStmt.cpp
+++ b/clang/lib/Parse/ParseStmt.cpp
@@ -560,11 +560,8 @@ StmtResult Parser::ParseExprStatement(ParsedStmtContext StmtCtx) {
   }
 
   Token *CurTok = nullptr;
-  // If the semicolon is missing at the end of REPL input, consider if
-  // we want to do value printing. Note this is only enabled in C++ mode
-  // since part of the implementation requires C++ language features.
   // Note we shouldn't eat the token since the callback needs it.
-  if (Tok.is(tok::annot_repl_input_end) && Actions.getLangOpts().CPlusPlus)
+  if (Tok.is(tok::annot_repl_input_end))
     CurTok = &Tok;
   else
     // Otherwise, eat the semicolon.
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index 452e00fa32b102..2a0f73b42d3088 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -2282,7 +2282,8 @@ void Sema::ActOnPopScope(SourceLocation Loc, Scope *S) {
 
     // Remove this name from our lexical scope, and warn on it if we haven't
     // already.
-    IdResolver.RemoveDecl(D);
+    if (!PP.isIncrementalProcessingEnabled())
+      IdResolver.RemoveDecl(D);
     auto ShadowI = ShadowingDecls.find(D);
     if (ShadowI != ShadowingDecls.end()) {
       if (const auto *FD = dyn_cast<FieldDecl>(ShadowI->second)) {
diff --git a/clang/test/Interpreter/execute.c b/clang/test/Interpreter/execute.c
new file mode 100644
index 00000000000000..44a3a32c930112
--- /dev/null
+++ b/clang/test/Interpreter/execute.c
@@ -0,0 +1,21 @@
+// REQUIRES: host-supports-jit
+// UNSUPPORTED: system-aix
+
+// RUN: cat %s | clang-repl -Xcc -xc -Xcc -Xclang -Xcc -verify | FileCheck %s
+// RUN: cat %s | clang-repl -Xcc -xc -Xcc -O2 -Xcc -Xclang -Xcc -verify| FileCheck %s
+int printf(const char *, ...);
+int i = 42; err // expected-error{{use of undeclared identifier}}
+int i = 42;
+struct S { float f; struct S *m;} s = {1.0, 0};
+// FIXME: Making foo inline fails to emit the function.
+int foo() { return 42; }
+void run() {                                                    \
+  printf("i = %d\n", i);                                        \
+  printf("S[f=%f, m=0x%llx]\n", s.f, (unsigned long long)s.m);  \
+  int r3 = foo();                                               \
+}
+run();
+// CHECK: i = 42
+// CHECK-NEXT: S[f=1.000000, m=0x0]
+
+%quit
diff --git a/clang/test/Interpreter/pretty-print.c b/clang/test/Interpreter/pretty-print.c
new file mode 100644
index 00000000000000..f6158ad4ecc99c
--- /dev/null
+++ b/clang/test/Interpreter/pretty-print.c
@@ -0,0 +1,8 @@
+// REQUIRES: host-supports-jit
+// UNSUPPORTED: system-aix
+// RUN: cat %s | clang-repl -Xcc -xc  | FileCheck %s
+// RUN: cat %s | clang-repl -Xcc -std=c++11 | FileCheck %s
+
+const char* c_str = "Hello, world!"; c_str
+
+// CHECK: Not implement yet.

@vgvassilev vgvassilev requested a review from AaronBallman May 10, 2024 19:42
Copy link
Collaborator

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

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

Generally looks good, but it seems that some parts of this PR are also incorporated into another PR, so it's a bit hard to review.

clang/lib/Interpreter/Interpreter.cpp Outdated Show resolved Hide resolved
@vgvassilev vgvassilev force-pushed the value-printing-c branch 2 times, most recently from 4a73cd3 to 3e92293 Compare May 21, 2024 17:46
@vgvassilev
Copy link
Contributor Author

Generally looks good, but it seems that some parts of this PR are also incorporated into another PR, so it's a bit hard to review.

Rebased, should be only the commit in question now, as the other one landed.

Copy link
Collaborator

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

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

LGTM, feel free to apply my suggestion if you like it.

clang/lib/Interpreter/Interpreter.cpp Outdated Show resolved Hide resolved
@vgvassilev
Copy link
Contributor Author

LGTM, feel free to apply my suggestion if you like it.

Applied it. Thanks for the review this unblock whole bunch of work for us..

@vgvassilev vgvassilev force-pushed the value-printing-c branch 2 times, most recently from 3094e60 to eb5d2ec Compare June 5, 2024 18:52
@vgvassilev vgvassilev merged commit 7091dfc into llvm:main Jun 6, 2024
7 checks passed
@vgvassilev vgvassilev deleted the value-printing-c branch June 6, 2024 08:49
@DavidSpickett
Copy link
Collaborator

DavidSpickett commented Jun 6, 2024

I have bisected a 32 bit Arm failure down to this change: https://lab.llvm.org/buildbot/#/builders/245/builds/25526

******************** TEST 'Clang-Unit :: Interpreter/./ClangReplInterpreterTests/10/26' FAILED ********************
Script(shard):
--
GTEST_OUTPUT=json:/home/tcwg-buildbot/worker/clang-armv8-quick/stage1/tools/clang/unittests/Interpreter/./ClangReplInterpreterTests-Clang-Unit-2678467-10-26.json GTEST_SHUFFLE=0 GTEST_TOTAL_SHARDS=26 GTEST_SHARD_INDEX=10 /home/tcwg-buildbot/worker/clang-armv8-quick/stage1/tools/clang/unittests/Interpreter/./ClangReplInterpreterTests
--
Script:
--
/home/tcwg-buildbot/worker/clang-armv8-quick/stage1/tools/clang/unittests/Interpreter/./ClangReplInterpreterTests --gtest_filter=InterpreterTest.Value
--
../llvm/clang/unittests/Interpreter/InterpreterTest.cpp:293: Failure
Expected equality of these values:
  V1.getInt()
    Which is: 0
  42

Is it possible that something in this code is truncating a value when interpreting types intended for a 64 bit system, on a 32 bit system? I don't see this failure on any of our Arm64 buildbots.

@vgvassilev
Copy link
Contributor Author

Is the bot configured to build for arm64 and run on arm32 somehow? But in either case the width of the int type should be the same, right?

@DavidSpickett
Copy link
Collaborator

It is a 32 bit container running on an arm64 host. As far as the compiler and tests know, it's a 32 bit system.

But in either case the width of the int type should be the same, right?

Yes.

So I am wondering if someone of the assumptions about types being larger than one another do not hold on 32 bit. Or your change has exposed an existing issue, it wouldn't be the first time.

@vgvassilev
Copy link
Contributor Author

So I am wondering if someone of the assumptions about types being larger than one another do not hold on 32 bit. Or your change has exposed an existing issue, it wouldn't be the first time.

My feeling is the latter but how can we get a debug build and debug?

@DavidSpickett
Copy link
Collaborator

My feeling is the latter but how can we get a debug build and debug?

I'm going to try a UBSAN build on the chance it pinpoints the issue right away, but failing that it'll be cross compiling to Arm 32 bit and using qemu-user to run the unit test binary. I will get you instructions on how to do that (will be tomorrow at this point).

If this is a generic 32 bit problem you could also build for 32 bit on x86. I don't know if it is because there are no bots for that.

@vitalybuka
Copy link
Collaborator

@vgvassilev
Copy link
Contributor Author

It's broken here https://lab.llvm.org/buildbot/#/builders/236/builds/11633

It seems broken for a different reason. Somehow we did not export that __clang_Interpreter_SetValueNoAlloc symbol. Is that some -fvisibility=hidden build?

@vitalybuka
Copy link
Collaborator

Not sure, nothing in cmake cmd:

cmake -DLLVM_APPEND_VC_REV=OFF -GNinja -DCMAKE_BUILD_TYPE=Release -DLLVM_CCACHE_BUILD=ON -DLLVM_USE_LINKER=lld -DLLVM_ENABLE_ASSERTIONS=ON -DLLVM_ENABLE_PER_TARGET_RUNTIME_DIR=OFF -DCMAKE_C_COMPILER=/b/sanitizer-aarch64-linux-bootstrap-hwasan/build/llvm_build0/bin/clang -DCMAKE_CXX_COMPILER=/b/sanitizer-aarch64-linux-bootstrap-hwasan/build/llvm_build0/bin/clang++ -DLLVM_ENABLE_PLUGINS=OFF '-DLLVM_ENABLE_PROJECTS='\''clang;lld;mlir'\''' -DLLVM_USE_SANITIZER=HWAddress -DLLVM_ENABLE_LIBCXX=ON '-DCMAKE_C_FLAGS=-nostdinc++ -isystem /b/sanitizer-aarch64-linux-bootstrap-hwasan/build/libcxx_build_hwasan/include -isystem /b/sanitizer-aarch64-linux-bootstrap-hwasan/build/libcxx_build_hwasan/include/c++/v1 -fsanitize=hwaddress -mllvm -hwasan-use-after-scope=1 -Wl,--rpath=/b/sanitizer-aarch64-linux-bootstrap-hwasan/build/libcxx_build_hwasan/lib -L/b/sanitizer-aarch64-linux-bootstrap-hwasan/build/libcxx_build_hwasan/lib -w' '-DCMAKE_CXX_FLAGS=-nostdinc++ -isystem /b/sanitizer-aarch64-linux-bootstrap-hwasan/build/libcxx_build_hwasan/include -isystem /b/sanitizer-aarch64-linux-bootstrap-hwasan/build/libcxx_build_hwasan/include/c++/v1 -fsanitize=hwaddress -mllvm -hwasan-use-after-scope=1 -Wl,--rpath=/b/sanitizer-aarch64-linux-bootstrap-hwasan/build/libcxx_build_hwasan/lib -L/b/sanitizer-aarch64-linux-bootstrap-hwasan/build/libcxx_build_hwasan/lib -w' '-DCMAKE_EXE_LINKER_FLAGS=-Wl,--rpath=/b/sanitizer-aarch64-linux-bootstrap-hwasan/build/libcxx_build_hwasan/lib -L/b/sanitizer-aarch64-linux-bootstrap-hwasan/build/libcxx_build_hwasan/lib' /b/sanitizer-aarch64-linux-bootstrap-hwasan/build/llvm-project/llvm

@vgvassilev
Copy link
Contributor Author

Very strange. I did not see a lot of platforms failing. If we decide to keep that commit, is there a way to disable this test for exactly that platform?

@vgvassilev
Copy link
Contributor Author

Not sure, nothing in cmake cmd:

cmake -DLLVM_APPEND_VC_REV=OFF -GNinja -DCMAKE_BUILD_TYPE=Release -DLLVM_CCACHE_BUILD=ON -DLLVM_USE_LINKER=lld -DLLVM_ENABLE_ASSERTIONS=ON -DLLVM_ENABLE_PER_TARGET_RUNTIME_DIR=OFF -DCMAKE_C_COMPILER=/b/sanitizer-aarch64-linux-bootstrap-hwasan/build/llvm_build0/bin/clang -DCMAKE_CXX_COMPILER=/b/sanitizer-aarch64-linux-bootstrap-hwasan/build/llvm_build0/bin/clang++ -DLLVM_ENABLE_PLUGINS=OFF '-DLLVM_ENABLE_PROJECTS='\''clang;lld;mlir'\''' -DLLVM_USE_SANITIZER=HWAddress -DLLVM_ENABLE_LIBCXX=ON '-DCMAKE_C_FLAGS=-nostdinc++ -isystem /b/sanitizer-aarch64-linux-bootstrap-hwasan/build/libcxx_build_hwasan/include -isystem /b/sanitizer-aarch64-linux-bootstrap-hwasan/build/libcxx_build_hwasan/include/c++/v1 -fsanitize=hwaddress -mllvm -hwasan-use-after-scope=1 -Wl,--rpath=/b/sanitizer-aarch64-linux-bootstrap-hwasan/build/libcxx_build_hwasan/lib -L/b/sanitizer-aarch64-linux-bootstrap-hwasan/build/libcxx_build_hwasan/lib -w' '-DCMAKE_CXX_FLAGS=-nostdinc++ -isystem /b/sanitizer-aarch64-linux-bootstrap-hwasan/build/libcxx_build_hwasan/include -isystem /b/sanitizer-aarch64-linux-bootstrap-hwasan/build/libcxx_build_hwasan/include/c++/v1 -fsanitize=hwaddress -mllvm -hwasan-use-after-scope=1 -Wl,--rpath=/b/sanitizer-aarch64-linux-bootstrap-hwasan/build/libcxx_build_hwasan/lib -L/b/sanitizer-aarch64-linux-bootstrap-hwasan/build/libcxx_build_hwasan/lib -w' '-DCMAKE_EXE_LINKER_FLAGS=-Wl,--rpath=/b/sanitizer-aarch64-linux-bootstrap-hwasan/build/libcxx_build_hwasan/lib -L/b/sanitizer-aarch64-linux-bootstrap-hwasan/build/libcxx_build_hwasan/lib' /b/sanitizer-aarch64-linux-bootstrap-hwasan/build/llvm-project/llvm

@vitalybuka could it be a ccache glitch?

@vitalybuka
Copy link
Collaborator

@vitalybuka could it be a ccache glitch?

Technically can, but I would not count on hash collision :)
Also there are two machines running same config consistently fail.

@vitalybuka
Copy link
Collaborator

Try // UNSUPPORTED: hwasan

@vitalybuka
Copy link
Collaborator

Actually I will try myself, as I have access to the bot.

vitalybuka added a commit that referenced this pull request Jun 6, 2024
DavidSpickett added a commit that referenced this pull request Jun 7, 2024
#89811 caused this test to fail,
somehow.

I think it may not be at fault, but actually be exposing some
existing undefined behaviour, see
#94741.

Skipping this for now to get the bots green again.
@DavidSpickett
Copy link
Collaborator

I will get you instructions on how to do that (will be tomorrow at this point).

#94741 (comment)

There is some underlying issue (#94741), so if you want to try and fix it, that's great, but it's also fine if you'd rather wait and see what I find on native hardware.

@DavidSpickett
Copy link
Collaborator

Actually, as someone pointed out, I've been conflating clang-repl with the constant expression interpreter. So the issue covers the latter and this must be something else.

@vgvassilev
Copy link
Contributor Author

Could be. Is there a way for me to debug this?

@vgvassilev
Copy link
Contributor Author

Perhaps we can disable this platform if we have a way to express the setup in lit.

@DavidSpickett
Copy link
Collaborator

You can debug it by cross-compiling a debug build, then running the unit tests binary and connecting to qemu's built in gdbserver. The Arm toolchain might have a copy of gdb in it already, but if not, gdb multiarch and lldb would also work (which are both installable from apt).

I'm going to try looking in a debugger myself today.

@vgvassilev
Copy link
Contributor Author

You can debug it by cross-compiling a debug build, then running the unit tests binary and connecting to qemu's built in gdbserver. The Arm toolchain might have a copy of gdb in it already, but if not, gdb multiarch and lldb would also work (which are both installable from apt).

I'm going to try looking in a debugger myself today.

Ok, this sounds good. I've never thought about that setup and perhaps is just fine to suppress that test.

@zmodem
Copy link
Collaborator

zmodem commented Jun 10, 2024

We're seeing the test fail on our mac builds: https://issues.chromium.org/issues/346289767

JIT session error: Symbols not found: [ ___clang_Interpreter_SetValueNoAlloc ]
 error: Failed to materialize symbols: { (main, { ____orc_init_func.incr_module_10, _c_str, $.incr_module_10.__inits.0 }) }
 error: Failed to materialize symbols: { (main, { ____orc_init_func.incr_module_10 }) }

Our current theory is that it's because we configure with -DCLANG_PLUGIN_SUPPORT=OFF and clang-repl has this in its cmakelists.txt:

# Support plugins.
if(CLANG_PLUGIN_SUPPORT)
export_executable_symbols_for_plugins(clang-repl)
endif()

Maybe it should call export_executable_symbols_for_plugins unconditionally? Or the new test should REQUIRES: plugins?

@vgvassilev
Copy link
Contributor Author

Ah, that’s well spotted. The current test should not require plugins. Would dropping this conditional make it work for your case?

@zmodem
Copy link
Collaborator

zmodem commented Jun 10, 2024

Would dropping this conditional make it work for your case?

I don't have a mac handy to try it right now. If you commit it, our system will test it automatically :) Or maybe @Thakis could give it a try?

@vgvassilev
Copy link
Contributor Author

Would dropping this conditional make it work for your case?

I don't have a mac handy to try it right now. If you commit it, our system will test it automatically :) Or maybe @Thakis could give it a try?

I am on my phone in the next couple of hours and I am not sure how to reproduce it. Maybe somebody who can should give it a shot?

@zmodem
Copy link
Collaborator

zmodem commented Jun 10, 2024

I was able to confirm on a local Mac that removing the if(CLANG_PLUGIN_SUPPORT) fixes the test, so I'll go ahead and commit that.

zmodem added a commit that referenced this pull request Jun 10, 2024
It's needed to make clang-repl work in -DCLANG_PLUGIN_SUPPORT=OFF
configured builds (at least on mac). See discussion on
#89811
Lukacma pushed a commit to Lukacma/llvm-project that referenced this pull request Jun 12, 2024
It's needed to make clang-repl work in -DCLANG_PLUGIN_SUPPORT=OFF
configured builds (at least on mac). See discussion on
llvm#89811
@aeubanks
Copy link
Contributor

So actually even the export_executable_symbols_for_plugins doesn't fix our bots. I've narrowed it down to -DLLVM_ENABLE_PIC=ON/OFF. Perhaps we're not exporting symbols when -DLLVM_ENABLE_PIC=OFF.

@vgvassilev
Copy link
Contributor Author

vgvassilev commented Jun 12, 2024

So actually even the export_executable_symbols_for_plugins doesn't fix our bots. I've narrowed it down to -DLLVM_ENABLE_PIC=ON/OFF. Perhaps we're not exporting symbols when -DLLVM_ENABLE_PIC=OFF.

That’s quite strange. Is that somehow related to ld64? @lhames do you have any clue?

@aeubanks
Copy link
Contributor

This was with -DLLVM_ENABLE_LLD=ON, so probably not? I'm guessing there's some CMake logic that doesn't fit with clang-repl's expectation of symbol availability, but that may be wrong.

@vgvassilev
Copy link
Contributor Author

Our current theory is that somehow the build system does not mark this symbol as exported. Maybe we can confirm that by making the symbol resolution for that particular one via the JIT absolute symbol definition. This is rather a workaround and the real fix should be in the build system.

diff --git a/clang/lib/Interpreter/Interpreter.cpp b/clang/lib/Interpreter/Interpreter.cpp
index 7a9527891427..4131d5ff0d99 100644
--- a/clang/lib/Interpreter/Interpreter.cpp
+++ b/clang/lib/Interpreter/Interpreter.cpp
@@ -43,6 +43,9 @@
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/TargetParser/Host.h"
 
+#include "llvm/ExecutionEngine/Orc/LLJIT.h"
+
+
 #include <cstdarg>
 
 using namespace clang;
@@ -405,6 +408,83 @@ createJITTargetMachineBuilder(const std::string &TT) {
   return llvm::orc::JITTargetMachineBuilder(llvm::Triple(TT));
 }
 
+extern "C" void REPL_EXTERNAL_VISIBILITY __clang_Interpreter_SetValueNoAlloc(
+    void *This, void *OutVal, void *OpaqueType, ...) {
+  Value &VRef = *(Value *)OutVal;
+  Interpreter *I = static_cast<Interpreter *>(This);
+  VRef = Value(I, OpaqueType);
+  if (VRef.isVoid())
+    return;
+
+  va_list args;
+  va_start(args, /*last named param*/ OpaqueType);
+
+  QualType QT = VRef.getType();
+  if (VRef.getKind() == Value::K_PtrOrObj) {
+    VRef.setPtr(va_arg(args, void *));
+  } else {
+    if (const auto *ET = QT->getAs<EnumType>())
+      QT = ET->getDecl()->getIntegerType();
+    switch (QT->castAs<BuiltinType>()->getKind()) {
+    default:
+      llvm_unreachable("unknown type kind!");
+      break;
+      // Types shorter than int are resolved as int, else va_arg has UB.
+    case BuiltinType::Bool:
+      VRef.setBool(va_arg(args, int));
+      break;
+    case BuiltinType::Char_S:
+      VRef.setChar_S(va_arg(args, int));
+      break;
+    case BuiltinType::SChar:
+      VRef.setSChar(va_arg(args, int));
+      break;
+    case BuiltinType::Char_U:
+      VRef.setChar_U(va_arg(args, unsigned));
+      break;
+    case BuiltinType::UChar:
+      VRef.setUChar(va_arg(args, unsigned));
+      break;
+    case BuiltinType::Short:
+      VRef.setShort(va_arg(args, int));
+      break;
+    case BuiltinType::UShort:
+      VRef.setUShort(va_arg(args, unsigned));
+      break;
+    case BuiltinType::Int:
+      VRef.setInt(va_arg(args, int));
+      break;
+    case BuiltinType::UInt:
+      VRef.setUInt(va_arg(args, unsigned));
+      break;
+    case BuiltinType::Long:
+      VRef.setLong(va_arg(args, long));
+      break;
+    case BuiltinType::ULong:
+      VRef.setULong(va_arg(args, unsigned long));
+      break;
+    case BuiltinType::LongLong:
+      VRef.setLongLong(va_arg(args, long long));
+      break;
+    case BuiltinType::ULongLong:
+      VRef.setULongLong(va_arg(args, unsigned long long));
+      break;
+      // Types shorter than double are resolved as double, else va_arg has UB.
+    case BuiltinType::Float:
+      VRef.setFloat(va_arg(args, double));
+      break;
+    case BuiltinType::Double:
+      VRef.setDouble(va_arg(args, double));
+      break;
+    case BuiltinType::LongDouble:
+      VRef.setLongDouble(va_arg(args, long double));
+      break;
+      // See REPL_BUILTIN_TYPES.
+    }
+  }
+  va_end(args);
+}
+
 llvm::Error Interpreter::CreateExecutor() {
   if (IncrExecutor)
     return llvm::make_error<llvm::StringError>("Operation failed. "
@@ -431,6 +511,12 @@ llvm::Error Interpreter::CreateExecutor() {
   if (!Err)
     IncrExecutor = std::move(Executor);
 
+  auto &J = IncrExecutor->GetExecutionEngine();
+  Err = J.getMainJITDylib().define(
+                                   llvm::orc::absoluteSymbols({{J.mangleAndIntern("__clang_Interpreter_SetValueNoAlloc"),
+                                                    {llvm::orc::ExecutorAddr::fromPtr(&__clang_Interpreter_SetValueNoAlloc),
+                                                     llvm::JITSymbolFlags::Exported}}}));
+
   return Err;
 }
 
@@ -860,83 +946,6 @@ __clang_Interpreter_SetValueWithAlloc(void *This, void *OutVal,
   return VRef.getPtr();
 }
 
-extern "C" void REPL_EXTERNAL_VISIBILITY __clang_Interpreter_SetValueNoAlloc(
-    void *This, void *OutVal, void *OpaqueType, ...) {
-  Value &VRef = *(Value *)OutVal;
-  Interpreter *I = static_cast<Interpreter *>(This);
-  VRef = Value(I, OpaqueType);
-  if (VRef.isVoid())
-    return;
-
-  va_list args;
-  va_start(args, /*last named param*/ OpaqueType);
-
-  QualType QT = VRef.getType();
-  if (VRef.getKind() == Value::K_PtrOrObj) {
-    VRef.setPtr(va_arg(args, void *));
-  } else {
-    if (const auto *ET = QT->getAs<EnumType>())
-      QT = ET->getDecl()->getIntegerType();
-    switch (QT->castAs<BuiltinType>()->getKind()) {
-    default:
-      llvm_unreachable("unknown type kind!");
-      break;
-      // Types shorter than int are resolved as int, else va_arg has UB.
-    case BuiltinType::Bool:
-      VRef.setBool(va_arg(args, int));
-      break;
-    case BuiltinType::Char_S:
-      VRef.setChar_S(va_arg(args, int));
-      break;
-    case BuiltinType::SChar:
-      VRef.setSChar(va_arg(args, int));
-      break;
-    case BuiltinType::Char_U:
-      VRef.setChar_U(va_arg(args, unsigned));
-      break;
-    case BuiltinType::UChar:
-      VRef.setUChar(va_arg(args, unsigned));
-      break;
-    case BuiltinType::Short:
-      VRef.setShort(va_arg(args, int));
-      break;
-    case BuiltinType::UShort:
-      VRef.setUShort(va_arg(args, unsigned));
-      break;
-    case BuiltinType::Int:
-      VRef.setInt(va_arg(args, int));
-      break;
-    case BuiltinType::UInt:
-      VRef.setUInt(va_arg(args, unsigned));
-      break;
-    case BuiltinType::Long:
-      VRef.setLong(va_arg(args, long));
-      break;
-    case BuiltinType::ULong:
-      VRef.setULong(va_arg(args, unsigned long));
-      break;
-    case BuiltinType::LongLong:
-      VRef.setLongLong(va_arg(args, long long));
-      break;
-    case BuiltinType::ULongLong:
-      VRef.setULongLong(va_arg(args, unsigned long long));
-      break;
-      // Types shorter than double are resolved as double, else va_arg has UB.
-    case BuiltinType::Float:
-      VRef.setFloat(va_arg(args, double));
-      break;
-    case BuiltinType::Double:
-      VRef.setDouble(va_arg(args, double));
-      break;
-    case BuiltinType::LongDouble:
-      VRef.setLongDouble(va_arg(args, long double));
-      break;
-      // See REPL_BUILTIN_TYPES.
-    }
-  }
-  va_end(args);
-}
-
 // A trampoline to work around the fact that operator placement new cannot
 // really be forward declared due to libc++ and libstdc++ declaration mismatch.
 // FIXME: __clang_Interpreter_NewTag is ODR violation because we get the same

Courtesy of @lhames.

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.

7 participants