-
Notifications
You must be signed in to change notification settings - Fork 13.5k
[ELF] Change build-id default to sha1 #93943
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
Conversation
cc @MaskRay |
@llvm/pr-subscribers-lld @llvm/pr-subscribers-lld-elf Author: Tatsuyuki Ishi (ishitatsuyuki) ChangesThe current default, build-id=fast, is only 8 bytes due to the usage of 64-bit XXH3. This is incompatible with RPM packaging tools which requires >=16 bytes 1. In Clang the ENABLE_LINKER_BUILD_ID define makes it pass --build-id without a specific hash type. When also defaulting to LLD, this provides a pretty broken default out-of-box. Using XXH3 was a considerable performance advantage when build-id was first implemented, because sha1 was really sha1 and rather slow. Nowadays sha1 is just 160-bit BLAKE3 which is decently fast and not cryptographically broken, so it should be a good default. Note that the default remains "fast" for wasm because sha1 for wasm is still real sha1. Close #43483. Full diff: https://github.com/llvm/llvm-project/pull/93943.diff 2 Files Affected:
diff --git a/lld/ELF/Options.td b/lld/ELF/Options.td
index ff61a566f52f7..54c6f51b28f36 100644
--- a/lld/ELF/Options.td
+++ b/lld/ELF/Options.td
@@ -57,7 +57,7 @@ def Bstatic: F<"Bstatic">, HelpText<"Do not link against shared libraries">;
def build_id: J<"build-id=">, HelpText<"Generate build ID note">,
MetaVarName<"[fast,md5,sha1,uuid,0x<hexstring>]">;
-def : F<"build-id">, Alias<build_id>, AliasArgs<["fast"]>, HelpText<"Alias for --build-id=fast">;
+def : F<"build-id">, Alias<build_id>, AliasArgs<["sha1"]>, HelpText<"Alias for --build-id=sha1">;
defm check_sections: B<"check-sections",
"Check section addresses for overlaps (default)",
diff --git a/lld/docs/ld.lld.1 b/lld/docs/ld.lld.1
index da3b926d02a28..6121ebc924ad4 100644
--- a/lld/docs/ld.lld.1
+++ b/lld/docs/ld.lld.1
@@ -119,7 +119,7 @@ are calculated from the object contents.
is not intended to be cryptographically secure.
.It Fl -build-id
Synonym for
-.Fl -build-id Ns = Ns Cm fast .
+.Fl -build-id Ns = Ns Cm sha1 .
.It Fl -call-graph-profile-sort Ns = Ns Ar algorithm
.Ar algorithm
may be:
|
eeb7985
to
4109ac7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: the majority of commits in llvm-project omit the trailing full stop in the subject line
Why not change XXH3 to XXH128? |
There is no XXH3_128 implementation in LLVM and porting it over takes significant work. I'd rather just default to BLAKE3 unless there's a significant performance difference. |
4109ac7
to
152bd3c
Compare
Removed full-stop. |
Is this good to merge now? (I don't have committer access) |
Could you add a release note for this? |
The current default, build-id=fast, is only 8 bytes due to the usage of 64-bit XXH3. This is incompatible with RPM packaging tools which requires >=16 bytes [1]. In Clang the ENABLE_LINKER_BUILD_ID define makes it pass --build-id without a specific hash type. When also defaulting to LLD, this provides a pretty broken default out-of-box. Using XXH3 was a considerable performance advantage when build-id was first implemented, because sha1 was really sha1 and rather slow. Nowadays sha1 is just 160-bit BLAKE3 which is decently fast and not cryptographically broken, so it should be a good default. Note that the default remains "fast" for wasm because sha1 for wasm is still real sha1. Close llvm#43483. [1]: https://github.com/rpm-software-management/rpm/blob/b7d427728b8ba8734ba47d51849a5736bdd727cd/build/files.c#L1883
d8c184e
to
18628b0
Compare
Sorry for the delay; added release notes. |
The current default, build-id=fast, is only 8 bytes due to the usage of 64-bit XXH3. This is incompatible with RPM packaging tools which requires >=16 bytes [1]. In Clang the ENABLE_LINKER_BUILD_ID define makes it pass --build-id without a specific hash type. When also defaulting to LLD, this provides a pretty broken default out-of-box. Using XXH3 was a considerable performance advantage when build-id was first implemented, because sha1 was really sha1 and rather slow. Nowadays sha1 is just 160-bit BLAKE3 which is decently fast and not cryptographically broken, so it should be a good default. Note that the default remains "fast" for wasm because sha1 for wasm is still real sha1. Close llvm#43483. [1]: https://github.com/rpm-software-management/rpm/blob/b7d427728b8ba8734ba47d51849a5736bdd727cd/build/files.c#L1883
The current default, build-id=fast, is only 8 bytes due to the usage of 64-bit XXH3. This is incompatible with RPM packaging tools which requires >=16 bytes 1.
In Clang the ENABLE_LINKER_BUILD_ID define makes it pass --build-id without a specific hash type. When also defaulting to LLD, this provides a pretty broken default out-of-box.
Using XXH3 was a considerable performance advantage when build-id was first implemented, because sha1 was really sha1 and rather slow. Nowadays sha1 is just 160-bit BLAKE3 which is decently fast and not cryptographically broken, so it should be a good default.
Note that the default remains "fast" for wasm because sha1 for wasm is still real sha1.
Close #43483.