Skip to content

Attempt to fix intermittent failures of pgo-branch-weights test. #67829

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 4, 2020
Merged

Attempt to fix intermittent failures of pgo-branch-weights test. #67829

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 19 additions & 11 deletions src/test/run-make-fulldeps/pgo-branch-weights/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,21 +15,29 @@ ifdef IS_MSVC
COMMON_FLAGS=-Cpanic=abort
endif

# For some very small programs GNU ld seems to not properly handle
# instrumentation sections correctly. Neither Gold nor LLD have that problem.
ifeq ($(UNAME),Linux)
Copy link
Contributor

@mati865 mati865 Jan 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Windows-gnu CI jobs will still use GNU LD version 2.27, is it recent/old enough to not have this bug?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is an ELF specific issue.

ifneq (,$(findstring x86,$(TARGET)))
COMMON_FLAGS=-Clink-args=-fuse-ld=gold
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I think we're almost certain to have lld around (and can fairly quickly build it if needed, since we have caching in place and such) but I'm less sure about gold on CI. Maybe it makes sense to use lld here? If that doesn't work well locally and this passes CI though I'm fine with it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought that Gold is part of any standard binutils distribution?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we'll find out :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, that's quite possible :) I had thought it would be more special (otherwise, why is ld the default?) but I guess there are edge cases and breaking the world is not an option for distros and such.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought that Gold is part of any standard binutils distribution?

This is true on most of Linux distributions but there are popular distros like openSUSE where gold is split to binutils-gold package and not installed by default AFAIK. This should be at least mentioned in the README so users know about this requirement.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it would be bad if this made run-make tests fail locally for some people. @Mark-Simulacrum, do we have a good way of using our own rust-lld in run-make tests?

(otherwise, why is ld the default?)

Gold can only handle ELF, ld can handle at lot more, which is why, I'd guess.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We add lld to the PATH of run-make tests if it's enabled (i.e., lld_enabled is set in config.toml). I imagine we could expose that similarly to # needs-matching-clang or so and ignore the test if lld is not built, and then make sure we're building lld on CI (we already do this on the test-various builer, I presume for WASM?). That should be a matter of adding --set rust.lld to RUST_CONFIGURE_ARGS, I think.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the info! I'll look into that in a follow-up PR if that's OK with you.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fine with me, yes. I don't know that it's even necessary (maybe worth waiting until someone complains about the gold solution).

endif
endif


all:
# We don't compile `opaque` with either optimizations or instrumentation.
# We don't compile `opaque` with either optimizations or instrumentation.
$(RUSTC) $(COMMON_FLAGS) opaque.rs
$(RUSTC) $(COMMON_FLAGS) opaque.rs || exit 1
# Compile the test program with instrumentation
mkdir -p "$(TMPDIR)"/prof_data_dir
mkdir -p "$(TMPDIR)/prof_data_dir" || exit 1
$(RUSTC) $(COMMON_FLAGS) interesting.rs \
-Cprofile-generate="$(TMPDIR)"/prof_data_dir -O -Ccodegen-units=1
$(RUSTC) $(COMMON_FLAGS) main.rs -Cprofile-generate="$(TMPDIR)"/prof_data_dir -O
-Cprofile-generate="$(TMPDIR)/prof_data_dir" -O -Ccodegen-units=1 || exit 1
$(RUSTC) $(COMMON_FLAGS) main.rs -Cprofile-generate="$(TMPDIR)/prof_data_dir" -O || exit 1
# The argument below generates to the expected branch weights
$(call RUN,main aaaaaaaaaaaa2bbbbbbbbbbbb2bbbbbbbbbbbbbbbbcc) || exit 1
"$(LLVM_BIN_DIR)"/llvm-profdata merge \
-o "$(TMPDIR)"/prof_data_dir/merged.profdata \
"$(TMPDIR)"/prof_data_dir
"$(LLVM_BIN_DIR)/llvm-profdata" merge \
-o "$(TMPDIR)/prof_data_dir/merged.profdata" \
"$(TMPDIR)/prof_data_dir" || exit 1
$(RUSTC) $(COMMON_FLAGS) interesting.rs \
-Cprofile-use="$(TMPDIR)"/prof_data_dir/merged.profdata -O \
-Ccodegen-units=1 --emit=llvm-ir
cat "$(TMPDIR)"/interesting.ll | "$(LLVM_FILECHECK)" filecheck-patterns.txt
-Cprofile-use="$(TMPDIR)/prof_data_dir/merged.profdata" -O \
-Ccodegen-units=1 --emit=llvm-ir || exit 1
cat "$(TMPDIR)/interesting.ll" | "$(LLVM_FILECHECK)" filecheck-patterns.txt
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should move these commands to a shell script and then use set -euo pipefail instead of this mess of exits?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not nice, but if we move this to a script then the error in the log will probably just be bash the_script.sh failed, right?