[clang-repl] Allow Interpreter::getSymbolAddress to take a mangled name.

[rsmith]

clang/include/clang/Interpreter/Interpreter.h

84

I find this flag name to be very confusing, given what "mangling" usually means within Clang. I think IsMangled = false means "this is a symbol name of the same kind that Clang would emit in IR", and IsMangled = true means "this is a symbol name of the same kind that would appear in an object file's symbol table" -- and the former is a mangled name in the sense in which the term is used in Clang.

Perhaps enum { IRName, LinkerName } instead of a bool flag would be clearer? But I'm not sure whether this flag is motivated given that your test case below wants / is testing the IsMangled = false behavior.

I wonder if a better interface here generally would be:

llvm::Expected<llvm::JITTargetAddress> getSymbolAddress(GlobalDecl GD) const;

... which could internally form the proper symbol name for the given GlobalDecl (or perhaps reuse CodeGen's decl -> mangling map rather than redoing the name mangling step).

clang/unittests/Interpreter/InterpreterTest.cpp

131–138

This function produces names that are not mangled in the sense that getSymbolAddress's IsMangled = true means. I would expect this test to fail on MacOS for that reason.

[v.g.vassilev]

Address comments

[rsmith]

clang/include/clang/Interpreter/Interpreter.h

72

Add a space before \returns please (here and below).

clang/unittests/Interpreter/IncrementalProcessingTest.cpp

20

Given that this file is unchanged, do we need the new includes?

clang/unittests/Interpreter/InterpreterTest.cpp

166

Use EXPECT_EQ and EXPECT_NE as appropriate.

214

EXPECT_TRUE on an integer seems surprising.

[v.g.vassilev]

Address comments and fix the windows bot failure.

[v.g.vassilev]

clang/unittests/Interpreter/IncrementalProcessingTest.cpp

20

A leftover from some other experiments. Thanks!

[aaron.ballman]

There is no information in this patch as to why the changes are needed or useful; please use a more descriptive summary next time.

clang/unittests/Interpreter/InterpreterTest.cpp

205–207

FWIW, Intel's internal testing is hitting crashes with this test. With -fno-delayed-template-parsing enabled, we get a failed assertion:

Assertion failed: OffsetBytes <= AllocationSize && "Offset out of bounds!", file d:\iusers\ayrivera\dev-xmain-web\llvm\l
lvm\lib\executionengine\runtimedyld\RuntimeDyldImpl.h, line 90

and when we enable the delayed template parsing, we get a different assertion:

Assertion failed: DC->getLexicalParent() == CurContext && "The next DeclContext should be lexically contained in the current one.", file D:/iusers/ayrivera/dev-xmain-web/llvm/clang/lib/Sema/SemaDecl.cpp, line 1305

I think this FIXME either needs to be addressed or the patch reverted until this is addressed.

237–240

This test is broken for some of our internal build bots at Intel. I think something suspicious is going on here, but I'm not certain of the intent behind the test, so I'm not certain the best way to fix it. The behavior of the test is that on an x86 Windows machine, sometimes this particular test fails:

[ RUN ] IncrementalProcessing.InstantiateTemplate^M
unknown file: error: SEH exception with code 0x3221225477 thrown in the test body.^M
[ FAILED ] IncrementalProcessing.InstantiateTemplate (35 ms)^M

but it's not a consistent failure (seems to happen about one out of every three runs).

callme is a templated member function of B but here we're trying to call it like it's a free function. That's... not good. We could either make callme a static member function so that it can be called in this manner, or we could try to come up with a magic incantation to call it as a PMF.

Can you investigate, @v.g.vassilev? If it is going to take considerable time to resolve, it might be worth reverting temporarily. Thanks!

[erichkeane]

clang/unittests/Interpreter/InterpreterTest.cpp

237–240

To be perhaps succinct, the problem is calling Pointer-to-member-function as a free-function. The cast on line 239 is completely invalid, and can't be done in C++ code itself without UB.

We believe this shows up on x86 in particular since there is ALSO a calling-convention mismatch here, but this cast to TemplateSpecFn seems completely invalid to me.

[aaron.ballman]

clang/unittests/Interpreter/InterpreterTest.cpp

205–207

Ignore this -- this comment was saved in my Phab drafts from an earlier investigation and I managed to not delete it when sending my other comment (below). Oops!

[v.g.vassilev]

clang/unittests/Interpreter/InterpreterTest.cpp

237–240

Thanks for the ping, @aaron.ballman. Does making the function static fixes it? If it does, please go ahead and commit the fix otherwise I can do it in 12 hours.

If it does not fix it, could you share the bot logs or at least the cmake configure line and the platform?

[eandrews]

clang/unittests/Interpreter/InterpreterTest.cpp

237–240

Committed here - https://reviews.llvm.org/rG3ad0c6b75ea5

[v.g.vassilev]

clang/unittests/Interpreter/InterpreterTest.cpp

237–240

Thanks!

[v.g.vassilev]

That change landed in 4fb0805c6525b13e50067b9ddfe8677a0b7b2d7c

clang/include/clang/CodeGen/ModuleBuilder.h

public:
   CodeGen::CGDebugInfo *getCGDebugInfo();
 
   /// Given a mangled name, return a declaration which mangles that way
   /// which has been added to this code generator via a Handle method.
   ///
   /// This may return null if there was no matching declaration.
   const Decl *GetDeclForMangledName(llvm::StringRef MangledName);
 
+  /// Given a global declaration, return a mangled name for this declaration
+  /// which has been added to this code generator via a Handle method.
+  llvm::StringRef GetMangledName(GlobalDecl GD);
+
   /// Return the LLVM address of the given global entity.
   ///
   /// \param isForDefinition If true, the caller intends to define the
   ///   entity; the object returned will be an llvm::GlobalValue of
   ///   some sort.  If false, the caller just intends to use the entity;
   ///   the object returned may be any sort of constant value, and the
   ///   code generator will schedule the entity for emission if a
   ///   definition has been registered with this code generator.

clang/include/clang/Interpreter/Interpreter.h

 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_INTERPRETER_INTERPRETER_H
 #define LLVM_CLANG_INTERPRETER_INTERPRETER_H
 
 #include "clang/Interpreter/PartialTranslationUnit.h"
 
+#include "clang/AST/GlobalDecl.h"
+
 #include "llvm/ExecutionEngine/JITSymbol.h"
 #include "llvm/Support/Error.h"
 
 #include <memory>
 #include <vector>
 
 namespace llvm {
 namespace orc {
public:
   llvm::Error ParseAndExecute(llvm::StringRef Code) {
     auto PTU = Parse(Code);
     if (!PTU)
       return PTU.takeError();
     if (PTU->TheModule)
       return Execute(*PTU);
     return llvm::Error::success();
   }
+
+  ///\returns the \c JITTargetAddress of a \c GlobalDecl. This interface uses
+  /// the CodeGenModule's internal mangling cache to avoid recomputing the
+  /// mangled name.
+  llvm::Expected<llvm::JITTargetAddress> getSymbolAddress(GlobalDecl GD) const;
+
+  ///\returns the \c JITTargetAddress of a given name as written in the IR.
+  llvm::Expected<llvm::JITTargetAddress>
+  getSymbolAddress(llvm::StringRef IRName) const;
+
+  ///\returns the \c JITTargetAddress of a given name as written in the object
+  /// file.
   llvm::Expected<llvm::JITTargetAddress>
-  getSymbolAddress(llvm::StringRef UnmangledName) const;
+  getSymbolAddressFromLinkerName(llvm::StringRef LinkerName) const;
 };
 } // namespace clang
 
 #endif // LLVM_CLANG_INTERPRETER_INTERPRETER_H

clang/lib/CodeGen/ModuleBuilder.cpp

const Decl *GetDeclForMangledName(StringRef MangledName) {
           return FD;
       } else if (auto TD = dyn_cast<TagDecl>(D)) {
         if (auto Def = TD->getDefinition())
           return Def;
       }
       return D;
     }
 
