Make register dump output more concise. #909

timsifive · 2023-10-30T19:32:36Z

Don't mention fields whose value is 0.
New format:
{sbautoincrement=1 sbaccess=64bit sbreadonaddr=1 }

timsifive · 2023-10-30T19:32:54Z

pdonahue-ventana

OK

en-sc · 2023-10-31T09:55:10Z

debug_reg_printer.c

-		curr += get_len_or_sprintf(buf, curr, ", ");
+		if (len) {
+			curr += len;
+			curr += get_len_or_sprintf(buf, curr, " ");


Suggested change

curr += get_len_or_sprintf(buf, curr, " ");

curr += get_len_or_sprintf(buf, curr, ", ");

I would suggest to leave the comma between the fields, since some value names include a space e.g. not supported.

Wouldn't you also like to skip the last separator (the one before the last field)? If so, I would suggest to move the check out of riscv_debug_reg_field_to_s() and rewrite the loop like so:

const char *separator = ""; for (struct riscv_debug_reg_field_list_t list; get_next; get_next = list.get_next) { list = get_next(context); if (<condition to skip the field>) continue; curr += get_len_or_sprintf(buf, curr, separator); curr += riscv_debug_reg_field_to_s(buf, curr, list.field, context, field_value); separator = ", "; }

I would suggest to leave the comma between the fields, since some value names include a space e.g. not supported.

Good point. I prefer to solve that by replacing whitespace in value names with _.

Wouldn't you also like to skip the last separator (the one before the last field)?

Yes, but I thought that was a pre-existing issue and it didn't bother me that much. I'll address it.

en-sc · 2023-10-31T12:16:44Z

debug_reg_printer.c

@@ -46,6 +46,9 @@ static unsigned int riscv_debug_reg_field_to_s(char *buf, unsigned int offset,
 		riscv_debug_reg_field_info_t field, riscv_debug_reg_ctx_t context,
 		uint64_t field_value)
 {
+	if (field_value == 0)


Wouldn't it be preferable to only skip value if it is zero and there is no name for such field value?
Like so:

static const char *get_field_value_name(riscv_debug_reg_field_info_t field, uint64_t field_value) { return field.values ? field.values[field_value] : NULL; }

Suggested change

if (field_value == 0)

if (field_value == 0 && !get_field_value_name(list.field, field_value))

I don't think that's preferable. Almost everywhere the 0 value means inactive, so not interesting to list.
E.g. dmstatus=0 would be {version=none stickyunavail=current ndmresetpending=false confstrptrvalid=invalid authbusy=ready authenticated=false} which is a lot to visually parse through just to see nothing is going on. And there's nothing preventing the XML to be edited in the future to be more consistent or whatever and to name every 0 field, in which case the output would be even larger.

I don't think that's preferable. Almost everywhere the 0 value means inactive, so not interesting to list.

The issue, in my opinion is that it is exactly almost everywhere. E.g. dtmcs.version == 0 means 0.11 which is quite important information, considering the use of this code in OpenOCD's riscv-013.c.

E.g. dmstatus=0 would be {version=none stickyunavail=current ndmresetpending=false confstrptrvalid=invalid authbusy=ready authenticated=false} which is a lot to visually parse through just to see nothing is going on.

IMHO this particular issue is caused by attempting to get a meaningful result from decoding a meaningless value and should be addressed in OpenOCD code. Please, take a look at this PR.

Ideally we could mark the handful of fields where 0 is significant, but I'm not going to do that work right now. It's much better to have the output be concise than it is to be readable by someone with no experience.

I chose the 0 example because it was easy. The problem is just as bad in a realistic example:
dmstatus=0x430382 {version=0.13 anyresumeack=1 allresumeack=1 impebreak=1 stickyunavail=current ndmresetpending=false confstrptrvalid=invalid authbusy=ready authenticated=true anyhalted=1 allhalted=1}
vs
dmstatus=0x430382 {version=0.13 anyresumeack=1 allresumeack=1 impebreak=1 authenticated=true anyhalted=1 allhalted=1}

The former has a bunch of uninteresting fields that just make it harder to read the ones you usually care about.

On top of that, the XML might change in the future to add more explicit true/false encodings, which would make the problem worse. The contents of that XML are primarily for the formatting of the document, not for this automated field generation.

@timsifive

Ideally we could mark the handful of fields where 0 is significant,
It's much better to have the output be concise than it is to be readable by someone with no experience.

The former seems an unfeasible goal since it won't be easy to maintain (you've actually alluded to that already). The latter seems like an aesthetic preference - there are people who prefer verbose printouts and there are people who hate these logs and want them to be as short as possible (me included). However, I can see the reason some may want to decode every field. I'd like to suggest a compromise: we can add an extra argument to the printing function and a corresponding riscv-specific OpenOCD command to control it. So we can enable verbose decoding of fields. I can even volunteer to provide a patch (if no objections).

en-sc · 2023-11-01T18:10:57Z

registers.py

+
+        # Remove whitespace from the names, so when they're displayed we can use
+        # only ' ' as a separator.
+        def remove_whitespace(s):


toCIdentifier() can be used here, if you like.

That works. Thanks.

en-sc · 2023-11-02T08:59:23Z

debug_reg_printer.c

+		uint64_t field_value = riscv_debug_reg_field_value(list.field, value);
+
+		if ((show == RISCV_DEBUG_REG_SHOW_ALL) ||
+				(show == RISCV_DEBUG_REG_HIDE_UNNAMED_0 && list.field.values) ||


This hides all fields with no named values. The correct condition:

Suggested change

(show == RISCV_DEBUG_REG_HIDE_UNNAMED_0 && list.field.values) ||

(show == RISCV_DEBUG_REG_HIDE_UNNAMED_0 && !(field_value == 0 && list.field.values && !list.field.values[0]) ||

Good catch. Thanks.

en-sc · 2023-11-03T07:07:14Z

debug_reg_printer.c

+
+		if ((show == RISCV_DEBUG_REG_SHOW_ALL) ||
+				(show == RISCV_DEBUG_REG_HIDE_UNNAMED_0 &&
+					(field_value != 0 || list.field.values)) ||


This will not hide an unnamed zero if there is a name for any other value (list.field.values == {[0] = NULL, [1] = "some name"})

Fixed, I think. I don't see any examples of this.

This will not hide an unnamed zero if there is a name for any other value (list.field.values == {[0] = NULL, [1] = "some name"})

For an example, please, take a look at csr_dcsr_cause_values ([0] is zero-initialized by default).

Add `show` argument that lets user decide how to deal with fields whose value is 0: * Show them all. * Show them if they have a symbolic name. * Don't show them. Use underscores instead of whitespace in field names, so we don't need a comma in the separator. New format: ``` {datacount=2 cmderr=not_supported} {version=0.13 anyresumeack=1 allresumeack=1 impebreak=1 authenticated=true anyhalted=1 allhalted=1} ```

timsifive requested a review from pdonahue-ventana October 30, 2023 19:32

pdonahue-ventana approved these changes Oct 31, 2023

View reviewed changes

en-sc reviewed Oct 31, 2023

View reviewed changes

timsifive force-pushed the concise_regdump branch from 1346eae to 2cabb86 Compare November 1, 2023 16:39

en-sc reviewed Nov 1, 2023

View reviewed changes

timsifive force-pushed the concise_regdump branch from 2cabb86 to a23afaf Compare November 1, 2023 23:08

en-sc reviewed Nov 2, 2023

View reviewed changes

timsifive force-pushed the concise_regdump branch from a23afaf to 0159002 Compare November 2, 2023 20:16

en-sc reviewed Nov 3, 2023

View reviewed changes

timsifive force-pushed the concise_regdump branch from 0159002 to bdae19a Compare November 3, 2023 17:25

timsifive added the codegen label Nov 6, 2023

en-sc approved these changes Nov 7, 2023

View reviewed changes

timsifive merged commit ece9f18 into master Nov 7, 2023

en-sc mentioned this pull request Dec 6, 2023

Update riscv/debug_defines (to sync with riscv-debug-spec commit 40b9a05) riscv-collab/riscv-openocd#973

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make register dump output more concise. #909

Make register dump output more concise. #909

timsifive commented Oct 30, 2023

timsifive commented Oct 30, 2023

pdonahue-ventana left a comment

en-sc Oct 31, 2023

en-sc Oct 31, 2023

timsifive Oct 31, 2023

timsifive Oct 31, 2023

en-sc Nov 1, 2023

en-sc Oct 31, 2023

timsifive Oct 31, 2023

en-sc Nov 1, 2023

timsifive Nov 1, 2023

aap-sc Nov 1, 2023

timsifive Nov 1, 2023

en-sc Nov 1, 2023

timsifive Nov 1, 2023

en-sc Nov 2, 2023

timsifive Nov 2, 2023

en-sc Nov 3, 2023

timsifive Nov 3, 2023

en-sc Nov 7, 2023

	curr += get_len_or_sprintf(buf, curr, " ");
	curr += get_len_or_sprintf(buf, curr, ", ");

	if (field_value == 0)
	if (field_value == 0 && !get_field_value_name(list.field, field_value))

	(show == RISCV_DEBUG_REG_HIDE_UNNAMED_0 && list.field.values) \|\|
	(show == RISCV_DEBUG_REG_HIDE_UNNAMED_0 && !(field_value == 0 && list.field.values && !list.field.values[0]) \|\|

Make register dump output more concise. #909

Make register dump output more concise. #909

Conversation

timsifive commented Oct 30, 2023

timsifive commented Oct 30, 2023

pdonahue-ventana left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment