[dv,sw] C messaging flow: part 1 (C side) #259

sriyerg · 2019-09-24T20:45:49Z

Updated make flow to pre-process the C sources before compiling
Added sw/lib/mprintf.c with variadic function to print formatted
string to UART
Added sw/lib/mprintf.h with MPRINTF() macro that switches between UART
based printing and DV printing
Updated linker scripts to reserve upper 32bytes of ram for C<->SV
messaging

TODOs:

Make flow update from PR [dv,sw] Makefile refactor #230 which is yet to be merged
Get all C sources to print with MPRINTF macro instead of uart_send_*()
(next PR)

What's in part 2:

python script that hacks pre-processed C output to extract C formatted
messages into data sructures that SV can lookup

sw/boot_rom/boot_rom.c

sw/lib/dv_mprintf.h

sw/boot_rom/rom_link.ld

sw/lib/mprintf.c

sw/lib/dv_mprintf.h

tjaychen · 2019-09-24T23:10:31Z

@sriyerg
the functionality of all this looks pretty good to me. Thanks for doing this!
I have a few small comments, although I am unsure if what I am suggesting would actually be better (the amount of code might be less, but there would be a bit more latency)

One thing I'm not too clear on, what happens when the SW side writes more than 15 arguments? It doesn't seem like compile would fail, so would we just get a weird output?

@moidx @gkelly
do you guys have any thoughts?

moidx

Thanks for working on this @sriyerg!

I added some comments wrt build dependency management. I'd like to keep the libraries target independent as much as possible.

I will try this out on an FPGA next.

sw/lib/mprintf.h

moidx · 2019-09-25T01:32:30Z

sw/lib/mprintf.h

+#define CONCAT2(a, b) CONCAT1(a, b)
+
+// if DV_SIM, then use DV_MPRINTF macro, else call mprintf() function
+#ifdef DV_SIM


if DV_SIM, then probably better to not include this file, and use this file as the C interface for FPGA/Verilator/Silicon?

The build system should be able to select dependencies based on target.

I understand what needs to change - thanks (wanted a quick proof-of-concept PR out for review).

moidx · 2019-09-25T01:36:11Z

sw/lib/uart.c

@@ -73,3 +74,12 @@ int uart_rcv_char(char *c) {
  *c = REG32(UART_RDATA(0));
  return 0;
 }
+
+#ifndef DV_SIM
+void uart_mprintf(int sev, const char *fmt, ...) {


Move uart_mprintf to mprintf.{h,c} and have mprintf.c include uart.h that way this will only get compiled by dependencies relying on mprintf.o.

Reworked the implementation - move the msg printing to sw/util and added target specific headers that are selectively picked up. PTAL, thanks!

Done.

Thanks for working on this @sriyerg!

I added some comments wrt build dependency management. I'd like to keep the libraries target independent as much as possible.

I will try this out on an FPGA next.

I have reworked the placement of the sources based on our DIF discussions on how we should be laying out the sources. PTAL - i think this aligns well with what we had initially decided.

moidx · 2019-09-25T01:36:54Z

sw/lib/mprintf.c

+
+#include "mprintf.h"
+
+#ifndef DV_SIM


This is something better handled at the build system dependency level.

Done, PTAL.

sw/boot_rom/rom_link.ld

sw/opts.mk

tjaychen · 2019-09-26T21:13:16Z

sw/exts/common/link.ld

@@ -6,13 +6,34 @@ GROUP( -lc -lgloss -lgcc -lsupc++ )
 SEARCH_DIR(.)
 __DYNAMIC  =  0;

+#ifdef DV_SIM


generally speaking i really like this approach, but I'm a bit split on whether we should include ifdefs in the link file, or have them specific to each target.

@moidx
@gkelly
do you guys have any guidance here? Is there a best practice?

Linker scripts traditionally do not support macros in them, but they are a valid input for the preprocessor util (cpp). What I am doing here is passing it through cpp and generating the output, which is devoid of any ifdefs. Linking stage uses the cpp output. see sw/rules.mk lines 29 and 51.

The only thing added as ifdef is the message section. Beside that there is no other modification. I'd prefer not creating target specific linker file since it creates a maintenance overhead (any updates to the 'original' would have to be replicated to target specific linker scripts as well).

ah o i see that makes sense.
That's okay with me, but I think it's probably best if the sw experts weigh in. I have seen in cases like this another link file is INCLUDED. So I'm just not sure what the preferred method is.

I am ok either way. We can start with preprocessor and switch to some other autogen script if things get too complex.

@gkelly to provide recommendation also.

I will prefer removing the #ifdef DV_SIM altogether. The existence of this fake mem makes 0 difference to the final image (since the msg_data section is removed altogether when the elf is converted to bin). Only difference is in DV sims, msg prints will result in strings written to the msg_data section during compile time, whereas in FPGA/Verilator/real FW builds, the msg_data section will remain empty. Will leave the decision to you though.

sw/boot_rom/srcs.mk

tjaychen · 2019-09-26T21:24:10Z

this looks pretty good to me. Thanks Sri for doing this! This is quite cool and I am looking forward to using it :)

