Conversation
see #20992 Co-authored-by: Jacob Young <jacobly0@users.noreply.github.com>
It's not actually useful after all.
instead of relying on the LLVM sancov pass. The LLVM pass is still executed if trace_pc_guard is requested, disabled otherwise. The LLVM backend emits the instrumentation directly. It uses `__sancov_pcs1` symbol name instead of `__sancov_pcs` because each element is 1 usize instead of 2. AIR: add CoveragePoint to branch hints which indicates whether those branches are interesting for code coverage purposes. Update libfuzzer to use the new instrumentation. It's simplified since we no longer need the constructor and the pcs are now in a continguous list. This is a regression in the fuzzing functionality because the instrumentation for comparisons is no longer emitted, resulting in worse fuzzer inputs generated. A future commit will add that instrumentation back.
It's useful to have TraceCmp based on the results of LLVM optimizations, while the code coverage bits were emitted by Zig manually, allowing more careful correlation to points of interest in the source code. This re-enables the sancov pass in `-ffuzz` mode, but only TraceCmp. Notably, IndirectCalls is off, which needs to be implemented manually in the LLVM backend, and StackDepth remains off, because it is not used by libfuzzer or AFL either. If stack depth is re-introduced, it can be done with better performance characteristics by being function call graph aware, and only lowered in call graph cycles, where its heuristic properties come in useful. Fixes the fuzzing regression.
This matches what LLVM's sancov pass does and is required so that optimization passes do not delete the instrumentation. However, this is currently triggering an error: "members of llvm.compiler.used must be named" so the next commit will add names to those globals.
because it marks the linker section, preventing garbage collection. Also, name the members because that is required by this intrinsic. Also, enable the StackDepth option in the sancov pass as a workaround for llvm/llvm-project#106464, otherwise, LLVM enables TracePCGuard even though we explicitly disable it.
* the pcs list is unsorted * use the function address Fixes entry points in ReleaseSafe mode.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #21075.
Closes #20992 by moving code coverage instrumentation to the LLVM backend explicitly, leaving LLVM's sancov pass to only do TraceCmp.
cc @kristoff-it - implications for https://github.com/kristoff-it/zig-afl-kit/ are that you can remove these:
Although leaving them in is harmless. And meanwhile you should see a speedup in iterations per second since it no longer has redundant instrumentation. Unfortunately you have to leave
__sancov_lowest_stackfor now due to llvm/llvm-project#106464.Before:
This is after it reaches the panic line:
After
The extra green dots on the panic line are the "else" clauses from each if statement. This is tracked by #20989 and will be solved by making empty else blocks not emit points of interest.
The final red dot is because when the input is found that triggers the panic, the process crashes, including libfuzzer, including the test runner, so the coverage is not reported. Ideally, any block that is terminated with a
@panicwould also be excluded from coverage for the same reason as auto-generated safety checks.