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.

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

Jump to bottom

test: symbolizer improvements #28814

Merged
merged 1 commit into from
Nov 8, 2023
Merged

test: symbolizer improvements #28814

merged 1 commit into from
Nov 8, 2023

Conversation

fanquake
Copy link
Member

@fanquake fanquake commented Nov 7, 2023 •
edited

It's not completely clear to me why this needs to be explicitly specified in some environments, and not in others, while at the same time that llvm-symbolizer is already in PATH, but this has fixed the 2 issues outlined in #28147.

Use LLVM_SYMBOLIZER_PATH as the env var, as that is somewhat also used inside LLVM, but not consistently, i.e it's checked for in the asan_symbolize script, but not in in the ubsan_symbolize script, or from in compiler-rt.

Alternative to #28804.

@DrahtBot
Copy link
Contributor

DrahtBot commented Nov 7, 2023 •
edited

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK maflcko

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

maflcko
maflcko approved these changes Nov 7, 2023
View reviewed changes
Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

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

nice, lgtm

test/fuzz/test_runner.py Outdated
@@ -16,11 +16,14 @@


def get_fuzz_env(*, target, source_dir):
symbolizer_path = "/usr/bin/llvm-symbolizer"
Copy link
Member

Choose a reason for hiding this comment

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

Is this true on all platforms? If not, what would happen?

Copy link
Member

Choose a reason for hiding this comment

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

Looks like the CI provides the answer already:

==18180==WARNING: invalid path to external symbolizer!
==18180==WARNING: Failed to use and restart external symbolizer!
test/fuzz/FuzzedDataProvider.h:212:47: runtime error: unsigned integer overflow: 9223372036854775807 - 9223372036854775808 cannot be represented in type 'uint64_t' (aka 'unsigned long')
    #0 0x556fc34a1b1b  (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1a12b1b) (BuildId: 3ec865ec03abc64911598eb6d119635991ba8624)
    #1 0x556fc3512d02  (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1a83d02) (BuildId: 3ec865ec03abc64911598eb6d119635991ba8624)
    #2 0x556fc3b52a8c  (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x20c3a8c) (BuildId: 3ec865ec03abc64911598eb6d119635991ba8624)
    #3 0x556fc33a6024  (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1917024) (BuildId: 3ec865ec03abc64911598eb6d119635991ba8624)
    #4 0x556fc33a7221  (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1918221) (BuildId: 3ec865ec03abc64911598eb6d119635991ba8624)
    #5 0x556fc33a7827  (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1918827) (BuildId: 3ec865ec03abc64911598eb6d119635991ba8624)
    #6 0x556fc3394f7b  (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1905f7b) (BuildId: 3ec865ec03abc64911598eb6d119635991ba8624)
    #7 0x556fc33bf4e6  (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x19304e6) (BuildId: 3ec865ec03abc64911598eb6d119635991ba8624)
    #8 0x7fe22802e0cf  (/lib/x86_64-linux-gnu/libc.so.6+0x280cf) (BuildId: 96ab1a8f3b2c9a2ed37c7388615e6a726d037e89)
    #9 0x7fe22802e188  (/lib/x86_64-linux-gnu/libc.so.6+0x28188) (BuildId: 96ab1a8f3b2c9a2ed37c7388615e6a726d037e89)
    #10 0x556fc3389e44  (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x18fae44) (BuildId: 3ec865ec03abc64911598eb6d119635991ba8624)

SUMMARY: UndefinedBehaviorSanitizer: unsigned-integer-overflow test/fuzz/FuzzedDataProvider.h:212:47 in 
MS: 0 ; base unit: 0000000000000000000000000000000000000000


artifact_prefix='./'; Test unit written to ./crash-da39a3ee5e6b4b0d3255bfef95601890afd80709
Base64:

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea, the worst that can happen is the problem we already have. Let's see if we can make this more generic.