tjaychen · 2019-09-26T22:03:21Z

approve from my end, would recommend waiting for a few others to chime in.

sw/boot_rom/boot_rom.c

moidx

Thanks @sriyerg!

I added some comments for re coding style guide, and some other comments re use of FPGA target, and verbosity of header info.

moidx · 2019-09-27T18:18:58Z

sw/boot_rom/rom_link.ld

 }

 /* Memory Allocation */
 _heap_size = 0xe000;
 _stack_size = 0x2000;
-_stack_start = ORIGIN(ram) + _heap_size + _stack_size;
+/* Reserving upper 32 bytes of ram for DV */
+_stack_start = ORIGIN(ram) + _heap_size + _stack_size - 0x20;


Can you make the - 0x20 portion configurable by DV_SIM?

moidx · 2019-09-27T18:24:18Z

sw/exts/common/link.ld

@@ -6,13 +6,34 @@ GROUP( -lc -lgloss -lgcc -lsupc++ )
 SEARCH_DIR(.)
 __DYNAMIC  =  0;

+#ifdef DV_SIM


I am ok either way. We can start with preprocessor and switch to some other autogen script if things get too complex.

@gkelly to provide recommendation also.

moidx · 2019-09-27T18:30:28Z

sw/util/fpga/msg_print.h

+
+// use predefined macros in ../msg.h with standard header (recommended)
+#define MSG_HEADER(msg_type, verbosity) \
+  msg_type verbosity ": (" __FILE__ ":" _xstr(__LINE__) ")"


I'd probably put the file info behind a verbose compile logging flag.

#if defined(MSG_VERBOSE_HEADER) #define MSG_HEADER(msg_type, verbosity) msg_type verbosity ": (" __FILE__ ":" _xstr(__LINE__) ")" #else #define MSG_HEADER(msg_type, verbosity) msg_type verbosity ": " #endif

Added MSG_HEADER_INCL_FILE_FILE to make it more precise and meaningful.

moidx · 2019-09-27T18:34:01Z

sw/Makefile

@@ -11,6 +11,7 @@
 ##               SW_SRCS to be built for that SW test.                                            ##
 ## SW_NAME:      This is the name of the SW test begin run. Optional if SW_DIR name (last dir) is ##
 ##               the same as the SW test name.                                                    ##
+## TARGET:       Target for which the sw is cross-compiled - dv|fpga                              ##


Can you add verilator too? Or rename fpga as something more generic, e.g. hw? Any inputs here from others?

hw is way too generic (dv sim pertains to hw too). Plus, our directory structure might get confusing (hw/, sw/, sw/util/hw, etc). Typical for asic design development is simulation (dv) and emulation (fpga). Emmulation efforts are used for testing full SW builds. I dont see Verilator as an independent platform having unique set of requirements from a SW build perspective (correct me if I am wrong). We could perhaps build SW for sim or emu efforts (ensure both modes work with Verilator based efforts) and use either 'mode' for Verilator based testing?

Naming things is always hard so I get that there's no ideal answer here, but "hw" does sound like a better name for me. fpga/ASIC/verilator should look the same to software (assuming we're targeting the same version of the IP...) and so a generic "hw" term makes sense to my mind.

In dv, we are indeed simulating the hw. So from my perspective, the dichotomy is more towards "simulation" and "production" SW build. Regardless, the targets for "simulation" will continue to be "dv", "fpga" and "verilator" and each will possibly have a different set of requirements.

