[CodeGen] Keep track of eagerly emitted globals

[Hahnfeld]

Summary

An inline virtual function must be emitted, but we need to remember
it and emit the same definition again in the future in case later
LLVM optimizations stripped it from the Module. The added test case
shows the problem; before this patch, it would fail with:

Symbols not found: [ _ZN1AD0Ev, _ZN1AD1Ev ]

[rjmccall]

I would like to avoid the overhead of this when we're not emitting for a REPL.

[Hahnfeld]

@v.g.vassilev do we have a means to detect this case? In D156897, I'm refactoring all accesses to EmittedDeferredDecl to go via addEmittedDeferredDecl, so we should just add an early return when not emitting for a REPL.

[v.g.vassilev]

In D156537#4554052, @Hahnfeld wrote:

@v.g.vassilev do we have a means to detect this case? In D156897, I'm refactoring all accesses to EmittedDeferredDecl to go via addEmittedDeferredDecl, so we should just add an early return when not emitting for a REPL.

We have getLangOpts().IncrementalExtensions. Does that work?

[Hahnfeld]

In D156537#4554413, @v.g.vassilev wrote:

In D156537#4554052, @Hahnfeld wrote:

@v.g.vassilev do we have a means to detect this case? In D156897, I'm refactoring all accesses to EmittedDeferredDecl to go via addEmittedDeferredDecl, so we should just add an early return when not emitting for a REPL.

We have getLangOpts().IncrementalExtensions. Does that work?

Perfect! After D157379, calling addEmittedDeferredDecl should immediately return in case we are not in incremental mode.

[v.g.vassilev]

@Hahnfeld, let's move forward and rely on a post-commit review here if necessary.

[aaron.ballman]

In D156537#4587832, @v.g.vassilev wrote:

@Hahnfeld, let's move forward and rely on a post-commit review here if necessary.

Because one of the codegen code owners has been active on the thread with concerns, I think it's better to wait a bit longer for an explicit sign-off that those concerns are addressed (this code impacts more than clang-repl).

[v.g.vassilev]

In D156537#4587874, @aaron.ballman wrote:

In D156537#4587832, @v.g.vassilev wrote:

@Hahnfeld, let's move forward and rely on a post-commit review here if necessary.

Because one of the codegen code owners has been active on the thread with concerns, I think it's better to wait a bit longer for an explicit sign-off that those concerns are addressed (this code impacts more than clang-repl).

D157379 seems to resolve the concern but indeed we can wait a little longer.

[rjmccall]

If we're avoiding the expense here when we're not emitting incremental extensions, this seems fine to me.

[Hahnfeld]

Disable RTTI to (hopefully) avoid problems on Windows.

[Hahnfeld]

I reverted the change yesterday because the test wasn't passing on Windows, for example https://lab.llvm.org/buildbot/#/builders/216/builds/25769/steps/7/logs/FAIL__Clang__inline-virtual_cpp. The problem is that the JIT cannot find ??_7type_info@@6B@ related to RTTI. I see that we have some special setup in ROOT/Cling to make this work, but it's not fully clear to me how this could be applied to clang-repl if we are building LLVM without RTTI. For now, I propose to disable RTTI for this test (it's not needed) and will re-commit once the build bots are back to a more healthy green state :)

clang/lib/CodeGen/CodeGenModule.cpp

void CodeGenModule::EmitGlobal(GlobalDecl GD) {
   }
 
   // Defer code generation to first use when possible, e.g. if this is an inline
   // function. If the global must always be emitted, do it eagerly if possible
   // to benefit from cache locality.
   if (MustBeEmitted(Global) && MayBeEmittedEagerly(Global)) {
     // Emit the definition if it can't be deferred.
     EmitGlobalDefinition(GD);
+    addEmittedDeferredDecl(GD);
     return;
   }
 
   // If we're deferring emission of a C++ variable with an
   // initializer, remember the order in which it appeared in the file.
   if (getLangOpts().CPlusPlus && isa<VarDecl>(Global) &&
       cast<VarDecl>(Global)->hasInit()) {
     DelayedCXXInitPosition[Global] = CXXGlobalInits.size();

clang/test/Interpreter/inline-virtual.cpp

+// REQUIRES: host-supports-jit
+// UNSUPPORTED: system-aix
+//
+// We disable RTTI to avoid problems on Windows for non-RTTI builds of LLVM
+// where the JIT cannot find ??_7type_info@@6B@.
+// RUN: cat %s | clang-repl -Xcc -fno-rtti | FileCheck %s
+// RUN: cat %s | clang-repl -Xcc -fno-rtti -Xcc -O2 | FileCheck %s
+
+extern "C" int printf(const char *, ...);
+
+struct A { int a; A(int a) : a(a) {} virtual ~A(); };
+
+// Then define the virtual destructor as inline out-of-line, in a separate
+// PartialTranslationUnit.
+inline A::~A() { printf("~A(%d)\n", a); }
+
+// Create one instance with new and delete it.
+A *a1 = new A(1);
+delete a1;
+// CHECK: ~A(1)
+
+// Also create one global that will be auto-destructed.
+A a2(2);
+// CHECK: ~A(2)