Skip to content

Commit

Permalink
Auto merge of #76390 - MaulingMonkey:pr-min-cdb-version, r=petrochenkov
Browse files Browse the repository at this point in the history
debuginfo:  Ignore HashMap .natvis tests before cdb 10.0.18362.1

CDB <10.0.18362.1 chokes on casts within HashMap's natvis visualizers.  This PR adds support for "min-cdb-version" (per existing "min-gdb-version" and "min-lldb-version" filters) and uses it.  CI uses a more recent version of CDB for testing and thus should still run the tests.

Credit to @petrochenkov per #76352 for helping catch this.

### SDK Testing

| Win 10 SDK                                                             |  x64 CDB          | rustc 1.47.0-nightly (bf43421 2020-08-25) built-in .natvis  | Note |
| --------------------------------------------------------------------- | ----------------- | ------------------------------------------------------------- | ---- |
| [10.0.19041.0](https://go.microsoft.com/fwlink/p/?linkid=2120843)     | 10.0.19041.1      | ✔️                                                            | CI
| [10.0.18362.1](https://go.microsoft.com/fwlink/?linkid=2083338)       | 10.0.18362.1      | ✔️                                                            | MaulingMonkey
| [10.0.17763.0](https://go.microsoft.com/fwlink/p/?LinkID=2033908)     | 10.0.17763.132    | ❌ `Unable to find type 'tuple<u64,u64> *' for cast.`
| [10.0.17134.12](https://go.microsoft.com/fwlink/p/?linkid=870807)     | 10.0.17134.12     | ❌ `Unable to find type 'tuple<u64,u64> *' for cast.`
| [10.0.16299.91](https://go.microsoft.com/fwlink/p/?linkid=864422)     | 10.0.16299.91     | ❌ `Unable to find type 'tuple<u64,u64> *' for cast.`
| [10.0.15063.468](https://go.microsoft.com/fwlink/p/?LinkId=845298)    | 10.0.15063.468    | ❌ `Unable to find type 'tuple<u64,u64> *' for cast.`
| [10.0.14393.795](https://go.microsoft.com/fwlink/p/?LinkId=838916)    | 10.0.14321.1024   | ❌ `Unable to find type 'tuple<u64,u64> *' for cast.` | petrochenkov
| [10.0.10586.212](https://go.microsoft.com/fwlink/p/?LinkID=698771)    | 10.0.10586.567    | ❌ `Expected ')' at '+ 1)].__1'`
| [10.0.10240](https://go.microsoft.com/fwlink/p/?LinkId=619296)        | 10.0.10240?       | ❔ Untested

### Rust Testing

```cmd
x.py test --stage 1 src/tools/tidy
x.py test --stage 1 --build x86_64-pc-windows-msvc src\test\debuginfo
```

Also verified test still fails when intentionally broken w/ CDB version >= min-cdb-version.
  • Loading branch information
bors committed Sep 6, 2020
2 parents 4b65872 + 4046f92 commit ac892a1
Show file tree
Hide file tree
Showing 4 changed files with 55 additions and 6 deletions.
4 changes: 4 additions & 0 deletions src/test/debuginfo/pretty-std-collections-hash.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
// CDB doesn't like how libstd.natvis casts to tuples before this version.
// https://github.com/rust-lang/rust/issues/76352#issuecomment-687640746
// min-cdb-version: 10.0.18362.1

// cdb-only
// compile-flags:-g

Expand Down
3 changes: 3 additions & 0 deletions src/tools/compiletest/src/common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,9 @@ pub struct Config {
/// Path to / name of the Microsoft Console Debugger (CDB) executable
pub cdb: Option<OsString>,

/// Version of CDB
pub cdb_version: Option<[u16; 4]>,

/// Path to / name of the GDB executable
pub gdb: Option<String>,

Expand Down
25 changes: 22 additions & 3 deletions src/tools/compiletest/src/header.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@ use std::path::{Path, PathBuf};
use tracing::*;

use crate::common::{CompareMode, Config, Debugger, FailMode, Mode, PassMode};
use crate::extract_gdb_version;
use crate::util;
use crate::{extract_cdb_version, extract_gdb_version};

#[cfg(test)]
mod tests;
Expand Down Expand Up @@ -105,6 +105,10 @@ impl EarlyProps {
props.ignore = true;
}

if config.debugger == Some(Debugger::Cdb) && ignore_cdb(config, ln) {
props.ignore = true;
}

if config.debugger == Some(Debugger::Gdb) && ignore_gdb(config, ln) {
props.ignore = true;
}
Expand All @@ -131,6 +135,21 @@ impl EarlyProps {

return props;

fn ignore_cdb(config: &Config, line: &str) -> bool {
if let Some(actual_version) = config.cdb_version {
if let Some(min_version) = line.strip_prefix("min-cdb-version:").map(str::trim) {
let min_version = extract_cdb_version(min_version).unwrap_or_else(|| {
panic!("couldn't parse version range: {:?}", min_version);
});

// Ignore if actual version is smaller than the minimum
// required version
return actual_version < min_version;
}
}
false
}

fn ignore_gdb(config: &Config, line: &str) -> bool {
if let Some(actual_version) = config.gdb_version {
if let Some(rest) = line.strip_prefix("min-gdb-version:").map(str::trim) {
Expand All @@ -142,8 +161,8 @@ impl EarlyProps {
if start_ver != end_ver {
panic!("Expected single GDB version")
}
// Ignore if actual version is smaller the minimum required
// version
// Ignore if actual version is smaller than the minimum
// required version
return actual_version < start_ver;
} else if let Some(rest) = line.strip_prefix("ignore-gdb-version:").map(str::trim) {
let (min_version, max_version) =
Expand Down
29 changes: 26 additions & 3 deletions src/tools/compiletest/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ pub fn parse_config(args: Vec<String>) -> Config {

let target = opt_str2(matches.opt_str("target"));
let android_cross_path = opt_path(matches, "android-cross-path");
let cdb = analyze_cdb(matches.opt_str("cdb"), &target);
let (cdb, cdb_version) = analyze_cdb(matches.opt_str("cdb"), &target);
let (gdb, gdb_version, gdb_native_rust) =
analyze_gdb(matches.opt_str("gdb"), &target, &android_cross_path);
let (lldb_version, lldb_native_rust) = matches
Expand Down Expand Up @@ -216,6 +216,7 @@ pub fn parse_config(args: Vec<String>) -> Config {
target,
host: opt_str2(matches.opt_str("host")),
cdb,
cdb_version,
gdb,
gdb_version,
gdb_native_rust,
Expand Down Expand Up @@ -773,8 +774,30 @@ fn find_cdb(target: &str) -> Option<OsString> {
}

/// Returns Path to CDB
fn analyze_cdb(cdb: Option<String>, target: &str) -> Option<OsString> {
cdb.map(OsString::from).or_else(|| find_cdb(target))
fn analyze_cdb(cdb: Option<String>, target: &str) -> (Option<OsString>, Option<[u16; 4]>) {
let cdb = cdb.map(OsString::from).or_else(|| find_cdb(target));

let mut version = None;
if let Some(cdb) = cdb.as_ref() {
if let Ok(output) = Command::new(cdb).arg("/version").output() {
if let Some(first_line) = String::from_utf8_lossy(&output.stdout).lines().next() {
version = extract_cdb_version(&first_line);
}
}
}

(cdb, version)
}

fn extract_cdb_version(full_version_line: &str) -> Option<[u16; 4]> {
// Example full_version_line: "cdb version 10.0.18362.1"
let version = full_version_line.rsplit(' ').next()?;
let mut components = version.split('.');
let major: u16 = components.next().unwrap().parse().unwrap();
let minor: u16 = components.next().unwrap().parse().unwrap();
let patch: u16 = components.next().unwrap_or("0").parse().unwrap();
let build: u16 = components.next().unwrap_or("0").parse().unwrap();
Some([major, minor, patch, build])
}

/// Returns (Path to GDB, GDB Version, GDB has Rust Support)
Expand Down

0 comments on commit ac892a1

Please sign in to comment.