Skip to content

Commit dac3902

Browse files
committed
fix(pack): whether stripping applies is a property of the TARGET, not the compiler
Keying `inBandDebugInfo` on `tc.compiler != MSVC` is wrong in both directions, and each direction is a configuration mcpp ships: clang -> x86_64-windows-msvc produces .pdb debug info, and would have been asked to strip in-band DWARF that is not there. Apple clang on macOS ships no llvm-strip, so the rule would REFUSE every `mcpp pack` on a Mac — for a format whose linked image carries a debug MAP (N_OSO stanzas naming the .o files) and leaves the DWARF outside it. `objcopy --only-keep-debug` there has nothing to copy, and .dSYM is dsymutil's job. `debug_info_is_in_band(canonicalTriple)` answers it segment-wise from the canonical triple, which both packers have already resolved. ELF and PE/MinGW are in-band; Mach-O and the MSVC ABI are not.
1 parent 3c53c18 commit dac3902

4 files changed

Lines changed: 86 additions & 14 deletions

File tree

src/pack/library_pipeline.cppm

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -354,13 +354,12 @@ export int build_and_pack_library(const std::string& targetName,
354354
.importLibrary = importLib,
355355
// From THIS leg's toolchain, for the same reason `archiveTool` is:
356356
// a fat package's foreign leg must not be stripped by the host's
357-
// tool. `inBandDebugInfo` is the one bit of "does this even apply"
358-
// — PE/MSVC keeps debug information in a separate `.pdb`.
357+
// tool. Whether stripping APPLIES is a question about the leg's
358+
// TARGET, not about its compiler — see mcpp.pack.strip.
359359
.stripTools = mcpp::pack::StripTools{
360360
.strip = mcpp::toolchain::binutils_tool(ctx->tc, "strip"),
361361
.objcopy = mcpp::toolchain::binutils_tool(ctx->tc, "objcopy"),
362-
.inBandDebugInfo =
363-
ctx->tc.compiler != mcpp::toolchain::CompilerId::MSVC,
362+
.inBandDebugInfo = mcpp::pack::debug_info_is_in_band(triple),
364363
},
365364
});
366365
mcpp::ui::status("Packed leg", std::format("{} [{}]", triple, tag.str()));

src/pack/pipeline.cppm

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import mcpp.pack;
2121
import mcpp.pack.strip;
2222
import mcpp.toolchain.model;
2323
import mcpp.toolchain.registry;
24+
import mcpp.toolchain.triple;
2425
import mcpp.ui;
2526