From the msg flow perspective, there are currently 2 approaches - print via uart (used by simulation/fpga, simulation/verilator and production) and print via 'elf' flow (used by simulation/dv, and can be potentially used by simulation/verilator as well).

Rather than tying the msging flow to DV, I made it as an independent setting in the Makefile called MSG_FLOW, which is set to uart by default. Setting TARGET=dv will switch MSG_FLOW to elf. I also made associated changes to the linker scripts.

How we should organize the TARGETs and what should be called what is discussion we need to have. I will prefer that discussion to not block the submission of this PR. Please let me know if this works for the time being.

i agree with Sri. There are probably going to be more cases down the line, where due to sheer design size, the FPGA will either not have some components or have different clock speeds from verilator. It seems good to de-couple those from specific features (like the MSG_FLOW).

moidx · 2019-09-27T18:40:01Z

sw/util/fpga/msg_print.h

+// Licensed under the Apache License, Version 2.0, see LICENSE for details.
+// SPDX-License-Identifier: Apache-2.0
+
+#ifndef _MSG_PRINT_H_


It would be nice if these files were in a more generic location. fpga, verilator and silicon will most likely consume this header.

@moidx
Sri and I talked about this and actually explicitly split these out to a sw/util directory, with the intent of these being non-target related. What do you think?

So, you are saying to move the code inside sw/util/fpga to sw/util?

o oops sorry. So i think msg_print.h is meant to be the target specific utilities. There's a different one for dv / fpga, but they dont really relate to hardware, so we decided to stick them in util.

msg.h is the more general one and is placed in sw/util i think.

Yep, this comment is more about the fact that util/fpga is used for fpga, verilator and silicon targets. I am not sure if util/hw would work better? In this case we'd have util/dv and util/hw. uti/hw/msg_print.h uses uart.h which is hw, so kind of makes sense?

Just a quick note on this - I created an PR #292 for the SV side (which monitors writes to SW_MSG_ADDR and prints the msg to the sim log file). The SV msg monitor is written as a pure Verilog module so it should technically work for Verilator sim as well. Lets talk more tomorrow and decide how to solve this. My thought is, we should use the ELF file based 'backdoor' msg print flow for DV AND Verilator runs and use the UART method for FPGA runs.

I'd prefer Verilator to just be left as is since it will give us better system test coverage.

I think maybe we can ping Philipp / Pirmin on this one. We are not really big users of verilator anyways, so it'd be good to get their opinion and how they think this plays out.

we = design / dv on our end.

moidx · 2019-09-27T18:48:06Z

sw/util/fpga/msg_print.h

+
+// print msgs with custom header (this will break DV flow!)
+#define MSG_PRINT(msg_header, fmt, ...) \
+  msg_print(uart_send_char, msg_header, fmt VA_ARGS(__VA_ARGS__));


Since you are using uart_send_char, can you include the #include "uart.h" header? This will help us better keep track of dependencies.

moidx · 2019-09-27T18:53:26Z

sw/util/msg.c

+
+#include <stdarg.h>
+
+static void _print_dec(void (*output)(char), unsigned int d) {


nit: can you name this _print_digit or _print_ascii_num? print_dec is a little confusing.

i vote for _print_digit, that does sound pretty good

moidx · 2019-09-27T18:57:30Z

sw/util/msg.c

+            } break;
+            case 'x':
+            case 'X': {
+              unsigned int n = va_arg(va, unsigned int);


'X' should be upper case hex. I think we want to stick to conventional string formats since this is going to be referenced in many places.

moidx · 2019-09-27T18:58:22Z

sw/util/msg.c

+              char c = va_arg(va, int);
+              output(c);
+            } break;
+            case 'h': {  // little endian hex dump


Same comment as above h and H are not standard. We should add print buffer functions instead.

@moidx
do you think we should just reference some open source version of print?

I am ok with just making this version more standard.

I will prefer submitting this PR and making any further changes needed as a part of next PR if you are okay with it (can file issues if needed).

sw/util/msg.h

asb

Sorry if I missed the earlier discussion, but is it necessary to have all the preprocessor magic vs just having a normal function with va_start/va_arg etc?

asb · 2019-10-01T12:35:20Z

sw/Makefile

@@ -11,6 +11,7 @@
 ##               SW_SRCS to be built for that SW test.                                            ##
 ## SW_NAME:      This is the name of the SW test begin run. Optional if SW_DIR name (last dir) is ##
 ##               the same as the SW test name.                                                    ##
+## TARGET:       Target for which the sw is cross-compiled - dv|fpga                              ##


Naming things is always hard so I get that there's no ideal answer here, but "hw" does sound like a better name for me. fpga/ASIC/verilator should look the same to software (assuming we're targeting the same version of the IP...) and so a generic "hw" term makes sense to my mind.

