Add basic CDB support to debuginfo compiletest s, to help catch `*.natvis` regressions, like those fixed in #60687. #60970

MaulingMonkey · 2019-05-20T00:15:51Z

First draft, feedback welcome.

Several Microsoft debuggers (VS, VS Code, WinDbg, CDB, ...) consume the *.natvis files we embed into rust *.pdb files. While this only tests CDB, that test coverage should help for all of them.

Changes

src\bootstrap

test.rs: Run CDB debuginfo tests on MSVC targets

src\test\debuginfo

issue-13213.rs: CDB has trouble with this, skip for now (newly discovered regression?)
pretty-std.rs: Was ignored, re-enable for CDB only to start with, add CDB tests.
should-fail.rs: Add CDB tests.

src\tools\compiletest:

Added "-cdb" option
Added Mode::DebugInfoCdb ("debuginfo-cdb")
Added run_debuginfo_cdb_test[_no_opt]
Renamed Mode::DebugInfoBoth -> DebugInfoGdbLldb ("debuginfo-gdb+lldb") since it's no longer clear what "Both" means.
Find CDB at the default Win10 SDK install path "C:\Program Files (x86)\Windows Kits\10\Debugger\*\cdb.exe"
Ignore CDB tests if CDB not found.

Issues

compute_stamp_hash: not sure if there's any point in hashing %ProgramFiles(x86)%
OsString lacks any *.natvis entries (would be nice to add in a followup changelist)
DSTs (array/string slices) which work in VS & VS Code fail in CDB.
I've avoided Mode::DebugInfoAll as 3 debuggers leads to pow(2,3)=8 possible combinations.

Reference

CDB is not part of the base Visual Studio install, but can be added via the Windows 10 SDK:
https://developer.microsoft.com/en-us/windows/downloads/windows-10-sdk
Installing just "Debugging Tools for Windows" is sufficient.

CDB appears to already be installed on appveyor CI, where this changelist can find it, based on it's use here:

rust/appveyor.yml

Line 227 in 0ffc573

- set PATH=%PATH%;"C:\Program Files (x86)\Windows Kits\10\Debuggers\X64"

CDB commands and command line reference:
https://docs.microsoft.com/en-us/windows-hardware/drivers/debugger/debugger-reference

