Minres / CoreDSL

Xtext project to parse CoreDSL files
Apache License 2.0
16 stars 3 forks source link

Clarify handling of out-of-bounds memory access #107

Open PhilippvK opened 1 year ago

PhilippvK commented 1 year ago

Given a register unsigned<XLEN> X[16] what should happen when someone tries to use X[25] in the behavior? This specific case might occur depending on what we end up for #94.

What do you think? @eyck @AtomCrafty @wysiwyng

eyck commented 1 year ago

This is simply an invalid CoreDSL description so the validator needs to flag it. Since sizes are defined there can be proper bounds checking implemented in the parser/validator. So no backend handling should be needed...

PhilippvK commented 1 year ago

And how about these cases?

AtomCrafty commented 1 year ago

@eyck That is incorrect. I can emit warnings if the value of the index expression is known at validation time, but that will not be the general case.

edit: Just checked and those warnings are already implemented.

AtomCrafty commented 1 year ago

For range accesses into address spaces and bit vectors it currently states that result is undefined if the range falls outside the bounds of the indexed element. I believe this is a sensible rule that should be extended to not only cover the range operator, but also the normal index access operator. Also the specification currently does not mention indexing into arrays, but that should generally follow the same rules as well.

I propose the following changes to the specification:

Currently: OOB Index Access OOB Range Access
Bit Vectors Unspecified Undefined Behavior
Arrays Unspecified Unspecified
Address Spaces Unspecified Undefined Behavior
Proposed: OOB Index Access OOB Range Access
Bit Vectors Undefined Behavior Undefined Behavior
Arrays Undefined Behavior Undefined Behavior
Address Spaces Undefined Behavior Undefined Behavior
eyck commented 1 year ago

And how about these cases?

* `X[rd]` with `unsigned<5> rd`

* `X[rd+2]` with `unsigned<4> rd`

Hmm, in the first case the range is 0-31. It can be detected and reported If the length of X is smaller than 32. The second one needs some further semantic analysis (or constraint propagation) but basically the resulting index range is 2-17. If the length of X is larger than 18 this is fine, otheriwse an error or warning should be reported. Still in the frontend (@AtomCrafty independetly what is being implemented) as static analysis.

Aside of this I agree with @AtomCrafty that we should update the spec.

AtomCrafty commented 1 year ago

From a language perspective, X[25] is a perfectly valid expression that will simply cause undefined behavior at runtime. We can add warnings if we detect an obvious out-of-bounds access, but it does not qualify as an error.

Using the type of the indexing expression to deduce possible value ranges like you suggest is not supported by the frontend. That would be a major new feature we could possibly add down the road, but it is not feasible right now. Also note that this could only ever be used to flag a small subset of expressions as "definitely unproblematic" and would not be generally able to decide whether an access could possibly be out of bounds at runtime. Deciding that would require a symbolic solver, which is far outside the scope of a language frontend.