Page MenuHomePhabricator

[ORC-RT] Add unit test infrastructure, extensible_rtti implementation, unit test
ClosedPublic

Authored by lhames on May 7 2021, 9:35 AM.

Details

Summary

Add unit test infrastructure for the ORC runtime, plus a cut-down
extensible_rtti system and extensible_rtti unit test.

Removes the placeholder.cpp source file.

Diff Detail

Event Timeline

lhames created this revision.May 7 2021, 9:35 AM
lhames requested review of this revision.May 7 2021, 9:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 7 2021, 9:35 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript
lhames updated this revision to Diff 343703.May 7 2021, 9:40 AM

Fix incomplete cargo-culting of unittest CMake.

lhames updated this revision to Diff 343704.May 7 2021, 9:42 AM

Include the whole patch. (Why are you like this arc?)

lhames added a comment.May 7 2021, 9:50 AM

I've tried to adapt this from the Xray and TSan test CMakeLists.txt. Most of it makes sense to me (as much as any CMake can), but I was surprised by this pattern which showed up in Xray and TSan (and all the other projects that I looked at):

if (APPLE)
  add_orc_lib("RTOrc.test.osx"
    $<TARGET_OBJECTS:RTOrc.osx>)
else()
  foreach(arch ${ORC_SUPPORTED_ARCH})
    add_orc_lib("RTOrc.test.${arch}"
      $<TARGET_OBJECTS:RTOrc.${arch}>)
  endforeach()
endif()

We're already building runtime libraries, but in the tests it looks like we're building a parallel archive for the runtime and writing it to the test directory for the tests to link against. Is there a reason the tests can't just link against the built runtimes?

Looks like this is broken on Linux. Looking in to that now...

The extensible RTTI infrastructure is copied (roughly) from LLVM? Might be worth a comment mentioning where it's from in case there's a desire to keep future bug fixes/etc in sync? (both the implementation and the test might benefit from that sort of pointer?)

lhames updated this revision to Diff 343799.May 7 2021, 7:03 PM

Fix Linux build, add comments noting that the runtime extensible_rtti
code was adapted from LLVM.

We're already building runtime libraries, but in the tests it looks like we're building a parallel archive for the runtime and writing it to the test directory for the tests to link against. Is there a reason the tests can't just link against the built runtimes?

I think on Apple platforms TSAN only builds a dylib for production [1]. Tests should link statically so it's building a static archive for this case. All other Unix platforms do the opposite. Not sure why, but at least it looks consistent. There's a related comment about that in [2]. Maybe @kubamracek can tell you more :)

The ORC runtime is a static archive in any case right? So I guess this won't apply here. (Disclaimer: not a compiler-rt expert)

[1] https://github.com/llvm/llvm-project/blob/a81e45b8bcb8eb274ad73357e10e2cdf8a314a8c/compiler-rt/lib/tsan/CMakeLists.txt#L140
[2] https://github.com/llvm/llvm-project/blob/c47620a838eae330c04453ed3900aab0ac4159cb/compiler-rt/lib/tsan/tests/CMakeLists.txt#L71

I think on Apple platforms TSAN only builds a dylib for production [1]....

I thought it might be something like that. It might also enable unit-testing of the runtime even if your production build doesn't target the host architecture, but I don't know how common that is?

The ORC runtime is a static archive in any case right? So I guess this won't apply here. (Disclaimer: not a compiler-rt expert)

Yes -- the ORC runtime should only ever be built statically.

@phosek It's really just the CMake changes that need reviewing here. The C++ code is adapted directly from code already in LLVM.

These CMake changes to enable unit testing are the last piece of build infrastructure that I expect to have to add. Everything from here on out is likely to be incremental tweaks and maintenance, plus the actual ORC runtime code. Would you like to continue with pre-commit review for future CMake changes, or switch to post-commit review for that?

phosek accepted this revision.May 10 2021, 10:47 AM

LGTM

compiler-rt/lib/orc/CMakeLists.txt
13

Can you also include extensible_rtti.h in here?

