Print more meaningful error message when __tls_base export is missing and tls symbols are used.
#5879
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR improves error messaging when the __tls_base export is missing from a module that uses thread-local storage (TLS) symbols. Previously, after PR #5852 made this an error condition, the error message was generic. This change provides more specific information by identifying which TLS symbol requires the __tls_base export.
Key changes:
- Added a new
MissingTlsBaseExporterror variant toLinkErrorthat includes the module and symbol name - Renamed
TlsSymbolWithoutTlstoNoTlsBaseGlobalExportinResolveErrorfor clarity - Replaced panicking
.expect()calls with proper error handling that convertsNoTlsBaseGlobalExportto the more informativeMissingTlsBaseExport
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lib/wasix/src/state/linker.rs
Outdated
| } | ||
| }; | ||
|
|
||
| // .expect("Internal error: bad in-progress symbol resolution"); |
There was a problem hiding this comment.
Remove this commented-out code. The error handling has been properly replaced with the match statement above, so this comment serves no purpose and should be deleted.
| // .expect("Internal error: bad in-progress symbol resolution"); |
|
@copilot fix the clippy error |
Co-authored-by: zebreus <[email protected]>
Fix clippy error and remove commented code
| Ok(e) => e, | ||
| Err(ResolveError::NoTlsBaseGlobalExport) => { | ||
| return Err(LinkError::MissingTlsBaseExport( | ||
| import.module().to_string(), | ||
| import.name().to_string(), | ||
| )); | ||
| } | ||
| Err(e) => { | ||
| panic!("Internal error: failed to resolve exported symbol: {e}") | ||
| } |
There was a problem hiding this comment.
@Arshia001 I don't really like having to match here, because that will still cause a panic if we unwrap that error type on another path. Do you have a better idea on how to handle this?
Previously it was fine not to exports
__tls_basefrom the main module, #5852 changed that to be an error. This PR prints a more meaningful error message when the__tls_baseexport is missing but tls symbols are used.