Add lazy loading support for code objects - #27
Conversation
When addFile is called with lazy=true, only kernel names are indexed from the ELF symbol table (.symtab) — no disassembly runs. Disassembly and source mapping happen on demand when a kernel is first accessed via getKernel(), getKernelLines(), getInstructionsForLine(), or getKernelArguments(). Changes: - kernelDB::addFile: new lazy parameter; indexes via getKernelNamesFromElf - kernelDB::getKernelNamesFromElf: reads .symtab for STT_FUNC/STT_AMDGPU_HSA_KERNEL symbols - kernelDB::ensureKernelLoaded: on-demand disassembly + source mapping for lazy kernels - getKernels() includes lazy kernel names; hasKernel() checks both maps - Python API: KernelDB(lazy=True), add_file(), scan_code_object(), has_kernel() - pybind11: expose addFile, scanCodeObject, hasKernel - pyproject.toml: fix license field format - Add examples/07_lazy_load/ Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ensureKernelLoaded() previously called scanCodeObject() which disassembles every kernel in the .hsaco, defeating lazy loading for code objects with many kernels. Now it calls scanCodeObjectForKernel() which uses parseDisassemblyForKernel() to skip non-target kernels, and scopes DWARF/argument processing to the single loaded kernel. The code object is not marked as fully scanned so other kernels from the same .hsaco can still be loaded on demand. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add getDisassemblyForSymbol() using --disassemble-symbols instead of
full -d, so scanCodeObjectForKernel() only disassembles the requested
kernel
- Fix shell quoting in invokeProgram() to handle kernel names with
parentheses or other shell metacharacters
- Fix trailing space in disassembly_params ("-d " -> "-d")
- Accept STT_OBJECT .kd symbols in getKernelNamesFromElf()
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The demangled kernel name can contain commas (e.g. C++ template args) which --disassemble-symbols interprets as multiple symbol names. Store the raw/mangled ELF symbol name in lazy_kernels_ and pass it to scanCodeObjectForKernel/getDisassemblyForSymbol instead. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The targetKernel parameter passed from scanCodeObjectForKernel is the raw (mangled) ELF symbol name, but parseDisassemblyForKernel compares it against demangled canonical names extracted from the disassembly output. This mismatch caused every kernel to be skipped, resulting in 0 parsed instructions. Normalize targetKernel through demangleName + getKernelName at the top of both parseDisassemblyForKernel and processKernelsWithAddressMap so comparisons use the same canonical form on both sides. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
getLineType dereferenced line.begin() without checking if the string was empty, causing a segfault when getBlockMarkers looped past EOF on disassembly output that contained headers but no kernel sections. Also fix getBlockMarkers to return early on EOF instead of looping forever, and replace the assert(mit != markers.end()) with a graceful skip so release builds don't crash. Add debug logging to ensureKernelLoaded and scanCodeObjectForKernel for diagnosing lazy loading issues. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The disassembly parser searched for '//' comment tokens by incrementing an index without bounds checking, causing a segfault when the disassembly line format didn't include address comments. Add bounds checks to all while-loops that search for '//' tokens in both getBlockMarkers and parseDisassemblyForKernel. Guard stoull calls with try/catch to handle unexpected formats. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- getBlockMarkers: replace assert(tmp.size()==2) with defensive check for _cbranch_ token format; guard against uninitialized iterator when no KERNEL line has been seen yet - getLineType: guard *(--it) against single-character strings - parseDisassemblyForKernel: replace abort() with graceful skip when branch instruction seen without active kernel; guard null current_block before addInstruction call Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
rwvo
left a comment
There was a problem hiding this comment.
This review was written by Claude on behalf of Rene van Oostrum.
Build verification
Omniprobe builds cleanly against this branch and all 27 tests pass (22 handler + 5 Triton integration). The lazy=false default preserves existing behavior — API-compatible, ABI-breaking (acceptable for a submodule built from source).
Lazy loading is interesting for Omniprobe
Omniprobe currently calls scanCodeObject() on first kernel dispatch, which disassembles the entire code object. For hipBLASLt's matrix transform .hsaco (960 kernels), this is expensive when only a handful of kernels are actually dispatched. Switching to addFile(path, agent, filter, lazy=true) would let Omniprobe defer disassembly to per-kernel on-demand loading — a natural fit for its dispatch-driven architecture.
That said, the following issues in the lazy path would need to be resolved first.
Must fix
-
Thread safety in
ensureKernelLoaded()— Two threads callinggetKernel("foo")concurrently will both findfooinlazy_kernels_(shared lock), both disassemble it (no lock), and both calladdKernel(). The secondaddKernel()replaces theunique_ptr, potentially destroying theCDNAKernelthat the first thread already holds a reference to via the returnedCDNAKernel&. kernelDB already exposes ashared_mutexcontract for thread safety — the lazy path needs to uphold it (e.g., a "loading in progress" sentinel so the second thread waits). -
Temp file leak in
getDisassemblyForSymbol()— IfinvokeProgram()returnsfalse, the temp file created bytmpnam()is neverunlinked. -
Unconditional
std::cerrdebug output — Five[KDB]prints to stderr on every lazy load. Should be behind a debug flag or removed.
Should fix
-
No test coverage for the lazy path — At minimum, a Python test that does
add_file(hsaco, lazy=True), verifieshas_kernel()returnsTrue, then callsget_kernel()and checks the result matches eager-loaded output. -
scanCodeObjectForKernel/parseDisassemblyForKernelshould beprivate:— Only called fromensureKernelLoaded(). -
Silent failure in
ensureKernelLoaded()— If disassembly fails, the kernel stays inlazy_kernels_forever. Every subsequentgetKernel()call re-attempts the disassembly, and the caller only sees a misleading "kernel does not exist" exception with no indication the real cause was a disassembly failure. Either remove the entry on failure (fail fast) or propagate the error. -
Replace
tmpnam()withmkstemp()—tmpnamis TOCTOU-racy; the rest of the codebase usesmkstemp-style patterns. -
Code duplication —
parseDisassemblyForKernelduplicates ~170 lines fromparseDisassembly. Consider adding an optionaltargetKernelparameter to the existing function (as was already done forprocessKernelsWithAddressMap).
Nice to have
getDisassemblyForSymbolis missing__attribute__((visibility("default")))— unlikegetDisassemblywhich has it. If internal-only, remove from the public header.- The
pyproject.tomllicense format change is unrelated — consider splitting into its own PR. - Bare
catch(...)blocks swallow all exceptions silently. Catching specific types (std::invalid_argument,std::out_of_range) and logging the malformed input would help debugging.
1. Thread safety: ensureKernelLoaded() now uses a loading_kernels_ set and condition_variable so concurrent threads wait instead of both disassembling the same kernel. 2. Temp file leak: replace tmpnam() with mkstemp() in both getDisassembly() and getDisassemblyForSymbol(); unlink on all paths including failure. 3. Remove all unconditional [KDB] stderr debug prints. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
4. Add test_lazy_loading.py: verifies lazy-loaded kernel discovery, has_kernel(), assembly, and per-line instructions match eager loading. 5. Move scanCodeObjectForKernel and parseDisassemblyForKernel to private. 6. Fail fast in ensureKernelLoaded(): always erase from lazy_kernels_ on failure so subsequent getKernel() calls get a clean error instead of retrying a doomed disassembly. 7. (Already done in must-fix #2: tmpnam -> mkstemp) 8. Eliminate ~170-line duplication: parseDisassembly() now delegates to parseDisassemblyForKernel(text, ""), using the safer bounds-checked version as the single implementation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Split out per reviewer feedback (nice-to-have #10). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
hipcc does not support std::condition_variable_any. Use a separate std::mutex + std::condition_variable for the loading sentinel instead of piggybacking on the shared_mutex. Add missing <condition_variable> include. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Thanks Rene for the thorough review! My agent thanks your agent for the thorough review. 🤖🤝🤖 (We're one review cycle away from the agents just cutting us out entirely.) All must-fix and should-fix items have been addressed, CI is green ✅. Please give it a spin with Omniprobe before we merge to make sure everything's working as intended. Thanks! |
|
My agent was happy with the work of yours and merged the PR. |
Summary
addFile(path, lazy=true)mode that indexes kernel names from ELF symbols without disassembling.ensureKernelLoaded(name)to disassemble a single kernel on demand when it is first dispatched.🤖 Generated with Claude Code