+    llvm::StringRef GetMangledName(GlobalDecl GD) {
+      return Builder->getMangledName(GD);
+    }
+
     llvm::Constant *GetAddrOfGlobal(GlobalDecl global, bool isForDefinition) {
       return Builder->GetAddrOfGlobal(global, ForDefinition_t(isForDefinition));
     }
 
     llvm::Module *StartModule(llvm::StringRef ModuleName,
                               llvm::LLVMContext &C) {
       assert(!M && "Replacing existing Module?");
       M.reset(new llvm::Module(ExpandModuleName(ModuleName, CodeGenOpts), C));

 CGDebugInfo *CodeGenerator::getCGDebugInfo() {
   return static_cast<CodeGeneratorImpl*>(this)->getCGDebugInfo();
 }
 
 const Decl *CodeGenerator::GetDeclForMangledName(llvm::StringRef name) {
   return static_cast<CodeGeneratorImpl*>(this)->GetDeclForMangledName(name);
 }
 
+llvm::StringRef CodeGenerator::GetMangledName(GlobalDecl GD) {
+  return static_cast<CodeGeneratorImpl *>(this)->GetMangledName(GD);
+}
+
 llvm::Constant *CodeGenerator::GetAddrOfGlobal(GlobalDecl global,
                                                bool isForDefinition) {
   return static_cast<CodeGeneratorImpl*>(this)
            ->GetAddrOfGlobal(global, isForDefinition);
 }
 
 llvm::Module *CodeGenerator::StartModule(llvm::StringRef ModuleName,
                                          llvm::LLVMContext &C) {

clang/lib/Interpreter/IncrementalExecutor.h

 
 namespace clang {
 class IncrementalExecutor {
   using CtorDtorIterator = llvm::orc::CtorDtorIterator;
   std::unique_ptr<llvm::orc::LLJIT> Jit;
   llvm::orc::ThreadSafeContext &TSCtx;
 
 public:
+  enum SymbolNameKind { IRName, LinkerName };
+
   IncrementalExecutor(llvm::orc::ThreadSafeContext &TSC, llvm::Error &Err,
                       const llvm::Triple &Triple);
   ~IncrementalExecutor();
 
   llvm::Error addModule(std::unique_ptr<llvm::Module> M);
   llvm::Error runCtors() const;
   llvm::Expected<llvm::JITTargetAddress>
-  getSymbolAddress(llvm::StringRef UnmangledName) const;
+  getSymbolAddress(llvm::StringRef Name, SymbolNameKind NameKind) const;
 };
 
 } // end namespace clang
 
 #endif // LLVM_CLANG_LIB_INTERPRETER_INCREMENTALEXECUTOR_H

clang/lib/Interpreter/IncrementalExecutor.cpp

llvm::Error IncrementalExecutor::addModule(std::unique_ptr<llvm::Module> M) {
   return Jit->addIRModule(llvm::orc::ThreadSafeModule(std::move(M), TSCtx));
 }
 
 llvm::Error IncrementalExecutor::runCtors() const {
   return Jit->initialize(Jit->getMainJITDylib());
 }
 
 llvm::Expected<llvm::JITTargetAddress>
-IncrementalExecutor::getSymbolAddress(llvm::StringRef UnmangledName) const {
-  auto Sym = Jit->lookup(UnmangledName);
+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->getAddress();
 }
 
 } // end namespace clang

clang/lib/Interpreter/IncrementalParser.h

 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_LIB_INTERPRETER_INCREMENTALPARSER_H
 #define LLVM_CLANG_LIB_INTERPRETER_INCREMENTALPARSER_H
 
 #include "clang/Interpreter/PartialTranslationUnit.h"
 
+#include "clang/AST/GlobalDecl.h"
+
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
 
 #include <list>
 #include <memory>
 namespace llvm {
 class LLVMContext;
public:
 
   const CompilerInstance *getCI() const { return CI.get(); }
 
   /// Parses incremental input by creating an in-memory file.
   ///\returns a \c PartialTranslationUnit which holds information about the
   /// \c TranslationUnitDecl and \c llvm::Module corresponding to the input.
   llvm::Expected<PartialTranslationUnit &> Parse(llvm::StringRef Input);
 
+  /// Uses the CodeGenModule mangled name cache and avoids recomputing.
+  ///\returns the mangled name of a \c GD.
+  llvm::StringRef GetMangledName(GlobalDecl GD) const;
+
 private:
   llvm::Expected<PartialTranslationUnit &> ParseOrWrapTopLevelDecl();
 };
 } // end namespace clang
 
 #endif // LLVM_CLANG_LIB_INTERPRETER_INCREMENTALPARSER_H

