detecting unsupported ECALLs and test checking improvements #374
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kpgriesser
commented
Jun 14, 2025
| @@ -29,7 +29,7 @@ | |||
| parser.add_argument("--machine", help="Machine type/configuration", default="[CORES:RV64GC]") | |||
| parser.add_argument("--args", help="Command line arguments to pass to the target executable", default="") | |||
| parser.add_argument("--startSymbol", help="ELF Symbol Rev should begin execution at", default="[0:main]") | |||
| parser.add_argument("--trcStartCycle", help="Starting cycle for rev tracer [default: 0 (off)]") | |||
| parser.add_argument("--trcStartCycle", help="Starting cycle for rev tracer [default: 0 (off)]", default=0) | |||
There was a problem hiding this comment.
This is not related to the PR. Not having the default value causes SST to error out if trcStartCycle is not set because it is set to Python 'None' type. The default=0 disables the tracer
leekillough
requested changes
Jun 15, 2025
src/RevSysCalls.cc
Outdated
| @@ -62,6 +68,7 @@ EcallStatus RevCore::ECALL_io_setup() { | |||
| output->verbose( | |||
| CALL_INFO, 2, 0, "ECALL: io_setup called by thread %" PRIu32 " on hart %" PRIu32 "\n", ActiveThreadID, HartToExecID | |||
| ); | |||
| UNIMPLEMENTED_BEHAVIOR; | |||
There was a problem hiding this comment.
UNIMPLEMENTED();would seem better.
|
Also note that I am getting a strange error in Release builds but not in Debug builds: It happens with |
|
@leekillough I've merged your requested changes and retested on Mac and ubuntu for Release and Debug builds. Everything appears to be passing. Are you still seeing failures for --enabledMemH=0? |
leekillough
approved these changes
Jun 17, 2025
…nt an error string but will continue the simulation. For regression testing, ctest will now search for a fail string. Minor unrelated change to SDL to provide default value for trcStartCycle
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The majority of ecall routines perform no operation but return 'SUCCESS'. This can lead to a false sense of security when writing RV code for REV that that does system calls that are essentially NOPs. This PR provides a compile time overridable macro to print an error string for these ECALLs. Originally I was going to make this a fatal error but it would have required adding a compile pragma to suppress the 'maybe no return' compiler warning. Also, I thought other projects might be running code with a rich mix of 'NOP' system calls that would not appreciate these kinds of failures. In the worst case, we can revert to NOP functionality using CMAKE_CXX_FLAGS = -D _REV_ECALL_OVERRIDE.
For regression testing, the test/CMakeList.txt was updated to provide a fail regular expression that detects these kinds of error strings. I ran the regressions on Ubuntu and MacOs as well as for both Debug and Release compilations and all current tests continue to pass.
I should also mention that the error string check is added to allow self checking tests to print error strings to the log file (standing for console output for now) and generate a regression test failure that is independent from the simulation return status code