Build: Fix bug causing ReleaseSmall to fail on Windows

[ghost]

closes: #13405

Due to the change in default behavior of ReleaseSmall (#13067), debug info (pdb here) are stripped by default. However because Compile.create defaults to null, producesPdbFile will report true for
lib/std/Build/Step/InstallArtifact.zig

zig/lib/std/Build/Step/InstallArtifact.zig

Line 33 in 378264d

.pdb_dir = if (artifact.producesPdbFile()) blk: {

Because of this, this only affects zig build and not zig build-exe. After this change, a simple program compiled with ReleaseSmall will successfully compile and won't try to copy/produce the pdb file.

Of course, this still allows the user to choose not to strip the build and will produce a pdb file.

[davidgmbb]

Couldn't it be possible to imitate the ReleaseFast code path for stripping? As far as I understand, this only happens with ReleaseSmall, but by using ReleaseFast the binary is stripped too and the bug doesn't happen. There has to be some place in the code where the ReleaseFast stripping is handled gracefully. Otherwise we risk introducing here a very hidden branch in an unexpected place that could cause some trouble. That's my opinion and obviously I could be missing some details or misinterpreting something since I am not that aware of the compiler internals.

[ghost]

Couldn't it be possible to imitate the ReleaseFast code path for stripping? As far as I understand, this only happens with ReleaseSmall, but by using ReleaseFast the binary is stripped too and the bug doesn't happen. There has to be some place in the code where the ReleaseFast stripping is handled gracefully. Otherwise we risk introducing here a very hidden branch in an unexpected place that could cause some trouble. That's my opinion and obviously I could be missing some details or misinterpreting something since I am not that aware of the compiler internals.

I don't think the current/intended behavior of ReleaseFast is to strip the binaries, but i could also be wrong or misinterpreting what you said 😄.

Linux:

~/Projects/zigProjects/bench
Zig-Shell ❯ zig build -Doptimize=ReleaseFast

~/Projects/zigProjects/bench 6s
Zig-Shell ❯ file zig-out/bin/bench          
zig-out/bin/bench: ELF 64-bit LSB executable, x86-64, version 1 (SYSV), statically linked, with debug_info, not stripped

Windows builds also produces pdb.

[davidgmbb]

Ok I take back what I said then. Apologies :)

[ghost]

Seems like CI is failing on debug.zig "manage resources correctly" ReleaseSmall test.

zig/lib/std/debug.zig

Lines 2185 to 2197 in 7cf2cbb

	test "manage resources correctly" {
	if (builtin.os.tag == .wasi) return error.SkipZigTest;
	if (builtin.os.tag == .windows and builtin.cpu.arch == .x86_64) {
	// https://github.com/ziglang/zig/issues/13963
	return error.SkipZigTest;
	}
	const writer = std.io.null_writer;
	var di = try openSelfDebugInfo(testing.allocator);
	defer di.deinit();
	try printSourceAtAddress(&di, writer, showMyTrace(), detectTTYConfig(std.io.getStdErr()));
	}

I assume it's triggered because strip is now set to true on ReleaseSmall, thus failing on the builtin strip check.

I haven't investigated further yet so i could be totally wrong, but does it make sense that this test should have been skipped for ReleaseSmall because there is no debug info?

[davidgmbb]

I guess, if I understood correctly, that one could use @import("builtin").strip_debug_info to conditionally execute this test only in situations where the binary was not stripped.

[ghost]

I think I have a better understanding of this now. The problem is more specifically because it's stripping the test (addTest in Build.zig). So it depends on the desired behavior of tests. Whether tests should also strip or not.

So there are three options here:

• Skip this test on strip as specified above.
• Don't strip tests regardless of release mode unless specified.
• Don't strip the lang/CI tests.

edit: for now, i am going with the 1st option.

[andrewrk]

The previous value was correct. The default is decided by the compiler rather than the build system. null means to use the compiler's default.

[ghost]

I think I understand this now, and it does make sense why the CI was initially failing. Thanks for letting me know and sorry for the bad initial commit.

I reverted the commits and modified it to just check inside the producesPdbFile function instead of changing the strip value. Hopefully that's better(?).