Skip to content
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.

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

Azure & Shippable: Upgrade LDC-LLVM to v9.0.0 #3166

Merged
merged 11 commits into from Sep 25, 2019
Merged

Conversation

kinke
Copy link
Member

@kinke kinke commented Sep 20, 2019

No description provided.

@kinke
Copy link
Member Author

kinke commented Sep 21, 2019

@Hardcode84: jit-rt hits deprecations such as:

jit_context.cpp:109:7: warning: 'LegacyRTDyldObjectLinkingLayer' is deprecated: ORCv1 layers (layers with the 'Legacy' prefix) are deprecated. Please use ORCv2 (see docs/ORCv2.rst)

@kinke kinke force-pushed the llvm9 branch 2 times, most recently from 1caf0b3 to 8a516a3 Compare September 21, 2019 12:32
@Hardcode84
Copy link
Contributor

jit-rt hits deprecations such as

ok, I'll check it after merge, thanks

@kinke kinke force-pushed the llvm9 branch 2 times, most recently from 3ca8ce9 to 6a7267d Compare September 21, 2019 16:17
@kinke
Copy link
Member Author

kinke commented Sep 21, 2019

@Hardcode84: Thanks - there's a transitioning guide in the release notes.

The PGO counter linkage changes on Windows probably render our LLVM workaround obsolete - will check. Edit: Yes, workaround removed.

@kinke
Copy link
Member Author

kinke commented Sep 21, 2019

Okay, only remaining bullet point is how to handle libfuzzer's dropped support for trace-pc-guard (see https://reviews.llvm.org/rL352564).

@JohanEngelen
Copy link
Member

Okay, only remaining bullet point is how to handle libfuzzer's dropped support for trace-pc-guard (see https://reviews.llvm.org/rL352564).

Let's not stress about this. We can simply drop it as well if Clang dropped support.

@kinke
Copy link
Member Author

kinke commented Sep 22, 2019

I already dropped it (as libFuzzer would just terminate the program after printing an error). The question was more wrt. how to adapt the tests - duplicate the 4 failing ones without trace-pc-guard stuff for LLVM 9+ or just remove the related checks for all LLVM versions?

@JohanEngelen
Copy link
Member

JohanEngelen commented Sep 22, 2019

For the trace-pc-guard stuff, this is an important Clang commit https://www.mail-archive.com/cfe-commits@lists.llvm.org/msg67181.html
So we can drop testing -fsanitize-coverage=trace-pc-guard, and change the code for -fsanitize=fuzzer to mean sancovOpts.Inline8bitCounters = true; instead of sancovOpts.TracePCGuard = true; (and add sancovOpts.PCTable =true; too).

Edit: I've used the github interface to modify the code, hope it works.
Edit2: I now realize that my code change is going to break a bunch of tests, because they all test for calls to _sanitizer_cov_trace_pc_guard. I think that is (no longer?) necessary, and fuzzing should work without it. So if we just remove all the filecheck lines checking those calls, the rest of fuzz functionality should be preserved.

@JohanEngelen
Copy link
Member

Tested the fuzz changes locally, and fuzzing seems to work as before.

@JohanEngelen JohanEngelen mentioned this pull request Sep 23, 2019
@kinke
Copy link
Member Author

kinke commented Sep 23, 2019

Thx Johan!

@JohanEngelen
Copy link
Member

Ah you already fixed the test :)
I want to add a little bit of extra testing. I'm having trouble pushing to this PR, I thought one can now push to other people's branches if it is a PR?

@kinke
Copy link
Member Author

kinke commented Sep 24, 2019

Yes, pushing should work, the only time I wasn't able to was when Stefanos used his default branch as PR branch.

@kinke
Copy link
Member Author

kinke commented Sep 24, 2019

sanitizers/link_fuzzer.d apparently fails to fail with -link-no-cpp with LLVM 9 and shared druntime/Phobos (new Travis job)...

@JohanEngelen
Copy link
Member

sanitizers/link_fuzzer.d apparently fails to fail with -link-no-cpp with LLVM 9 and shared druntime/Phobos (new Travis job)...

Hmm, so that simply means that c++ libs are no longer necessary to fuzz?

@kinke
Copy link
Member Author

kinke commented Sep 24, 2019

With static druntime/Phobos (Azure: building both shared/static, lit-tests only testing default static libs), the C++ lib is apparently still required, for both Linux and Mac...

@JohanEngelen
Copy link
Member

I think the test is meant to only test that -fsanitize=fuzzer -link-no-cpp means no C++ libs (i.e. that you can override what -fsanitize=fuzzer is adding to linker line). We need something like a dry-run -v output, without actually invoking the linker.

Including some of Nicholas' fixes in ldc-developers#3144.
The linkage for the counters changed from `internal` to `linkonce_odr
 dso_local`, and the previous regex didn't allow underscores.
The C++ lib apparently isn't required with shared druntime/Phobos.
@kinke kinke merged commit 655dca4 into ldc-developers:master Sep 25, 2019
@kinke kinke deleted the llvm9 branch September 25, 2019 22:26
@kinke
Copy link
Member Author

kinke commented Sep 25, 2019

Johan, you're welcome to follow-up with additional tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants