Skip to content

Factor out a general purpose predicate script from the csmith driver #1068

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
Merged
merged 3 commits into from
Oct 10, 2017
Merged

Factor out a general purpose predicate script from the csmith driver #1068

merged 3 commits into from
Oct 10, 2017

Conversation

fitzgen
Copy link
Member

@fitzgen fitzgen commented Oct 10, 2017

This allows it to be used by both fuzzers and when using creduce. See each commit for details.

r? @pepyakin

pepyakin
pepyakin reviewed Oct 10, 2017
View reviewed changes
Copy link
Contributor

@pepyakin pepyakin left a comment

Choose a reason for hiding this comment

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

Looks great!

But there is one issue. Docs state that you can run

path/to/rust-bindgen/csmith-fuzzing/predicate.py

but actually you can't. See my comment on predicate.py

csmith-fuzzing/predicate.py Outdated

def main():
args = parser.parse_args()
os.environ["RUST_BACKTRACE"] = "1"
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be cooler to specify RUST_BACKTRACE=full?

csmith-fuzzing/predicate.py Outdated

def run_bindgen(args, bindings):
child = run(
["cargo", "run", "--", args.input, "-o", bindings] + shlex.split(args.bindgen_args),
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

csmith-fuzzing/predicate.py Outdated
def run_bindgen(args, bindings):
child = run(
["cargo", "run", "--", args.input, "-o", bindings] + shlex.split(args.bindgen_args),
cwd=os.path.abspath(os.path.dirname(sys.argv[0])),
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't quite get why do we need this.

If I run this script like this:

~/rust-bindgen $ ./csmith-fuzzing/predicate.py csmith-fuzzing/csmith.h

It wouldn't find csmith.h, because real path would be ./csmith-fuzzing/csmith-fuzzing/csmith.h.

If we would run:

~/rust-bindgen/csmith-fuzzing $ ./predicate.py csmith.h

we would be already in csmith-fuzzing.

Maybe it would be better to drop cwd and do:

cargo run --manifest-path $(basedir)/../Cargo.toml ...

?

Copy link
Member Author

Choose a reason for hiding this comment

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

Great catch!

csmith-fuzzing/driver.py Outdated
output.name,
"-o", test.name,
])
BINDGEN_ARGS = "--with-derive-partialeq --with-derive-eq --with-derive-partialord --with-derive-ord --with-derive-hash --with-derive-default"
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't this be more legible?

BINDGEN_ARGS = "--with-derive-partialeq \
    --with-derive-eq \
    --with-derive-partialord \
    --with-derive-ord \
    --with-derive-hash \
    --with-derive-default"

As a little bonus: more clear diffs

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes :)

CONTRIBUTING.md Outdated
# Invoke the general purpose predicate script that comes in the
# `bindgen` repository.
path/to/rust-bindgen/csmith-fuzzing/predicate.py \
$PREDICATE_FLAGS \
Copy link
Contributor

Choose a reason for hiding this comment

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

There are no $PREDICATE_FLAGS and I think it would be nice if we would provide default one, or at least placeholder.

CONTRIBUTING.md

... | grep "pub _base: MyInvalidBaseTypeThatShouldntBeHere"
And you can always write your own, arbitrary predicate script if you prefer.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should provide a link to the old hand-written predicate script example?
I think it may be very helpful for those who stumbled on lack of predicate.py functionality. Before extending predicate.py one might want to do experiments which may be more productive with bare shell scripts.

CONTRIBUTING.md
# Make sure that the reduced test case is valid C/C++ by compiling it. If it
# isn't valid C/C++, this command will exit with a nonzero exit code and cause
# the whole script to do the same.
clang[++ --std=c++14] -c ./pre_processed_header.hpp
Copy link
Contributor

Choose a reason for hiding this comment

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

predicate.py doesn't do this anymore and predicate.sh doesn't contain this either.
Is it useful?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ultimately, I don't think so.

@fitzgen
Add a predicate script for determining whether bindgen can process …
cfb7e7d 
…a given header

First, `bindgen` is run on the input header. Then the emitted bindings are
compiled with `rustc`. Finally, the compiled bindings' layout tests are run.

By default, this script will exit zero if all of the above steps are successful,
and non-zero if any of them fail. This is appropriate for determining if some
test case (perhaps generated with `csmith` or another fuzzer) uncovers any bugs
in `bindgen`.

However, this script can also be used when reducing (perhaps with `creduce`) a
known-bad test case into a new, smaller test case that exhibits the same bad
behavior. In this mode, you might expect that the emitted bindings fail to
compile with `rustc`, and want to exit non-zero early if that is not the
case. See the "reducing arguments" section for details and what knobs are
available.
@fitzgen
Make the csmith driver use predicate.py instead of its own half r…
b70eb51 
…eimplementation
@fitzgen
Recommend using predicate.py with creduce in CONTRIBUTING.md
349c1f9 
It is a lot more streamlined than writing a big nasty ol' bash script.
@pepyakin
Copy link
Contributor

Nice!

@pepyakin
Copy link
Contributor

@bors-servo r+

@bors-servo
Copy link

📌 Commit 349c1f9 has been approved by pepyakin

@bors-servo
Copy link

⌛ Testing commit 349c1f9 with merge ad6a73f...

bors-servo pushed a commit that referenced this pull request Oct 10, 2017
Auto merge of #1068 - fitzgen:predicate-script, r=pepyakin
ad6a73f 
Factor out a general purpose predicate script from the `csmith` driver

This allows it to be used by both fuzzers and when using `creduce`. See each commit for details.

r? @pepyakin
@bors-servo
Copy link

💔 Test failed - status-travis

@pepyakin
Copy link
Contributor

Looks like spurious, i've restarted the job

@bors-servo
Copy link

☀️ Test successful - status-travis
Approved by: pepyakin
Pushing ad6a73f to master...

@bors-servo bors-servo merged commit 349c1f9 into rust-lang:master Oct 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pepyakin pepyakin pepyakin left review comments

Assignees

@pepyakin pepyakin

Labels
S-tests-failed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@fitzgen @pepyakin @bors-servo @highfive