-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Fix for rust and swift coverages #6517
Conversation
@@ -99,7 +99,12 @@ function run_fuzz_target { | |||
return 0 | |||
fi | |||
|
|||
llvm-profdata merge -j=1 -sparse $profraw_file_mask -o $profdata_file | |||
llvm-profdata merge -j=1 -sparse $profraw_file_mask -o $profdata_file || ( | |||
# on error, try to first run llvm-cov-rel to fix profraw file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is pretty hacky to me. Why can't you just do the latter when using rust or swift?
Or would doing the latter for every language work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please begin with capital and end with period.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, or we can do it in every case, as it leaves clang-14 profraw files identical
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this. This is better than #6449 but I think could still be improved.
infra/base-images/base-builder/cargo
Outdated
@@ -35,7 +35,11 @@ then | |||
# go into fuzz directory if not already the case | |||
cd fuzz || true | |||
fuzz_src_abspath=`pwd` | |||
export RUSTFLAGS="$RUSTFLAGS --remap-path-prefix fuzz_targets=$fuzz_src_abspath/fuzz_targets" | |||
# default directory is fuzz_targets, but some projects like image-rs use fuzzers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please begin with capital and end with period.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, done
@@ -0,0 +1,180 @@ | |||
package main |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the future I would prefer code for OSS-Fuzz be written in python.
I still need to figure out how to remove the go toolchain from the runner image because we didn't stick to this policy. All of the other code in OSS-Fuzz is python or bash (also an unfortunate choice), lets try to do python only from now on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, do you want me to rewrite this in python ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not necessary for this PR, but in future, I'd prefer it if we had as little go code as possible yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR LGTM, but if it won't take to long, could you please convert this script to Python?
Thanks!
infra/base-images/base-builder/cargo
Outdated
# default directory is fuzz_targets, but some projects like image-rs use fuzzers | ||
while read i; do | ||
export RUSTFLAGS="$RUSTFLAGS --remap-path-prefix $i=$fuzz_src_abspath/$i" | ||
# bash while syntax so that we modify RUSTFLAGS in main shell instead of a subshell |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please begin with capital and end with period.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -99,7 +99,12 @@ function run_fuzz_target { | |||
return 0 | |||
fi | |||
|
|||
llvm-profdata merge -j=1 -sparse $profraw_file_mask -o $profdata_file | |||
llvm-profdata merge -j=1 -sparse $profraw_file_mask -o $profdata_file || ( | |||
# on error, try to first run llvm-cov-rel to fix profraw file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please begin with capital and end with period.
} | ||
|
||
relativizeAddress(data, offset+16, dataref, sectPrfCnts, sectPrfData) | ||
// cf CountersDelta -= sizeof(*SrcData); in __llvm_profile_merge_from_buffer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not delete this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment explains the addition afterwards
log.Fatalf("needs exactly three arguments : binary, profraw, output") | ||
} | ||
|
||
// first find llvm profile sections addresses in the elf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please begin with capital and end with period.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
sectPrfCnts = f.Sections[i].Addr | ||
} else if f.Sections[i].Name == "__llvm_prf_data" { | ||
sectPrfData = f.Sections[i].Addr | ||
// maybe rather sectPrfCntsEnd as f.Sections[i].Addr + f.Sections[i].Size for __llvm_prf_cnts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please begin with capital and end with period.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
log.Fatalf("Elf has not __llvm_prf_cnts and __llvm_prf_data sections") | ||
} | ||
|
||
// process profraw file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please begin with capital and end with period.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
log.Fatalf("failed to process file: %v", err) | ||
} | ||
|
||
// write output file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please begin with capital and end with period.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
1d17253
to
ad39e43
Compare
Cf google#6268 Latest clang-14 and clang-13 used by rust or swift have a slightly different profraw file format llvm-cov-rel is tool that will update the profraw file produced by clang-13 to one readable by clang-14 llvm-cov tools
So that projects not using default fuzz_targets subdir get the good remap, and hence the good coverage report
ad39e43
to
5d27ce5
Compare
@jonathanmetzman I pushed a new version with the right syntax for comments |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -0,0 +1,180 @@ | |||
package main |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR LGTM, but if it won't take to long, could you please convert this script to Python?
Thanks!
Looks like base-runner dit not get updated yet so today's coverage builds still fail I will wait to see how it goes before trying python |
Still no updated version of base-runner ... |
ping @jonathanmetzman why do the docker images not updated ?
|
Getting some fixes :-) But none of the pure rust projects are working yet... because of a new change in LLVM 14 cf #6551 |
Fixes #6268
Replaces #6449
Introduces lvm-cov-rel, a tool to convert profraw files from version 5 and 7/clang-13 to version 7/clang-14
cc @jonathanmetzman @inferno-chromium
Tested with
with projects suricata, image-rs and swift-nio