target/riscv: allow hexadecimal values to expose_csr-like commands

[aap-sc]

No description provided.

[TommyMurphyTM1234]

Small comments

[en-sc]

LGTM (reviewed internally).

[JanMatCodasip]

I will be able to review this change by the end of the week.

[JanMatCodasip]

LGTM, thanks.

I am sending one optional suggestion and one general comment.

[zqb-all]

LGTM, I wanted this feature too but haven't really put in the time to implement it yet, thank you for implementing it

[aap-sc]

Note to reviewers:

the following checks were added in most recent update:

...
		/* If we are here and register address string starts with zero, this is
		 * an indication that most likely user has an incorrect input because:
		 * - decimal numbers typically do not start with "0"
		 * - octals are not supported by our interface
		 * - hexadecimal numbers should have "0x" prefix
		 * Thus such input is rejected. */
		if (strncmp(reg_address_str, "0", 1) == 0 && strlen(reg_address_str) > 1)
			return false;
...

Would you kindly take a look?

[TommyMurphyTM1234]

Note to reviewers:

the following checks were added in most recent update:

...
		/* If we are here and register address string starts with zero, this is
		 * an indication that most likely user has an incorrect input because:

This comment seems a bit misleading because in the code there is a prior explicit check for 0x prefix, so a register number starting with 0 IS valid, if the next character is x/X.

On a more general point, why not just allow decimal numbers that start with 0(s)? Might not somebody legitimately use a Tcl script that generates register numbers of a similar form as generated by C's printf("04%u", x); padded with leading 0s? Why go to the bother of explicitly excluding such number representation? Do many people actually try to use octal representation in such circumstances?

[aap-sc]

This comment seems a bit misleading because in the code there is a prior explicit check for 0x prefix, so a register number starting with 0 IS valid, if the next character is x/X.

But there is a precondition: "If we are here". Meaning we've already checked for hexadecimal numbers a few lines above.

On a more general point, why not just allow decimal numbers that start with 0(s)?

Because it suspiciously looks like someone copy-pasted a hexadecimal number from some documentation and forgot to add a leading "0x".

Might not somebody legitimately use a Tcl script that generates register numbers of a similar form as generated by C's printf("04%u", x); padded with leading 0s?

Same point as above.

Do many people actually try to use octal representation in such circumstances?

People may copy-paste a hexadecimal number.

[JanMatCodasip]

I have submitted few last minor comments and I do not expect to have any more. Other than that the change looks fine.

[JanMatCodasip]

LGTM, thank you.

[aap-sc]

This comment seems a bit misleading because in the code there is a prior explicit check for 0x prefix, so a register number starting with 0 IS valid, if the next character is x/X.

But there is a precondition: "If we are here". Meaning we've already checked for hexadecimal numbers a few lines above.

On a more general point, why not just allow decimal numbers that start with 0(s)?

Because it suspiciously looks like someone copy-pasted a hexadecimal number from some documentation and forgot to add a leading "0x".

Might not somebody legitimately use a Tcl script that generates register numbers of a similar form as generated by C's printf("04%u", x); padded with leading 0s?

Same point as above.

Do many people actually try to use octal representation in such circumstances?

People may copy-paste a hexadecimal number.

@TommyMurphyTM1234 could you please clarify is my reasoning is fine with you? Or do you still have objections or concerns?

[aap-sc]

@TommyMurphyTM1234 could you please clarify is my reasoning is fine with you? Or do you still have objections or concerns?

@TommyMurphyTM1234 ping)

[aap-sc]

Well, if no additional objections follow, I'm going to merge this by the end of week (around Friday).

[TommyMurphyTM1234]

@TommyMurphyTM1234 could you please clarify is my reasoning is fine with you? Or do you still have objections or concerns?

@TommyMurphyTM1234 ping)

Apologies for the delay - I have no strong objections - definitely none that should hold this up further.

[aap-sc]

@JanMatCodasip , @en-sc

This MR introduced a regression since these lines were deleted. It seems that I've made a mistake while my changes were rebased (our internal change reviewed by @en-sc had this code). The regression was detected by our internal testsuite.

I'll post a patch shortly and will try to extend riscv-tests.

[aap-sc]

MR with the fix: #1176