asb · 2019-10-01T12:51:48Z

sw/util/dv/msg_print.h

+#define _WR_VA_ARG_N(...) \
+  _WR_VA_ARG_(CONCAT2(_WR_VA_ARG, VA_NARGS(__VA_ARGS__)), __VA_ARGS__)
+
+// print msgs with a cusotm header


Do you think the SW pieces build for FPGA / Verilator / final silicon should be identical? @asb / @moidx

I am not in favor of hw (we are after all, verifying hw in DV as well). To me, this sounds like a dichotomy between 'simulation' and 'production' software builds, the former being used in DV and the latter being used in FPGA/Verilator/silicon.

If yes, then to me, the following makes more sense -
SW_BUILD_MODE=sim|prod

OR

SW_BUILD_MODE=dv_sim|prod

SW_BUILD_MODE=sim (or dv_sim) will exclusively be used by DV efforts (I would argue that Verilator falls into this as well and the DV msg flow will work for Verilator as well, but upto the Verilator users to decide).

I would assume that to not be the case - I think FPGA/Verilator/DV/Silicon will have different requirements, so we should perhaps continue with TARGET=dv|verilator|fpga|silicon.

As for msging flow, I can use something else for differentiation instead of TARGET (perhaps MSG_FLOW=elf|uart)

would it be easier if we planned for a quick sync between all of us?

I agree with Sri that keeping SW pieces identical for FPGA / silicon / verilator will probably be a tall order in the long run. If nothing else they'll likely vary in clock speeds and perhaps how some of the more 'analog' modules are modeled.

I also think the idea of making specific functions features that each TARGET can select seems a lot more flexible.

I am also in favor of handling configuration options at the feature level, since it seems to yield the most flexible build implementation. Expanding the previous example:

TARGET=dv ifeq ($(TARGET), "dv") MSG_FLOW = "elf" else ifeq ($(TARGET), "fpga") MSG_FLOW = "uart" endif

asb · 2019-10-01T16:03:01Z

Hi Sri - we've been discussing this a bit at lowRISC. We're finding it pretty hard to review without a high level summary of the goals, requirements etc. Would you be able to write up a brief rationale / design doc?

sriyerg · 2019-10-01T18:19:10Z

Hi Sri - we've been discussing this a bit at lowRISC. We're finding it pretty hard to review without a high level summary of the goals, requirements etc. Would you be able to write up a brief rationale / design doc?

sure, will do.

sriyerg · 2019-10-01T20:30:01Z

sw/boot_rom/rom_link.ld

+_flash_start = 0x20000000;
+_flash_end = _flash_start + _flash_size;
+
+#if MSG_FLOW == elf


replaced DV_SIM with MSG_FLOW flag. this can be turned on independently for any target (dv/verilator/verifying full production fw stack in dv, etc).

- Added generic sw/util/msg.{c, h} with variadic function to print formatted string via real hardware - Added standard implementation-independent msg print macros in msg.h (MSG_INFO, MSG_ERROR etc) - These standard msg print macros rely on additional macros defined in implementation/flow-specific macros defined in sw/util/msg_<flow>/msg_print.h - by default, 'uart' flow is selected, so the msgs are printed char by char via UART using the generic function defined in msg.c - for DV (or simulation really), added MSG_FLOW=elf where sw/util/msg_elf/msg_print.h header is picked, which converts msg prints to writes to pre-determined addr in ram - Updated linker scripts to reserve upper 32bytes of ram for C<->SV messaging

moidx

Pending for feedback from design review before proceeding. Thanks

gkelly · 2019-10-04T19:44:40Z

My concerns with this change are that it adds an API that's hard to satisfy, and one that isn't very 'familiar', by which I mean it's not a libc interface and doesn't consume a libc interface. If we want to take any of the code written against this API and and run it under a different harness (or an OS) then we'll need to merge this code there or replicate the macro-based API.

