Limit symbol visibility in the shared libraries - #2822
Conversation
dosubot
Bot
added
the
size:L
WalkthroughAdds platform-specific linker configuration for symbol visibility control to C API and JNI libraries using linker scripts for Android/Unix and exported symbol lists for Apple platforms. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @csukuangfj, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refines the build process for the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable optimization by limiting symbol visibility in the shared libraries for Linux, Android, and macOS. This should help reduce binary size and improve loading times. The approach using linker scripts and exported symbol lists is sound. My review includes a couple of suggestions to improve the maintainability of the newly added generate.sh scripts by parameterizing hardcoded paths and refining a pattern match, making them more robust for future development.
| #!/usr/bin/env bash | ||
| set -ex | ||
|
|
||
| nm -g ../../build/lib/libsherpa-onnx-c-api.dylib | awk '$2=="T" && $3 ~ /^_Sherpa/ {print $3}' | sort > ./sherpa-onnx-symbols-c.exp | ||
|
|
There was a problem hiding this comment.
To improve maintainability, it's better to avoid hardcoding the path to the dylib. The current path ../../build/lib/libsherpa-onnx-c-api.dylib is brittle as it assumes a specific build directory structure. A more robust approach is to pass the library path as a command-line argument to the script.
| #!/usr/bin/env bash | |
| set -ex | |
| nm -g ../../build/lib/libsherpa-onnx-c-api.dylib | awk '$2=="T" && $3 ~ /^_Sherpa/ {print $3}' | sort > ./sherpa-onnx-symbols-c.exp | |
| #!/usr/bin/env bash | |
| set -ex | |
| if [ $# -ne 1 ]; then | |
| echo "Usage: $0 /path/to/libsherpa-onnx-c-api.dylib" | |
| exit 1 | |
| fi | |
| dylib_path="$1" | |
| nm -g "${dylib_path}" | awk '$2=="T" && $3 ~ /^_Sherpa/ {print $3}' | sort > ./sherpa-onnx-symbols-c.exp | |
| #!/usr/bin/env bash | ||
| set -ex | ||
|
|
||
| nm -g ../../build/lib/libsherpa-onnx-jni.dylib | awk '$2=="T" && $3 ~ /^_Java_com_k2fsa/ {print $3}' | sort > ./sherpa-onnx-symbols.exp | ||
|
|
There was a problem hiding this comment.
This script can be improved for better maintainability in two ways:
- Avoid Hardcoded Path: The path
../../build/lib/libsherpa-onnx-jni.dylibis hardcoded, which is brittle. It's better to pass the path as a script argument. - More Specific
awkPattern: The pattern^_Java_com_k2fsais a bit too broad. Using a more specific pattern like^_Java_com_k2fsa_sherpa_onnx_would be more precise and safer for future changes.
Here is a suggested version that addresses both points:
| #!/usr/bin/env bash | |
| set -ex | |
| nm -g ../../build/lib/libsherpa-onnx-jni.dylib | awk '$2=="T" && $3 ~ /^_Java_com_k2fsa/ {print $3}' | sort > ./sherpa-onnx-symbols.exp | |
| #!/usr/bin/env bash | |
| set -ex | |
| if [ $# -ne 1 ]; then | |
| echo "Usage: $0 /path/to/libsherpa-onnx-jni.dylib" | |
| exit 1 | |
| fi | |
| dylib_path="$1" | |
| nm -g "${dylib_path}" | awk '$2=="T" && $3 ~ /^_Java_com_k2fsa_sherpa_onnx_/ {print $3}' | sort > ./sherpa-onnx-symbols.exp | |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
sherpa-onnx/jni/CMakeLists.txt (1)
51-59: JNI symbol visibility wiring looks correct; consider target_link_options laterThe platform conditions and linker flag wiring to
sherpa-onnx-symbols.lds/.explook sound and match the new scripts. For future cleanup, you might prefertarget_link_options(sherpa-onnx-jni PRIVATE "LINKER:…")overLINK_FLAGS, but this is not blocking.sherpa-onnx/c-api/generate.sh (1)
1-5: Mac-only helper script is fine; optionally parameterize the library pathThe
nm | awk | sortpipeline is straightforward and matches the_Sherpa*naming scheme. If you plan to use this more widely, consider accepting the library path as an argument or documenting that it’s macOS-only and assumes../../build/lib/libsherpa-onnx-c-api.dylib.sherpa-onnx/jni/sherpa-onnx-symbols.lds (1)
1-6: Consider explicitly exporting JNI_OnLoad / JNI_OnUnload for robustnessThe version script nicely restricts exports to
Java_com_k2fsa_sherpa_onnx*. If you ever addJNI_OnLoad/JNI_OnUnload, they’ll currently be hidden. It’s cheap and safe to future‑proof by adding them toglobal::{ global: Java_com_k2fsa_sherpa_onnx*; + JNI_OnLoad; + JNI_OnUnload; local: *; };sherpa-onnx/c-api/CMakeLists.txt (1)
15-23: C API symbol visibility config is consistent; could be scoped to shared buildsThe linker script hookup for
sherpa-onnx-c-apilooks correct and matches the JNI approach. To make intent clearer, you might move this block inside the existingif(BUILD_SHARED_LIBS)or switch totarget_link_optionsso it’s explicit that it only matters for shared libraries.sherpa-onnx/jni/sherpa-onnx-symbols.exp (1)
1-110: Symbol list looks consistent; consider automating drift checksThe
_Java_com_k2fsa_sherpa_onnx_*entries look consistent with the JNI surface. To avoid future drift, it’d be helpful to standardize on regenerating this viagenerate.sh(or add a simple CI check thatnmoutput matches this list). Also, if you introduceJNI_OnLoad/JNI_OnUnloadat some point, remember to add them here as well as to the version script.sherpa-onnx/c-api/sherpa-onnx-symbols-c.exp (1)
1-149: C API export list aligns with naming scheme; keep it in sync with generate.shThe
_SherpaOffline*/_SherpaOnnx*entries look coherent and match the prefixes used in the version script and generator. As you evolve the C API, it’d be good to standardize on regenerating or validating this list viac-api/generate.shso newly added public functions don’t get accidentally hidden (or vice versa).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
sherpa-onnx/c-api/CMakeLists.txt(1 hunks)sherpa-onnx/c-api/generate.sh(1 hunks)sherpa-onnx/c-api/sherpa-onnx-symbols-c.exp(1 hunks)sherpa-onnx/c-api/sherpa-onnx-symbols-c.lds(1 hunks)sherpa-onnx/jni/CMakeLists.txt(1 hunks)sherpa-onnx/jni/generate.sh(1 hunks)sherpa-onnx/jni/sherpa-onnx-symbols.exp(1 hunks)sherpa-onnx/jni/sherpa-onnx-symbols.lds(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-06T04:18:47.981Z
Learnt from: litongjava
Repo: k2-fsa/sherpa-onnx PR: 2440
File: sherpa-onnx/java-api/src/main/java/com/k2fsa/sherpa/onnx/core/Core.java:4-6
Timestamp: 2025-08-06T04:18:47.981Z
Learning: In sherpa-onnx Java API, the native library names in Core.java (WIN_NATIVE_LIBRARY_NAME = "sherpa-onnx-jni.dll", UNIX_NATIVE_LIBRARY_NAME = "libsherpa-onnx-jni.so", MACOS_NATIVE_LIBRARY_NAME = "libsherpa-onnx-jni.dylib") are copied directly from the compiled binary filenames and should not be changed to match other libraries' naming conventions.
Applied to files:
sherpa-onnx/c-api/generate.shsherpa-onnx/jni/sherpa-onnx-symbols.ldssherpa-onnx/jni/generate.shsherpa-onnx/jni/CMakeLists.txtsherpa-onnx/c-api/CMakeLists.txtsherpa-onnx/jni/sherpa-onnx-symbols.exp
📚 Learning: 2025-08-06T04:23:50.237Z
Learnt from: litongjava
Repo: k2-fsa/sherpa-onnx PR: 2440
File: sherpa-onnx/java-api/src/main/java/com/k2fsa/sherpa/onnx/core/Core.java:4-6
Timestamp: 2025-08-06T04:23:50.237Z
Learning: The sherpa-onnx JNI library files are stored in Hugging Face repository at https://huggingface.co/csukuangfj/sherpa-onnx-libs under versioned directories like jni/1.12.7/, and the actual Windows JNI library filename is "sherpa-onnx-jni.dll" as defined in Core.java constants.
Applied to files:
sherpa-onnx/jni/sherpa-onnx-symbols.ldssherpa-onnx/jni/generate.shsherpa-onnx/jni/CMakeLists.txtsherpa-onnx/c-api/CMakeLists.txtsherpa-onnx/c-api/sherpa-onnx-symbols-c.ldssherpa-onnx/jni/sherpa-onnx-symbols.exp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: ubuntu-latest Release static tts-ON
- GitHub Check: ubuntu-latest Debug shared tts-ON
- GitHub Check: ubuntu-latest Debug shared tts-OFF
- GitHub Check: rknn shared ON
- GitHub Check: ubuntu-latest Release shared tts-ON
- GitHub Check: ubuntu-latest Release static tts-OFF
- GitHub Check: rknn shared OFF
- GitHub Check: Debug shared-OFF tts-ON
- GitHub Check: Debug shared-ON tts-ON
- GitHub Check: Debug shared-OFF tts-OFF
- GitHub Check: Release shared-OFF tts-ON
- GitHub Check: Debug shared-ON tts-OFF
- GitHub Check: Release shared-ON tts-OFF
- GitHub Check: Release shared-ON tts-ON
- GitHub Check: Release shared-OFF tts-OFF
- GitHub Check: Debug shared tts-ON
- GitHub Check: Release shared tts-OFF
- GitHub Check: Release static tts-OFF
- GitHub Check: Release static tts-ON
🔇 Additional comments (2)
sherpa-onnx/c-api/sherpa-onnx-symbols-c.lds (1)
1-8: Version script looks good; ensure prefixes are reserved for public C APIRestricting exports to
SherpaOnnx*andSherpaOffline*is a reasonable convention and aligns with the.explist. Just make sure those prefixes are reserved for public C API symbols so you don’t accidentally export internal helpers on Linux/Android that aren’t present in the macOS list.sherpa-onnx/jni/generate.sh (1)
1-5: JNI export generator is fine; be aware of macOS-only and JNI_OnLoadThe script correctly captures
_Java_com_k2fsa…JNI entrypoints for the.explist. If you ever rely onJNI_OnLoad/JNI_OnUnload, you’ll need to add them manually (they won’t be picked up by this filter) and keep them in sync with the version script.
There was a problem hiding this comment.
Pull request overview
This PR implements symbol visibility control for shared libraries to limit the exposed symbols to only those necessary for JNI and C API consumers. This reduces symbol pollution, minimizes the risk of symbol conflicts, and can potentially improve dynamic linker performance.
Key changes:
- Added linker version scripts (
.lds) for Linux/Android and exported symbol lists (.exp) for macOS - Added helper shell scripts to generate symbol export lists from built libraries
- Updated CMakeLists.txt files to apply symbol visibility constraints at link time
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
sherpa-onnx/jni/sherpa-onnx-symbols.lds |
Linker version script for JNI library limiting visibility to Java_com_k2fsa_sherpa_onnx* symbols on Linux/Android |
sherpa-onnx/jni/sherpa-onnx-symbols.exp |
macOS exported symbols list for JNI library containing 110 JNI native method symbols |
sherpa-onnx/jni/generate.sh |
Helper script to extract JNI symbols from built macOS library |
sherpa-onnx/jni/CMakeLists.txt |
Applies linker version script on Linux/Android and export list on macOS for JNI library |
sherpa-onnx/c-api/sherpa-onnx-symbols-c.lds |
Linker version script for C API library limiting visibility to SherpaOnnx* and SherpaOffline* symbols |
sherpa-onnx/c-api/sherpa-onnx-symbols-c.exp |
macOS exported symbols list for C API library containing 149 C API function symbols |
sherpa-onnx/c-api/generate.sh |
Helper script to extract C API symbols from built macOS library |
sherpa-onnx/c-api/CMakeLists.txt |
Applies linker version script on Linux/Android and export list on macOS for C API library |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| set_target_properties(sherpa-onnx-jni PROPERTIES | ||
| LINK_FLAGS "-Wl,--version-script=${CMAKE_CURRENT_SOURCE_DIR}/sherpa-onnx-symbols.lds" | ||
| ) | ||
| elseif(APPLE) | ||
| set_target_properties(sherpa-onnx-jni PROPERTIES | ||
| LINK_FLAGS "-Wl,-exported_symbols_list,${CMAKE_CURRENT_SOURCE_DIR}/sherpa-onnx-symbols.exp" |
There was a problem hiding this comment.
The LINK_FLAGS property is deprecated in modern CMake. Consider using target_link_options() instead for better compatibility with CMake 3.13+. For example:
if(ANDROID OR (UNIX AND NOT APPLE))
target_link_options(sherpa-onnx-jni PRIVATE
"LINKER:--version-script=${CMAKE_CURRENT_SOURCE_DIR}/sherpa-onnx-symbols.lds"
)
elseif(APPLE)
target_link_options(sherpa-onnx-jni PRIVATE
"LINKER:-exported_symbols_list,${CMAKE_CURRENT_SOURCE_DIR}/sherpa-onnx-symbols.exp"
)
endif()This provides better shell escaping and is the recommended approach.
| set_target_properties(sherpa-onnx-jni PROPERTIES | |
| LINK_FLAGS "-Wl,--version-script=${CMAKE_CURRENT_SOURCE_DIR}/sherpa-onnx-symbols.lds" | |
| ) | |
| elseif(APPLE) | |
| set_target_properties(sherpa-onnx-jni PROPERTIES | |
| LINK_FLAGS "-Wl,-exported_symbols_list,${CMAKE_CURRENT_SOURCE_DIR}/sherpa-onnx-symbols.exp" | |
| target_link_options(sherpa-onnx-jni PRIVATE | |
| "LINKER:--version-script=${CMAKE_CURRENT_SOURCE_DIR}/sherpa-onnx-symbols.lds" | |
| ) | |
| elseif(APPLE) | |
| target_link_options(sherpa-onnx-jni PRIVATE | |
| "LINKER:-exported_symbols_list,${CMAKE_CURRENT_SOURCE_DIR}/sherpa-onnx-symbols.exp" |
| set_target_properties(sherpa-onnx-c-api PROPERTIES | ||
| LINK_FLAGS "-Wl,--version-script=${CMAKE_CURRENT_SOURCE_DIR}/sherpa-onnx-symbols-c.lds" | ||
| ) | ||
| elseif(APPLE) | ||
| set_target_properties(sherpa-onnx-c-api PROPERTIES | ||
| LINK_FLAGS "-Wl,-exported_symbols_list,${CMAKE_CURRENT_SOURCE_DIR}/sherpa-onnx-symbols-c.exp" |
There was a problem hiding this comment.
The LINK_FLAGS property is deprecated in modern CMake. Consider using target_link_options() instead for better compatibility with CMake 3.13+. For example:
if(ANDROID OR (UNIX AND NOT APPLE))
target_link_options(sherpa-onnx-c-api PRIVATE
"LINKER:--version-script=${CMAKE_CURRENT_SOURCE_DIR}/sherpa-onnx-symbols-c.lds"
)
elseif(APPLE)
target_link_options(sherpa-onnx-c-api PRIVATE
"LINKER:-exported_symbols_list,${CMAKE_CURRENT_SOURCE_DIR}/sherpa-onnx-symbols-c.exp"
)
endif()This provides better shell escaping and is the recommended approach.
| set_target_properties(sherpa-onnx-c-api PROPERTIES | |
| LINK_FLAGS "-Wl,--version-script=${CMAKE_CURRENT_SOURCE_DIR}/sherpa-onnx-symbols-c.lds" | |
| ) | |
| elseif(APPLE) | |
| set_target_properties(sherpa-onnx-c-api PROPERTIES | |
| LINK_FLAGS "-Wl,-exported_symbols_list,${CMAKE_CURRENT_SOURCE_DIR}/sherpa-onnx-symbols-c.exp" | |
| target_link_options(sherpa-onnx-c-api PRIVATE | |
| "LINKER:--version-script=${CMAKE_CURRENT_SOURCE_DIR}/sherpa-onnx-symbols-c.lds" | |
| ) | |
| elseif(APPLE) | |
| target_link_options(sherpa-onnx-c-api PRIVATE | |
| "LINKER:-exported_symbols_list,${CMAKE_CURRENT_SOURCE_DIR}/sherpa-onnx-symbols-c.exp" |
| #!/usr/bin/env bash | ||
| set -ex | ||
|
|
||
| nm -g ../../build/lib/libsherpa-onnx-jni.dylib | awk '$2=="T" && $3 ~ /^_Java_com_k2fsa/ {print $3}' | sort > ./sherpa-onnx-symbols.exp |
There was a problem hiding this comment.
The script hardcodes the macOS library path and extension (.dylib), making it platform-specific. Consider adding platform detection or documenting that this script is macOS-only. Alternatively, make the script more portable:
if [[ "$OSTYPE" == "darwin"* ]]; then
LIB_EXT="dylib"
else
LIB_EXT="so"
fi
nm -g ../../build/lib/libsherpa-onnx-jni.${LIB_EXT} | awk '$2=="T" && $3 ~ /^_Java_com_k2fsa/ {print $3}' | sort > ./sherpa-onnx-symbols.expNote that on Linux, the underscore prefix may not be present in symbol names.
| nm -g ../../build/lib/libsherpa-onnx-jni.dylib | awk '$2=="T" && $3 ~ /^_Java_com_k2fsa/ {print $3}' | sort > ./sherpa-onnx-symbols.exp | |
| if [[ "$OSTYPE" == "darwin"* ]]; then | |
| LIB_EXT="dylib" | |
| SYMBOL_PREFIX="_" | |
| else | |
| LIB_EXT="so" | |
| SYMBOL_PREFIX="" | |
| fi | |
| nm -g ../../build/lib/libsherpa-onnx-jni.${LIB_EXT} | awk -v prefix="$SYMBOL_PREFIX" '$2=="T" && $3 ~ ("^" prefix "Java_com_k2fsa") {print $3}' | sort > ./sherpa-onnx-symbols.exp |
| nm -g ../../build/lib/libsherpa-onnx-c-api.dylib | awk '$2=="T" && $3 ~ /^_Sherpa/ {print $3}' | sort > ./sherpa-onnx-symbols-c.exp | ||
|
|
There was a problem hiding this comment.
The script hardcodes the macOS library path and extension (.dylib), making it platform-specific. Consider adding platform detection or documenting that this script is macOS-only. Alternatively, make the script more portable:
if [[ "$OSTYPE" == "darwin"* ]]; then
LIB_EXT="dylib"
else
LIB_EXT="so"
fi
nm -g ../../build/lib/libsherpa-onnx-c-api.${LIB_EXT} | awk '$2=="T" && $3 ~ /^_Sherpa/ {print $3}' | sort > ./sherpa-onnx-symbols-c.expNote that on Linux, the underscore prefix may not be present in symbol names.
| nm -g ../../build/lib/libsherpa-onnx-c-api.dylib | awk '$2=="T" && $3 ~ /^_Sherpa/ {print $3}' | sort > ./sherpa-onnx-symbols-c.exp | |
| # Detect platform and set library extension and symbol prefix | |
| if [[ "$OSTYPE" == "darwin"* ]]; then | |
| LIB_EXT="dylib" | |
| SYMBOL_PREFIX="_Sherpa" | |
| else | |
| LIB_EXT="so" | |
| SYMBOL_PREFIX="Sherpa" | |
| fi | |
| nm -g ../../build/lib/libsherpa-onnx-c-api.${LIB_EXT} | awk -v prefix="$SYMBOL_PREFIX" '$2=="T" && $3 ~ ("^" prefix) {print $3}' | sort > ./sherpa-onnx-symbols-c.exp |
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.