Copy link
Member Author

@fanquake fanquake Nov 7, 2023 •
edited

Choose a reason for hiding this comment

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

I guess there are also other issues here, as the ASAN job does find the symbolizer, but not at that path:

AddressSanitizer: reading suppressions file at /ci_container_base/test/sanitizer_suppressions/ubsan
==28779==Using llvm-symbolizer found at: /usr/bin/llvm-symbolizer-17
==28779==AddressSanitizer Init done
Error parsing command line arguments: ==28779==__tls_get_addr: DTLS_Find 0x7f7962720770 2

Copy link
Member

Choose a reason for hiding this comment

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

Yes, the task does not have llvm installed, but only llvm-17

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea, my question is why is it now failing, if it's successfully finding a symbolizer.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, it looks like the verbosity=2 output is breaking things that are parsing output:

/usr/bin/python3.11 ../test/util/test_runner.py
2023-11-07 15:13:31,154 - ERROR - Error mismatch:
Expected: Error parsing command line arguments: Invalid command 'foo'

and the ASAN job here would otherwise be working as expected.

@fanquake fanquake force-pushed the symbolizer_improvements branch 2 times, most recently from 00b78a2 to 87d6d69 Compare November 7, 2023 16:54
@fanquake
Copy link
Member Author

fanquake commented Nov 7, 2023

Changed the approach slightly, which should fix the failing job. This should continue to work everywhere. Might also send a change to LLVM, to try and consolidate the LLVM_SYMBOLIZER_PATH usage.

@fanquake
fuzz: explicitly specify llvm-symbolizer path in runner
49d9532 
It's not completely clear to me why this needs to be explicitly
specified in some environments, and not in others, while at the same time
that `llvm-symbolizer` is already in PATH, but this has fixed the 2 issues
outlined in bitcoin#28147.

Use `LLVM_SYMBOLIZER_PATH` as the env var, as that is somewhat also used
inside LLVM, but not consistently, i.e it's checked for in the asan_symbolize
script, but not in in the ubsan_symbolize script, or from in compiler-rt.
@DrahtBot DrahtBot removed the CI failed label Nov 7, 2023
@maflcko
Copy link
Member

maflcko commented Nov 7, 2023

lgtm ACK 49d9532

@maflcko
Copy link
Member

maflcko commented Nov 8, 2023

Missing test prefix in pull title?

@fanquake fanquake changed the title sanitizer: symbolizer improvements test: symbolizer improvements Nov 8, 2023
@DrahtBot DrahtBot added the Tests label Nov 8, 2023
@fanquake fanquake merged commit b5d8f00 into bitcoin:master Nov 8, 2023
16 checks passed
@fanquake fanquake deleted the symbolizer_improvements branch November 8, 2023 09:47
@fanquake fanquake mentioned this pull request Nov 13, 2023
Merged
fanquake added a commit that referenced this pull request Nov 15, 2023
@fanquake
Merge #28865: test: migrate to some per-symbol ubsan suppressions
a73715e 
fd30e96 test: migrate to some per-symbol ubsan suppressions (fanquake)

Pull request description:

  Now that the symbolizer should be hanging around (#28814), migrate some file-wide suppressions to be symbol specific. Should assist in catching new issues that may otherwise go unnoticed due to file-wide suppression.

  Only tested (so far) on aarch64 using the native ASAN & FUZZ CI.

ACKs for top commit:
  maflcko:
    lgtm ACK fd30e96
  dergoegge:
    utACK fd30e96 (if CI is green)

Tree-SHA512: fbc44464d22813969dd4d1cdeab00042fa45f0af9bf1aed4fd3b688dc7b3c377a7c0f5f0c0a37ba65b649cfb5c7ff8ab2774500fe182d702c4340ca19f08479f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maflcko maflcko maflcko approved these changes

Assignees
No one assigned
Labels
Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@fanquake @DrahtBot @maflcko