It's also fairly opinionated in the logging prefixes and levels. This isn't necessarily a bad thing, but the way this is layered seems like it's very tightly coupled.

How does this sound: split this into a libc-like layer (puts, printf, etc) and a 'msg' layer that does the logging verbosity, prefix, etc. If we add a little bit of logic to the puts/printf layer that tests if the format string (or written string) is in the NOLOAD section then we can use your bypass to print it out. Otherwise, it behaves normally.

In my eyes this has two benefits: separation of the concerns of logging decoration and actual IO, and switching to a libc API that's familiar enough that we can use or reproduce it reliably later.

Thoughts?

gkelly · 2019-10-04T19:39:22Z

sw/exts/common/link.ld

+    flash  (rx)     : ORIGIN = _flash_start, LENGTH = _flash_end
+#if MSG_FLOW == elf
+    /* Fake memory for extracting messages from print strings from elf */
+    msg_mem(!r!w!x) : ORIGIN = _msg_mem_start, LENGTH = _msg_mem_size


Does this need to have a MEMORY section, or can it just be marked (NOLOAD)?

I think it can be. I will give it a try.

gkelly · 2019-10-04T19:39:45Z

sw/exts/common/link.ld

@@ -101,6 +139,12 @@ SECTIONS
    {
        [ .stabstr ]
    }
