Verify initial stack-pointer to be inside static RAM
#41
Conversation
4789857 to
52f513f
Compare
52f513f to
4b625c5
Compare
tests/verify.rs
Outdated
| let bounds = elf::get_bounds(&[data, bss, uninit])?; | ||
|
|
||
| // Is the initial stack-pointer inside 'static RAM'? | ||
| assert!(bounds.contains(&initial_sp)); |
There was a problem hiding this comment.
This is not the correct thing to assert even though the test passes.
the bounds range span all static variables in RAM. flip-link will put the stack right below that. we want to check (assert) that the stack and the static variables do not overlap
the tricky bit is that initial_sp points to the start of the stack but when you use the stack that address will not be written to: data is written to the address immediately below that (this is because on ARM the stack is of the full descending type).
So if the SP is 0x2000_3000 and you push 0xAAAA_AAAA (32-bit word) onto it the memory that is written is:
0x2000_3000this address is not written to0x2000_2FFF= 0xAA0x2000_2FFE= 0xAA0x2000_2FFD= 0xAA0x2000_2FFC= 0xAA- new SP value =
0x2000_2FFC
On the other hand if you have a static variable located at 0x2000_3000 and write to it then address 0x2000_3000 will be written to. Thus initial_sp can be equal to bounds.start().
so the cases here are
initial_sp <= bounds.start= OK (stack is below static variables)initial_sp > bounds.start && initial_sp < bounds.end= BAD (overlap)initial_sp > bounds.end= BAD (stack is above static variables -> if the stack grows it could corrupt static variables)
There was a problem hiding this comment.
Ah kay. Thank you for the elaborate explanation! 😄
There was a problem hiding this comment.
I am thinking of putting this explanation either into a doc-comment or some kind of CONTRIBUTING.md, since I found it very useful. What do you think would be a good place for it?
There was a problem hiding this comment.
I tend to favor 'put it where it's hard to miss' and to me that's usually near the code that implements or verifies the thing because (hurried) devs tend to miss markdown files that are not the README (:raising_hand: ).
A separate markdown file would also be OK but then we need to link back from the related code ("before working on this module / function read some-file.md). Keeping that link up to date could become a chore -- though this code base is small and doesn't change much.
|
bors r+ |
|
Build succeeded: |
This PR verifies that the
initial stack-pointerof each of the four example apps is inside thestatic RAMregion, after being linked withflip-link.I've made the choice to have one test for all example apps, instead of one test for each test. This approach is a bit easier to implement, but has the drawback of less error-resolution. E.g. if one of the examples would fail to compile the tests fail completely, instead of running the other ones successfully.
Depends on #40 and fixes #25.