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

Add --regress=regression-type command line option. #53

Merged
merged 2 commits into from
Feb 21, 2020

Conversation

pnkfelix
Copy link
Member

@pnkfelix pnkfelix commented Feb 20, 2020

Added --regress=ice flag to resolve #34.

Also added some other variants, to resolve #28.

Here are all the variants provided:

  • --regress=error is the default: an error status (program rejection or ICE) is a regression.

  • --regress=success is probably what you want for feature request: Invert status #28, to see where a fix was injected.

  • --regress=ice is what you want if you're looking for where an ICE was injected.

  • --regress=non-error is what you want if you know the code should be a static error, but the bug are causing either ICEs or invalid acceptances to show up, and you want to identify the first occurrence of either. (Sounds like a pretty niche case, I admit.)


One potentially subtle point: In order to post-process the error output, I had to capture it. The code prior to this PR never captured stdout/stderr; it either directed it to /dev/null, or inherited its parent's IO descriptors.

I didn't see a convenient way to get a tee effect (i.e. the ideal thing would be to both capture the output but also emit it, without internally buffering and re-emitting it (*)). Since I didn't see a way to do tee, I did the simple dumb thing: If we need to capture the output, then we do so. But if we do that in a context where the code was previously inheriting the descriptors, then we also re-emit the output after it runs.

(The main consequence of this, I think, is that you will lose the timing information of how the output to stdout and stderr was originally interleaved.)

(*): then again, maybe tee buffers as well, at least by default? What do i know...

…e based on more subtle predicate than just `exit_status.success()`.
Also added some other variants, to resolve rust-lang#28.

Just to spell it out:

 * --regress=error is the default: an error status (program rejection or
   ICE) is a regression.

 * --regress=success is probably what you want for rust-lang#28, to see where a
   fix was injected.

 * --regress=ice is what you want if you're looking for where an ICE was
   injected.

 * --regress=non-error is what you want if you know the code should be a static
   error, but the bug is causing either ICEs or invalid successes to show up.
@pnkfelix
Copy link
Member Author

pnkfelix commented Feb 20, 2020

I didn't see a convenient way to get a tee effect

Hmm, then again, cargo-bisect-rustc seems to already import a crate named tee ... investigating...

  • (well, I don't know if that's going to resolve my stdio issue out of the box...)

@spastorino
Copy link
Member

spastorino commented Feb 21, 2020

@pnkfelix this is great :).

@spastorino spastorino merged commit 7ac0397 into rust-lang:master Feb 21, 2020
@chrissimpkins
Copy link
Member

chrissimpkins commented Feb 21, 2020

I didn't see a convenient way to get a tee effect (i.e. the ideal thing would be to both capture the output but also emit it, without internally buffering and re-emitting it (*)). Since I didn't see a way to do tee, I did the simple dumb thing: If we need to capture the output, then we do so. But if we do that in a context where the code was previously inheriting the descriptors, then we also re-emit the output after it runs.

(The main consequence of this, I think, is that you will lose the timing information of how the output to stdout and stderr was originally interleaved.)

Perhaps something along these lines to maintain the interleaving?: https://github.com/oconnor663/os_pipe.rs/blob/b34383b222680b2aa4550fc1f1b89a8733d70e5c/src/lib.rs#L37-L83

@chrissimpkins
Copy link
Member

I built these changes and ran a few --regress=ice tests against known ICE date ranges today. This works great!

Now that stderr is captured and available, it might be helpful to expand this approach to support a command line defined substring for the stderr text search term. It looks like this should be reasonably straightforward with something along the lines of

let saw_ice = || -> bool {
            stderr_utf8.contains("error: internal compiler error")
        };

that you used for the ICE testing here.

@pnkfelix
Copy link
Member Author

pnkfelix commented Feb 24, 2020

it might be helpful to expand this approach to support a command line defined substring for the stderr text search term.

I could imagine doing this via further extension of --regress, such as --regress=error-pattern=<string-literal-pattern>.

However I also might want to see examples of the use cases you have in mind. (Such examples would help drive decisions about whether to jump straight to some sort of regexp.)

@chrissimpkins
Copy link
Member

chrissimpkins commented Feb 24, 2020

One issue that I came across in the last few weeks was a stack overflow runtime error that did not raise the hardcoded ICE error substring used in this PR:

rust-lang/rust#69096 (comment)

Maybe a very edge case and best for a script but it is something that could be supported with a user defined command line string that defines the regression.

There are perhaps more involved regex based searches though none immediately come to mind in my limited experience.

@pnkfelix
Copy link
Member Author

pnkfelix commented Feb 24, 2020

Stack overflow is an excellent example.

And another, that I should have thought of myself based on my own experience, is a link failure.

I'll think more about this. It seems worth looking into adding; the hardest parts are probably figuring out the format of the command line option and then adding the parsing for it.)

@chrissimpkins
Copy link
Member

chrissimpkins commented Feb 24, 2020

Maybe something like this?:

cargo bisect-rustc --regress=error-pattern --pattern="fatal runtime error: stack overflow" [ ... ]

I was trying to figure out if/how StructOpt/clap supports linked CLI option validation (i.e., how do you declare that when X is present, Y must not be present; when X is present, Y must be present?) over the weekend and haven't come across documentation yet. Embedding it all into a single option as you suggested earlier would work around that issue. Or we just manually validate in run().

@chrissimpkins
Copy link
Member

chrissimpkins commented Feb 24, 2020

And for regex support in the future (if required), maybe add a new regex-specific regress definition?:

cargo bisect-rustc --regress=error-pattern --pattern="fatal runtime error: stack overflow" [ ... ]
cargo bisect-rustc --regress=error-regex-pattern --pattern="\[E\d{4}\]" [ ... ]

Alexendoo added a commit to Alexendoo/cargo-bisect-rustc that referenced this pull request Apr 20, 2020
This matches the definition [used in glacier], which includes exit status
101 without an accompanying diagnostic (such as rust-lang/rust#21599), and when
rustc is killed by a signal (rust-lang/rust#13368)

This also means no processing modes are capturing stdio, but I didn't
remove it as it may be desired for the regex features discussed in rust-lang#53

[used in glacier]: https://github.com/rust-lang/glacier/blob/77029d8e7f755bd95913d3c741738674d5ccadc3/src/lib.rs#L51-L56
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.

Flag to check for internal compiler errors feature request: Invert status
3 participants