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

std.Build: add build-id option #22516

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Jan200101
Copy link
Contributor

@Jan200101 Jan200101 commented Jan 17, 2025

adds a build-id option to the build runner and a build_id property to std.Build.

the value of std.Build.build_id is used when std.Build.Step.Compile.build_id has not been explicitly specified.

This is useful for distro packagers and anyone who wants to specify a build-id for a whole project unless explicitly specified in the build logic itself.

@Jan200101 Jan200101 force-pushed the PR/build_id_option branch 3 times, most recently from 1ed8703 to ff42fd6 Compare January 26, 2025 16:53
@alexrp
Copy link
Member

alexrp commented Jan 26, 2025

I have mixed feelings about this.

On the one hand, this is clearly useful and solves a real problem.

On the other hand, it feels iffy that a project's build.zig could have its own build-id logic that could potentially work completely differently and in ways distro packagers might not expect or want, or even be able to override easily. I feel like I'd want the --build-id flag added here - which is interpreted by the build runner itself - to override any such build script logic.

Maybe this is an unwarranted concern, though.

@Jan200101
Copy link
Contributor Author

I have mixed feelings about this.

On the one hand, this is clearly useful and solves a real problem.

On the other hand, it feels iffy that a project's build.zig could have its own build-id logic that could potentially work completely differently and in ways distro packagers might not expect or want, or even be able to override easily. I feel like I'd want the --build-id flag added here - which is interpreted by the build runner itself - to override any such build script logic.

Maybe this is an unwarranted concern, though.

Forcing the build-id across a whole project would reintroduce issues with MicroZig.

the rp2 bootrom is compiled into an elf file and is expected to be 256 bytes, adding a build-id would expand that to ~276 bytes and cause some errors in the code as is.
Its possible to fix the code in MicroZig but the hardware expects the bootrom to be exactly 256 bytes.
There is no way around this either, both binutils ld, gold and lld unconditionally append the build-id to the output elf regardless of linker script or configuration.

https://github.com/ZigEmbeddedGroup/microzig/blob/ceefa564ae6b7c76ced733bbf2e4df2430a4dc3f/port/raspberrypi/rp2xxx/build.zig#L233

Most people don't know what a build-id is let alone specify one themselves

@alexrp
Copy link
Member

alexrp commented Jan 26, 2025

Ok, fair enough.

For the few projects that do define a -Dbuild-id, do we want to have some kind of integration between that and --build-id so that using --build-id isn't a gotcha?

@Jan200101
Copy link
Contributor Author

maybe?
the only project I can find on github with a build-id option is zig-bootstrap though that may not be an exhaustive list.
I'm also not quite sure how this would be best handled, the option could be named any variation of build and id and adding a bunch of compatibility code to Build.option would be a pain to maintain.

@Jan200101
Copy link
Contributor Author

actually I just realized that zig also has a build-id option within its build.zig which should be removed with this PR

@Jan200101 Jan200101 requested a review from alexrp January 30, 2025 08:02
Copy link
Member

@alexrp alexrp left a comment

Choose a reason for hiding this comment

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

I don't see anything problematic about the code. But the high-level concern about --build-id vs -Dbuild-id remains. I'd like to get Andrew's thoughts on that.

@alexrp alexrp requested a review from andrewrk January 30, 2025 08:20
@alexrp alexrp added this to the 0.14.0 milestone Jan 30, 2025
@andrewrk andrewrk removed this from the 0.14.0 milestone Mar 1, 2025
@Jan200101 Jan200101 force-pushed the PR/build_id_option branch from a9f4fd5 to e57673e Compare March 6, 2025 11:57
@Jan200101
Copy link
Contributor Author

rebased on master, would be neat if this could be added to a milestone again since quite a few distros rely on build-id

@Jan200101
Copy link
Contributor Author

CI failure is unrelated to the change in the PR

error: RPC failed; curl 56 OpenSSL SSL_read: SSL_ERROR_SYSCALL, errno 0

I'll kick off another build and pray it works