+
+#if MSG_FLOW == elf
+    .msg_data _msg_mem_start : {


I think this can be made (NOLOAD) and then it doesn't need to be stripped with objcopy later.

My concerns with this change are that it adds an API that's hard to satisfy, and one that isn't very 'familiar', by which I mean it's not a libc interface and doesn't consume a libc interface. If we want to take any of the code written against this API and and run it under a different harness (or an OS) then we'll need to merge this code there or replicate the macro-based API.

It's also fairly opinionated in the logging prefixes and levels. This isn't necessarily a bad thing, but the way this is layered seems like it's very tightly coupled.

How does this sound: split this into a libc-like layer (puts, printf, etc) and a 'msg' layer that does the logging verbosity, prefix, etc. If we add a little bit of logic to the puts/printf layer that tests if the format string (or written string) is in the NOLOAD section then we can use your bypass to print it out. Otherwise, it behaves normally.

In my eyes this has two benefits: separation of the concerns of logging decoration and actual IO, and switching to a libc API that's familiar enough that we can use or reproduce it reliably later.

Thoughts?

The msg print APIs (MSG_INFO*, MSG_WARNING, MSG_ERROR and MSG_FATAL) which is what is exposed, do have a printf-like interface. I am not sure I understand what's different about those that's making them incompatible with libc.

The logging, prefixes, header construction etc does form the 'msg' layer and is fully customizable based on the target specific requirements (you could define all of those to be empty strings if they are deemed unnecessary) and are intended to happen behind-the-scenes when these APIs are invoked. These macros finally invoke the MSG_PRINT() macro which is intended to be implementation-specific. It can very well be defined to call libc-like functions underneath.

In a way, there is already a separation between msg layer and the libc layer. Only difference being the msg layer (macro APIs) is what is being exposed for use and the libc layer being called underneath. If we go the other way around, I am not sure how we can support other things. For DV, the environment is very resource constrained (when simulating the whole chip in RTL cycle by cycle, every bit of processing on the CPU adds significant about of test run time (which is of no value to the actual test). The macro based solution gives us the fastest possible implementation in terms of sim run time.

Re. your point about plucking this code out and running it on a different harness - is your thought process to cherry-pick individual sources from the shared lib code rather than the whole thing (I would expect you would rather want to build the whole library together as one piece, which will include the APIs as well).

My concerns with this change are that it adds an API that's hard to satisfy, and one that isn't very 'familiar', by which I mean it's not a libc interface and doesn't consume a libc interface. If we want to take any of the code written against this API and and run it under a different harness (or an OS) then we'll need to merge this code there or replicate the macro-based API.
It's also fairly opinionated in the logging prefixes and levels. This isn't necessarily a bad thing, but the way this is layered seems like it's very tightly coupled.
How does this sound: split this into a libc-like layer (puts, printf, etc) and a 'msg' layer that does the logging verbosity, prefix, etc. If we add a little bit of logic to the puts/printf layer that tests if the format string (or written string) is in the NOLOAD section then we can use your bypass to print it out. Otherwise, it behaves normally.
In my eyes this has two benefits: separation of the concerns of logging decoration and actual IO, and switching to a libc API that's familiar enough that we can use or reproduce it reliably later.
Thoughts?

The msg print APIs (MSG_INFO*, MSG_WARNING, MSG_ERROR and MSG_FATAL) which is what is exposed, do have a printf-like interface. I am not sure I understand what's different about those that's making them incompatible with libc.

You're right, I misunderstood how this API was supposed to work. This could absolutely be layered on top of a libc using the _impl.h approach.

The logging, prefixes, header construction etc does form the 'msg' layer and is fully customizable based on the target specific requirements (you could define all of those to be empty strings if they are deemed unnecessary) and are intended to happen behind-the-scenes when these APIs are invoked. These macros finally invoke the MSG_PRINT() macro which is intended to be implementation-specific. It can very well be defined to call libc-like functions underneath.

In a way, there is already a separation between msg layer and the libc layer. Only difference being the msg layer (macro APIs) is what is being exposed for use and the libc layer being called underneath. If we go the other way around, I am not sure how we can support other things. For DV, the environment is very resource constrained (when simulating the whole chip in RTL cycle by cycle, every bit of processing on the CPU adds significant about of test run time (which is of no value to the actual test). The macro based solution gives us the fastest possible implementation in terms of sim run time.

Re. your point about plucking this code out and running it on a different harness - is your thought process to cherry-pick individual sources from the shared lib code rather than the whole thing (I would expect you would rather want to build the whole library together as one piece, which will include the APIs as well).

Yes, we would probably want to build the entire library together and provide our own _impl.h.

tjaychen · 2019-10-04T20:21:29Z

My concerns with this change are that it adds an API that's hard to satisfy, and one that isn't very 'familiar', by which I mean it's not a libc interface and doesn't consume a libc interface. If we want to take any of the code written against this API and and run it under a different harness (or an OS) then we'll need to merge this code there or replicate the macro-based API.

It's also fairly opinionated in the logging prefixes and levels. This isn't necessarily a bad thing, but the way this is layered seems like it's very tightly coupled.

How does this sound: split this into a libc-like layer (puts, printf, etc) and a 'msg' layer that does the logging verbosity, prefix, etc. If we add a little bit of logic to the puts/printf layer that tests if the format string (or written string) is in the NOLOAD section then we can use your bypass to print it out. Otherwise, it behaves normally.

In my eyes this has two benefits: separation of the concerns of logging decoration and actual IO, and switching to a libc API that's familiar enough that we can use or reproduce it reliably later.

Thoughts?

@gkelly
I might be missing something, but I am having some troubles seeing this might work. I thought the message flow macros that Sri defined were the ones to create the NOLOAD region to start with, so how would the printf layer check?

Also, if there's a libc like layer, I'm guessing that means everyone would call printf directly? Wouldn't that function call lose some of the information Sri wants to keep? For example line number and file since it's not typically passed through a printf function call?

I might be thinking in completely the wrong direction, feel free to correct me.

asb · 2019-10-07T15:26:10Z

Hi Sri, @imphil and I just sat down to work through the (based on your high-level overview doc - thanks for that). We now have a much clearer idea of what you're trying to achieve and why. The ELF hacking isn't pretty, but does seem the sensible way of achieving the goal of avoiding writing out format string contents. Feedback:

Philipp comments that the formatting strings seem to be SV style rather than C-style. Given this is used from C, can we standardise on that?
It's not clear to me that there is sufficiently comprehensive error checking. e.g. on the SV side, what happens if there are too many args for the format string or too few args for the format string? From a quick look, it seemed it would hang in the latter case.
Related to the above, it's worth investigating the use of __attribute__ format so the C compiler can check for obvious format string related errors

sriyerg · 2019-10-07T17:51:05Z

Hi Sri, @imphil and I just sat down to work through the (based on your high-level overview doc - thanks for that). We now have a much clearer idea of what you're trying to achieve and why. The ELF hacking isn't pretty, but does seem the sensible way of achieving the goal of avoiding writing out format string contents. Feedback:

Philipp comments that the formatting strings seem to be SV style rather than C-style. Given this is used from C, can we standardise on that?

It's not clear to me that there is sufficiently comprehensive error checking. e.g. on the SV side, what happens if there are too many args for the format string or too few args for the format string? From a quick look, it seemed it would hang in the latter case.

Related to the above, it's worth investigating the use of __attribute__ format so the C compiler can check for obvious format string related errors

Agree - the string formatting function attempts to support SV use cases as well. Since it will never be used in DV, we can make them pure C-style. I just picked up an existing implementation so there is something to support non-DV efforts. I will prefer SW folks to work on it and get it to whatever shape they want rather than fix it myself.
Error checking in my opinion is not required from the SV side since the C compilation will error out if incorrect number of args is passed so we wouldn't even get to the sim run phase. Plus, its just messaging (only meant to serve as simulation infrastructure piece) which is not critical and hence doesn't need to be feature rich.
Will investigate the use of attribute if it helps make it more robust.

Thank you for your comments - I have made PR #357 which splits this PR into first of 2 parts - 'regular' print msg via UART. I have clearly documented the generic set of APIs that can be made implementation specific. I hope that is easier to review. I will update the proposal doc tomorrow.

gkelly · 2019-10-07T22:02:37Z

My concerns with this change are that it adds an API that's hard to satisfy, and one that isn't very 'familiar', by which I mean it's not a libc interface and doesn't consume a libc interface. If we want to take any of the code written against this API and and run it under a different harness (or an OS) then we'll need to merge this code there or replicate the macro-based API.
It's also fairly opinionated in the logging prefixes and levels. This isn't necessarily a bad thing, but the way this is layered seems like it's very tightly coupled.
How does this sound: split this into a libc-like layer (puts, printf, etc) and a 'msg' layer that does the logging verbosity, prefix, etc. If we add a little bit of logic to the puts/printf layer that tests if the format string (or written string) is in the NOLOAD section then we can use your bypass to print it out. Otherwise, it behaves normally.
In my eyes this has two benefits: separation of the concerns of logging decoration and actual IO, and switching to a libc API that's familiar enough that we can use or reproduce it reliably later.
Thoughts?

@gkelly
I might be missing something, but I am having some troubles seeing this might work. I thought the message flow macros that Sri defined were the ones to create the NOLOAD region to start with, so how would the printf layer check?

The last callee before argument formatting would need to check the format string to see where it is in memory. If it's in .rodata then we know we can use the formatting-bypass path. However, I don't think it's necessary to do that and it's actually a layering violation. 😛

Also, if there's a libc like layer, I'm guessing that means everyone would call printf directly? Wouldn't that function call lose some of the information Sri wants to keep? For example line number and file since it's not typically passed through a printf function call?

I might be thinking in completely the wrong direction, feel free to correct me.

No, I think you're right and I was misunderstanding the intent of the change and the _impl.h header. Code not written against msg.h might call non-msg APIs, but the _impl.h we provide will also be calling that output path.

- provide a generic set of APIs to print msgs - provide ability to attach severity (msg type) and verbosity to msgs - provide ability to attach a header to the msg underneath - privide ability to construct the header based on severity, verbosity file and line number for ease of debug - provide a generic msg_print() function that can parse a formatted string and take variable number of args - provide implementation specific APIs in sw/util/msg_uart/msg_api_impl.h to do the prints via UART - PR lowRISC#259 is being split to make the review easier - this PR focuses on providing a high level API and 'typical' SW implementation of it using UART, - Subsequent PR will focus on DV specific implementation Signed-off-by: Srikrishna Iyer <sriyer@google.com>

- provide a generic set of APIs to print msgs - provide ability to attach severity (msg type) and verbosity to msgs - provide ability to attach a header to the msg underneath - privide ability to construct the header based on severity, verbosity file and line number for ease of debug - provide a generic msg_print() function that can parse a formatted string and take variable number of args - provide implementation specific APIs in sw/util/msg_uart/msg_api_impl.h to do the prints via UART - PR #259 is being split to make the review easier - this PR focuses on providing a high level API and 'typical' SW implementation of it using UART, - Subsequent PR will focus on DV specific implementation Signed-off-by: Srikrishna Iyer <sriyer@google.com>

imphil · 2020-04-08T16:24:02Z

@sriyerg I guess this one can be close in favor of #1768?

sriyerg · 2020-04-08T16:28:49Z

@sriyerg I guess this one can be close in favor of #1768?

Yes it can.

sriyerg requested review from tjaychen and moidx September 24, 2019 20:45