TIR test suite.#11

vext01 · 2019-03-27T13:09:36Z

This change adds a rustc flag --emit yk-tir which causes ykrustc to
dump the TIR in a textual format and exit.

Then we add a test new test suite kind which allows us to write tests
against the textual TIR dumps. It works in much the same way as the
existing MirOpt test suite.

Note that this needs a ykpack change to go in first. Raising a draft PR for initial discussion.

What do you think of the general approach?

[I think generate_tir() can be tidied up some in light of this change, but I didn't want to do that in this PR].

ltratt · 2019-03-27T13:15:56Z

src/librustc_interface/passes.rs

    // Output Yorick debug sections into binary targets.
    if tcx.sess.crate_types.borrow().contains(&config::CrateType::Executable) {
+        // Are we dumping the textual version of the TIR at the same time?
+        let tir_dump_fn = if tcx.sess.opts.output_types.contains_key(&OutputType::YkTir) {


let tir_dump_fn = tcsx.sess...contains_key(...).map(|_| Some(outputs.path(OutputType::YkTir))); or similar should work.

ltratt · 2019-03-27T13:16:34Z

src/librustc_yk_sections/mir_cfg.rs

    sorted_def_ids.sort();

+    // Only if the user requested, open a file for the textual TIR dump.
+    let mut dump_file: Option<File> = dump_filename.map_or_else::<Result<Option<File>, io::Error>, _, _>(


Are the type annotations needed? They make this very hard to read.

Hah, ironically I added them because I found it hard to read.

I'm not proud of this line. I think I should unroll it into something simpler.

I'd just drop the annotations myself and then I think it'll be fine.

ltratt · 2019-03-27T13:17:09Z

src/tools/compiletest/src/runtest.rs

+            self.fatal_proc_rec("compilation failed!", &proc_res);
+        }
+
+        if !proc_res.status.success() {


This is the same condition as the if above which seems odd.

Oops. This is a copy/paste mistake. Nice catch.

ltratt · 2019-03-27T13:17:58Z

src/tools/compiletest/src/runtest.rs

+            let mut test_lines = vec![ExpectedLine::Elision];
+            for l in test_text.lines() {
+                if l.is_empty() {
+                    // ignore


It might be easier to put continue in here. Or maybe for l in test_text.lines().filter(|x| !x.is_empty()) is even clearer?

Just following the precedent set in the MirOpt suite. This function was a copy-paste-modify kind of affair.

Here's the code this was based on in rustc:
https://github.com/rust-lang/rust/blob/237bf3244fffef501cf37d4bda00e1fce3fcfb46/src/tools/compiletest/src/runtest.rs#L2944-L2946

Might as well follow upstream.

ltratt · 2019-03-27T13:19:04Z

src/tools/compiletest/src/runtest.rs

+            for l in test_text.lines() {
+                if l.is_empty() {
+                    // ignore
+                } else if l.starts_with("//") && l.split_at("//".len()).1.trim() == "..." {


This feels like it would be clearer (and faster) as something like:

if l.starts_with("//") { if l["".len()..].trim() == "..." { ... } else { ... } }

As before, let's follow upstream's example.

ltratt · 2019-03-27T13:19:54Z

The test looks pretty much as I expected/hoped, so this looks like it's going in the right direction to me.!

vext01 · 2019-03-27T13:42:55Z

Great!

Do you think I should take the time here to tidy up generate_mir, which could accept an enum for the output type (binary or text)?

It's not essential.

ltratt · 2019-03-27T13:43:14Z

Do you think I should take the time here to tidy up generate_mir, which could accept an enum for the output type (binary or text)?

Dunno.

vext01 · 2019-03-27T14:16:10Z

OK, let me have a play and see if I can get some tidying done fairly quickish.

vext01 · 2019-03-28T11:11:34Z

see if I can get some tidying done

The last commit makes the interface and internals of generate_tir() more palatable. Only compile tested at the moment. Testing now.

This was surprisingly hard to get through the borrow checker!

ltratt · 2019-03-28T11:13:08Z

src/librustc_yk_sections/mir_cfg.rs

+            // In this case the file at `tir_path` is a raw binary file which we use to make an
+            // object file for linkage.
+            let obj = YkExtraLinkObject::new(&tir_path, SECTION_NAME);
+            fs::remove_file(tir_path)?; // Now we have our object, we can remove the temp file.


We might want to suppress error if the temporary file can't be removed (it's not the end of the world)?

Up to you. It's super unlikely to happen.

ltratt · 2019-03-28T11:13:42Z

src/librustc_yk_sections/mir_cfg.rs

+        },
+    };
+
+    let mut enc = match default_file {


let mut enc = default_file.map(|ref mut f| Some(...)); or similar?

Although I doubt it works, does |mut f| fix things?

Funnily enough, if I remove the ref it asks me to also remove mut, so the closest we would get is:

169 | let mut enc = default_file.map(|f| ykpack::Encoder::from(&mut f)); | ^- `f` dropped here while still borrowed | | | borrowed value does not live long enough ... 206 | } | - borrowed value needs to live until here

OK, let's give up ;)

vext01 · 2019-03-28T15:17:03Z

One outstanding comment for you here:
#11 (comment)

Besides that, I think I've covered your other comments.

Note that the companion PR for ykpack needs to go in first:
softdevteam/ykpack#7

vext01 · 2019-03-28T15:23:52Z

[I'd like to add more/better tests, but we'd need to implement more of the TIR conversions to be able to make sense of TIR dumps -- Currently there are too many 'unimplemented's to know where in the code we are].

ltratt · 2019-03-28T17:11:09Z

I think we're ready to squash?

vext01 · 2019-03-28T17:13:19Z

Not just yet. One moment.

vext01 · 2019-03-28T17:15:29Z

In addition to that last commit, I propose (as a separate commit) renaming mir_cfg.rs to emit_tir.rs or something similar.

Thoughts?

ltratt · 2019-03-28T18:59:13Z

Fine with me.

vext01 · 2019-03-29T09:53:15Z

I've renamed the file and shut the tidy checker up.

OK for me to squash?

This change adds a rustc flag `--emit yk-tir` which causes ykrustc to dump the TIR in a textual format and exit. Then we add a test new test suite kind which allows us to write tests against the textual TIR dumps. It works in much the same way as the existing MirOpt test suite. In fact, we re-use a lot of that code.

The old name was a throwback to when we though we would simply serialise MIR, instead of designing our own tracing byte-code.

ltratt · 2019-03-29T12:28:21Z

Please squash.

vext01 · 2019-03-29T13:43:00Z

splat.

ltratt · 2019-03-29T18:55:49Z

bors r+

11: TIR test suite. r=ltratt a=vext01 This change adds a rustc flag `--emit yk-tir` which causes ykrustc to dump the TIR in a textual format and exit. Then we add a test new test suite kind which allows us to write tests against the textual TIR dumps. It works in much the same way as the existing MirOpt test suite. Note that this needs a ykpack change to go in first. Raising a draft PR for initial discussion. What do you think of the general approach? [I think `generate_tir()` can be tidied up some in light of this change, but I didn't want to do that in this PR]. Co-authored-by: Edd Barrett <vext01@gmail.com>

bors · 2019-03-30T01:15:43Z

Build succeeded

buildbot/buildbot-build-script

remove directory libstd/sys/vxworks/backtrace which is not used any more

This is a combination of 18 commits. Commit softdevteam#2: Additional examples and some small improvements. Commit softdevteam#3: fixed mir-opt non-mir extensions and spanview title elements Corrected a fairly recent assumption in runtest.rs that all MIR dump files end in .mir. (It was appending .mir to the graphviz .dot and spanview .html file names when generating blessed output files. That also left outdated files in the baseline alongside the files with the incorrect names, which I've now removed.) Updated spanview HTML title elements to match their content, replacing a hardcoded and incorrect name that was left in accidentally when originally submitted. Commit softdevteam#4: added more test examples also improved Makefiles with support for non-zero exit status and to force validation of tests unless a specific test overrides it with a specific comment. Commit softdevteam#5: Fixed rare issues after testing on real-world crate Commit softdevteam#6: Addressed PR feedback, and removed temporary -Zexperimental-coverage -Zinstrument-coverage once again supports the latest capabilities of LLVM instrprof coverage instrumentation. Also fixed a bug in spanview. Commit softdevteam#7: Fix closure handling, add tests for closures and inner items And cleaned up other tests for consistency, and to make it more clear where spans start/end by breaking up lines. Commit softdevteam#8: renamed "typical" test results "expected" Now that the `llvm-cov show` tests are improved to normally expect matching actuals, and to allow individual tests to override that expectation. Commit softdevteam#9: test coverage of inline generic struct function Commit softdevteam#10: Addressed review feedback * Removed unnecessary Unreachable filter. * Replaced a match wildcard with remining variants. * Added more comments to help clarify the role of successors() in the CFG traversal Commit softdevteam#11: refactoring based on feedback * refactored `fn coverage_spans()`. * changed the way I expand an empty coverage span to improve performance * fixed a typo that I had accidently left in, in visit.rs Commit softdevteam#12: Optimized use of SourceMap and SourceFile Commit softdevteam#13: Fixed a regression, and synched with upstream Some generated test file names changed due to some new change upstream. Commit softdevteam#14: Stripping out crate disambiguators from demangled names These can vary depending on the test platform. Commit softdevteam#15: Ignore llvm-cov show diff on test with generics, expand IO error message Tests with generics produce llvm-cov show results with demangled names that can include an unstable "crate disambiguator" (hex value). The value changes when run in the Rust CI Windows environment. I added a sed filter to strip them out (in a prior commit), but sed also appears to fail in the same environment. Until I can figure out a workaround, I'm just going to ignore this specific test result. I added a FIXME to follow up later, but it's not that critical. I also saw an error with Windows GNU, but the IO error did not specify a path for the directory or file that triggered the error. I updated the error messages to provide more info for next, time but also noticed some other tests with similar steps did not fail. Looks spurious. Commit softdevteam#16: Modify rust-demangler to strip disambiguators by default Commit softdevteam#17: Remove std::process::exit from coverage tests Due to Issue #77553, programs that call std::process::exit() do not generate coverage results on Windows MSVC. Commit softdevteam#18: fix: test file paths exceeding Windows max path len

vext01 assigned ltratt Mar 27, 2019

ltratt reviewed Mar 27, 2019

View reviewed changes

ltratt reviewed Mar 28, 2019

View reviewed changes

vext01 mentioned this pull request Mar 28, 2019

Add implementation of Display for TIR structures. softdevteam/ykpack#7

Merged

vext01 added 2 commits March 29, 2019 11:54

Rename mir_cfg.rs to emit_tir.rs.

26a4331

The old name was a throwback to when we though we would simply serialise MIR, instead of designing our own tracing byte-code.

vext01 force-pushed the yk-tir-test-suite branch from 4f924cf to 26a4331 Compare March 29, 2019 13:42

vext01 marked this pull request as ready for review March 29, 2019 15:43

bors bot merged commit 26a4331 into master Mar 30, 2019

bors bot deleted the yk-tir-test-suite branch March 30, 2019 01:15

bors bot pushed a commit that referenced this pull request Sep 22, 2019

Merge pull request #11 from Wind-River/cleanup-2

7726b54

remove directory libstd/sys/vxworks/backtrace which is not used any more

Conversation

vext01 commented Mar 27, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ltratt commented Mar 27, 2019

Uh oh!

vext01 commented Mar 27, 2019

Uh oh!

ltratt commented Mar 27, 2019

Uh oh!

vext01 commented Mar 27, 2019

Uh oh!

vext01 commented Mar 28, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vext01 commented Mar 28, 2019

Uh oh!

vext01 commented Mar 28, 2019

Uh oh!

ltratt commented Mar 28, 2019

Uh oh!

vext01 commented Mar 28, 2019

Uh oh!

vext01 commented Mar 28, 2019

Uh oh!

ltratt commented Mar 28, 2019

Uh oh!

vext01 commented Mar 29, 2019

Uh oh!

ltratt commented Mar 29, 2019

Uh oh!

vext01 commented Mar 29, 2019

Uh oh!

ltratt commented Mar 29, 2019

Uh oh!

bors bot commented Mar 30, 2019

Build succeeded

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development