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

Update fuzzing seed-corpus generation #256

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open

Update fuzzing seed-corpus generation #256

wants to merge 4 commits into from

Conversation

mborland
Copy link
Member

Thanks to @Flamefire for providing this fix. Right now we have one file with many lines, but this breaks it into many files with one line each to better seed the fuzzer.

@Flamefire
Copy link
Contributor

Ping @grafikrobot in case he has any feedback on the Jam part. This looks quite involved for just running a Python script in Jam but I didn't found anything better.

fuzzing/Jamfile Outdated

# Create the output corpus directories
make /tmp/corpus/$(fuzzer) : : common.MkDir ;
make /tmp/mincorpus/$(fuzzer) : : common.MkDir ;
make $(corpus) : $(seed_files) : make-corpus ;
Copy link
Member

Choose a reason for hiding this comment

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

I like adding a dependency to the script make rules run. As it reruns the targets if it's only the script that changes.

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'm not sure what you mean by this, or how to implement the suggestion

Copy link
Contributor

Choose a reason for hiding this comment

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

Would that be this?

Suggested change
make $(corpus) : $(seed_files) : make-corpus ;
make $(corpus) : $(seed_files) : make-corpus <dependency> $(.make-corpus-script) ;

BTW: Does it automatically add a dependency on the input files?

Copy link
Member

Choose a reason for hiding this comment

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

@Flamefire yes that would be it. And yes, the make rule adds dependencies for inputs (all B2 rules do).

Copy link
Member

Choose a reason for hiding this comment

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

Oops.. There should be an additional : there.. make-corpus : <dependency> $(.make-corpus-script)

Copy link
Member Author

Choose a reason for hiding this comment

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

This is giving the error

error: '/home/runner/work/charconv/boost-root/libs/charconv/fuzzing/make-corpus.py' is not a valid property specification

https://github.com/boostorg/charconv/actions/runs/12792451062/job/35662937747?pr=256#step:7:44

Copy link
Member

Choose a reason for hiding this comment

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

There shouldn't be a space in the dependency. It should be <dependency>$(.make-corpus-script).

fuzzing/Jamfile Show resolved Hide resolved
fuzzing/Jamfile Outdated Show resolved Hide resolved
Copy link

codecov bot commented Jan 15, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.85%. Comparing base (dacb62a) to head (9ac6e57).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #256   +/-   ##
========================================
  Coverage    94.85%   94.85%           
========================================
  Files           69       69           
  Lines         9077     9077           
========================================
  Hits          8610     8610           
  Misses         467      467           

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dacb62a...9ac6e57. Read the comment docs.

@mborland
Copy link
Member Author

@Flamefire have you seen these directory not found errors in Locale? They seem sporadic

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.

3 participants