Add stack overflow detection to sw-emulator #1785
Conversation
clundin25
force-pushed
the
stack-overflow-monitor
branch
from
6534995 to
15c3882
Compare
clundin25
requested review from
korran,
fdamato,
jlmahowa-amd,
mhatrevi,
jhand2,
swenson,
vsonims,
rusty1968,
bluegate010,
ajisaxena and
JohnTraverAmd
as code owners
clundin25
force-pushed
the
stack-overflow-monitor
branch
from
15c3882 to
c1836a9
Compare
clundin25
force-pushed
the
stack-overflow-monitor
branch
from
c1836a9 to
d6f8397
Compare
clundin25
force-pushed
the
stack-overflow-monitor
branch
from
d6f8397 to
a99ab23
Compare
clundin25
force-pushed
the
stack-overflow-monitor
branch
from
a99ab23 to
eca5e52
Compare
| @@ -262,7 +401,24 @@ impl<TBus: Bus> Cpu<TBus> { | |||
| /// | |||
| /// * `RvException` - Exception with cause `RvExceptionCause::IllegalRegister` | |||
| pub fn write_xreg(&mut self, reg: XReg, val: RvData) -> Result<(), RvException> { | |||
| self.xregs.write(reg, val) | |||
| // XReg::X2 is the sp register. | |||
| if reg == XReg::X2 { | |||
There was a problem hiding this comment.
Theoretically, we can set the sp to another value as a temporary, or when initializing (we set it to 0 at the beginning of the ROM), and this isn't necessarily a stack overflow, so we might have to be a little careful here.
There was a problem hiding this comment.
That's a good point. An actionable change would be adding a bounds check ensuring sp is not greater than the start of the stack region.
With that change edge cases will at least result in a failure.
| @@ -262,7 +401,24 @@ impl<TBus: Bus> Cpu<TBus> { | |||
| /// | |||
| /// * `RvException` - Exception with cause `RvExceptionCause::IllegalRegister` | |||
| pub fn write_xreg(&mut self, reg: XReg, val: RvData) -> Result<(), RvException> { | |||
| self.xregs.write(reg, val) | |||
| // XReg::X2 is the sp register. | |||
There was a problem hiding this comment.
It might be good to document why this works: LLVM appears to always pre-allocate the stack by subtracting, and then using only positive relative offsets to SP. (Is that guaranteed though?)
There was a problem hiding this comment.
I'll take a closer look at this.
Added stack monitoring and overflow detection to the SW emulator
If a stack overflow is detected, the emulator will panic and cause test failure.
This resolves #1735