Page MenuHomePhabricator

Adding support for overriding LLVM_ENABLE_RUNTIMES for runtimes builds.
ClosedPublic

Authored by plotfi on Sep 4 2019, 3:25 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

plotfi created this revision.Sep 4 2019, 3:25 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 4 2019, 3:25 PM
beanz added a comment.Sep 4 2019, 5:02 PM

I feel like the default behavior should be that if you set LLVM_ENABLE_RUNTIMES it builds them for all targets you request. If you want to disable some runtimes for certain targets that should be an opt-out setting rather than an opt-in to build for targets.

@beanz - I can see where you are coming from there. But, the thing is that when you are building for a wide variety of targets at once, that doesn't make sense since not all the runtimes are compatible with all targets (e.g. unwind doesn't support Windows). Calculating the set difference and passing that along seems more complicated, though plausible. Note that this approach preserves the existing behaviour - unless you *explicitly* state the runtimes for a specific target, the LLVM_ENABLE_RUNTIMES passed globally will continue to be passed through, so from a user's perspective nothing has changed (and that is intentional - no one should need to update their CMake caches for this change).

beanz added a comment.Sep 5 2019, 6:37 PM

@compnerd, it doesn't look like LLVM_ENABLE_RUNTIMES passed globally is still passed through if you are generating runtimes for multiple targets. It looks like that only gets passed through if you specify RUNTIMES_${name}_LLVM_ENABLE_RUNTIMES.

That looks like a behavior change to me.

@beanz - I must be mistaken about something. The condition is if LLVM_ENABLE_RUNTIMES is NOT specified for the current target, we pass in the global value (which was previously passed via the PASSTHROUGH_PREFIXES), and if it is specified, we will pass in the specified value. So, the only behaviour change that is visible is if the user specified a target specific LLVM_ENABLE_RUNTIMES value it is now honoured.

beanz accepted this revision.Sep 9 2019, 4:50 PM

I see. Sorry for the noise. This looks fine.

@plotfi in the future when you upload diffs, please provide full context, it makes reading and understanding what is going on way easier.

This revision is now accepted and ready to land.Sep 9 2019, 4:50 PM
This revision was automatically updated to reflect the committed changes.

Hi. I think this might be causing some linker failures on our x64 bots:

[304/396] Linking CXX shared library /b/s/w/ir/k/recipe_cleanup/clangdPn0KT/llvm_build_dir/lib/clang/10.0.0/lib/x86_64-unknown-fuchsia/libclang_rt.ubsan_standalone.so
FAILED: /b/s/w/ir/k/recipe_cleanup/clangdPn0KT/llvm_build_dir/lib/clang/10.0.0/lib/x86_64-unknown-fuchsia/libclang_rt.ubsan_standalone.so 
... The long link invocation
ld.lld: error: undefined symbol: _Unwind_GetIP
>>> referenced by sanitizer_symbolizer_markup.cpp:113 (compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_markup.cpp:113)
>>>               compiler-rt/lib/sanitizer_common/CMakeFiles/RTSanitizerCommonSymbolizer.x86_64.dir/sanitizer_symbolizer_markup.cpp.obj:(__sanitizer::Unwind_Trace(_Unwind_Context*, void*))

ld.lld: error: undefined symbol: _Unwind_Backtrace
>>> referenced by sanitizer_symbolizer_markup.cpp:124 (compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_markup.cpp:124)
>>>               compiler-rt/lib/sanitizer_common/CMakeFiles/RTSanitizerCommonSymbolizer.x86_64.dir/sanitizer_symbolizer_markup.cpp.obj:(__sanitizer::BufferedStackTrace::UnwindSlow(unsigned long, unsigned int))

ld.lld: error: undefined symbol: typeinfo for std::type_info
>>> referenced by ubsan_type_hash_itanium.cpp:231 (compiler-rt/lib/ubsan/ubsan_type_hash_itanium.cpp:231)
>>>               compiler-rt/lib/ubsan/CMakeFiles/RTUbsan_cxx.x86_64.dir/ubsan_type_hash_itanium.cpp.obj:(__ubsan::checkDynamicType(void*, void*, unsigned long))
... A bunch more link errors

Could you take a look? Thanks.

Where are your bots?

PL

