[clang-repl] Adapt to the recent dylib-related changes in ORC.

[v.g.vassilev]

Summary

ORC splits into separate dylibs symbols coming from the process and symbols materialized in the Jit. This patch adapts intent of the existing interface and adds a regression test to make sure both Jit'd and compiled symbols can be found.

[lhames]

Otherwise LGTM!

clang/lib/Interpreter/IncrementalExecutor.cpp

96–100

I think this should be equivalent to

auto SO =
  makeJITDylibSearchFlags(&Jit->getMainJITDylib(),
                          Jit->getPlatformJITDylib().get(),
                          Jit->getProcessSymbolsJITDylib.get());

but this is purely cosmetic -- either way works.

[v.g.vassilev]

Address review comment.

[v.g.vassilev]

Address style comment.

[mgorny]

This change is causing test regressions on 32-bit architectures (I've reproduced with -m32 on amd64 and in 32-bit nspawn container on arm64):

FAIL: Clang-Unit :: Interpreter/./ClangReplInterpreterTests/1/13 (1 of 1584)
******************** TEST 'Clang-Unit :: Interpreter/./ClangReplInterpreterTests/1/13' FAILED ********************
Script(shard):
--
GTEST_OUTPUT=json:/home/mgorny/llvm-project/build/tools/clang/unittests/Interpreter/./ClangReplInterpreterTests-Clang-Unit-1816215-1-13.json GTEST_SHUFFLE=0 GTEST_TOTAL_SHARDS=13 GTEST_SHARD_INDEX=1 /home/mgorny/llvm-project/build/tools/clang/unittests/Interpreter/./ClangReplInterpreterTests
--

Script:
--
/home/mgorny/llvm-project/build/tools/clang/unittests/Interpreter/./ClangReplInterpreterTests --gtest_filter=IncrementalProcessing.FindMangledNameSymbol
--
/home/mgorny/llvm-project/clang/unittests/Interpreter/InterpreterTest.cpp:246: Failure
Expected equality of these values:
  (unsigned long long)&printf
    Which is: 18446744073567794308
  Addr->getValue()
    Which is: 4153209988

/home/mgorny/llvm-project/clang/unittests/Interpreter/InterpreterTest.cpp:246
Expected equality of these values:
  (unsigned long long)&printf
    Which is: 18446744073567794308
  Addr->getValue()
    Which is: 4153209988


********************

[mgorny]

Changing the type from unsigned long long to uintptr_t fix the test for me.

clang/unittests/Interpreter/InterpreterTest.cpp

246

[v.g.vassilev]

In D159115#4638323, @mgorny wrote:

Changing the type from unsigned long long to uintptr_t fix the test for me.

Ah! Nice catch! Can you commit the fix?

[thesamesam]

In D159115#4638325, @v.g.vassilev wrote:

In D159115#4638323, @mgorny wrote:

Changing the type from unsigned long long to uintptr_t fix the test for me.

Ah! Nice catch! Can you commit the fix?

Done, thanks to @mgorny for the testing and thank you @v.g.vassilev for the quick ack!

[mgorny]

Thank you both!

[tuliom]

This new test does not work on ppc64le.
The returned addresses are different:

Expected equality of these values:
  (uintptr_t)&printf
    Which is: 140735424286016
  Addr->getValue()
    Which is: 140735424167152

[mgorny]

In D159115#4640154, @tuliom wrote:

This new test does not work on ppc64le.

I'm sorry, by "new" you mean the version with uintptr_t or the original as well?

[mgorny]

In D159115#4640154, @tuliom wrote:

This new test does not work on ppc64le.

I've tested right now (as of 3398744a6106c83993611bd3c5e79ec6b94417dc) and all clang tests passed for me on Gentoo/ppc64le.

[tuliom]

In D159115#4640388, @mgorny wrote:

I'm sorry, by "new" you mean the version with uintptr_t or the original as well?

Both with uintptr_t as well as unsigned long long.

In D159115#4641721, @mgorny wrote:

I've tested right now (as of 3398744a6106c83993611bd3c5e79ec6b94417dc) and all clang tests passed for me on Gentoo/ppc64le.

Did Gentoo switch the default long double to IEEE 128-bit floating point?
If not, that could explain why this new code is broken on ppc64le.

[tuliom]

Let me elaborate my previous comment...
I'm using Fedora, which has switched the default long double on ppc64le to IEEE 128-bit floating point.
glibc provides 2 printf implementations on ppc64le (different symbols). The headers control which implementation is used.

I wonder if the Jit is taking the right symbol.

[mgorny]

Did Gentoo switch the default long double to IEEE 128-bit floating point?

LDBL_MANT_DIG is 106, so I guess not yet on this system.

glibc provides 2 printf implementations on ppc64le (different symbols). The headers control which implementation is used.

Is that actually a regression, or merely the test wasn't checking it before? My educated guess would be that the code in the test redefines printf without the header mangling, so it finds and compares the "non-IEEE" symbol against the "IEEE" symbol that's exposed to the test case via the headers.

[tuliom]

In D159115#4641834, @mgorny wrote:

Is that actually a regression, or merely the test wasn't checking it before?

I can't tell because the test didn't exist before.

My educated guess would be that the code in the test redefines printf without the header mangling, so it finds and compares the "non-IEEE" symbol against the "IEEE" symbol that's exposed to the test case via the headers.

I agree with you.

[v.g.vassilev]

In D159115#4642111, @tuliom wrote:

In D159115#4641834, @mgorny wrote:

Is that actually a regression, or merely the test wasn't checking it before?

I can't tell because the test didn't exist before.

My educated guess would be that the code in the test redefines printf without the header mangling, so it finds and compares the "non-IEEE" symbol against the "IEEE" symbol that's exposed to the test case via the headers.

I agree with you.

Would that be a problem with the RuntimeDyld part of the JIT on ppc64le? Maybe @lkail could help us sort it out?

cc: @lhames

clang/lib/Interpreter/IncrementalExecutor.cpp

 
 llvm::Error IncrementalExecutor::runCtors() const {
   return Jit->initialize(Jit->getMainJITDylib());
 }
 
 llvm::Expected<llvm::orc::ExecutorAddr>
 IncrementalExecutor::getSymbolAddress(llvm::StringRef Name,
                                       SymbolNameKind NameKind) const {
-  auto Sym = (NameKind == LinkerName) ? Jit->lookupLinkerMangled(Name)
-                                      : Jit->lookup(Name);
-
-  if (!Sym)
-    return Sym.takeError();
-  return Sym;
+  using namespace llvm::orc;
+  auto SO = makeJITDylibSearchOrder({&Jit->getMainJITDylib(),
+                                     Jit->getPlatformJITDylib().get(),
+                                     Jit->getProcessSymbolsJITDylib().get()});
+
+  ExecutionSession &ES = Jit->getExecutionSession();
+
+  auto SymOrErr =
+      ES.lookup(SO, (NameKind == LinkerName) ? ES.intern(Name)
+                                             : Jit->mangleAndIntern(Name));
+  if (auto Err = SymOrErr.takeError())
+    return std::move(Err);
+  return SymOrErr->getAddress();
 }
 
 } // end namespace clang

