This is an archive of the discontinued LLVM Phabricator instance.

[CodeGen] Keep track of eagerly emitted globals
ClosedPublic

Authored by Hahnfeld on Jul 28 2023, 6:56 AM.

Details

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 ]

Diff Detail

Unit TestsFailed

Event Timeline

Hahnfeld created this revision.Jul 28 2023, 6:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 28 2023, 6:56 AM
Hahnfeld requested review of this revision.Jul 28 2023, 6:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 28 2023, 6:56 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

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

@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 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?

@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 accepted this revision.Aug 15 2023, 3:59 AM

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

This revision is now accepted and ready to land.Aug 15 2023, 3:59 AM

@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).

@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.

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

This revision was landed with ongoing or failed builds.Aug 17 2023, 4:43 AM
This revision was automatically updated to reflect the committed changes.
Hahnfeld reopened this revision.Aug 17 2023, 5:29 AM
This revision is now accepted and ready to land.Aug 17 2023, 5:29 AM
Hahnfeld updated this revision to Diff 551401.Aug 17 2023, 11:56 PM

Disable RTTI to (hopefully) avoid problems on Windows.

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 :)

This revision was landed with ongoing or failed builds.Aug 18 2023, 12:44 AM
This revision was automatically updated to reflect the committed changes.