Ignore -C strip on MSVC

[icedrocket]

tl;dr - Define -Cstrip to only ever affect the binary; no other build artifacts.

This is necessary to improve cross-platform behavior consistency: if someone wanted debug information to be contained only in separate files on all platforms, they would set -Cstrip=symbols and -Csplit-debuginfo=packed, but this would result in no PDB files on MSVC.

Resolves #114215

[rustbot]

r? @b-naber

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

[icedrocket]

There may be cases where someone doesn't want a PDB file to be created. One way around this is to prevent a PDB file from being created when split-debuginfo=off.

[b-naber]

One way around this is to prevent a PDB file from being created when split-debuginfo is none

I don't understand what you're trying to say here, do you mean that you could prevent a PDB file from being created on MSVC by using split-debuginfo=off? off is not supported on MSVC, so there wouldn't be a way to prevent a PDB file from being created if we ignore strip afaict.

[icedrocket]

Currently, the value of the strip option determines whether or not a PDB file is created, and I'm proposing to remove that condition or use a different one. The title of the pull request is just my main proposal.

Yes, MSVC doesn't actually support split-debuginfo=off, but we can use it as a way to prevent the PDB file from being created. I think the strip option should only be applied to the executable itself, not to a separate debug file, as it would break cross-platform behavior consistency.

[b-naber]

I think it would make sense to treat split-debuginfo that way on msvc. That would likely require an mcp though and would also be a prerequisite for landing this pr.

[apiraino]

earmarking this PR, waiting on that MCP seconding

@rustbot author s-waiting-on-mcp

[rustbot]

These commits modify compiler targets.
(See the Target Tier Policy.)

This PR changes config.example.toml. If appropriate, please also update CONFIG_CHANGE_HISTORY in src/bootstrap/src/lib.rs and change-id in config.example.toml.

[icedrocket]

r? @bjorn3

[icedrocket]

@rustbot review

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

[michaelwoerister]

Thanks for keeping at this, @icedrocket!

@bors r+ rollup=never

[bors]

📌 Commit e82f46a has been approved by michaelwoerister

It is now in the queue for this repository.

[bors]

⌛ Testing commit e82f46a with merge 7f2fc33...

[bors]

☀️ Test successful - checks-actions
Approved by: michaelwoerister
Pushing 7f2fc33 to master...

[rust-timer]

Finished benchmarking commit (7f2fc33): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	2.4%	[2.4%, 2.4%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.4%	[2.4%, 2.4%]	1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 671.419s -> 670.936s (-0.07%)
Artifact size: 315.41 MiB -> 315.45 MiB (0.01%)

[weihanglo]

Since this is in 1.79.0 already, should we revert this PR in Cargo?

[evmar]

I found this change when I was trying to figure out why my link lines always include /DEBUG.

I might be misreading the code, but it seems .debuginfo() is called unconditionally, which means that flag is now passed unconditionally, regardless of any profile's debug or strip settings(?). This not only produces a pdb, it also produces a debug info section in the resulting binary. It is the sort of thing I would expect some sort of debug-stripping flag to remove. (Edit: this assumption is wrong, see later comments)

Note how most of these sections are empty, but the debug directory one isn't.

$ llvm-objdump -x target/i586-pc-windows-msvc/release/rdll.dll 
[...]
The Data Directory
Entry 0 00002059 00000041 Export Directory [.edata (or where ever we found it)]
Entry 1 00000000 00000000 Import Directory [parts of .idata]
Entry 2 00000000 00000000 Resource Directory [.rsrc]
Entry 3 00000000 00000000 Exception Directory [.pdata]
Entry 4 00000000 00000000 Security Directory
Entry 5 00000000 00000000 Base Relocation Directory [.reloc]
Entry 6 00002000 00000038 Debug Directory
[...]

That data is an IMAGE_DEBUG_DIRECTORY which points at data that is visualized in ghidra as

[workingjubilee]

@evmar Hi, can you open a new issue about your problem? Thanks!

[retep998]

I found this change when I was trying to figure out why my link lines always include /DEBUG.

I might be misreading the code, but it seems .debuginfo() is called unconditionally, which means that flag is now passed unconditionally, regardless of any profile's debug or strip settings(?). This not only produces a pdb, it also produces a debug info section in the resulting binary. It is the sort of thing I would expect some sort of debug-stripping flag to remove.

/DEBUG being unconditionally applied was always intentional for several reasons:

• The cost of /DEBUG being enabled is low, as the debug info section is only a few bytes. The link time penalty is also fairly low.
• It allows backtraces on Windows to actually have symbols instead of just addresses. On other platforms backtraces have symbols even without debuginfo due to the nature of how they handle linking. Being able to have this on Windows as well is a huge benefit.
• For the rare occasion where someone really insists on not having a PDB generated, nor a debug info section, they can pass a custom linker argument of /DEBUG:NONE to override /DEBUG.

[evmar]

(@retep998 Happy to open a new bug if it helps, lemme know)

/DEBUG:NONE is a good idea!

I tried adding to my build.rs

    println!("cargo::rustc-link-arg=/DEBUG:NONE");

And then built with

$ RUSTFLAGS='--print link-args' cargo build --release --target i586-pc-windows-msvc  --examples      

And the tail of the printed link line looks like:

[...] "/DEBUG" "/PDBALTPATH:%_PDB%" "/Brepro" "/noentry" "/nodefaultlib" "/DEBUG:NONE"

(where /DEBUG is from this PR, and noentry etc. are other flags I have already added) but the resulting binary seems to still include this section.

I think the relevant flag-parsing bit of lld is here and it seems like that ought to be enough, so now I am just confused.

Is there a way I can rerun the command directly to see if removing /DEBUG helps? It seems it refers to temporary files so if I copy-paste it to try it myself it fails.

[evmar]

I think I found it, it's this logic, which appears to have been tickled in my case by the /Brepro flag. Not your problem.

I am happy with the /DEBUG:NONE solution, I agree with your take that this is sufficiently esoteric that it doesn't need an easy user-facing knob.

[evmar]

Just in case anyone finds this in the future, I asked the author of the relevant bits of LLVM and he pointed me at some relevant discussion:
https://reviews.llvm.org/D51652

tldr LLVM is making a subtle and intentional choice to include this debug section.