clang/lib/Interpreter/IncrementalParser.cpp

if (CodeGenerator *CG = getCodeGen(Act.get())) {
     CG->StartModule("incr_module_" + std::to_string(PTUs.size()),
                     M->getContext());
 
     PTU->TheModule = std::move(M);
   }
 
   return PTU;
 }
+
+llvm::StringRef IncrementalParser::GetMangledName(GlobalDecl GD) const {
+  CodeGenerator *CG = getCodeGen(Act.get());
+  assert(CG);
+  return CG->GetMangledName(GD);
+}
+
 } // end namespace clang

clang/lib/Interpreter/Interpreter.cpp

llvm::Error Interpreter::Execute(PartialTranslationUnit &T) {
 
   if (auto Err = IncrExecutor->runCtors())
     return Err;
 
   return llvm::Error::success();
 }
 
 llvm::Expected<llvm::JITTargetAddress>
-Interpreter::getSymbolAddress(llvm::StringRef UnmangledName) const {
+Interpreter::getSymbolAddress(GlobalDecl GD) const {
+  if (!IncrExecutor)
+    return llvm::make_error<llvm::StringError>("Operation failed. "
+                                               "No execution engine",
+                                               std::error_code());
+  llvm::StringRef MangledName = IncrParser->GetMangledName(GD);
+  return getSymbolAddress(MangledName);
+}
+
+llvm::Expected<llvm::JITTargetAddress>
+Interpreter::getSymbolAddress(llvm::StringRef IRName) const {
+  if (!IncrExecutor)
+    return llvm::make_error<llvm::StringError>("Operation failed. "
+                                               "No execution engine",
+                                               std::error_code());
+
+  return IncrExecutor->getSymbolAddress(IRName, IncrementalExecutor::IRName);
+}
+
+llvm::Expected<llvm::JITTargetAddress>
+Interpreter::getSymbolAddressFromLinkerName(llvm::StringRef Name) const {
   if (!IncrExecutor)
     return llvm::make_error<llvm::StringError>("Operation failed. "
                                                "No execution engine",
                                                std::error_code());
 
-  return IncrExecutor->getSymbolAddress(UnmangledName);
+  return IncrExecutor->getSymbolAddress(Name, IncrementalExecutor::LinkerName);
 }

clang/unittests/Interpreter/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
+  ${LLVM_TARGETS_TO_BUILD}
   Core
   )
 
 add_clang_unittest(ClangReplInterpreterTests
   IncrementalProcessingTest.cpp
   InterpreterTest.cpp
   )
 target_link_libraries(ClangReplInterpreterTests PUBLIC
   clangAST
   clangBasic
   clangInterpreter
   clangFrontend
+  clangSema
   )
 
 # Exceptions on Windows are not yet supported.
 if(NOT WIN32)
   add_subdirectory(ExceptionTests)
 endif()