…tvis` regressions, like those fixed in rust-lang#60687. Several Microsoft debuggers (VS, VS Code, WinDbg, CDB, ...) consume the `*.natvis` files we embed into rust `*.pdb` files. While this only tests CDB, that test coverage should help for all of them. CHANGES src\bootstrap - test.rs: Run CDB debuginfo tests on MSVC targets src\test\debuginfo - issue-13213.rs: CDB has trouble with this, skip for now (newly discovered regression?) - pretty-std.rs: Was ignored, re-enable for CDB only to start with, add CDB tests. - should-fail.rs: Add CDB tests. src\tools\compiletest: - Added "-cdb" option - Added Mode::DebugInfoCdb ("debuginfo-cdb") - Added run_debuginfo_cdb_test[_no_opt] - Renamed Mode::DebugInfoBoth -> DebugInfoGdbLldb ("debuginfo-gdb+lldb") since it's no longer clear what "Both" means. - Find CDB at the default Win10 SDK install path "C:\Program Files (x86)\Windows Kits\10\Debugger\*\cdb.exe" - Ignore CDB tests if CDB not found. ISSUES - `compute_stamp_hash`: not sure if there's any point in hashing `%ProgramFiles(x86)%` - `OsString` lacks any `*.natvis` entries (would be nice to add in a followup changelist) - DSTs (array/string slices) which work in VS & VS Code fail in CDB. - I've avoided `Mode::DebugInfoAll` as 3 debuggers leads to pow(2,3)=8 possible combinations. REFERENCE CDB is not part of the base Visual Studio install, but can be added via the Windows 10 SDK: https://developer.microsoft.com/en-us/windows/downloads/windows-10-sdk Installing just "Debugging Tools for Windows" is sufficient. CDB appears to already be installed on appveyor CI, where this changelist can find it, based on it's use here: https://github.com/rust-lang/rust/blob/0ffc57311030a1930edfa721fe57d0000a063af4/appveyor.yml#L227 CDB commands and command line reference: https://docs.microsoft.com/en-us/windows-hardware/drivers/debugger/debugger-reference

rust-highfive · 2019-05-20T00:16:01Z

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

rust-highfive · 2019-05-20T00:34:07Z

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

travis_time:end:02ef9050:start=1558311429257100545,finish=1558311518335147427,duration=89078046882
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
$ export GCP_CACHE_BUCKET=rust-lang-ci-cache
$ export AWS_ACCESS_KEY_ID=AKIA46X5W6CZEJZ6XT55
---

[00:04:01] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:04:01] tidy error: /checkout/src/test/debuginfo/pretty-std.rs:2: line longer than 100 chars
[00:04:01] tidy error: /checkout/src/test/debuginfo/pretty-std.rs:84: line longer than 100 chars
[00:04:01] tidy error: /checkout/src/test/debuginfo/pretty-std.rs:113: line longer than 100 chars
[00:04:01] tidy error: /checkout/src/tools/compiletest/src/runtest.rs:6: line longer than 100 chars
[00:04:01] tidy error: /checkout/src/tools/compiletest/src/runtest.rs:712: line longer than 100 chars
[00:04:01] tidy error: /checkout/src/tools/compiletest/src/runtest.rs:1971: line longer than 100 chars
[00:04:01] tidy error: /checkout/src/tools/compiletest/src/main.rs:876: line longer than 100 chars
[00:04:06] some tidy checks failed
[00:04:06] 
[00:04:06] 
[00:04:06] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:04:06] 
[00:04:06] 
[00:04:06] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:04:06] Build completed unsuccessfully in 0:01:17
[00:04:06] Build completed unsuccessfully in 0:01:17
[00:04:06] Makefile:67: recipe for target 'tidy' failed
[00:04:06] make: *** [tidy] Error 1
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:00ed7c22
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Mon May 20 00:22:53 UTC 2019
---
travis_time:end:06696936:start=1558311774591622184,finish=1558311774596566900,duration=4944716
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:0012c6b4
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:027f1604
travis_time:start:027f1604
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:0baaa217
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

alexcrichton

This looks great to me, thanks so much for this @MaulingMonkey! We don't have a great way to test this on AppVeyor before sending through bors, but that's probably ok as it looks like you're probably running the tests locally already and there may just be a CI issue or two to work through on AppVeyor.

alexcrichton · 2019-05-20T15:53:24Z

src/tools/compiletest/src/main.rs

+}
+
+fn find_cdb(target: &String) -> Option<OsString> {
+    if cfg!(windows) && is_pc_windows_msvc_target(target) {


Could this be inverted to reduce indentation? e.g.:

if !current_condition { return None; } // ...

alexcrichton · 2019-05-20T15:53:41Z

src/tools/compiletest/src/main.rs

@@ -1,5 +1,6 @@
 #![crate_name = "compiletest"]
 #![feature(test)]
+#![feature(path_buf_capacity)]


Could this use existing stable methods instead of adding a new unstable feature?

Done! Can't reserve capacity without it, but that's an unimportant optimization at best.

alexcrichton · 2019-05-20T15:55:01Z

src/tools/compiletest/src/runtest.rs

+    if config.mode == DebugInfoCdb {
+        match config.cdb {
+            None => env::var_os("ProgramFiles(x86)").hash(&mut hash),
+            Some(ref s) if s.is_empty() => env::var_os("ProgramFiles(x86)").hash(&mut hash),


Shouldn't this case in theory be impossible with the previous validation of --cdb?

I believe so, yes.

alexcrichton · 2019-05-20T15:55:40Z

src/tools/compiletest/src/runtest.rs

-    if config.mode == DebugInfoGdb || config.mode == DebugInfoBoth {
+    if config.mode == DebugInfoCdb {
+        match config.cdb {
+            None => env::var_os("ProgramFiles(x86)").hash(&mut hash),


Although the gdb section does this, I think this is probably fine to just ignore and do nothing in this branch? It seems like this would probably want to just be config.cdb.hash(&mut hash).

(I don't think 100% fidelity is really that necessary here and being simple is probably the best way to go)

Works for me.

- Don't add path_buf_capacity feature. - Convert `find_cdb` to early return style, reducing indentation - Simplify `compute_stamp_hash` for CDB to just hash it's path, if any.

MaulingMonkey · 2019-05-20T22:07:43Z

I've been mostly just testing the stage 1 debuginfo tests. If I'm understanding the rustc build pipeline properly this should be sufficient, as I'm not touching the compiler itself here just tests - but if there's anything else I should be testing just let me know:

./x.py test --stage 1 --build x86_64-pc-windows-msvc src/test/debuginfo
./x.py test --stage 1 --build i686-pc-windows-msvc src/test/debuginfo
./x.py test --stage 1 --build x86_64-pc-windows-gnu src/test/debuginfo
./x.py test --stage 1 src/tools/tidy

alexcrichton · 2019-05-21T14:00:24Z

@bors: r+

👍

bors · 2019-05-21T14:00:29Z

📌 Commit 56b18ce has been approved by alexcrichton

…pport, r=alexcrichton Add basic CDB support to debuginfo compiletest s, to help catch `*.natvis` regressions, like those fixed in rust-lang#60687. First draft, feedback welcome. Several Microsoft debuggers (VS, VS Code, WinDbg, CDB, ...) consume the `*.natvis` files we embed into rust `*.pdb` files. While this only tests CDB, that test coverage should help for all of them. # Changes ## src\bootstrap - test.rs: Run CDB debuginfo tests on MSVC targets ## src\test\debuginfo - issue-13213.rs: CDB has trouble with this, skip for now (newly discovered regression?) - pretty-std.rs: Was ignored, re-enable for CDB only to start with, add CDB tests. - should-fail.rs: Add CDB tests. ## src\tools\compiletest: - Added "-cdb" option - Added Mode::DebugInfoCdb ("debuginfo-cdb") - Added run_debuginfo_cdb_test[_no_opt] - Renamed Mode::DebugInfoBoth -> DebugInfoGdbLldb ("debuginfo-gdb+lldb") since it's no longer clear what "Both" means. - Find CDB at the default Win10 SDK install path "C:\Program Files (x86)\Windows Kits\10\Debugger\\*\cdb.exe" - Ignore CDB tests if CDB not found. # Issues - `compute_stamp_hash`: not sure if there's any point in hashing `%ProgramFiles(x86)%` - `OsString` lacks any `*.natvis` entries (would be nice to add in a followup changelist) - DSTs (array/string slices) which work in VS & VS Code fail in CDB. - I've avoided `Mode::DebugInfoAll` as 3 debuggers leads to pow(2,3)=8 possible combinations. # Reference CDB is not part of the base Visual Studio install, but can be added via the Windows 10 SDK: https://developer.microsoft.com/en-us/windows/downloads/windows-10-sdk Installing just "Debugging Tools for Windows" is sufficient. CDB appears to already be installed on appveyor CI, where this changelist can find it, based on it's use here: https://github.com/rust-lang/rust/blob/0ffc57311030a1930edfa721fe57d0000a063af4/appveyor.yml#L227 CDB commands and command line reference: https://docs.microsoft.com/en-us/windows-hardware/drivers/debugger/debugger-reference

Centril · 2019-05-23T06:35:39Z

@bors rollup=never

bors · 2019-05-23T16:24:32Z

⌛ Testing commit 56b18ce with merge 79e607c4d5d639d45e74551c51768f202ee431fd...

varkor · 2019-05-23T17:03:24Z

@bors retry

varkor · 2019-05-23T19:28:17Z

(Prioritised #61085.)

bors · 2019-05-23T20:58:32Z

⌛ Testing commit 56b18ce with merge 8869ee0...

…excrichton Add basic CDB support to debuginfo compiletest s, to help catch `*.natvis` regressions, like those fixed in #60687. First draft, feedback welcome. Several Microsoft debuggers (VS, VS Code, WinDbg, CDB, ...) consume the `*.natvis` files we embed into rust `*.pdb` files. While this only tests CDB, that test coverage should help for all of them. # Changes ## src\bootstrap - test.rs: Run CDB debuginfo tests on MSVC targets ## src\test\debuginfo - issue-13213.rs: CDB has trouble with this, skip for now (newly discovered regression?) - pretty-std.rs: Was ignored, re-enable for CDB only to start with, add CDB tests. - should-fail.rs: Add CDB tests. ## src\tools\compiletest: - Added "-cdb" option - Added Mode::DebugInfoCdb ("debuginfo-cdb") - Added run_debuginfo_cdb_test[_no_opt] - Renamed Mode::DebugInfoBoth -> DebugInfoGdbLldb ("debuginfo-gdb+lldb") since it's no longer clear what "Both" means. - Find CDB at the default Win10 SDK install path "C:\Program Files (x86)\Windows Kits\10\Debugger\\*\cdb.exe" - Ignore CDB tests if CDB not found. # Issues - `compute_stamp_hash`: not sure if there's any point in hashing `%ProgramFiles(x86)%` - `OsString` lacks any `*.natvis` entries (would be nice to add in a followup changelist) - DSTs (array/string slices) which work in VS & VS Code fail in CDB. - I've avoided `Mode::DebugInfoAll` as 3 debuggers leads to pow(2,3)=8 possible combinations. # Reference CDB is not part of the base Visual Studio install, but can be added via the Windows 10 SDK: https://developer.microsoft.com/en-us/windows/downloads/windows-10-sdk Installing just "Debugging Tools for Windows" is sufficient. CDB appears to already be installed on appveyor CI, where this changelist can find it, based on it's use here: https://github.com/rust-lang/rust/blob/0ffc57311030a1930edfa721fe57d0000a063af4/appveyor.yml#L227 CDB commands and command line reference: https://docs.microsoft.com/en-us/windows-hardware/drivers/debugger/debugger-reference

bors · 2019-05-23T23:46:05Z

☀️ Test successful - checks-travis, status-appveyor
Approved by: alexcrichton
Pushing 8869ee0 to master...

rust-highfive assigned alexcrichton May 20, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 20, 2019

Fix CDB support tidy check line length failures.

0a423a7

alexcrichton reviewed May 20, 2019

View reviewed changes

Address CDB review feedback

56b18ce

- Don't add path_buf_capacity feature. - Convert `find_cdb` to early return style, reducing indentation - Simplify `compute_stamp_hash` for CDB to just hash it's path, if any.

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 21, 2019

Centril mentioned this pull request May 22, 2019

Rollup of 4 pull requests #61032

Closed

bors added the merged-by-bors This PR was explicitly merged by bors. label May 23, 2019

bors merged commit 56b18ce into rust-lang:master May 23, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add basic CDB support to debuginfo compiletest s, to help catch `*.natvis` regressions, like those fixed in #60687. #60970

Add basic CDB support to debuginfo compiletest s, to help catch `*.natvis` regressions, like those fixed in #60687. #60970

MaulingMonkey commented May 20, 2019

rust-highfive commented May 20, 2019

rust-highfive commented May 20, 2019

alexcrichton left a comment

alexcrichton May 20, 2019

MaulingMonkey May 20, 2019

alexcrichton May 20, 2019

MaulingMonkey May 20, 2019

alexcrichton May 20, 2019

MaulingMonkey May 20, 2019

alexcrichton May 20, 2019

MaulingMonkey May 20, 2019

MaulingMonkey commented May 20, 2019

alexcrichton commented May 21, 2019

bors commented May 21, 2019

Centril commented May 23, 2019

bors commented May 23, 2019

varkor commented May 23, 2019

varkor commented May 23, 2019

bors commented May 23, 2019

bors commented May 23, 2019

Add basic CDB support to debuginfo compiletest s, to help catch *.natvis regressions, like those fixed in #60687. #60970

Add basic CDB support to debuginfo compiletest s, to help catch *.natvis regressions, like those fixed in #60687. #60970

Conversation

MaulingMonkey commented May 20, 2019

Changes

src\bootstrap

src\test\debuginfo

src\tools\compiletest:

Issues

Reference

rust-highfive commented May 20, 2019

rust-highfive commented May 20, 2019

alexcrichton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaulingMonkey commented May 20, 2019

alexcrichton commented May 21, 2019

bors commented May 21, 2019

Centril commented May 23, 2019

bors commented May 23, 2019

varkor commented May 23, 2019

varkor commented May 23, 2019

bors commented May 23, 2019

bors commented May 23, 2019

Add basic CDB support to debuginfo compiletest s, to help catch `*.natvis` regressions, like those fixed in #60687. #60970

Add basic CDB support to debuginfo compiletest s, to help catch `*.natvis` regressions, like those fixed in #60687. #60970