fix: symbols / sections followups - #95
Merged
Merged
Conversation
The ELF symbol collection filtered on STT_FUNC, so symbols() returned only functions on Linux, while Windows (export directory) and macOS (LC_SYMTAB) both return variables as well. As a result, get_symbol() could never resolve a namespaced global variable through its demangled name fallback on Linux, even though the README documents exactly that usage. Variables at global scope were unaffected because the Itanium ABI leaves them unmangled, so dlsym resolved them directly, which is why no test caught this. Also collect STT_OBJECT, and add a namespaced variable test. Signed-off-by: Martin Olivier <martin.olivier@live.fr>
get_symbol() re-demangled every symbol during its fallback lookup, even though symbols() already stores a demangled_name for each entry. On MSVC that was especially costly, since each demangle_symbol call performs two UnDecorateSymbolName calls. Reuse the stored demangled_name and skip C symbols explicitly. The previous code relied on demangle_symbol returning an empty string to skip them implicitly, whereas demangled_name falls back to the raw symbol name, so the filter on symbol_type::CPP preserves the behavior. Signed-off-by: Martin Olivier <martin.olivier@live.fr>
Left over from the section collection work: it sat in the middle of the file rather than in the include block, and nothing in tests.cpp uses it. Signed-off-by: Martin Olivier <martin.olivier@live.fr>
The version was hardcoded in the header, CMakeLists.txt, the README badge, the conan and FetchContent snippets, and the example, with nothing keeping them in sync. Expose DYLIB_VERSION_MAJOR/MINOR/PATCH from dylib.hpp and have CMakeLists.txt parse them, so the build system can no longer disagree with the header. The macros also let users feature-detect an API at compile time, which was not possible before. Bump to 3.1.0: sections(), collection_error and section_collection_error are new public API. Signed-off-by: Martin Olivier <martin.olivier@live.fr>
It only reads m_handle, and every other accessor (get_symbol, symbols, sections) is already const, so a const library was unusable with it. Signed-off-by: Martin Olivier <martin.olivier@live.fr>
The size of the symbol table is deduced from the distance between DT_SYMTAB and DT_STRTAB, which assumes the string table directly follows the symbol table. Nothing in the ELF specification guarantees that ordering, and the reverse layout made the subtraction underflow into a huge unsigned value, walking far past the symbol table. Bail out when the layout does not hold, and iterate with an unsigned counter matching the type of the loop bound. Signed-off-by: Martin Olivier <martin.olivier@live.fr>
The POSIX constructor opened the library file and kept the descriptor until destruction, even though it is only needed by symbols() on macOS and by sections() on macOS and Linux. A process loading many plugins paid one descriptor per loaded library for nothing. Keep the resolved path instead and open the file through an RAII guard only while it is read. This also keeps both methods const and free of mutable state, so concurrent const calls stay safe. Note that the constructor no longer reports an unreadable library file: that error now surfaces from symbols() or sections(), which are the operations that actually need to read it. Signed-off-by: Martin Olivier <martin.olivier@live.fr>
The example was never built by CI, and its CMakeLists fetches a released
tag rather than the local tree, so nothing kept it working. It calls
get_variable("example::magic"), a namespaced variable, which was broken
on Linux until the previous STT_OBJECT fix, without any job noticing.
Add a job that overrides the fetch with FETCHCONTENT_SOURCE_DIR_DYLIB so
the example is built and run against the current branch on the three
supported platforms, and refresh the version string it prints.
Signed-off-by: Martin Olivier <martin.olivier@live.fr>
The example README shows the expected output of a run, which includes the version string returned by example::dylib::info(). That string was bumped to 3.1.0 in lib.cpp, leaving the documented output stale. Signed-off-by: Martin Olivier <martin.olivier@live.fr>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-ups from a review pass over the codebase after the section collection feature landed. One commit per issue to make review easier.
Bug fix
symbols()returned only functions on Linux (bc7d2ec)The ELF symbol collection filtered on
STT_FUNC, while Windows (export directory) and macOS (LC_SYMTAB) both return variables too. As a resultget_symbol()could never resolve a namespaced global variable through its demangled name fallback on Linux, even though the README documents exactly that usage.Variables at global scope were unaffected, because the Itanium ABI leaves them unmangled and
dlsymresolved them directly, which is why no test caught it. Theexample/program hits this: it callsget_variable("example::magic").Now collects
STT_OBJECTas well, with a namespaced variable added to the test library.Other changes
203b6d5get_symbol()re-demangled every symbol during its fallback, althoughsymbols()already stores ademangled_name. On MSVC that meant fourUnDecorateSymbolNamecalls per symbol instead of two.2846138<iostream>include sitting mid-file in the tests.2773130dylib.hppnow exposesDYLIB_VERSION_MAJOR/MINOR/PATCHand CMake parses them from the header. The macros also let users feature-detect an API at compile time. Bumped to 3.1.0, sincesections(),collection_errorandsection_collection_errorare new public API.41b8dabnative_handle()is nowconst, like every other accessor.29e68c8DT_SYMTAB/DT_STRTABdistance, which assumes an ordering the spec does not guarantee. The reverse layout underflowed into a huge unsigned value. Bails out instead, and iterates with a matching unsigned counter.aa3856asymbols()on macOS andsections()on macOS and Linux. Now opened through an RAII guard only while the file is read, so a process loading many plugins no longer pays a descriptor per library.4b50716FETCHCONTENT_SOURCE_DIR_DYLIBand builds/runs it against the branch on all three platforms.Behavior change worth noting
aa3856ameans the constructor no longer reports an unreadable library file. That error now surfaces fromsymbols()orsections(), the operations that actually need to read it.Validation
Locally on macOS: clean build with
DYLIB_WARNING_AS_ERRORS=ONacross C++11/14/17/20, all 19 tests pass, clang-format and clang-tidy clean, and the example builds and runs against the local tree. The descriptor change was verified by inspecting/dev/fdbefore and aftersymbols()/sections().The Linux and Windows legs are unverified locally and rely on CI. The new Windows
examplejob in particular is unproven; if it trips, the likely cause is the binary path or the DLL-next-to-exe assumption rather than library code.