feat(Singlepass): add config for strict memory boundary checking#6351
feat(Singlepass): add config for strict memory boundary checking#6351
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a configurable “strict memory boundary checks” mode for the Singlepass compiler, allowing RISC-V Singlepass to default to the faster signal-based OOB behavior while preserving the previous always-check behavior behind a flag.
Changes:
- Introduce
strict_memory_boundary_checksonSinglepassconfig (defaultfalse) and document it. - Plumb the flag through Singlepass codegen so memory ops can request explicit bounds checks even for static memories.
- Update RISC-V Singlepass memory codegen to honor
need_check, plus adjust tests/ignores accordingly.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
lib/compiler-singlepass/src/config.rs |
Adds the new config field + builder-style setter. |
lib/compiler-singlepass/src/codegen.rs |
Uses the config flag to force need_check for memory ops. |
lib/compiler-singlepass/src/machine_riscv.rs |
Makes bounds checking conditional on need_check for RISC-V memory ops. |
tests/compilers/issues.rs |
Adds a regression test intended to validate strict bounds behavior. |
tests/ignores.txt |
Adds RISC-V Singlepass ignores for spec tests affected by relaxed boundary behavior; tweaks comment text. |
lib/compiler-singlepass/README.md |
Documents the new option and updates supported target list. |
Comments suppressed due to low confidence (1)
lib/compiler-singlepass/src/machine_riscv.rs:1048
cond(aliased totmp_base) is released viarelease_gpr(cond)and then immediately reused as a scratch register for the alignment check (emit_and(..., Location::GPR(cond))). This breaks the register-usage bookkeeping and is fragile if future changes allocate temps between these points. Delay releasingconduntil after the alignment-check block (or acquire a fresh temp for the alignment check).
self.release_gpr(tmp_bound);
self.release_gpr(cond);
let align = value_size as u32;
if check_alignment && align != 1 {
self.assembler.emit_and(
Size::S64,
Location::GPR(tmp_addr),
Location::Imm64((align - 1) as u64),
Location::GPR(cond),
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| } | ||
| } | ||
|
|
||
| /// Configure whether Singlepass should always emit strict memory boundary |
There was a problem hiding this comment.
Shouldn't a major behavior change like this use the old behaviour by default?
We could switch the default with the next major version?
There was a problem hiding this comment.
Given the change is tightly communicated with our customer, we can do it. Moreover, it's going to align with other Singlepass platforms.
There was a problem hiding this comment.
Our customers are a limited subset of our users though.
In general I'm against significant behaviour changes in minor versions.
Guess should have @syrusakbary chime in.
There was a problem hiding this comment.
Sure, that’s a good point! Keep in mind that the current RISC-V port of Singlepass is still considered experimental, so we may have some flexibility to make this change.
As for minor versions, it’s a bit tricky. On one hand, we aim to do a major release once a year, but in reality, many minor releases introduce breaking changes and arguably should be treated as major versions. This might be worth discussing further.
syrusakbary
left a comment
There was a problem hiding this comment.
The strict memory boundary checking is what MemoryStyle::Dynamic implies.
All MemoryStyle::Dynamic memories must do strict memory boundary checking, so we don't need an extra setting?
Yes, that behavior has been preserved.
The situation comes from a divergence in the original Singlepass RISC-V implementation compared to x86_64 and AArch64. On those architectures, When I added |
So far, we've been using the strict boundary checking for the RISC-V target of the Singlepass compiler. Works fine, however, it makes the memory operations rather slow and so, similarly to other targets, let's rely on the access violation memory signals.
@tsahee @wakabat
For preserving the old behavior, I added a new config option
strict_memory_boundary_checks.