This is an archive of the discontinued LLVM Phabricator instance.

[Support/BLAKE3] Rename blake3_* -> llvm_blake3_* to avoid symbol collisions
ClosedPublic

Authored by jbms on Feb 13 2023, 10:35 PM.

Details

Summary

While some blake3 symbols are already prefixed, a number of symbols with hidden visibility have been left without an llvm_ prefix. This results in symbol collisions when statically linking llvm into a binary that also uses the external blake3 library.

Diff Detail

Event Timeline

jbms created this revision.Feb 13 2023, 10:35 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 13 2023, 10:35 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
jbms requested review of this revision.Feb 13 2023, 10:35 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 13 2023, 10:35 PM
jbms retitled this revision from Rename blake3_* -> llvm_blake3_* to avoid symbol collisions to [Support/BLAKE3] Rename blake3_* -> llvm_blake3_* to avoid symbol collisions.Feb 13 2023, 10:38 PM
MaskRay added a subscriber: MaskRay.EditedFeb 13 2023, 10:56 PM

LGTM in spirit, it matches what we did to blake3.c and other libraries (e.g. Support/regexec.c) How do you verify that all blake3 non-local symbols are now prefixed?
I think this is a good candidate for the release/16.x branch (https://github.com/llvm/llvm-project/issues?q=is%3Aopen+is%3Aissue+milestone%3A%22LLVM+16.0.0+Release%22)

edit: I can verify with llvm-nm -jgU /tmp/Rel/lib/libLLVMSupport.a | grep '^blake3'

Can you fix blake3_neon.c for aarch64 as well?

jbms added a comment.Feb 13 2023, 11:11 PM

Can you fix blake3_neon.c for aarch64 as well?

I believe that is already fixed by the #define in blake3_impl.h

akyrtzi added a comment.EditedFeb 13 2023, 11:36 PM

.s files can use the preprocessor as well. I'd recommend to add a file that do the renames using #define, like in blake3_impl.h, and include it in all the .s files.
That way the changes to the original files is minimized, which is useful for pulling in future changes from the original repo.

jbms added a comment.Feb 14 2023, 12:12 AM

.s files can use the preprocessor as well. I'd recommend to add a file that do the renames using #define, like in blake3_impl.h, and include it in all the .s files.
That way the changes to the original files is minimized, which is useful for pulling in future changes from the original repo.

The MSVC ".asm" files aren't preprocessed, though. Given that, it may be simpler to just use a sed script to apply the renames to all the assembly files when importing (which is what I used to generate the patch in the first place):

sed -i -e 's/\b\(_\?\)blake3_/\1llvm_blake3_/g' *.S *.asm

The MSVC ".asm" files aren't preprocessed, though.

Good point, for now let's use #defines for .S files and rename inline for the .asm files; later on we could consider setting up manual preprocessing via CMake for the .asm files as well.
Essentially my suggestion is to pull the renaming #defines out of blake3_impl.h and into a separate file, then have it included from blake3_impl.h and the .S files. The .asm files get renames inline for now.

jbms updated this revision to Diff 497396.Feb 14 2023, 11:22 AM

The MSVC ".asm" files aren't preprocessed, though.

Good point, for now let's use #defines for .S files and rename inline for the .asm files; later on we could consider setting up manual preprocessing via CMake for the .asm files as well.
Essentially my suggestion is to pull the renaming #defines out of blake3_impl.h and into a separate file, then have it included from blake3_impl.h and the .S files. The .asm files get renames inline for now.

Done.

MaskRay accepted this revision.Feb 14 2023, 11:29 AM

The MSVC ".asm" files aren't preprocessed, though.

Good point, for now let's use #defines for .S files and rename inline for the .asm files; later on we could consider setting up manual preprocessing via CMake for the .asm files as well.
Essentially my suggestion is to pull the renaming #defines out of blake3_impl.h and into a separate file, then have it included from blake3_impl.h and the .S files. The .asm files get renames inline for now.

This plan looks good to me. I am fine with changing .asm files. If we use a CMake approach, gn and bazel (unsupported build systems) users need to port this logic as well...

This revision is now accepted and ready to land.Feb 14 2023, 11:29 AM

@mstorsjo Will you be kindly to test this on Windows to see that there is no more external blake3_* definitions? :)

@mstorsjo Will you be kindly to test this on Windows to see that there is no more external blake3_* definitions? :)

This built fine in both mingw/x86_64 and msvc/x86_64 configs.

akyrtzi added inline comments.Feb 14 2023, 1:58 PM
llvm/lib/Support/BLAKE3/blake3_impl.h
35

Could you exclude the whitespace changes from this file?

jbms updated this revision to Diff 497459.Feb 14 2023, 3:19 PM
jbms marked an inline comment as done.
akyrtzi accepted this revision.Feb 14 2023, 3:21 PM

Thank you for the fix! 🙇🏻‍♂️

@mstorsjo Will you be kindly to test this on Windows to see that there is no more external blake3_* definitions? :)

This built fine in both mingw/x86_64 and msvc/x86_64 configs.

Thanks for verification!

Can you fix blake3_neon.c for aarch64 as well?

I confirm that this renames all blake3_* external symbols on Linux aarch64 when neon is enabled.

This revision was landed with ongoing or failed builds.Feb 14 2023, 4:21 PM
This revision was automatically updated to reflect the committed changes.