This revision is now accepted and ready to land.May 10 2021, 10:47 AM
lhames added inline comments.May 10 2021, 12:18 PM
compiler-rt/lib/orc/CMakeLists.txt
13

Will do. Thanks for catching that!

This revision was landed with ongoing or failed builds.May 10 2021, 5:17 PM
This revision was automatically updated to reflect the committed changes.

Hi, this makes our linux and windows bots fail to run cmake, with this diag (this cmake invocation is on macos):

 Running cmake -GNinja -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_ASSERTIONS=OFF '-DLLVM_ENABLE_PROJECTS=clang;compiler-rt;lld;chrometools;clang-tools-extra;libcxx' '-DLLVM_TARGETS_TO_BUILD=AArch64;ARM;Mips;PowerPC;SystemZ;WebAssembly;X86' -DLLVM_ENABLE_PIC=OFF -DLLVM_ENABLE_UNWIND_TABLES=OFF -DLLVM_ENABLE_TERMINFO=OFF -DLLVM_ENABLE_Z3_SOLVER=OFF -DCLANG_PLUGIN_SUPPORT=OFF -DCLANG_ENABLE_STATIC_ANALYZER=OFF -DCLANG_ENABLE_ARCMT=OFF '-DBUG_REPORT_URL=https://crbug.com and run tools/clang/scripts/process_crashreports.py (only works inside Google) which will upload a report' -DLLVM_INCLUDE_GO_TESTS=OFF -DENABLE_X86_RELAX_RELOCATIONS=NO -DLLVM_ENABLE_DIA_SDK=OFF '-DCOMPILER_RT_SANITIZERS_TO_BUILD=asan;dfsan;msan;hwasan;tsan;cfi' -DLIBCXX_ENABLE_SHARED=OFF -DLIBCXX_ENABLE_STATIC=OFF -DLIBCXX_INCLUDE_TESTS=OFF -DLIBCXX_ENABLE_EXPERIMENTAL_LIBRARY=OFF -DLLVM_ENABLE_LIBXML2=FORCE_ON '-DLLVM_TARGETS_TO_BUILD=X86;ARM;AArch64' '-DLLVM_ENABLE_PROJECTS=clang;compiler-rt;libcxx;compiler-rt' -DCMAKE_INSTALL_PREFIX=/opt/s/w/ir/cache/builder/src/third_party/llvm-bootstrap-install -DCMAKE_C_FLAGS= -DCMAKE_CXX_FLAGS= -DCMAKE_EXE_LINKER_FLAGS= -DCMAKE_SHARED_LINKER_FLAGS= -DCMAKE_MODULE_LINKER_FLAGS= -DLLVM_ENABLE_ASSERTIONS=ON -DCOMPILER_RT_BUILD_BUILTINS=ON -DCOMPILER_RT_BUILD_CRT=OFF -DCOMPILER_RT_BUILD_LIBFUZZER=OFF -DCOMPILER_RT_BUILD_MEMPROF=OFF -DCOMPILER_RT_BUILD_SANITIZERS=OFF -DCOMPILER_RT_BUILD_XRAY=OFF -DCOMPILER_RT_ENABLE_IOS=OFF -DCOMPILER_RT_ENABLE_WATCHOS=OFF -DCOMPILER_RT_ENABLE_TVOS=OFF -DDARWIN_osx_ARCHS=x86_64 /opt/s/w/ir/cache/builder/src/third_party/llvm/llvm
...
CMake Error at /b/s/w/ir/cache/builder/src/third_party/llvm/compiler-rt/cmake/Modules/AddCompilerRT.cmake:415 (sanitizer_test_compile):
   Unknown CMake command "sanitizer_test_compile".
 Call Stack (most recent call first):
   /b/s/w/ir/cache/builder/src/third_party/llvm/compiler-rt/lib/orc/unittests/CMakeLists.txt:68 (generate_compiler_rt_tests)
   /b/s/w/ir/cache/builder/src/third_party/llvm/compiler-rt/lib/orc/unittests/CMakeLists.txt:97 (add_orc_unittest)

Hi Nico,

Thanks for the heads up. I'm investigating now.

  • Lang.