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

Use a DWARF version consistent with rustc #694

Merged
merged 1 commit into from
Nov 10, 2022

Conversation

glandium
Copy link
Contributor

@glandium glandium commented Jul 7, 2022

Rustc defaults to DWARF-2 on some targets, and DWARF-4 on others.
However using -g with the C compiler yields whatever default version the
C compiler prefers.

One side effect is that the DWARF debug info shipped in some libraries
with rustc itself (e.g. libcompiler_builtins and others) have recently
switched to DWARF-5 as a side effect of upgrading the clang version
used on rustc CI. (rust-lang/rust#98746)

Ideally, the preferred DWARF version would be given by the rust compiler
and/or cargo, but that's not the case at the moment, so the next best
thing is something that aligns with the current defaults, although
work in under way to add a rustc flag that would allow to pick the
preferred DWARF version (rust-lang/rust#98350)

@glandium
Copy link
Contributor Author

glandium commented Jul 7, 2022

The CI failures don't seem to be my doing.

@dot-asm
Copy link
Contributor

dot-asm commented Jul 9, 2022

The CI failures don't seem to be my doing.

Right, see #677.

@@ -2771,6 +2775,23 @@ 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.

@dot-asm
Copy link
Contributor

dot-asm commented Jul 10, 2022

On a related note, mingw32 seems to suffer from the same problem. Or at least it appears to be a reasonable conclusion to reach based on the facts that a) attempt to use latest mingw toolchain renders debug-build executables non-executable (see #677 (comment)), and b) imposing -gdwarf-2 flags on C and C++ compilers mitigates the problem (see #677 (comment)).

@mati865
Copy link
Contributor

mati865 commented Jul 10, 2022

Depending on toolchain version you will end up with one of these Dwarf versions: 2, 4, 5 when targeting 32-bit mingw-w64.
Toolchain used to build Rust is rather ancient one so I'm fairly sure it uses Dwarf-2 by default. How would potential upgrade path for it look like if we choose one version? I.e. should upgrade of mingw-w64 toolchain also change Dwarf version in the spec?

@glandium glandium force-pushed the dwarf-version branch 2 times, most recently from 02d6087 to 932a55c Compare July 12, 2022 02:00
|| 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.

@dot-asm
Copy link
Contributor

dot-asm commented Jul 12, 2022

For reference, CUDA failure appears to be transient, it failed to download package index files. They suggest it can be side effect of "Mirror sync in progress".

@dot-asm
Copy link
Contributor

dot-asm commented Jul 12, 2022

How would potential upgrade path for it look like if we choose one version?

Not that I can offer any real answers here, but what would be definitely helpful if linker was producing working executable files even when faced with mismatching DWARF versions. One can even wonder if it would be possible to classify it as a GNU ld bug. More specifically such an ungraceful failure mentioned above...

@mati865
Copy link
Contributor

mati865 commented Jul 12, 2022

One can even wonder if it would be possible to classify it as a GNU ld bug.

IMO it's surely a bug in GNU ld but it's not clear if we can do anything other than workarounds here.

@glandium
Copy link
Contributor Author

glandium commented Aug 9, 2022

ping?

@thomcc
Copy link
Member

thomcc commented Nov 10, 2022

Can you rebase? The CI failures should be fixed now.

Copy link
Member

@thomcc thomcc left a comment

Choose a reason for hiding this comment

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

This largely looks fine. I'd really prefer long-term this information come from cargo, via an env var or something. That seems necessary to do the right thing here if rustc allows setting this via a flag, for example (Well, I suppose we could parse rustflags...)

I won't block on this, though.

My one concern is if folks are using compilers that don't support this argument, then this might cause issues. I'm not sure if such compilers exist, but I suppose we'll find out...

Rustc defaults to DWARF-2 on some targets, and DWARF-4 on others.
However using -g with the C compiler yields whatever default version the
C compiler prefers.

One side effect is that the DWARF debug info shipped in some libraries
with rustc itself (e.g. libcompiler_builtins and others) have recently
switched to DWARF-5 as a side effect of upgrading the clang version
used on rustc CI. (rust-lang/rust#98746)

Ideally, the preferred DWARF version would be given by the rust compiler
and/or cargo, but that's not the case at the moment, so the next best
thing is something that aligns with the current defaults, although
work in under way to add a rustc flag that would allow to pick the
preferred DWARF version (rust-lang/rust#98350)
@glandium
Copy link
Contributor Author

Rebased.

@thomcc
Copy link
Member

thomcc commented Nov 10, 2022

Thanks

@thomcc thomcc merged commit 0fff25a into rust-lang:main Nov 10, 2022
@glandium glandium deleted the dwarf-version branch November 11, 2022 02:00
@thomcc
Copy link
Member

thomcc commented Nov 20, 2022

This is in 1.0.77.

One concern I have here is that in rust-lang/rust#104385 we'll be updating the DWARF version to DWARF-4 on Apple targets.

@glandium, are you familiar with how this will impact this patch? Is there an issue with cc using DWARF-2 when the rust compiler uses DWARF-4? I'd assume there isn't, but figured you'd know.

If there is, I suppose cc will need do version-detection on rustc to handle this case... Which is gross, but is not exactly technically challenging.

@glandium
Copy link
Contributor Author

Mixing versions shouldn't cause problems. We've been mixing DWARF-2 and DWARF-4 on mac builds of Firefox for a very long time (DWARF-2 from rust, DWARF-4 from clang). DWARF-5 was only causing problems because not all tooling supports it.
Ideally, though, cc-rs would get the information from rustc somehow.

@thomcc
Copy link
Member

thomcc commented Nov 21, 2022

Mixing versions shouldn't cause problems. We've been mixing DWARF-2 and DWARF-4 on mac builds of Firefox for a very long time (DWARF-2 from rust, DWARF-4 from clang). DWARF-5 was only causing problems because not all tooling supports it.

That's what I thought, but wanted to confirm, thanks.

Ideally, though, cc-rs would get the information from rustc somehow.

Yeah, I agree. Probably through cargo, although that's a bit of a headache for cargo.

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.

4 participants