llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang-codegen
@llvm/pr-subscribers-clang
Author: Emery Conrad (conrade-ctc)
<details>
<summary>Changes</summary>
When an input fails, Sema still performs the implicit instantiations that the
input asked for, and marks them done. The consumers then drop them:
`InProcessPrintingASTConsumer::HandleTopLevelDecl`, and CodeGen's
`HandleTopLevelDecl` and `HandleCXXStaticMemberVarInstantiation`, return early
after any error. Sema does not instantiate them again, so a later input gets
only a declaration:
```
template <class T> T twice(T x) { return x + x; }
extern "C" double fail(double *p) { return twice(*p) + no_such_name; } // error
extern "C" double ok(double x) { return twice(x); }
// JIT session error: Symbols not found: [ _Z5twiceIdET_S0_ ]
```
In incremental mode, this PR lets a valid implicit instantiation reach CodeGen
after an error. CodeGen only records it as a deferred decl, so no IR is emitted
until a later input uses it. The failed input's own declarations still stay
out. I think CodeGen is the right place for this, because a normal compile
drops the module after an error.
Tests: `FailedInputInstantiationTest.*` (function templates and static data
members fail without the fix; the other cases are regression tests), undo tests
around a failed input, and a lit test.
Not in this PR: a declaration emitted before the error in the same input still
reaches the module. A later redefinition then reports `definition with same
mangled name`. See #<!-- -->226252.
🤖 Done with the help of [Claude Code](https://claude.com/claude-code) (Opus
5.5, human in the loop)
---
Full diff: https://github.com/llvm/llvm-project/pull/226253.diff
4 Files Affected:
- (modified) clang/lib/CodeGen/ModuleBuilder.cpp (+18-2)
- (modified) clang/lib/Interpreter/IncrementalAction.cpp (+12-2)
- (added) clang/test/Interpreter/failed-input-keeps-instantiations.cpp (+24)
- (modified) clang/unittests/Interpreter/InterpreterTest.cpp (+137)
``````````diff
diff --git a/clang/lib/CodeGen/ModuleBuilder.cpp
b/clang/lib/CodeGen/ModuleBuilder.cpp
index 0b00362487d2a..279a6b3ad867f 100644
--- a/clang/lib/CodeGen/ModuleBuilder.cpp
+++ b/clang/lib/CodeGen/ModuleBuilder.cpp
@@ -178,8 +178,22 @@ namespace {
Builder->AppendLinkerOptions(Opt);
}
+ /// In incremental mode an error drops the failed input, not the module.
+ /// Sema instantiates a specialization only once, so a valid implicit
+ /// instantiation must reach CodeGen even after an error. If not, a later
+ /// input that uses it gets only a declaration.
+ bool isKeptAfterError(const Decl *D) const {
+ if (!Ctx->getLangOpts().IncrementalExtensions || D->isInvalidDecl())
+ return false;
+ if (const auto *FD = dyn_cast<FunctionDecl>(D))
+ return FD->getTemplateSpecializationKind() ==
TSK_ImplicitInstantiation;
+ if (const auto *VD = dyn_cast<VarDecl>(D))
+ return VD->getTemplateSpecializationKind() ==
TSK_ImplicitInstantiation;
+ return false;
+ }
+
void HandleCXXStaticMemberVarInstantiation(VarDecl *VD) override {
- if (Diags.hasErrorOccurred())
+ if (Diags.hasErrorOccurred() && !isKeptAfterError(VD))
return;
Builder->HandleCXXStaticMemberVarInstantiation(VD);
@@ -191,7 +205,9 @@ namespace {
return true; // We can't CodeGen more but pass to other consumers.
// FIXME: Why not return false and abort parsing?
- if (Diags.hasUnrecoverableErrorOccurred())
+ if (Diags.hasUnrecoverableErrorOccurred() &&
+ !llvm::all_of(DG,
+ [this](const Decl *D) { return isKeptAfterError(D); }))
return true;
HandlingTopLevelDeclRAII HandlingDecl(*this);
diff --git a/clang/lib/Interpreter/IncrementalAction.cpp
b/clang/lib/Interpreter/IncrementalAction.cpp
index 85f00c36dd5aa..46d1c9862f1ce 100644
--- a/clang/lib/Interpreter/IncrementalAction.cpp
+++ b/clang/lib/Interpreter/IncrementalAction.cpp
@@ -132,14 +132,24 @@
InProcessPrintingASTConsumer::InProcessPrintingASTConsumer(
std::unique_ptr<ASTConsumer> C, Interpreter &I)
: MultiplexConsumer(std::move(C)), Interp(I) {}
+static bool isImplicitInstantiation(const Decl *D) {
+ const auto *FD = dyn_cast<FunctionDecl>(D);
+ return FD && FD->getTemplateSpecializationKind() ==
TSK_ImplicitInstantiation;
+}
+
bool InProcessPrintingASTConsumer::HandleTopLevelDecl(DeclGroupRef DGR) {
if (DGR.isNull())
return true;
CompilerInstance *CI = Interp.getCompilerInstance();
DiagnosticsEngine &Diags = CI->getDiagnostics();
- if (Diags.hasErrorOccurred())
- return true;
+ if (Diags.hasErrorOccurred()) {
+ // Keep the failed input's own declarations out of CodeGen. Sema does not
+ // repeat the implicit instantiations it made, so pass those on now.
+ if (!llvm::all_of(DGR, isImplicitInstantiation))
+ return true;
+ return MultiplexConsumer::HandleTopLevelDecl(DGR);
+ }
for (Decl *D : DGR)
if (auto *TLSD = llvm::dyn_cast<TopLevelStmtDecl>(D))
diff --git a/clang/test/Interpreter/failed-input-keeps-instantiations.cpp
b/clang/test/Interpreter/failed-input-keeps-instantiations.cpp
new file mode 100644
index 0000000000000..7cb3cd3f3a92e
--- /dev/null
+++ b/clang/test/Interpreter/failed-input-keeps-instantiations.cpp
@@ -0,0 +1,24 @@
+// REQUIRES: host-supports-jit
+// UNSUPPORTED: system-aix
+// RUN: cat %s | clang-repl 2>&1 | FileCheck %s
+
+// A failed input still triggers the implicit instantiations it uses. Sema
+// does not repeat them, so a later input must still get their definitions.
+
+extern "C" int printf(const char *, ...);
+
+template <class T> T twice(T x) { return x + x; }
+extern "C" int twice_fail(int *p) { return twice(*p) + no_such_name; }
+// CHECK-DAG: error: use of undeclared identifier 'no_such_name'
+extern "C" int twice_after(int x) { return twice(x); }
+printf("twice_after = %d\n", twice_after(21));
+// CHECK-DAG: twice_after = 42
+
+template <class T> struct Holder { static T value; };
+template <class T> T Holder<T>::value = T(7);
+extern "C" int holder_fail() { return Holder<int>::value + no_such_name; }
+// CHECK-DAG: error: use of undeclared identifier 'no_such_name'
+printf("Holder<int>::value = %d\n", Holder<int>::value);
+// CHECK-DAG: Holder<int>::value = 7
+
+%quit
diff --git a/clang/unittests/Interpreter/InterpreterTest.cpp
b/clang/unittests/Interpreter/InterpreterTest.cpp
index 3becd00c12820..4bf2422d1fe07 100644
--- a/clang/unittests/Interpreter/InterpreterTest.cpp
+++ b/clang/unittests/Interpreter/InterpreterTest.cpp
@@ -248,6 +248,57 @@ TEST_F(InterpreterTest, UndoCommand) {
EXPECT_FALSE(Err12);
}
+// A failed input is not a PTU, so Undo acts on the last good input.
+TEST_F(InterpreterTest, UndoAfterFailedInput) {
+#ifdef __EMSCRIPTEN__
+ GTEST_SKIP() << "Test fails for Emscipten builds";
+#endif
+ std::unique_ptr<Interpreter> Interp = createInterpreter();
+
+ // Nothing to undo: a failed input does not count.
+ auto Err1 = Interp->Parse("int bad = ;").takeError();
+ EXPECT_EQ("Parsing failed.", llvm::toString(std::move(Err1)));
+ auto Err2 = Interp->Undo();
+ EXPECT_EQ("Operation failed. No input left to undo",
+ llvm::toString(std::move(Err2)));
+
+ // Undo after a failed input removes the last good input.
+ cantFail(Interp->Parse("int kept = 1;"));
+ auto Err3 = Interp->Parse("int bad = kept + ;").takeError();
+ EXPECT_EQ("Parsing failed.", llvm::toString(std::move(Err3)));
+ cantFail(Interp->Undo());
+ auto Err4 = Interp->Parse("int use = kept;").takeError();
+ EXPECT_EQ("Parsing failed.", llvm::toString(std::move(Err4)));
+ auto Err5 = Interp->Undo();
+ EXPECT_EQ("Operation failed. No input left to undo",
+ llvm::toString(std::move(Err5)));
+
+ // The name is free again.
+ cantFail(Interp->Parse("double kept = 2.0;"));
+}
+
+// Undo frees a C-linkage name, so the same definition can come back.
+TEST_F(InterpreterTest, UndoExternCDefinition) {
+#ifdef __EMSCRIPTEN__
+ GTEST_SKIP() << "Test fails for Emscipten builds";
+#endif
+ std::unique_ptr<Interpreter> Interp = createInterpreter();
+
+ cantFail(Interp->Parse("extern \"C\" int f() { return 1; }"));
+ cantFail(Interp->Undo());
+ cantFail(Interp->Parse("extern \"C\" int f() { return 1; }"));
+
+ cantFail(Interp->ParseAndExecute("extern \"C\" int g() { return 1; }"));
+ cantFail(Interp->Undo());
+ cantFail(Interp->ParseAndExecute("extern \"C\" int g() { return 2; }"));
+ auto G = cantFail(Interp->getSymbolAddress("g")).toPtr<int (*)()>();
+ EXPECT_EQ(2, G());
+
+ cantFail(Interp->Parse("extern \"C\" { int h() { return 1; } }"));
+ cantFail(Interp->Undo());
+ cantFail(Interp->Parse("extern \"C\" { int h() { return 1; } }"));
+}
+
static std::string MangleName(NamedDecl *ND) {
ASTContext &C = ND->getASTContext();
std::unique_ptr<MangleContext> MangleC(C.createMangleContext());
@@ -350,6 +401,92 @@ TEST_F(InterpreterTest, InstantiateTemplate) {
EXPECT_EQ(42, fn(NewA.getPtr()));
}
+// A failed input must keep the implicit instantiations it made. A later input
+// that uses them must get their definitions.
+struct FailedInputInstantiationTest : InterpreterTest {
+ std::unique_ptr<Interpreter> Interp;
+
+ void SetUp() override {
+ InterpreterTest::SetUp();
+ // FIXME: We cannot yet handle delayed template parsing.
+ Interp = createInterpreter({"-fno-delayed-template-parsing"});
+ }
+
+ void ExpectParseFails(llvm::StringRef Code) {
+ llvm::Error Err = Interp->Parse(Code).takeError();
+ EXPECT_EQ("Parsing failed.", llvm::toString(std::move(Err)));
+ }
+
+ template <typename Fn> Fn *Lookup(llvm::StringRef Name) {
+ return cantFail(Interp->getSymbolAddress(Name)).toPtr<Fn *>();
+ }
+};
+
+TEST_F(FailedInputInstantiationTest, FunctionTemplate) {
+ cantFail(Interp->ParseAndExecute(
+ "template <class T> T twice(T x) { return x + x; }"));
+ ExpectParseFails("extern \"C\" double twice_fail(double *p) {"
+ " return twice(*p) + no_such_name; }");
+ cantFail(Interp->ParseAndExecute(
+ "extern \"C\" double twice_after(double x) { return twice(x); }"));
+ EXPECT_EQ(6.0, Lookup<double(double)>("twice_after")(3.0));
+}
+
+TEST_F(FailedInputInstantiationTest, MemberOfClassTemplate) {
+ cantFail(Interp->ParseAndExecute(
+ "template <class T> struct Box { T v; T twice() { return v + v; } };"));
+ ExpectParseFails("extern \"C\" int box_fail(Box<int> *b) {"
+ " return b->twice() + no_such_name; }");
+ cantFail(Interp->ParseAndExecute("extern \"C\" int box_after(int v) { "
+ "Box<int> b{v}; return b.twice(); }"));
+ EXPECT_EQ(8, Lookup<int(int)>("box_after")(4));
+}
+
+TEST_F(FailedInputInstantiationTest, StaticDataMemberOfClassTemplate) {
+ cantFail(Interp->ParseAndExecute(
+ "template <class T> struct Holder { static T value; };"
+ "template <class T> T Holder<T>::value = T(7);"));
+ ExpectParseFails("extern \"C\" int holder_fail() {"
+ " return Holder<int>::value + no_such_name; }");
+ cantFail(Interp->ParseAndExecute(
+ "extern \"C\" int holder_after() { return Holder<int>::value; }"));
+ EXPECT_EQ(7, Lookup<int()>("holder_after")());
+}
+
+TEST_F(FailedInputInstantiationTest, VariableTemplate) {
+ cantFail(Interp->ParseAndExecute("template <class T> T five = T(5);"));
+ ExpectParseFails(
+ "extern \"C\" int five_fail() { return five<int> + no_such_name; }");
+ cantFail(Interp->ParseAndExecute(
+ "extern \"C\" int five_after() { return five<int>; }"));
+ EXPECT_EQ(5, Lookup<int()>("five_after")());
+}
+
+TEST_F(FailedInputInstantiationTest, VTable) {
+ cantFail(
+ Interp->ParseAndExecute("struct V { virtual int f() { return 3; } };"));
+ ExpectParseFails(
+ "extern \"C\" int vtable_fail() { V v; return v.f() + no_such_name; }");
+ cantFail(Interp->ParseAndExecute(
+ "extern \"C\" int vtable_after() { V v; return v.f(); }"));
+ EXPECT_EQ(3, Lookup<int()>("vtable_after")());
+}
+
+TEST_F(FailedInputInstantiationTest, UndoThenReuseTemplate) {
+ cantFail(Interp->ParseAndExecute(
+ "template <class T> T thrice(T x) { return x + x + x; }"));
+ cantFail(Interp->ParseAndExecute(
+ "extern \"C\" int thrice_a(int x) { return thrice(x); }"));
+ ExpectParseFails("extern \"C\" int thrice_fail(int x) { return thrice(x) + "
+ "no_such_name; }");
+ // The failed input is not a PTU, so this undoes thrice_a.
+ cantFail(Interp->Undo());
+ ExpectParseFails("extern \"C\" int thrice_b(int x) { return thrice_a(x); }");
+ cantFail(Interp->ParseAndExecute(
+ "extern \"C\" int thrice_a(int x) { return thrice(x) + 1; }"));
+ EXPECT_EQ(7, Lookup<int(int)>("thrice_a")(2));
+}
+
TEST_F(InterpreterTest, Value) {
std::vector<const char *> Args = {"-fno-sized-deallocation"};
std::unique_ptr<Interpreter> Interp = createInterpreter(Args);
``````````
</details>
https://github.com/llvm/llvm-project/pull/226253
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