clang/unittests/Interpreter/InterpreterTest.cpp

#endif
   // Lower the PTU
   if (llvm::Error Err = Interp->Execute(PTU)) {
     // We cannot execute on the platform.
     consumeError(std::move(Err));
     return;
   }
 
   std::string MangledName = MangleName(FD);
-  auto Addr = cantFail(Interp->getSymbolAddress(MangledName));
-  EXPECT_NE(0U, Addr.getValue());
+  auto Addr = Interp->getSymbolAddress(MangledName);
+  EXPECT_FALSE(!Addr);
+  EXPECT_NE(0U, Addr->getValue());
   GlobalDecl GD(FD);
-  EXPECT_EQ(Addr, cantFail(Interp->getSymbolAddress(GD)));
+  EXPECT_EQ(*Addr, cantFail(Interp->getSymbolAddress(GD)));
+  cantFail(
+      Interp->ParseAndExecute("extern \"C\" int printf(const char*,...);"));
+  Addr = Interp->getSymbolAddress("printf");
+  EXPECT_FALSE(!Addr);
+  EXPECT_EQ((unsigned long long)&printf, Addr->getValue());
 }
 
 static void *AllocateObject(TypeDecl *TD, Interpreter &Interp) {
   std::string Name = TD->getQualifiedNameAsString();
   const clang::Type *RDTy = TD->getTypeForDecl();
   clang::ASTContext &C = Interp.getCompilerInstance()->getASTContext();
   size_t Size = C.getTypeSize(RDTy);
   void *Addr = malloc(Size);