introduce execution status for riscv_program #964

aap-sc · 2023-11-16T14:34:54Z

This commit addresses #933

Change-Id: I318d5fd6af3a17d1c1099401427ca80ffbec0007

aap-sc · 2023-11-16T14:41:31Z

Note to reviewers:

this commit has lot's of TODO statements. I plan to work on most of these in a subsequent commits:

moving exec_status to the output parameter of riscv_program_exec is the next thing I plan to implement if this PR is approved. I wanted to produce a minimal diff without causing too much disturbance across the codebase.
error-handling adjustment may take a while. But I hope that these comments only illustrate existing defects without introducing new ones.

src/target/riscv/riscv-013.c

aap-sc · 2023-11-22T17:51:58Z

@JanMatCodasip , @timsifive could you take a look once again?

timsifive

Looks good overall.

src/target/riscv/program.h

src/target/riscv/riscv-013.c

src/target/riscv/program.c

src/target/riscv/riscv-013.c

timsifive

One last comment.

src/target/riscv/program.c

JanMatCodasip

Thank you for the changes, it looks fine overall. I have completed my review and don't expect to have any more major comments than what I have posted.

src/target/riscv/riscv-013.c

src/target/riscv/program.h

src/target/riscv/riscv-013.c

src/target/riscv/riscv.c

src/target/riscv/riscv-013.c

JanMatCodasip · 2023-12-11T07:18:59Z

src/target/riscv/program.h

+typedef enum {
+	RISCV_DBGBUF_EXEC_RESULT_NOT_EXECUTED,
+	RISCV_DBGBUF_EXEC_RESULT_UNKNOWN,
+	RISCV_DBGBUF_EXEC_RESULT_EXCEPTION,
+	RISCV_DBGBUF_EXEC_RESULT_UNKNOWN_ERROR,
+	RISCV_DBGBUF_EXEC_RESULT_SUCCESS
+} riscv_progbuf_exec_result_t;
+


Thank you for unifying the names (debug_buffer -> progbuf) in many places in the code.

One last nitpick - RISCV_DBGBUF_* items in this enum can be renamed to RISCV_PROGBUF_*, too.

Suggested change

typedef enum {

RISCV_DBGBUF_EXEC_RESULT_NOT_EXECUTED,

RISCV_DBGBUF_EXEC_RESULT_UNKNOWN,

RISCV_DBGBUF_EXEC_RESULT_EXCEPTION,

RISCV_DBGBUF_EXEC_RESULT_UNKNOWN_ERROR,

RISCV_DBGBUF_EXEC_RESULT_SUCCESS

} riscv_progbuf_exec_result_t;

typedef enum {

RISCV_PROGBUF_EXEC_RESULT_NOT_EXECUTED,

RISCV_PROGBUF_EXEC_RESULT_UNKNOWN,

RISCV_PROGBUF_EXEC_RESULT_EXCEPTION,

RISCV_PROGBUF_EXEC_RESULT_UNKNOWN_ERROR,

RISCV_PROGBUF_EXEC_RESULT_SUCCESS

} riscv_progbuf_exec_result_t;

strange, I though I did that ...

Random comment: It is much easier to review changes if you make a separate PR for renaming a bunch of stuff without changing any functionality.

Random comment: It is much easier to review changes if you make a separate PR for renaming a bunch of stuff without changing any functionality.

I moved these renames to a separate change set specifically to simplify the review. If this is still inconvenient, I can factor-out this in a separate MR.

@timsifive I've splitted the whole change set into 2 distinct commits: one with functional changes the other is with the rename of dbgbuf->progbuf. I've tested them separately. If necessary, I can move the second changeset to a separate MR.

@JanMatCodasip : addressed.

src/target/riscv/riscv-013.c

src/target/riscv/program.h

src/target/riscv/riscv-013.c

src/target/riscv/program.c

JanMatCodasip · 2023-12-11T09:19:53Z

src/target/riscv/riscv-013.c

+	unsigned int abstractcs_err;
+	if (execute_abstract_command(target, command, &abstractcs_err) != ERROR_OK)


I fully agree that errors should always be taken into account. That however does not mean that the caller of execute_abstract_command() must always introspect the detailed cause of the error (cmderr) :-)

After thinking about it more, I believe it would be good to have 4 distinct return codes for execute_abstract_command():

(1) ERROR_OK - Command succeeded. In this case the caller needs not look at cmderr (because cmderr is guaranteed to be 0).

