[PATCH] D133084: [ORC-RT] Refactor ORC runtime CMake for future test tool(s).

Fri Sep 2 14:51:55 PDT 2022

phosek accepted this revision.
phosek added a comment.
This revision is now accepted and ready to land.

LGTM

================
Comment at: compiler-rt/lib/orc/tests/CMakeLists.txt:15
+# FIXME: This should be set for all unit tests.
+  -std=c++17
   ${ORC_CFLAGS}
----------------
lhames wrote:
> phosek wrote:
> > Do you see a build failure without this flag? This should now be enabled globally for all of compiler-rt: https://github.com/llvm/llvm-project/blob/9185d6e6bca8ec41b48661c371e813498f840455/compiler-rt/CMakeLists.txt#L71
> @phosek I do see a failure without it. The problem is that `generate_compiler_rt_tests` results in a call to `clang_compile`, which uses `add_custom_command` to perform the compilation: https://github.com/llvm/llvm-project/blob/9185d6e6bca8ec41b48661c371e813498f840455/compiler-rt/cmake/Modules/CompilerRTCompile.cmake#L103.
> 
> Since `add_custom_command` doesn't know about `CMAKE_CXX_STANDARD` we don't set the standard in that command.
> 
> Looks like gwp_asan hit a similar issue recently -- they also specify c++17 manually in their config: https://github.com/llvm/llvm-project/commit/474145c0b2420cb316bb8a9dcc031d613679d496
> 
>  
OK that's something we should address but it doesn't need to block this change.

================
Comment at: compiler-rt/lib/orc/tests/tools/orc-rt-executor.cpp:1
+#include <cstring>
+#include <iostream>
----------------
Should this file also have the copyright header?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133084/new/

https://reviews.llvm.org/D133084