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.

Sign up for GitHub

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

Jump to bottom

Use a DWARF version consistent with rustc #694

Merged
merged 1 commit into from Nov 10, 2022
Merged

Use a DWARF version consistent with rustc #694

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
29 changes: 26 additions & 3 deletions src/lib.rs
View file
Open in desktop
Expand Up @@ -214,13 +214,17 @@ enum ToolFamily {

impl ToolFamily {
/// What the flag to request debug info for this family of tools look like
fn add_debug_flags(&self, cmd: &mut Tool) {
fn add_debug_flags(&self, cmd: &mut Tool, dwarf_version: Option<u32>) {
match *self {
ToolFamily::Msvc { .. } => {
cmd.push_cc_arg("-Z7".into());
}
ToolFamily::Gnu | ToolFamily::Clang => {
cmd.push_cc_arg("-g".into());
cmd.push_cc_arg(
dwarf_version
.map_or_else(|| "-g".into(), |v| format!("-gdwarf-{}", v))
.into(),
);
}
}
}
Expand Down Expand Up @@ -1589,7 +1593,7 @@ impl Build {
cmd.args.push("-G".into());
}
let family = cmd.family;
family.add_debug_flags(cmd);
family.add_debug_flags(cmd, self.get_dwarf_version());
}

if self.get_force_frame_pointer() {
Expand Down Expand Up @@ -2848,6 +2852,25 @@ impl Build {
})
}

fn get_dwarf_version(&self) -> Option<u32> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would argue that it would be more than appropriate to add a commentary stating that the version choice reflects rustc defaults as of version X through Y. Idea is that whenever defaults change, there would be a hint that corresponding adjustment would be needed.

// Tentatively matches the DWARF version defaults as of rustc 1.62.
let target = self.get_target().ok()?;
if target.contains("android")
|| target.contains("apple")
|| target.contains("dragonfly")
|| target.contains("freebsd")
|| target.contains("netbsd")
|| target.contains("openbsd")
|| target.contains("windows-gnu")
Copy link
Contributor

Choose a reason for hiding this comment

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

Just in case. The original remark was specifically about mingw32, the 32-bit target. The 64-bit one was not observed to fail. And the thing is that "windows-gnu" would cover both 32- and 64-bit variants. At the same time the flag appears to be recognized by the 64-bit toolchain and causes no run-time problems. In other words, even though this line doesn't pinpoint the specific problem, it doesn't seem to incur any collateral damage. If it's acceptable is up to the actual cc-rs maintainers.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have no idea what will happen on 64-bit targets because they typically use SEH instead of DWARF.
I worry it can work with GNU tools but introduce bugs like rust-lang/rust#99143 with LLVM.

Copy link
Contributor

Choose a reason for hiding this comment

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

64-bit targets because they typically use SEH instead of DWARF.

Note that the 32-bit problem is not really/necessarily about exception handling. Because it was specifically debug-build binaries, ones packed with debugging information, that were rendered unexecutable. This is not to say that I'm saying that the 64-bit toolchain uses DWARF for debugging symbols. Because I'm not, as I don't really know. But I can confirm that -gdwarf appears to be an acceptable option. It can just as well be ignored, but the binaries are executable, and that's what counts for the moment.

it can work with GNU tools but introduce bugs ... with LLVM.

But it won't be affected by this target.contains. Right? So that if anything, you should double-check and report back. Or keep this in mind and suggest a workaround later on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have no idea what will happen on 64-bit targets because they typically use SEH instead of DWARF. I worry it can work with GNU tools but introduce bugs like rust-lang/rust#99143 with LLVM.

Exceptions handling and Debug info are more or less orthogonal. -g defaults to DWARF on mingw targets for both GCC and clang. I'm not sure what rust was doing for those targets before #99143, though.

{
Some(2)
} else if target.contains("linux") {
Some(4)
} else {
None
}
}

fn get_force_frame_pointer(&self) -> bool {
self.force_frame_pointer.unwrap_or_else(|| self.get_debug())
}
Expand Down
25 changes: 20 additions & 5 deletions tests/test.rs
View file
Open in desktop
Expand Up @@ -20,7 +20,7 @@ fn gnu_smoke() {
test.cmd(0)
.must_have("-O2")
.must_have("foo.c")
.must_not_have("-g")
.must_not_have("-gdwarf-4")
.must_have("-c")
.must_have("-ffunction-sections")
.must_have("-fdata-sections");
Expand Down Expand Up @@ -52,19 +52,34 @@ fn gnu_opt_level_s() {
.must_not_have("-Oz");
}

#[test]
fn gnu_debug() {
let test = Test::gnu();
test.gcc().debug(true).file("foo.c").compile("foo");
test.cmd(0).must_have("-gdwarf-4");

let test = Test::gnu();
test.gcc()
.target("x86_64-apple-darwin")
.debug(true)
.file("foo.c")
.compile("foo");
test.cmd(0).must_have("-gdwarf-2");
}

#[test]
fn gnu_debug_fp_auto() {
let test = Test::gnu();
test.gcc().debug(true).file("foo.c").compile("foo");
test.cmd(0).must_have("-g");
test.cmd(0).must_have("-gdwarf-4");
test.cmd(0).must_have("-fno-omit-frame-pointer");
}

#[test]
fn gnu_debug_fp() {
let test = Test::gnu();
test.gcc().debug(true).file("foo.c").compile("foo");
test.cmd(0).must_have("-g");
test.cmd(0).must_have("-gdwarf-4");
test.cmd(0).must_have("-fno-omit-frame-pointer");
}

Expand All @@ -78,7 +93,7 @@ fn gnu_debug_nofp() {
.force_frame_pointer(false)
.file("foo.c")
.compile("foo");
test.cmd(0).must_have("-g");
test.cmd(0).must_have("-gdwarf-4");
test.cmd(0).must_not_have("-fno-omit-frame-pointer");

let test = Test::gnu();
Expand All @@ -87,7 +102,7 @@ fn gnu_debug_nofp() {
.debug(true)
.file("foo.c")
.compile("foo");
test.cmd(0).must_have("-g");
test.cmd(0).must_have("-gdwarf-4");
test.cmd(0).must_not_have("-fno-omit-frame-pointer");
}

Expand Down