(2) ERROR_CMD_FAILED (or similar name) - Command was executed but failed due to cmderr!=0. The caller can look at cmderr to determine the actual failure reason, if it wishes, but it does not need to do so. All depends on the caller's situation, and how detailed information about the error it needs to have.

(3) ERROR_FAIL - During the execution of the command, the DMI read or write operation failed. The target (or the connection to it) is seriously broken. The value in cmderr is irrelevant in this case.

(4) ERROR_TIMEOUT_REACHED - Timeout occurred. The value in cmderr is not relevant in this case, either.

At the moment, cases (2) and (3) is merged into one ERROR_FAIL return code. It would be good to split it because those are two different outcomes. Please, let me know if you have similar or different opinion. But no need to make the changes directly in this merge request. It would be good to agree on the strategy.

Based on the above, I still believe the third argument of execute_abstract_command() should be made optional. The caller should detect the overall status (1) - (4) through the return code. And only in case of (2), the cmderr can be introspected, but it is not mandatory to do so in all cases.

aap-sc · 2023-12-11T10:41:06Z

That however does not mean that the caller of execute_abstract_command() must always introspect the detailed cause of the error (cmderr) :-)

It would be good to agree on the strategy.

If there are lot's of cases when we don't want to introspect the detailed cause, then I agree that it may be better to return generic error-codes with cmderr being optional. Though I still need to do some investigation before forming a definite opinion regarding must always introspect the detailed cause part.

JanMatCodasip

From my point of view:

