docs(CAVES): address Copilot review nits from #3#6
Merged
Conversation
Three review nits from #3 plus one extra ABI wording fix: - Unify the ADRP/LDR placeholder names in both cave diagrams. The pair previously read `page(HOOK_SLOT_RVA) + lo12(SLOT)` and `page(entry_slot_va) + lo12(slot)`; using two names for the same address obscures the relocation pattern. - Fix the inline code that was broken across a hard line wrap (`-Werror=unused-` / `variable`) so Markdown actually renders the flag as code instead of opening an unterminated backtick span. - Tighten the AAPCS64 wording: W9 isn't an arg-9 slot. The integer argument sequence ends at x7, x8 is the indirect-result register, and x9–x15 are call-clobbered scratch — which is exactly why touching W9 from the entry cave is safe. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Updates docs/CAVES.md to address follow-up review nits from #3 by tightening ABI terminology, unifying placeholder symbol names in the cave diagrams, and fixing a Markdown inline-code rendering issue.
Changes:
- Unify ADRP/LDR placeholder naming in both
observerandentrycave diagrams to reflect a single underlying symbol/address. - Fix a broken inline code span for
-Werror=unused-variableby removing an in-word line break. - Refine AAPCS64 wording to correctly describe
x9–x15/w9as call-clobbered scratch registers rather than argument slots.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| | Substitute argument registers | ❌ (cave restores `x0..x7` before `B orig+4`) | ✅ (cave passes pristine `x0..x7` through and never restores) | | ||
| | Hooks routed through a single shared dispatcher | ✅ (one slot, identified by `W6 = hook_id`) | ❌ (each site has its own slot under an entry-slot table) | | ||
| | `W6` (= 7th C arg) survives across the cave | ❌ (clobbered with `hook_id`) | ✅ (only `W9` is touched, an unused arg-9+ slot under AAPCS64) | | ||
| | `W6` (= 7th C arg) survives across the cave | ❌ (clobbered with `hook_id`) | ✅ (only `W9` is touched, and `x9–x15` are AAPCS64 call-clobbered scratch — never an argument slot) | |
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-up to #3, addressing three Copilot review comments that landed after merge plus one related ABI wording fix.
What
Pure docs; no runtime changes.
Test plan
🤖 Generated with Claude Code