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

Allow stripping of dead code #260

Merged
merged 1 commit into from
Apr 16, 2021

Conversation

paholg
Copy link

@paholg paholg commented Apr 16, 2021

The -Clink-dead-code flag was added to fix an error with certain
targets.

However, it can also cause problems.

Example: You depend on crate A which links to library B. A includes an
unused reference to B::foo. If the specific version of B we have is
missing foo, we now get a linker error whereas we would not if
stripping dead code.

Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

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

Thanks! Can you add a small test that passes the --strip-dead-code flag to a simple "hello world" style fuzz target just so that we kick the tires on this a bit and hopefully catch any future regressions during, say, a refactoring that break this flag?

src/options.rs Outdated

/// Dead code is linked by default to prevent a potential error with some
/// optimized targets. This flag allows you to opt out of it.
#[structopt(long = "strip-dead-code")]
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: structopt should infer this long name, so this line shouldn't be necessary here.

Copy link
Author

Choose a reason for hiding this comment

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

I am not clear why, but without specifying the name, the run_with_crash test fails, but only when it's run as part of the full test suite.

Copy link
Author

@paholg paholg Apr 16, 2021

Choose a reason for hiding this comment

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

Ah, I figured out the issue I was running into. Because the test ids (and therefore directories) are non-deterministic, on repeated runs, I the run_*_with_crash tests can end up looking at the wrong output.

Running cargo clean between tests is a workaround, but I'll open another PR to make the project directories deterministic, as the failures are very confusing.

Edit: Nevermind, making the directories deterministic is not enough. I'm not sure the actual issue, but it seems related to tests getting the wrong output, e.g. run_with_crash can fail, and shows output that appears to be from run_without_sanitizer_with_crash. Limiting test-threads to 1 causes the failures to go away.

The `-Clink-dead-code` flag was added to fix an error with certain
targets.

However, it can also cause problems.

Example: You depend on crate A which links to library B. A includes an
unused reference to `B::foo`. If the specific version of B we have is
missing `foo`, we now get a linker error whereas we would not if
stripping dead code.
@paholg paholg force-pushed the allow-not-linking-dead-code branch from 52878e5 to d3d6253 Compare April 16, 2021 18:36
Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

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

Thanks!

@fitzgen fitzgen merged commit 8c7db6a into rust-fuzz:master Apr 16, 2021
@paholg
Copy link
Author

paholg commented Apr 16, 2021

@fitzgen Thanks for the quick review / merge!

If it's not too much trouble, would you mind doing a version bump / publish soon?

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.

2 participants