1. There is one last open thread (introduce execution status for riscv_program #964 (comment)) with cosmetic rename stuff.
1. This pull request should be rebased (and conflicts solved).

After that I consider it ready for merge. Thanks.

aap-sc · 2023-12-12T17:43:55Z

rebased and addressed all comments. @timsifive , @JanMatCodasip could you please take a look?

src/target/riscv/riscv-013.c

aap-sc · 2023-12-14T12:16:20Z

Note to reviewers: the latest update is just a squash of unsigned int -> uint32_t replacing. No new changes introduced.

JanMatCodasip

Looks good, thank you.

Change-Id: I3b283b49dea88a6f3d2159be3c9f6c6da604aa9e Signed-off-by: Parshintsev Anatoly <anatoly.parshintsev@syntacore.com>

Change-Id: I29e2192d5ce9d0f13010d8a09bd4ef50f5c8844b Signed-off-by: Parshintsev Anatoly <anatoly.parshintsev@syntacore.com>

aap-sc · 2023-12-22T08:56:29Z

@timsifive , @JanMatCodasip there was a small merge conflict with recently committed #978 (in riscv013_clear_abstract_error) . I've resolved it. Can we merge this one?

aap-sc · 2023-12-28T10:06:34Z

@timsifive, @JanMatCodasip gentle ping. Also @en-sc could you please take a look?

en-sc

LGTM

JanMatCodasip

Looks good to me, thank you.

JanMatCodasip requested changes Nov 16, 2023

View reviewed changes

src/target/riscv/riscv-013.c Outdated Show resolved Hide resolved

src/target/riscv/riscv-013.c Outdated Show resolved Hide resolved

aap-sc force-pushed the aap-sc/dbgbuf_prg_exec_status branch from 464fb8d to 960a623 Compare November 20, 2023 19:26

aap-sc requested a review from JanMatCodasip November 22, 2023 11:33

aap-sc force-pushed the aap-sc/dbgbuf_prg_exec_status branch from 960a623 to f5bc52d Compare November 29, 2023 17:31

timsifive requested changes Dec 1, 2023

View reviewed changes

aap-sc force-pushed the aap-sc/dbgbuf_prg_exec_status branch from f5bc52d to 53e739d Compare December 4, 2023 13:11

timsifive requested changes Dec 5, 2023

View reviewed changes

src/target/riscv/program.c Outdated Show resolved Hide resolved

JanMatCodasip requested changes Dec 6, 2023

View reviewed changes

aap-sc requested review from timsifive and JanMatCodasip December 8, 2023 05:35

JanMatCodasip reviewed Dec 11, 2023

View reviewed changes

JanMatCodasip reviewed Dec 12, 2023

View reviewed changes

aap-sc force-pushed the aap-sc/dbgbuf_prg_exec_status branch from 5fb71bf to 0712489 Compare December 12, 2023 17:39

aap-sc requested a review from JanMatCodasip December 12, 2023 17:44

timsifive requested changes Dec 12, 2023

View reviewed changes

aap-sc force-pushed the aap-sc/dbgbuf_prg_exec_status branch from 0712489 to d085c8e Compare December 12, 2023 21:04

aap-sc requested a review from timsifive December 12, 2023 21:06

aap-sc force-pushed the aap-sc/dbgbuf_prg_exec_status branch from d085c8e to a49b707 Compare December 12, 2023 21:07

timsifive previously approved these changes Dec 12, 2023

View reviewed changes

aap-sc force-pushed the aap-sc/dbgbuf_prg_exec_status branch from a49b707 to b5196c1 Compare December 14, 2023 12:14

JanMatCodasip previously approved these changes Dec 18, 2023

View reviewed changes

aap-sc added 2 commits December 22, 2023 11:35

introduce execution status for riscv_program

928f10a

Change-Id: I3b283b49dea88a6f3d2159be3c9f6c6da604aa9e Signed-off-by: Parshintsev Anatoly <anatoly.parshintsev@syntacore.com>

rename dbgbuf to progbuf

aded275

Change-Id: I29e2192d5ce9d0f13010d8a09bd4ef50f5c8844b Signed-off-by: Parshintsev Anatoly <anatoly.parshintsev@syntacore.com>

aap-sc dismissed stale reviews from JanMatCodasip and timsifive via aded275 December 22, 2023 08:45

aap-sc force-pushed the aap-sc/dbgbuf_prg_exec_status branch from b5196c1 to aded275 Compare December 22, 2023 08:45

aap-sc requested review from JanMatCodasip and timsifive December 22, 2023 08:52

en-sc approved these changes Dec 28, 2023

View reviewed changes

JanMatCodasip approved these changes Jan 2, 2024

View reviewed changes

JanMatCodasip merged commit 02901ff into riscv-collab:riscv Jan 4, 2024
4 checks passed

aap-sc mentioned this pull request Jan 9, 2024

execute_fence does not handle errors #933

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

introduce execution status for riscv_program #964

introduce execution status for riscv_program #964

aap-sc commented Nov 16, 2023

aap-sc commented Nov 16, 2023

aap-sc commented Nov 22, 2023

timsifive left a comment

timsifive left a comment

JanMatCodasip left a comment

JanMatCodasip Dec 11, 2023

aap-sc Dec 11, 2023

timsifive Dec 11, 2023

aap-sc Dec 12, 2023 •

edited

Loading

aap-sc Dec 12, 2023

timsifive Dec 12, 2023

JanMatCodasip Dec 11, 2023 •

edited

Loading

aap-sc commented Dec 11, 2023

JanMatCodasip left a comment •

edited

Loading

aap-sc commented Dec 12, 2023

aap-sc commented Dec 14, 2023 •

edited

Loading

JanMatCodasip left a comment

aap-sc commented Dec 22, 2023

aap-sc commented Dec 28, 2023

en-sc left a comment

JanMatCodasip left a comment

		unsigned int abstractcs_err;
		if (execute_abstract_command(target, command, &abstractcs_err) != ERROR_OK)

introduce execution status for riscv_program #964

introduce execution status for riscv_program #964

Conversation

aap-sc commented Nov 16, 2023

aap-sc commented Nov 16, 2023

aap-sc commented Nov 22, 2023

timsifive left a comment

Choose a reason for hiding this comment

timsifive left a comment

Choose a reason for hiding this comment

JanMatCodasip left a comment

Choose a reason for hiding this comment

JanMatCodasip Dec 11, 2023

Choose a reason for hiding this comment

aap-sc Dec 11, 2023

Choose a reason for hiding this comment

timsifive Dec 11, 2023

Choose a reason for hiding this comment

aap-sc Dec 12, 2023 • edited Loading

Choose a reason for hiding this comment

aap-sc Dec 12, 2023

Choose a reason for hiding this comment

timsifive Dec 12, 2023

Choose a reason for hiding this comment

JanMatCodasip Dec 11, 2023 • edited Loading

Choose a reason for hiding this comment

aap-sc commented Dec 11, 2023

JanMatCodasip left a comment • edited Loading

Choose a reason for hiding this comment

aap-sc commented Dec 12, 2023

aap-sc commented Dec 14, 2023 • edited Loading

JanMatCodasip left a comment

Choose a reason for hiding this comment

aap-sc commented Dec 22, 2023

aap-sc commented Dec 28, 2023

en-sc left a comment

Choose a reason for hiding this comment

JanMatCodasip left a comment

Choose a reason for hiding this comment

aap-sc Dec 12, 2023 •

edited

Loading

JanMatCodasip Dec 11, 2023 •

edited

Loading

JanMatCodasip left a comment •

edited

Loading

aap-sc commented Dec 14, 2023 •

edited

Loading