2627
namespace mcpp::pack {
@@ -195,7 +196,16 @@ export int build_and_pack(Options opts, bool modeFromUser,
195196
plan->stripTools = mcpp::pack::StripTools{
196197
.strip = mcpp::toolchain::binutils_tool(ctx->tc, "strip"),
197198
.objcopy = mcpp::toolchain::binutils_tool(ctx->tc, "objcopy"),
198-
.inBandDebugInfo = ctx->tc.compiler != mcpp::toolchain::CompilerId::MSVC,
199+
// The CANONICAL triple, resolved the same way the library packer
200+
// resolves it: an empty `targetTriple` means "this host", and asking
201+
// the empty string would answer "in-band" for macOS and MSVC alike.
202+
.inBandDebugInfo = mcpp::pack::debug_info_is_in_band(
203+
ctx->tc.targetTriple.empty()
204+
? mcpp::toolchain::triple::host_triple().str()
205+
: [&] {
206+
auto t = mcpp::toolchain::triple::parse(ctx->tc.targetTriple);
207+
return t ? t->str() : ctx->tc.targetTriple;
208+
}()),
199209
};
200210

201211
mcpp::ui::info("Packing", std::format("{} v{} ({}{})",

src/pack/strip.cppm

Lines changed: 49 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -39,14 +39,30 @@
3939
// exact name, so the build-id survives and `--add-gnu-debuglink` still has
4040
// something to pair with.
4141
//
42-
// EMPTY TOOL IS NOT ALWAYS AN ERROR
42+
// EMPTY TOOL IS NOT ALWAYS AN ERROR, AND WHICH CASE IT IS BELONGS TO THE
43+
// TARGET FORMAT — NOT TO THE COMPILER
4344
//
44-
// PE/MSVC keeps debug information in a separate `.pdb` by design, so there is
45-
// nothing in-band to remove and no binutils to remove it with. That is
46-
// `NotApplicable`. Every other format carries DWARF inside the image, so a
47-
// missing `strip` there is a REFUSAL — the same stance `run_library_pack`
48-
// already takes for a missing archiver, and for the same reason: shipping the
49-
// artifact anyway is the silent-wrong-answer this feature exists to remove.
45+
// Two of the three formats keep debug information OUTSIDE the image, and for
46+
// them an absent `strip` is the right answer rather than a missing dependency:
47+
//
48+
// PE/MSVC a separate `.pdb`, by design.
49+
// Mach-O the linked image carries a DEBUG MAP (N_OSO stanzas naming the
50+
// `.o` files); the DWARF itself never enters the `.dylib` unless
51+
// `dsymutil` is run, and what it then produces is a `.dSYM`
52+
// bundle beside the image. `strip` there would remove the symbol
53+
// table — a different thing — and `objcopy --only-keep-debug` has
54+
// nothing to copy.
55+
//
56+
// ELF and PE/MinGW carry DWARF inside the image, so a missing `strip` there is
57+
// a REFUSAL — the same stance `run_library_pack` already takes for a missing
58+
// archiver, and for the same reason: shipping the artifact anyway is the
59+
// silent-wrong-answer this feature exists to remove.
60+
//
61+
// ⚠️ KEYING THIS ON THE COMPILER WOULD BE WRONG IN BOTH DIRECTIONS. clang
62+
// targeting `x86_64-windows-msvc` produces `.pdb` debug info and would be
63+
// asked to strip in-band; Apple's clang ships no `llvm-strip`, so a
64+
// compiler-keyed rule would REFUSE every `mcpp pack` on macOS for a format that
65+
// has nothing to strip in the first place.
5066

5167
export module mcpp.pack.strip;
5268

@@ -68,11 +84,19 @@ struct StripTools {
6884
std::filesystem::path strip;
6985
std::filesystem::path objcopy;
7086
// Does this leg's format carry debug information inside the image?
71-
// False only for PE/MSVC (`.pdb`). Resolved once by the caller so this
72-
// module never has to know what a toolchain is.
87+
// Ask `debug_info_is_in_band` rather than filling this by hand — see the
88+
// header for the two formats where the answer is no, and for why the
89+
// question is about the TARGET and not about the compiler.
7390
bool inBandDebugInfo = true;
7491
};
7592

93+
// Is debug information carried INSIDE an image built for `canonicalTriple`?
94+
//
95+
// A string question about the canonical triple (`arch-os[-env]`), deliberately:
96+
// this module knows nothing about toolchains, and both packers have already
97+
// resolved that triple by the time they ask.
98+
bool debug_info_is_in_band(std::string_view canonicalTriple);
99+
76100
enum class StripOutcome { Stripped, NotApplicable };
77101

78102
struct StripResult {
@@ -131,6 +155,22 @@ std::uintmax_t size_of(const std::filesystem::path& p) {
131155

132156
} // namespace
133157

158+
bool debug_info_is_in_band(std::string_view canonicalTriple) {
159+
// `arch-os[-env]`. Splitting rather than substring-matching: an arch or a
160+
// vendor segment could contain either of these words, and mcpp has been
161+
// bitten before by a triple predicate that answered on a substring.
162+
std::vector<std::string_view> seg;
163+
for (std::size_t i = 0; i <= canonicalTriple.size(); ) {
164+
auto j = canonicalTriple.find('-', i);
165+
if (j == std::string_view::npos) { seg.push_back(canonicalTriple.substr(i)); break; }
166+
seg.push_back(canonicalTriple.substr(i, j - i));
167+
i = j + 1;
168+
}
169+
if (seg.size() >= 2 && seg[1] == "macos") return false; // debug map + .dSYM
170+
if (seg.size() >= 3 && seg[2] == "msvc") return false; // separate .pdb
171+
return true;
172+
}
173+
134174
std::vector<std::string> strip_args(ArtifactShape shape) {
135175
// dh_strip's own division. See the header for the measurement behind the
136176
// archive row — it is the one that turns a package into an unlinkable one.

tests/unit/test_pack_relocate.cpp

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -307,6 +307,29 @@ TEST(PackStrip, EveryShapeDropsCommentAndNote) {
307307
}
308308
}
309309

310+
TEST(PackStrip, WhetherStrippingAppliesIsAskedOfTheTargetNotTheCompiler) {
311+
// Keying this on the compiler is wrong in BOTH directions, and each
312+
// direction is a real configuration mcpp ships:
313+
//
314+
// clang -> x86_64-windows-msvc produces .pdb debug info; a
315+
// compiler-keyed rule would try to strip
316+
// in-band DWARF that is not there.
317+
// Apple clang on macOS ships no `llvm-strip`, so a
318+
// compiler-keyed rule would REFUSE every
319+
// `mcpp pack` on a Mac — for a format
320+
// whose image carries a debug MAP and
321+
// leaves the DWARF in the .o files.
322+
EXPECT_TRUE (mcpp::pack::debug_info_is_in_band("x86_64-linux-gnu"));
323+
EXPECT_TRUE (mcpp::pack::debug_info_is_in_band("aarch64-linux-musl"));
324+
EXPECT_TRUE (mcpp::pack::debug_info_is_in_band("x86_64-windows-gnu"));
325+
EXPECT_FALSE(mcpp::pack::debug_info_is_in_band("x86_64-windows-msvc"));
326+
EXPECT_FALSE(mcpp::pack::debug_info_is_in_band("aarch64-macos"));
327+
EXPECT_FALSE(mcpp::pack::debug_info_is_in_band("x86_64-macos"));
328+
// Segment-wise, not substring: mcpp has been bitten by a triple predicate
329+
// that answered on a substring before.
330+
EXPECT_TRUE(mcpp::pack::debug_info_is_in_band("macos64-linux-gnu"));
331+
}
332+
310333
TEST(PackStrip, MsvcHasNothingInBandToRemove) {
311334
// PE/MSVC keeps debug information in a separate `.pdb`. An empty `strip`
312335
// tool there is the right answer, not a missing dependency — and the two

0 commit comments

Comments
 (0)