Hi. I think this might be causing some linker failures on our x64 bots:

 [304/396] Linking CXX shared library /b/s/w/ir/k/recipe_cleanup/clangdPn0KT/llvm_build_dir/lib/clang/10.0.0/lib/x86_64-unknown-fuchsia/libclang_rt.ubsan_standalone.so
 FAILED: /b/s/w/ir/k/recipe_cleanup/clangdPn0KT/llvm_build_dir/lib/clang/10.0.0/lib/x86_64-unknown-fuchsia/libclang_rt.ubsan_standalone.so 
 ... The long link invocation
 ld.lld: error: undefined symbol: _Unwind_GetIP
>>> referenced by sanitizer_symbolizer_markup.cpp:113 (compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_markup.cpp:113)
>>>               compiler-rt/lib/sanitizer_common/CMakeFiles/RTSanitizerCommonSymbolizer.x86_64.dir/sanitizer_symbolizer_markup.cpp.obj:(__sanitizer::Unwind_Trace(_Unwind_Context*, void*))
 
 ld.lld: error: undefined symbol: _Unwind_Backtrace
>>> referenced by sanitizer_symbolizer_markup.cpp:124 (compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_markup.cpp:124)
>>>               compiler-rt/lib/sanitizer_common/CMakeFiles/RTSanitizerCommonSymbolizer.x86_64.dir/sanitizer_symbolizer_markup.cpp.obj:(__sanitizer::BufferedStackTrace::UnwindSlow(unsigned long, unsigned int))
 
 ld.lld: error: undefined symbol: typeinfo for std::type_info
>>> referenced by ubsan_type_hash_itanium.cpp:231 (compiler-rt/lib/ubsan/ubsan_type_hash_itanium.cpp:231)
>>>               compiler-rt/lib/ubsan/CMakeFiles/RTUbsan_cxx.x86_64.dir/ubsan_type_hash_itanium.cpp.obj:(__ubsan::checkDynamicType(void*, void*, unsigned long))
 ... A bunch more link errors

Could you take a look? Thanks.

phosek added a subscriber: phosek.Sep 10 2019, 5:34 PM

I think I see the issue, if you look at LLVMExternalProjectUtils.cmake, it uses the following logic for PASSTHROUGH_PREFIXES:

list(APPEND ARG_PASSTHROUGH_PREFIXES ${nameCanon})
foreach(prefix ${ARG_PASSTHROUGH_PREFIXES})
  foreach(variableName ${variableNames})
    if(variableName MATCHES "^${prefix}")
      string(REPLACE ";" "|" value "${${variableName}}")
      list(APPEND PASSTHROUGH_VARIABLES
        -D${variableName}=${value})
    endif()
  endforeach()
endforeach()

We then set LIST_SEPARATOR | when invoking ExternalProject_Add.

Changing the list separator is really critical when passing through lists, as is the case for LLVM_ENABLE_RUNTIMES where the value would typically be -DLLVM_ENABLE_RUNTIMES=compiler-rt;libcxx;libcxxabi;libunwind. With your change, only the first value (i.e. compiler-rt) would be passed through to the llvm_ExternalProject_Add and the rest will be dropped hence this error we're seeing (because none of the other runtimes was built).

Nice! Thanks @phosek. I’m fine with a revert. Currently away from keyboard.

I think I see the issue, if you look at LLVMExternalProjectUtils.cmake, it uses the following logic for PASSTHROUGH_PREFIXES:

list(APPEND ARG_PASSTHROUGH_PREFIXES ${nameCanon})
foreach(prefix ${ARG_PASSTHROUGH_PREFIXES})
  foreach(variableName ${variableNames})
    if(variableName MATCHES "^${prefix}")
      string(REPLACE ";" "|" value "${${variableName}}")
      list(APPEND PASSTHROUGH_VARIABLES
        -D${variableName}=${value})
    endif()
  endforeach()
endforeach()

We then set LIST_SEPARATOR | when invoking ExternalProject_Add.

Changing the list separator is really critical when passing through lists, as is the case for LLVM_ENABLE_RUNTIMES where the value would typically be -DLLVM_ENABLE_RUNTIMES=compiler-rt;libcxx;libcxxabi;libunwind. With your change, only the first value (i.e. compiler-rt) would be passed through to the llvm_ExternalProject_Add and the rest will be dropped hence this error we're seeing (because none of the other runtimes was built).