clang/unittests/Interpreter/IncrementalProcessingTest.cpp

 #include "clang/AST/ASTContext.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/Basic/TargetInfo.h"
 #include "clang/CodeGen/ModuleBuilder.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Interpreter/Interpreter.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Parse/Parser.h"
+#include "clang/Sema/Lookup.h"
 #include "clang/Sema/Sema.h"
+#include "clang/Sema/TemplateDeduction.h"
 #include "llvm/ADT/Triple.h"
 #include "llvm/IR/LLVMContext.h"
 #include "llvm/IR/Module.h"
 #include "llvm/Support/Host.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "gtest/gtest.h"
 
 #include <memory>

clang/unittests/Interpreter/InterpreterTest.cpp

 //===- unittests/Interpreter/InterpreterTest.cpp --- Interpreter tests ----===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // Unit tests for Clang's Interpreter library.
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/Interpreter/Interpreter.h"
 
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclGroup.h"
+#include "clang/AST/Mangle.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/TextDiagnosticPrinter.h"
+#include "clang/Sema/Lookup.h"
+#include "clang/Sema/Sema.h"
+
+#include "llvm/Support/TargetSelect.h"
 
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 using namespace clang;
 
 namespace {
 using Args = std::vector<const char *>;
TEST(InterpreterTest, DeclsAndStatements) {
   EXPECT_FALSE(!!R2);
   using ::testing::HasSubstr;
   EXPECT_THAT(DiagnosticsOS.str(),
               HasSubstr("error: unknown type name 'var1'"));
   auto Err = R2.takeError();
   EXPECT_EQ("Parsing failed.", llvm::toString(std::move(Err)));
 }
 
+static std::string MangleName(NamedDecl *ND) {
+  ASTContext &C = ND->getASTContext();
+  std::unique_ptr<MangleContext> MangleC(C.createMangleContext());
+  std::string mangledName;
+  llvm::raw_string_ostream RawStr(mangledName);
+  MangleC->mangleName(ND, RawStr);
+  return RawStr.str();
+}
+
+struct LLVMInitRAII {
+  LLVMInitRAII() {
+    llvm::InitializeNativeTarget();
+    llvm::InitializeNativeTargetAsmPrinter();
+  }
+  ~LLVMInitRAII() { llvm::llvm_shutdown(); }
+} LLVMInit;
+
+TEST(IncrementalProcessing, FindMangledNameSymbol) {
+
+  std::unique_ptr<Interpreter> Interp = createInterpreter();
+
+  auto &PTU(cantFail(Interp->Parse("int f(const char*) {return 0;}")));
+  EXPECT_EQ(1U, DeclsSize(PTU.TUPart));
+  auto R1DeclRange = PTU.TUPart->decls();
+
+  NamedDecl *FD = cast<FunctionDecl>(*R1DeclRange.begin());
+  // 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_TRUE(Addr != 0);
+  GlobalDecl GD(FD);
+  EXPECT_TRUE(Addr == cantFail(Interp->getSymbolAddress(GD)));
+}
+
+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);
+
+  // Tell the interpreter to call the default ctor with this memory. Synthesize:
+  // new (loc) ClassName;
+  static unsigned Counter = 0;
+  std::stringstream SS;
+  SS << "auto _v" << Counter++ << " = "
+     << "new ((void*)" << Addr << ")" << Name << "();";
+
+  auto R = Interp.ParseAndExecute(SS.str());
+  if (!R)
+    return nullptr;
+
+  return Addr;
+}
+
+static NamedDecl *LookupSingleName(Interpreter &Interp, const char *Name) {
+  Sema &SemaRef = Interp.getCompilerInstance()->getSema();
+  ASTContext &C = SemaRef.getASTContext();
+  DeclarationName DeclName = &C.Idents.get(Name);
+  LookupResult R(SemaRef, DeclName, SourceLocation(), Sema::LookupOrdinaryName);
+  SemaRef.LookupName(R, SemaRef.TUScope);
+  assert(!R.empty());
+  return R.getFoundDecl();
+}
+
+TEST(IncrementalProcessing, InstantiateTemplate) {
+  std::unique_ptr<Interpreter> Interp = createInterpreter();
+
+  llvm::cantFail(Interp->Parse("void* operator new(__SIZE_TYPE__, void* __p);"
+                               "extern \"C\" int printf(const char*,...);"
+                               "class A {};"
+                               "struct B {"
+                               "  template<typename T>"
+                               "  int callme(T) { return 42; }"
+                               "};"));
+  auto &PTU = llvm::cantFail(Interp->Parse("auto _t = &B::callme<A*>;"));
+  auto PTUDeclRange = PTU.TUPart->decls();
+  EXPECT_TRUE(std::distance(PTUDeclRange.begin(), PTUDeclRange.end()));
+
+  // Lower the PTU
+  if (llvm::Error Err = Interp->Execute(PTU)) {
+    // We cannot execute on the platform.
+    consumeError(std::move(Err));
+    return;
+  }
+
+  TypeDecl *TD = cast<TypeDecl>(LookupSingleName(*Interp, "A"));
+  void *NewA = AllocateObject(TD, *Interp);
+
+  // Find back the template specialization
+  VarDecl *VD = static_cast<VarDecl *>(*PTUDeclRange.begin());
+  UnaryOperator *UO = llvm::cast<UnaryOperator>(VD->getInit());
+  NamedDecl *TmpltSpec = llvm::cast<DeclRefExpr>(UO->getSubExpr())->getDecl();
+
+  std::string MangledName = MangleName(TmpltSpec);
+  typedef int (*TemplateSpecFn)(void *);
+  auto fn = (TemplateSpecFn)cantFail(Interp->getSymbolAddress(MangledName));
+  EXPECT_TRUE(42 == fn(NewA));
+}
+
 } // end anonymous namespace