Skip to content

Commit bc426f0

Browse files
AndyAyersMSCopilot
andcommitted
Address PR #128967 review feedback
- compileresult.cpp findRelocationInRange: guard against size_t wraparound on rangeLo + windowSize, and short-circuit when windowSize == 0 so the helper's half-open range contract is self-contained (Copilot review). - neardiffer.cpp compareOffsetsWasm: clarify the docstring and inline comment to note that the recorded reloc payload sits at opcode-byte + 1 for un-prefixed i32.const/call/etc., but at opcode-byte + 2 for un-prefixed memory ops (i32.load/i32.store/...) after the align u32. The existing immediate-window scan covers both cases unchanged; this is a comment-only update (adamperlin review). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent cf1d517 commit bc426f0

2 files changed

Lines changed: 28 additions & 16 deletions

File tree

src/coreclr/tools/superpmi/superpmi-shared/compileresult.cpp

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1182,9 +1182,17 @@ const Agnostic_RecordRelocation* CompileResult::findRelocationInRange(size_t ori
11821182
if (RecordRelocation == nullptr)
11831183
return nullptr;
11841184

1185+
if (windowSize == 0)
1186+
return nullptr;
1187+
11851188
const size_t rangeLo = originalBufferStart + originalBufferOffset;
11861189
const size_t rangeHi = rangeLo + windowSize;
11871190

1191+
// Guard against size_t wraparound (rangeLo near SIZE_MAX). Bail rather than
1192+
// scan with an inverted half-open range that could match unrelated relocs.
1193+
if (rangeHi < rangeLo)
1194+
return nullptr;
1195+
11881196
const unsigned int count = RecordRelocation->GetCount();
11891197
const Agnostic_RecordRelocation* items = RecordRelocation->GetRawItems();
11901198
for (unsigned int i = 0; i < count; i++)

src/coreclr/tools/superpmi/superpmi/neardiffer.cpp

Lines changed: 20 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -645,12 +645,14 @@ bool NearDiffer::compareOffsets(
645645
//
646646
// For wasm32, integer immediates that materialize JIT handles (function indices,
647647
// type indices, memory addresses, globals) are emitted as 5-byte ULEB128/SLEB128
648-
// placeholders (`80 80 80 80 00`) immediately following the 1-byte opcode, with
649-
// the real target recorded in a separate reloc table keyed by the byte offset of
650-
// the *payload* (i.e. opcode-byte + 1). Both baseline and diff JITs write the
651-
// same placeholder bytes, so byte-by-byte comparison naturally succeeds at reloc
652-
// sites without any patching. Any immediate mismatch surfaced to this comparator
653-
// therefore falls into one of three cases:
648+
// placeholders (`80 80 80 80 00`) that the JIT separately records in a reloc
649+
// table. For un-prefixed forms like `i32.const`/`call`, the placeholder sits at
650+
// `opcode-byte + 1`. For un-prefixed memory ops (`i32.load`, `i32.store`, ...),
651+
// the layout is `<opcode> <align>:u32 <reloc-offset>:u32`, so the relocatable
652+
// payload sits at `opcode-byte + 2`. Either way, both baseline and diff JITs
653+
// write the same placeholder bytes, so byte-by-byte comparison naturally
654+
// succeeds at reloc sites without any patching. Any immediate mismatch surfaced
655+
// to this comparator therefore falls into one of three cases:
654656
//
655657
// 1. Hard-coded non-reloc literal that genuinely differs across baseline/diff.
656658
// This is a real codegen change; return false.
@@ -664,10 +666,11 @@ bool NearDiffer::compareOffsets(
664666
// Return false.
665667
//
666668
// `blockOffset` is the opcode-byte offset within the framed buffer, per the
667-
// coredistools Wasm32 contract. The recorded reloc location is the payload byte
668-
// (one byte past the opcode for the un-prefixed `i32.const`/`call`/etc. that the
669-
// JIT actually relocates today). We search a small window starting at
670-
// `blockOffset + 1` to be robust to multi-byte prefixed opcodes.
669+
// coredistools Wasm32 contract. The recorded reloc location is the payload byte,
670+
// which may be `opcode-byte + 1` (un-prefixed i32.const/call/etc.),
671+
// `opcode-byte + 2` (un-prefixed loads/stores after the align u32), or further
672+
// for prefixed opcodes. We search the entire immediate window
673+
// `[blockOffset+1, blockOffset+instrLen)` to handle all of these uniformly.
671674
//
672675
bool NearDiffer::compareOffsetsWasm(
673676
const void* payload, size_t blockOffset, size_t instrLen, uint64_t offset1, uint64_t offset2)
@@ -679,12 +682,13 @@ bool NearDiffer::compareOffsetsWasm(
679682

680683
const DiffData* data = (const DiffData*)payload;
681684

682-
// Payload byte for an un-prefixed wasm opcode is opcode-byte + 1. For prefixed
683-
// forms (0xFC/0xFD/0xFE) the payload sits one or more bytes deeper, all of
684-
// which lie inside [blockOffset+1, blockOffset+instrLen). Walk that range and
685-
// accept the first match; mismatched relocations are extremely rare in
686-
// practice and a window-scan is much simpler than threading prefix-length
687-
// information through coredistools.
685+
// The relocated payload byte may live at `opcode-byte + 1` (un-prefixed
686+
// i32.const/call/etc.), `opcode-byte + 2` (un-prefixed memory ops, after
687+
// the align u32), or even deeper for prefixed forms (0xFC/0xFD/0xFE).
688+
// Rather than threading per-opcode payload offsets through coredistools,
689+
// walk the entire immediate window [blockOffset+1, blockOffset+instrLen)
690+
// and accept the first reloc found. Multiple relocs in a single instruction
691+
// are not emitted today.
688692
const size_t windowStart = blockOffset + 1;
689693
const size_t windowSize = (instrLen > 1) ? (instrLen - 1) : 0;
690694

0 commit comments

Comments
 (0)