@Jan200101 Jan200101 force-pushed the PR/build_id_option branch from e57673e to 013a228 Compare March 7, 2025 09:59
@@ -202,6 +202,15 @@ pub fn main() !void {
next_arg, @errorName(err),
});
};
} else if (mem.eql(u8, arg, "--build-id")) {
builder.build_id = .fast;
Copy link
Member

Choose a reason for hiding this comment

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

Just out of curiosity, why this default in particular? Is there precedent in other build systems / toolchains?

Copy link
Contributor Author

@Jan200101 Jan200101 Mar 26, 2025

Choose a reason for hiding this comment

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

I copied the handling directly from main.zig to keep it the same across build and build-exe

zig/src/main.zig

Lines 1681 to 1682 in b350049

} else if (mem.eql(u8, arg, "--build-id")) {
build_id = .fast;

gcc and clang use sha1 as the default as it was the first build-id to be implemented, I know that the initial zig implementation had it as a boolean option, either on or off, but I am not sure why fast was chosen specifically.

Copy link
Member

Choose a reason for hiding this comment

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

Do you happen to know what distros tend to use here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fedora (and I assume anything in the same family) explicitly uses sha1

Arch builds gcc with --enable-linker-build-id which automatically passes --build-id to the linker, implying sha1

it might be best to just remove the default here and to demand that the type is explicit

Copy link
Member

Choose a reason for hiding this comment

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

Some context: #15459 (comment)

At least according to man ld, that comment is wrong.

I think it would be good to verify what ld and ld.lld do here (hopefully they agree), and then change Zig's default, both here and in src/main.zig, to be the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some context: #15459 (comment)

At least according to man ld, that comment is wrong.

I think I found the reason for this:

LD defaults to sha1 for ELF and md5 for PE
LDD defaults to sha1 for ELF, forced GUID for the msvc compatible linker and fast for wasm

so as a whole there doesn't appear to be one right default for all platforms

Copy link
Member

@alexrp alexrp Mar 26, 2025

Choose a reason for hiding this comment

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

That's fun. So in summary, defaults look as follows:

  • Mach-O doesn't seem to have a build ID mechanism.
  • ld, lld, and mold all seem to agree on sha1 for ELF.
  • lld uses fast for WASM (UUID v5 based on xxHash).
  • ld uses md5 for COFF, lld uses a variation on fast (both written as a "GUID").

ELF and WASM can in theory support any style. But the current scheme that linkers use for COFF only allows up to 16 bytes, which means that COFF "can't" support sha1 (20 bytes). That's quite unfortunate.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, I've argued myself into thinking that fast is a good default because it's consistent (ish) across binary formats.

The only missing piece here is that src/link/Coff.zig needs to pass --build-id to LLD when the style is fast. (LLD does not appear to support other styles for COFF at the moment, even though all the non-sha1 styles could be made to work.) Would you mind making that change?

Comment on lines +1376 to +1379
\\ --build-id[=style] At a minor link-time expense, coordinates stripped binaries
\\ fast, uuid, sha1, md5 with debug symbols via a '.note.gnu.build-id' section
\\ 0x[hexstring] Maximum 32 bytes
\\ none (default) Disable build-id
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
\\ --build-id[=style] At a minor link-time expense, coordinates stripped binaries
\\ fast, uuid, sha1, md5 with debug symbols via a '.note.gnu.build-id' section
\\ 0x[hexstring] Maximum 32 bytes
\\ none (default) Disable build-id
\\ --build-id[=style] At a minor link-time expense, embeds a build ID in binaries
\\ fast 8-byte non-cryptographic hash (COFF, ELF, WASM)
\\ sha1, tree 20-byte cryptographic hash (ELF, WASM)
\\ md5 16-byte cryptographic hash (ELF)
\\ uuid 16-byte random UUID (ELF, WASM)
\\ 0x[hexstring] Constant ID, maximum 32 bytes (ELF, WASM)
\\ none (default) No build ID

And likewise in src/main.zig?

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.

3 participants