tactcomplabs / rev

RISC-V SST CPU Component
Other
17 stars 21 forks source link

Fast print utility #321

Closed kpgriesser closed 4 weeks ago

kpgriesser commented 1 month ago

This custom ECALL is intended to help debug code by providing non-intrusive printf functionality to help speed up rev application and rev test code development. It is currently limited to 1K output chars and takes a format string and up to 6 numeric parameters.

Advantages:

Disadvantages:

Example:

#define printf rev_fast_printf
// ... bunch of code
      printf("Failed: check_data[%d]=0x%lx dram_src[%d]=0x%lx\n",
              i, check_data[i], i, dram_src[i]);

Output:

RevCPU[cpu0:operator():10080000]: <rev-print>Failed: check_data[0]=0xffffffffaced0000 dram_src[0]=0xffffffffaced0000
</rev-print>
leekillough commented 1 month ago

Please wait for my review. I'm chasing a storm.

kpgriesser commented 1 month ago

To John's point, I could add variants which support standard 'NOTICE, WARNING, FATAL' prefixes which would benefit. Since I'm using sst::Output we could use FATAL to exit the simulation ( an improvement over just crashing on an assert ). For these, I would suggest another PR that builds on this one.

leekillough commented 1 month ago

asprintf() can be used to avoid the 1024-character limit. It automatically allocates memory, which should be passed to free(). It returns -1 if there is an error.

leekillough commented 1 month ago

Go ahead if it passes all tests

kpgriesser commented 1 month ago

Keeping this open after finding a fly in the ointment. When compiling with gcc instead of g++ the default function parameters will not compile. For the time being, the function will be omitted from syscalls.h unless c++ is detected.

+#ifdef __cplusplus
 REV_SYSCALL( 9110, int rev_fast_printf(const char* format, uint64_t a1=0, uint64_t a2=0, uint64_t a3=0, uint64_t a4=0, uint64_t a5=0, uint64_t a6=0) );
+#endif
leekillough commented 1 month ago

Keeping this open after finding a fly in the ointment. When compiling with gcc instead of g++ the default function parameters will not compile. For the time being, the function will be omitted from syscalls.h unless c++ is detected.

+#ifdef __cplusplus
 REV_SYSCALL( 9110, int rev_fast_printf(const char* format, uint64_t a1=0, uint64_t a2=0, uint64_t a3=0, uint64_t a4=0, uint64_t a5=0, uint64_t a6=0) );
+#endif

Up to now our syscalls.h API has been C-compatible. Default arguments is a C++ feature.

You can use <stdarg.h> varargs to make it C-compatible, but you'll need to use a sentinel value to indicate the end of list, or make the count depend on an earlier parameter.

leekillough commented 1 month ago

You can use <stdarg.h> varargs to make it C-compatible, but you'll need to use a sentinel value to indicate the end of list, or make the count depend on an earlier parameter.

The vfprintf() functions can be used to pass varargs to your version of printf, and the argument list length will be automatically deduced from the format string. You need to declare your function like this:

#include <stdarg.h>

int rev_fast_printf( const char* format, ... ) {
   va_list args;
   va_start( args, format );
   vfprintf( stdout, format, args );
   va_end( args );
   return 0;
}

This is C-compatible and will accept any number of arguments of any types. You can also use the GCC __attribute__((format)) on the function to enforce matching of format to arguments (see this ).

You might need to use vsnprintf() instead of vprintf() if you need to buffer to a string instead of producing direct IO.

leekillough commented 1 month ago

I am going to modify your changes to allow a general printf format string and work with C.

leekillough commented 1 month ago

Please see this commit:

https://github.com/leekillough/rev/commit/12818964da2110efaa8ef84e6601043f09f9f9f0

It's sort of a hack, but it:

  1. Uses varargs C syntax to pass arguments, exactly how vprintf and related functions work. There is no need to test for C++.
  2. Uses __attribute__((format ...)) to indicate that rev_fast_printf() is a printf()-style function, and produces warnings if the arguments don't match the format string.
  3. Grabs 32- or 64-bit RISC-V ECALL arguments based on XLEN. Right now it really can only work for XLEN-sized integers. I have not tested it with XLEN==32 because my toolchain doesn't support RV32.
  4. Calls asprintf() on the host to automatically allocate memory for the string result, and calls free() if there are no errors.
leekillough commented 1 month ago

On that last asprintf point: I realize it might not exist on MacOS. I'm willing to go back to the 1024 or whatever snprintf buffer limit.

The rest of my changes are much more important.