Page MenuHomePhabricator

Make windows resource generation more robust to misconfiguration.
Needs ReviewPublic

Authored by stellaraccident on Sep 25 2021, 3:11 PM.

Details

Reviewers
stella.stamenova
Summary

I was experiencing this in an out-of-tree project when building a DLL in a context that didn't have LLVM_VERSION_* and friends set. It causes obscure errors in the resource compiler if not integers.

Diff Detail

Unit TestsFailed

TimeTest
110 msx64 debian > ORC-x86_64-linux.TestCases/Linux/x86-64::trivial-cxa-atexit.S
Script: -- : 'RUN: at line 3'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang -m64 -c -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/orc/X86_64LinuxConfig/TestCases/Linux/x86-64/Output/trivial-cxa-atexit.S.tmp /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/orc/TestCases/Linux/x86-64/trivial-cxa-atexit.S
100 msx64 debian > ORC-x86_64-linux.TestCases/Linux/x86-64::trivial-static-initializer.S
Script: -- : 'RUN: at line 7'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang -m64 -c -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/orc/X86_64LinuxConfig/TestCases/Linux/x86-64/Output/trivial-static-initializer.S.tmp /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/orc/TestCases/Linux/x86-64/trivial-static-initializer.S
90 msx64 debian > ORC-x86_64-linux.TestCases/Linux/x86-64::trivial-tls.S
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang -m64 -c -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/orc/X86_64LinuxConfig/TestCases/Linux/x86-64/Output/trivial-tls.S.tmp /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/orc/TestCases/Linux/x86-64/trivial-tls.S

Event Timeline

stellaraccident requested review of this revision.Sep 25 2021, 3:11 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 25 2021, 3:11 PM

LGTM. I find it easier to read if it's all done together (see comment), but it's fine either way.

llvm/cmake/modules/AddLLVM.cmake
386

Since the issue you saw was when LLVM_VERSION* was not defined, I think it would be a bit cleaner if you made all the assignments together e.g.:

if (NOT DEFINED ARG_VERSION_MAJOR)
  if (${LLVM_VERSION_MAJOR})
    set(ARG_VERSION_MAJOR ${LLVM_VERSION_MAJOR})
  else
    set(ARG_VERSION_MAJOR 0)
  endif
endif

It's not any less verbose, but it's a bit more explicit.