Skip to content

Commit

Permalink
Auto merge of rust-lang#81688 - pnkfelix:fix-llvm-version-check-in-ru…
Browse files Browse the repository at this point in the history
…n-make-tests, r=simulacrum

Use `# min-llvm-version: 11.0` to force a minimum LLVM version

Use `# min-llvm-version: 11.0` to force a minimum LLVM version, rather than ad-hoc internal solution.

In particular: the specific code to define LLVM_VERSION_11_PLUS here was, for some reason, using `$(shell ...)` with bash-specific variable replacement code. On non-bash platforms like dash, that `shell` invocation would fail, and the
LLVM_VERSION_11_PLUS check would always fail, the test would always be ignored, and thus be treated as a "success" (in the sense that `--bless` would never do anything).

 * Note in particular that GNU Make treats the SHELL variable as a very special case: it does not inherit the value of SHELL from the user's environment. Except on Windows. See more explanation in the [GNU Make docs](https://www.gnu.org/software/make/manual/html_node/Choosing-the-Shell.html).
 * The effect of this is that these tests end up using `/bin/sh` (except on Windows) for their `$(shell ...)` invocations, and thus we see differing behaviors depending on whether your `/bin/sh` links to `/bin/dash` or to `/bin/bash`.

This was causing me a lot of pain.
  • Loading branch information
bors committed Feb 5, 2021
2 parents 6a388dc + 5ce6710 commit 9e5d58f
Show file tree
Hide file tree
Showing 4 changed files with 4 additions and 22 deletions.
6 changes: 1 addition & 5 deletions src/test/run-make-fulldeps/coverage-llvmir/Makefile
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
# needs-profiler-support
# min-llvm-version: 11.0

-include ../coverage/coverage_tools.mk

Expand Down Expand Up @@ -48,12 +49,7 @@ else
-DINSTR_PROF_ORDERFILE='$(DATA_SECTION_PREFIX)__llvm_orderfile'
endif

ifeq ($(LLVM_VERSION_11_PLUS),true)
all: test_llvm_ir
else
$(info Rust option `-Z instrument-coverage` requires LLVM 11 or higher. Test skipped.)
all:
endif

test_llvm_ir:
# Compile the test program with non-experimental coverage instrumentation, and generate LLVM IR
Expand Down
7 changes: 2 additions & 5 deletions src/test/run-make-fulldeps/coverage-reports/Makefile
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
# ignore-test Broken; accidentally silently ignored on Linux CI; FIXME(#81688)
# needs-profiler-support
# ignore-windows-gnu
# min-llvm-version: 11.0

# FIXME(mati865): MinGW GCC miscompiles compiler-rt profiling library but with Clang it works
# properly. Since we only have GCC on the CI ignore the test for now.
Expand Down Expand Up @@ -67,12 +69,7 @@ ifdef RUSTC_BLESS_TEST
DEBUG_FLAG=--debug
endif

ifeq ($(LLVM_VERSION_11_PLUS),true)
all: $(patsubst $(SOURCEDIR)/lib/%.rs,%,$(wildcard $(SOURCEDIR)/lib/*.rs)) $(patsubst $(SOURCEDIR)/%.rs,%,$(wildcard $(SOURCEDIR)/*.rs))
else
$(info Rust option `-Z instrument-coverage` requires LLVM 11 or higher. Test skipped.)
all:
endif

# Ensure there are no `expected` results for tests that may have been removed or renamed
.PHONY: clear_expected_if_blessed
Expand Down
6 changes: 1 addition & 5 deletions src/test/run-make-fulldeps/coverage-spanview/Makefile
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
# needs-profiler-support
# min-llvm-version: 11.0

-include ../coverage/coverage_tools.mk

Expand All @@ -20,12 +21,7 @@ For revisions in Pull Requests (PR):
endef
export SPANVIEW_HEADER

ifeq ($(LLVM_VERSION_11_PLUS),true)
all: $(patsubst $(SOURCEDIR)/lib/%.rs,%,$(wildcard $(SOURCEDIR)/lib/*.rs)) $(patsubst $(SOURCEDIR)/%.rs,%,$(wildcard $(SOURCEDIR)/*.rs))
else
$(info Rust option `-Z instrument-coverage` requires LLVM 11 or higher. Test skipped.)
all:
endif

# Ensure there are no `expected` results for tests that may have been removed or renamed
.PHONY: clear_expected_if_blessed
Expand Down
7 changes: 0 additions & 7 deletions src/test/run-make-fulldeps/coverage/coverage_tools.mk
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,3 @@
# Therefore, `-C link-dead-code` is no longer automatically enabled.

UNAME = $(shell uname)

# Rust option `-Z instrument-coverage` uses LLVM Coverage Mapping Format version 4,
# which requires LLVM 11 or greater.
LLVM_VERSION_11_PLUS := $(shell \
LLVM_VERSION=$$("$(LLVM_BIN_DIR)"/llvm-config --version) && \
LLVM_VERSION_MAJOR=$${LLVM_VERSION/.*/} && \
[ $$LLVM_VERSION_MAJOR -ge 11 ] && echo true || echo false)

0 comments on commit 9e5d58f

Please sign in to comment.