-
Notifications
You must be signed in to change notification settings - Fork 41
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add ISA test #29
base: main
Are you sure you want to change the base?
Add ISA test #29
Conversation
f11cf92
to
44b3b0b
Compare
[[cheri::interrupt_state(disabled)]] void *get_interrupts_disabled_sentry() | ||
{ | ||
void *returnCap; | ||
asm volatile("cjal %0, 4" : "=C"(returnCap)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is beautifully horrifying, can you add a comment explaining it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is just a jump and link to the next instruction, putting the link cap in returnCap
. I guess might be better written with local label rather than hardcoded offset?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably "cjal %0, 1f;\n1:"
is the right thing to do, but if you want to be extra horrifying, there's GCC's asm goto
... :D
If we ever have a programming guide, we should point to this example as something really contra-indicated. Would you object to a comment here and on the similar method below that this is deliberately unsafe handling of a dangerous thing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or maybe even simplified to &get_interrupts_disabled_sentry
given the way the ABI works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally, this is really great. The combinator approach works out really nicely, even if C++ will never be as nice as Haskell. ;)
A small pile of nits, but nothing big.
[[cheri::interrupt_state(disabled)]] void *get_interrupts_disabled_sentry() | ||
{ | ||
void *returnCap; | ||
asm volatile("cjal %0, 4" : "=C"(returnCap)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably "cjal %0, 1f;\n1:"
is the right thing to do, but if you want to be extra horrifying, there's GCC's asm goto
... :D
If we ever have a programming guide, we should point to this example as something really contra-indicated. Would you object to a comment here and on the similar method below that this is deliberately unsafe handling of a dangerous thing?
* We call this with values we expect to fault and want to know the address | ||
* of the faulting instruction. Since it is so simple it will have no | ||
* prelude so we can use the address of the function as the address of the | ||
* store. Obviously we can't allow it to be inlined. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
I'm not sure about the names; these are not intrinsically going to fault, they're just used that way ("He's not evil, he's just written that way")... maybe
perform_store
or something like that? -
Perhaps this and
faulting_load
should be in assembler rather than relying on the (sensible, probably not going to change, but still not contractual) behavior of the compiler?
* flexible ways before attempting tests. | ||
*/ | ||
template<class T> | ||
using CapabilityFilter = Capability<T> (*)(Capability<T>); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This really is Haskell in C++! 👍
*/ | ||
void do_load_test(CauseCode expectedFault, | ||
CapabilityFilter<int *> baseFilter, | ||
size_t anExpectedMCause = MCauseCheri) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very minor nit, but maybe the argument order should be baseFilter, expectedFault, anExpectedMCause
, so that it's read as something like "I expect a load with this filter to cause this fault and this code"?
// Storing untagged cap via data-only cap is allowed. | ||
do_store_test( | ||
std::nullopt, | ||
permission_filter<PermissionSet{Permission::Load, Permission::Store}>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not identity_filter
?
* nops so that we can test things like half of an instruction being in | ||
* bounds. Clang doesn't seem to emit a prelude for this function. If it | ||
* does (e.g. at -O0) our expected error PC calculations will be wrong due | ||
* to alignment. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe these, too, should be in assembler?
c1c6ad6
to
3da6479
Compare
This specifically aims to test features that are not likely to occur in normal operation, such as exceptions. Hits quite a few potential corner cases but some are best tested in bare metal assembly and can't be easily tested in an RTOS tests.
Co-authored-by: Nathaniel Filardo <[email protected]>
The static casts in the pointer DebugFormatArgumentAdapters prevented function pointers from being printed as the compiler does not permit static casts from function pointer to void*. Removing them seems to work fine so not sure what they were there for.
Still some TODOs to resolve.
… review. Also address a few other review comments.
// sentry {}", reinterpret_cast<void*>(interruptsDisabledSentry.get())); | ||
// TEST(interruptsDisabledSentry.type() == 2, | ||
// "Expected type 2 but got {}", | ||
// interruptsDisabledSentry.type()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there no good way to get these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got the linker error in the comment. Suspect it might be a bug @davidchisnall ?
This adds a test that attempts to trigger various ISA corner cases and increase test coverage.