Repository navigation
[rocjitsu][DBI][Pass kernarg to probe 2/4] Instrument with a prologue - #12114
Conversation
accauble
changed the title
github-actions
Bot
added
documentation
✅ All Checks Passed — Ready for Review
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🎉 All checks passed! This PR is ready for review. |
accauble
changed the title
atgutier
left a comment
atgutier
left a comment
There was a problem hiding this comment.
Just some agent comments:
MAJOR
emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/code/patch/instrumentor.cpp:1142 — Pointer arguments
can produce a prologue without a kernarg wrapper
Reasoning: A LogBufferPtrLo or LogBufferPtrHi request activates a prologue that loads a payload
pointer and a saved kernarg pointer from offsets beyond the original kernarg image. On an ordinary
launch, the kernarg SGPR still points to the original image, so those loads can read past it; the
prologue then replaces the guest’s kernarg pointer with the loaded value.
Evidence: entry_prologue.cpp:133-165 emits the loads and pointer restore. entry_prologue.h:73-76
explicitly says that Instrumentor does not yet emit the matching .rocjitsu.kernarg record. The new
instrumentor tests cover rejection paths but have no accepted pointer argument or dispatch case
(tests/patch/instrumentor_test.cpp:2271-2278).
Suggested fix: Emit and consume the DBI wrapper contract before accepting these argument sources, or
reject them until that path exists. Add a test that patches and dispatches a kernel with a kernarg
pointer.
Confidence: High for the built-in launch paths; an external caller could construct a matching
wrapper independently.
emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/code/patch/entry_prologue.cpp:22 — Indirect SGPR writes
can overwrite persistent entry storage
Reasoning: Storage selection places the pointer pair above the highest encoded SGPR operand. A
kernel’s s_movrel* instruction can instead select an SGPR through M0 at runtime. For example, with
an encoded base of s4 and M0 selecting s6, the planner can choose s6 for persistent storage, which
the kernel then overwrites before a later probe reads it.
Evidence: analysis/liveness.cpp:165-190 scans only decoded operands. The existing probe analysis
explicitly identifies scalar relative access as absent from that model and rejects it (patch/
probe_live_in.cpp:41-64,184-187). The new kernel storage path has no equivalent gate.
Suggested fix: Reject kernels with indirect scalar register access when planning entry storage,
unless the reachable register range can be bounded. Add a kernel test whose relative write reaches
the proposed pair.
Confidence: High for the analysis gap and collision mechanism; no runtime reproduction was run.
It's a good point, but I'm not constructing a kernarg wrapper yet. I have added a TODO to make sure the two place get connected.
Addressed! |
e17fa69 to
62430a9
Compare
kuhar
left a comment
kuhar
left a comment
There was a problem hiding this comment.
This would be safer if Instrumentor::patch() rejected LogBufferPtrLo / LogBufferPtrHi until the wrapper contract is enforced. The TODO at emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/code/patch/instrumentor.cpp:1156 still leaves callers with a successful executable ELF and no matching wrapper metadata or launch guard. The builders can remain available for the runtime follow-up.
The missing wrapper has an observable effect on this head: a straight-line gfx950 simulator reproduction passes with a manually supplied wrapper. Dispatching the same patched code with the original kernarg pointer changes it from 0x02000000 to 0xdeadbeef and passes 0xbad to the probe, reading sentinels beyond the declared 16-byte kernarg image. Please cover both the wrapper requirement and successful wrapped execution in regression tests.
6b44d47 to
5261860
Compare
Okay added to |
accauble
changed the title
atgutier
left a comment
atgutier
left a comment
There was a problem hiding this comment.
Agent identified another issue that I think we probably want to fix:
[MAJOR] emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/code/patch/instrumentor.cpp:759 — A branch back
to the kernel entry reruns the prologue
Reasoning: The first prologue run loads the log pointer from the kernarg wrapper, then restores the
guest’s original kernarg pointer. If the kernel branches back to its entry, the second run reads
wrapper fields through that original pointer, corrupting the log pointer and kernarg pointer.
Evidence: instrumentor.cpp:693-715 does not reject an entry with a predecessor, and
instrumentor.cpp:1439-1442 splices the prologue over that entry. entry_prologue.cpp:175-183 performs
the loads and pointer restore. instrumentor.h:453-456 permits wrapper-supplying callers to emit this
path through patch_with_debug_summaries(); the normal patch() path rejects it. The PR’s docs/dbi-
design.md:408 also identifies reentry as unguarded.
Suggested fix: Place the prologue on a dispatch-only path, or reject kernels that can return to the
entry until that path exists. Add a regression with a branch back to entry and distinct wrapper and
guest kernarg contents.
Confidence: High. The failure follows from the emitted control flow; I did not run it on the PR
head.
As described in a comment hidden between a host of other comments, I ended up doing the dispatch-only fix in a separate PR in this stack, PR #12541. Didn't do it directly here because the PR was getting too unwieldy for my tastes, but I plan to merge the stack at the same time so we don't have a lull where the prologue can be re-entered. |
atgutier
left a comment
atgutier
left a comment
There was a problem hiding this comment.
Got it. Thanks! This LGTM other than the re-entry issue, and if it is fixed-forward this can proceed.
…tion * Logging buffer will be passed via kernarg and we need a persistent SGPR pair for it throughout the instrumented run. Reserve it at the kernel entry. * Also reserve a temporary pair at the kernel entry to restore the pointer to the kernel's original kernargs. We will be using a wrapper to pass the logging buffer ptr.
* log buffer pointer is assumed to be stored in a persistent register pair which was chosen as a dead register for the kernel * reject a probe body that writes to this pair. Not the final design, since this is too conservative. Better design will choose dead register pair that is dead for kernel AND probe and fall back to spilling the register pair if that is not possible
* Instrument kernel entry with the prologue that loads the log
buffer pointer into the persistent SGPR pair.
* Added planner for that instrumentation and do that if any probe
requests the buffer ptr as an argument
* Choose the register pair against what the probes clobber so
that we can actually instrument the kernel with the probes
- Eventual backup plan will be to use kernel-dead reg and spill
before probe calls
- Decode the probe clobber summary once rather than per site
* Cave structure now [probe bodies][entry prologue][trampolines]
* Checks for indirect writes to the log buffer register pair * Do not instrument kernels if the entry block has a predecessor of if the entry falls in the middle of a block so that it does not get run a second time * Add TODO to make sure if prologue written, then kernarg wrapper needs to be written
* Update patch() to refuse probes that read the LogBufferPtr until a runtime build the kernarg wrapper. patch_with_debug_summaries (called only by patch and unit tests) not changed so that tests can still happen. * Also reject indirect control flow where we don't know the target (since it could be the entry)
* Instead of hacking checks that will limit instrumentation, undo the checks that will make sure the prologue is not executed more than once * Follow-up commits will implement dispatch-only prologue
* Extracts an "Spgr Tail Reserve", which was already in use by DBT. DBT uses 8, not 6, however. Keeping DBT the same for now but set things up to easily change back to something not DBT-specific
* Trampoline builder requires the same logic as entry prologue (from an earlier PR, but still reasonably in the scope of this PR) so extract the logic and have both call it
e98bf2c to
0eb81ae
Compare
|
Sorry to everyone for the merge. The intention was to squash. |
#12541) ## Motivation PR #12114 placed the prologue at the kernel entry, which made it susceptible to being run multiple times (an error). This PR replaces this with a separate stub and redirecting the kernel descriptor to it. This way, the prologue is run before the kernel starts and avoids reentry. ## Issue Tracking Part of Issue #8879. ## Testing Updated tests to reflect the new layout.
TheRock Submodule Bump ActivityNewest first
|
Motivation
This is part 2 of a stack of 3 PRs that is meant to implement necessary infrastructure to pass a pointer for the logging buffer from the host to the probe. The intended design is to pass this via a kernarg, which needs to be read and saved in a persistent register pair at the beginning of the kernel (called the prologue).
This part is the meat of the implementation while consuming (testing) happens in part 3. This PR is in charge of:
Technical Details
LogBufferPtrLoandLogBufferPtrHi. Instrument with the prologue if any probe declares these.Issue Tracking
Part of Issue #8879.
Testing