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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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'
.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
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...
@mstorsjo Will you be kindly to test this on Windows to see that there is no more external blake3_* definitions? :)
llvm/lib/Support/BLAKE3/blake3_impl.h | ||
---|---|---|
35 | Could you exclude the whitespace changes from this file? |
I confirm that this renames all blake3_* external symbols on Linux aarch64 when neon is enabled.
Could you exclude the whitespace changes from this file?