Page MenuHomePhabricator

Add missing vtable anchors
ClosedPublic

Authored by weimingz on Apr 4 2018, 12:35 AM.

Details

Summary

This patch adds anchor() for MemoryBuffer, raw_fd_ostream, RTDyldMemoryManager, SectionMemoryManager, etc.

Diff Detail

Repository
rL LLVM

Event Timeline

weimingz created this revision.Apr 4 2018, 12:35 AM

When I statically linking a project (built with RTTI) against LLVM OrcJIT (default built with no-RTTI), I got a couple of "undefined reference to type info" error.

Looks similar to http://lists.llvm.org/pipermail/llvm-dev/2016-February/095671.html

However, I'm not sure where to put the definition of anchor() for ObjectMemoryBuffer class. It's used by ThinLTOCodeGenerator.cpp and MCJIT.cpp, which belong to different libs.

Looks like ObjectMemoryBuffer is part of ExecutionEngine, so having libLTO
dependent on ExecutionEngine seems basically correct for the dependencies
that are there already. Whether or not those dependencies should be there
is another question, I guess. (maybe ObjectMemoryBuffer should be moved
somewhere more common, etc - but I wouldn't worry about doing that unless
the LTO dependency breaks things)

If I choose to define ObjectMemoryBuffer::anchor() in MCJIT.cpp (the module that uses ObjectMemoryBuffer), then I got error like:
../../lib/libLLVMLTO.a(ThinLTOCodeGenerator.cpp.o): In function `ObjectMemoryBuffer':
llvm/ExecutionEngine/ObjectMemoryBuffer.h:41: undefined reference to `vtable for llvm::ObjectMemoryBuffer'

You'd have to add the ExecutionEngine library as a dependency from the LTO
library - by modifying lib/LTO/LLVMBuild.txt - then it should link, I would
think.

weimingz updated this revision to Diff 141926.Apr 10 2018, 3:45 PM

add dependency upon MCJIT for LTO due to the use of ObjectMemoryBuffer

Is it sufficient to put the dependency MCJIT in lib/LTO/LLVMBuild.txt (since that's where the dependency on is) & not in the tools that /use/ the LTO lib (I expect they should pickup the indirect dependency implicitly?)?

I put a dependency on tools/llvm-lto as well. From my build (default CMake config), it works.

I meant what happens if you remove the changes to tools/lto/LLVMBuild.txt
and tools/llvm-lto/LLVMBuild.txt and /only/ have a change to
lib/LTO/LLVMBuild.txt - is that sufficient to make the build work?

I meant what happens if you remove the changes to tools/lto/LLVMBuild.txt
and tools/llvm-lto/LLVMBuild.txt and /only/ have a change to
lib/LTO/LLVMBuild.txt - is that sufficient to make the build work?

I tried before and it didn't work. I think it's because lib/LTO is for the generation of libLLVMLTO.a (at least for the default build config), so the dependency on MCJIT has no effect on the archive generation.

That sort of surprises me - if that were the case, there would be no point
having LLVMBuild.txt files in any libraries, I would think?

That sort of surprises me - if that were the case, there would be no point
having LLVMBuild.txt files in any libraries, I would think?

Hi David,

Do you mean changes like:
diff --git a/lib/LTO/CMakeLists.txt b/lib/LTO/CMakeLists.txt

  • a/lib/LTO/CMakeLists.txt

+++ b/lib/LTO/CMakeLists.txt
@@ -13,4 +13,5 @@ add_llvm_library(LLVMLTO

DEPENDS
intrinsics_gen
llvm_vcsrevision_h

+ LLVMLTO
)
In this case, I think the dependency rule won't affect the build of libLLVMLTO.a. I tried and it didn't work. Or maybe I misunderstood what you meant?

I meant that lib/LTO/LLVMBuild.txt should include "ExecutionEngine" in its
required_libraries. I wouldn't expect any changes would be needed to
tools/lto/* or tools/llvm-lto/*.

weimingz updated this revision to Diff 142081.Apr 11 2018, 3:24 PM

@dblaikie you're right. I modified CMakeLists.txt rather than LLVMBuild.txt

dblaikie accepted this revision.Apr 11 2018, 3:27 PM

Great - thanks for sticking with it, sorry for my dodgy explanations!

If you're adding in anchors, are you using -Wweak-vtables to find them? If you're pushing through to make LLVM (& hopefully Clang/other LLVM subprojects) -Wweak-vtables clean, perhaps you can turn on the warning in the CMake config once it's clean so we don't regress this again? (it'd also be interesting to do some kind of analysis to see whether avoiding weak vtables is /actually/ worthwhile - like maybe making a temporary/local/non-committed change to change all the explicit anchors into inline functions (unanchoring them) & see if builds are much larger or longer, etc)

This revision is now accepted and ready to land.Apr 11 2018, 3:27 PM
Closed by commit rL329861: Add missing vtable anchors (authored by weimingz, committed by ). · Explain WhyApr 11 2018, 4:13 PM
This revision was automatically updated to reflect the committed changes.

Great - thanks for sticking with it, sorry for my dodgy explanations!

If you're adding in anchors, are you using -Wweak-vtables to find them? If you're pushing through to make LLVM (& hopefully Clang/other LLVM subprojects) -Wweak-vtables clean, perhaps you can turn on the warning in the CMake config once it's clean so we don't regress this again? (it'd also be interesting to do some kind of analysis to see whether avoiding weak vtables is /actually/ worthwhile - like maybe making a temporary/local/non-committed change to change all the explicit anchors into inline functions (unanchoring them) & see if builds are much larger or longer, etc)

I got linker error when my code tries to statically link against some LLVM libs (like libLLVMOrc). It's a good idea to turn on -Wweak-vtables to check if we miss other cases. I will try it. Thanks