target/riscv: Reject size 2 soft breakpoints when C extension not supported #908

MarekVCodasip · 2023-08-25T13:31:42Z

This patch disables software breakpoints of size 2 for targets which don't support compressed instructions.

Change-Id: I8200b22a51c97ba2aa89e6328beadde8dd35cdd5

src/target/riscv/riscv.c

JanMatCodasip

Thanks.

src/target/riscv/riscv.c

timsifive

Looks good overall. Just have an OpenOCD style comment.

src/target/riscv/riscv.c

…ported This patch disables software breakpoints of size 2 for targets which don't support compressed instructions. Change-Id: I8200b22a51c97ba2aa89e6328beadde8dd35cdd5 Signed-off-by: Marek Vrbka <[email protected]>

fanghuaqi · 2023-10-07T09:21:03Z

src/target/riscv/riscv.c

-		/** @todo check RVC for size/alignment */
-		if (!(breakpoint->length == 4 || breakpoint->length == 2)) {
-			LOG_TARGET_ERROR(target, "Invalid breakpoint length %d", breakpoint->length);
+		const bool c_extension_supported = riscv_supports_extension(target, 'C');


There is a code size reduction isa called Zc*(ratified), see https://github.com/riscv/riscv-code-size-reduction

If Zc extension is used, the instruction can be also 16 bit, so this check by just riscv_supports_extension(target, 'C') maybe not correct, since as described here riscvarchive/riscv-code-size-reduction#144 (comment), this misa.c only set when all normal compress instruction are implemented, but in some case, for example, rv32imaf_zca, it will not set misa.c bit, but it still contains 16 bit instruction, so I think sw breakpoint can still set here, so the check by misa.c bit is not enough.

also here riscv-software-src/riscv-isa-sim#1172

Hi @MarekVCodasip @timsifive @tariqkurd-repo , is there any feedback on my comments regarding the new added Zc* extensions will affect this compress instructions existing checking.

Implementing Zca and not C will not set misa.C
I think you actually need to specify C to set it
Is that your understanding @aswaterman ?

Implementing Zca and not C will not set misa.C I think you actually need to specify C to set it Is that your understanding @aswaterman ?

If I implement rv32imaf_zca_zcb_zcf_zcmp_zcmt it contains all C required zca_zcf will the misa.C bit be set? This riscv_supports_extension function only use the misa.C bit to check whether compressed instructions are supported.

TommyMurphyTM1234 · 2023-10-07T09:34:00Z

Are there specific reasons not to simply always use the "lowest common denominator" of 32-bit software breakpoints regardless of the target's 16-bit instruction capabilities? Off the top of my head the only one that I can think of is the inability to write one at the instruction address 2 bytes from the "top" of a memory region. Are there other reasons not to just take the simpler approach?

JanMatCodasip · 2023-10-12T07:58:15Z

TommyMurphyTM1234: Are there specific reasons not to simply always use the "lowest common denominator" of 32-bit software breakpoints regardless of the target's 16-bit instruction capabilities?

By placing a SW breakpoint (ebreak or c.ebreak), we must not overwrite any other instruction than the one on which the break is placed. That's because the subsequent instruction after the break can be a target of a branch. And as such, it must remain intact.

Using always the 32-bit SW breaks is therefore not an option.

TommyMurphyTM1234 · 2023-10-12T08:25:35Z

TommyMurphyTM1234: Are there specific reasons not to simply always use the "lowest common denominator" of 32-bit software breakpoints regardless of the target's 16-bit instruction capabilities?

By placing a SW breakpoint (ebreak or c.ebreak), we must not overwrite any other instruction than the one on which the break is placed. That's because the subsequent instruction after the break can be a target of a branch. And as such, it must remain intact.

Using always the 32-bit SW breaks is therefore not an option.

Thanks for that explanation @JanMatCodasip 👍

fanghuaqi · 2023-10-18T02:01:19Z

Hi @MarekVCodasip @timsifive @JanMatCodasip , using this commit, I am not able to set breakpoint at 2 bytes aligned address, with Zc* extension enabled which also provided compressed instructions.

JanMatCodasip · 2023-10-19T06:54:18Z

@fanghuaqi

Is there a reliable way how OpenOCD can detect the c.ebreak support in the target hardware, please?

If so, we may consider implementing it in OpenOCD.
Otherwise we should probably revert/rework this commit, and simply obey the breakpoint length as asked by GDB (or by the user).

fanghuaqi · 2023-10-19T07:08:56Z

@fanghuaqi

Is there a reliable way how OpenOCD can detect the c.ebreak support in the target hardware, please?

If so, we may consider implementing it in OpenOCD.

Otherwise we should probably revert/rework this commit, and simply obey the breakpoint length as asked by GDB (or by the user).

Hi @JanMatCodasip , I didn't know whether there is a way to check whether c.ebreak supported in hardware, don't DM provide some information about it? If not, may be make some notes in the doc will be good.

TommyMurphyTM1234 · 2023-10-19T14:51:51Z

Earlier:

target/riscv: Reject size 2 soft breakpoints when C extension not supported #908 (comment)

it was stated that:

There is a code size reduction isa called Zc*(ratified), see https://github.com/riscv/riscv-code-size-reduction

If Zc extension is used, the instruction can be also 16 bit

But, as far as I can see, support for c.ebreak is orthogonal to support for the Zc* extension/sub-extensions.

The Zc* extension/sub-extensions make no mention of c.ebreak - only The RISC-V Instruction Set Manual
Volume I: Unprivileged ISA mentions/specifies it.

https://riscv.org/technical/specifications/

That being the case shouldn't a check of $misa for the C bit being set should be sufficient for checking if the target supports c.ebreak?

TommyMurphyTM1234 · 2023-10-19T15:13:37Z

Perhaps this is another option?

https://sourceware.org/legacy-ml/gdb-patches/2018-09/msg00815.html

this patch instead examines
the existing instruction at a breakpoint target to determine which
breakpoint instruction to use. If the existing instruction is a
compressed instruction, C.EBREAK is used, otherwise EBREAK is used.

timsifive · 2023-10-19T15:17:59Z

The simple option is to revert this change. gdb already has good logic to decide what breakpoint size to use, because it knows what size instruction is being replaced.

What was the original reason for this change? I don't see a rationale in the description.

timsifive · 2023-10-19T15:46:31Z

From what I can tell, c.ebreak is defined in the unprivileged spec, where it is part of the "C" Standard Extension chapter. That means to me that if c.ebreak is implemented then misa.C must be set.

That feels like a spec bug, though. If you are allowed to implement 16-bit instructions without setting misa.C, which the Zc* extensions clearly do, then obviously you want c.ebreak as well, without being required to implement all the other C extension instructions. I filed riscvarchive/riscv-code-size-reduction#228 for this.

I would expect the outcome to be something that allows c.ebreak even without C, and the result for OpenOCD is that we should revert this change.

fanghuaqi · 2023-10-20T03:44:10Z

c.break is part of Zca which is the only part of C without(compressed F/D instructions).

fanghuaqi · 2023-10-20T03:47:26Z

src/target/riscv/riscv.c

@@ -1198,15 +1198,17 @@ static int riscv_add_breakpoint(struct target *target, struct breakpoint *breakp
 	LOG_TARGET_DEBUG(target, "@0x%" TARGET_PRIxADDR, breakpoint->address);
 	assert(breakpoint);
 	if (breakpoint->type == BKPT_SOFT) {
-		/** @todo check RVC for size/alignment */


@timsifive I think this patch is added due to there is a todo here, is it possible to check whether 16bit instruction execute ok without any exception, such as test c.nop, if no exception, compress instruction is supported, otherwise no.

JanMatCodasip · 2023-10-20T05:37:33Z

Tim: The simple option is to revert this change.

I am in agreement.

Tim: What was the original reason for this change? I don't see a rationale in the description.

The reason was to prevent user mistakes and improve robustness (an attempt that in the end has the opposite effect: additional issues).

TommyMurphyTM1234 · 2023-10-20T06:29:18Z

c.break is part of Zca which is the only part of C without(compressed F/D instructions).

Did you mean c.ebreak?
Can you clarify where in the Zc* spec that is stated?
I couldn't see it.

fanghuaqi · 2023-10-20T08:48:18Z

c.break is part of Zca which is the only part of C without(compressed F/D instructions).

Did you mean c.ebreak? Can you clarify where in the Zc* spec that is stated? I couldn't see it.

Hi @TommyMurphyTM1234 , please check it here riscvarchive/riscv-code-size-reduction@4abffe7

TommyMurphyTM1234 · 2023-10-20T09:01:23Z

c.break is part of Zca which is the only part of C without(compressed F/D instructions).

Did you mean c.ebreak? Can you clarify where in the Zc* spec that is stated? I couldn't see it.

Hi @TommyMurphyTM1234 , please check it here riscv/riscv-code-size-reduction@4abffe7

The ratified Zc* extension is listed here:

https://wiki.riscv.org/display/HOME/Recently+Ratified+Extensions#:~:text=RISC%2DV%20Code%20Size%20Reduction

The ratified specification does not mention c.ebreak or the text in the commit that you linked to.

https://drive.google.com/file/d/1zmWDmfbtVY9I6hn0vuLTbk5rsSPc44sL/view?usp=drive_link

Are you referring to some proposed draft update?
I still don't see how what you linked to clarifies that Zc* necessarily implies support for c.ebreak.

timsifive · 2023-10-20T22:36:33Z

@TommyMurphyTM1234, see riscvarchive/riscv-code-size-reduction#228.

TLDR: Every Zc* requires Zca, and Zca is all of the existing C extension, except some 16-bit floating point ops.

JanMatCodasip requested changes Aug 25, 2023

View reviewed changes

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

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

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

MarekVCodasip force-pushed the disable-soft-bp-size-2-non-compressed branch from 159a49d to 58fd2a3 Compare August 28, 2023 06:40

JanMatCodasip previously approved these changes Aug 28, 2023

View reviewed changes

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

MarekVCodasip dismissed JanMatCodasip’s stale review via 257c30c August 28, 2023 13:16

MarekVCodasip force-pushed the disable-soft-bp-size-2-non-compressed branch from 58fd2a3 to 257c30c Compare August 28, 2023 13:16

JanMatCodasip previously approved these changes Aug 28, 2023

View reviewed changes

timsifive requested changes Aug 29, 2023

View reviewed changes

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

MarekVCodasip dismissed JanMatCodasip’s stale review via 1faadde September 4, 2023 05:46

MarekVCodasip force-pushed the disable-soft-bp-size-2-non-compressed branch from 257c30c to 1faadde Compare September 4, 2023 05:46

MarekVCodasip force-pushed the disable-soft-bp-size-2-non-compressed branch from 1faadde to 8ad4176 Compare September 4, 2023 05:47

timsifive approved these changes Sep 5, 2023

View reviewed changes

timsifive merged commit 54372dd into riscv-collab:riscv Sep 7, 2023
5 checks passed

fanghuaqi reviewed Oct 7, 2023

View reviewed changes

fanghuaqi reviewed Oct 20, 2023

View reviewed changes

timsifive mentioned this pull request Oct 20, 2023

Revert "target/riscv: Reject size 2 soft breakpoints when C extension not supported" #940

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

target/riscv: Reject size 2 soft breakpoints when C extension not supported #908

target/riscv: Reject size 2 soft breakpoints when C extension not supported #908

MarekVCodasip commented Aug 25, 2023

JanMatCodasip left a comment

timsifive left a comment

fanghuaqi Oct 7, 2023

fanghuaqi Oct 7, 2023

fanghuaqi Oct 11, 2023

tariqkurd-repo Oct 11, 2023

fanghuaqi Oct 12, 2023

TommyMurphyTM1234 commented Oct 7, 2023 •

edited

Loading

JanMatCodasip commented Oct 12, 2023 •

edited

Loading

TommyMurphyTM1234 commented Oct 12, 2023 •

edited

Loading

fanghuaqi commented Oct 18, 2023

JanMatCodasip commented Oct 19, 2023

fanghuaqi commented Oct 19, 2023

TommyMurphyTM1234 commented Oct 19, 2023 •

edited

Loading

TommyMurphyTM1234 commented Oct 19, 2023 •

edited

Loading

timsifive commented Oct 19, 2023

timsifive commented Oct 19, 2023

fanghuaqi commented Oct 20, 2023

fanghuaqi Oct 20, 2023

JanMatCodasip commented Oct 20, 2023 •

edited

Loading

TommyMurphyTM1234 commented Oct 20, 2023

fanghuaqi commented Oct 20, 2023

TommyMurphyTM1234 commented Oct 20, 2023

timsifive commented Oct 20, 2023

target/riscv: Reject size 2 soft breakpoints when C extension not supported #908

target/riscv: Reject size 2 soft breakpoints when C extension not supported #908

Conversation

MarekVCodasip commented Aug 25, 2023

JanMatCodasip left a comment

Choose a reason for hiding this comment

timsifive left a comment

Choose a reason for hiding this comment

fanghuaqi Oct 7, 2023

Choose a reason for hiding this comment

fanghuaqi Oct 7, 2023

Choose a reason for hiding this comment

fanghuaqi Oct 11, 2023

Choose a reason for hiding this comment

tariqkurd-repo Oct 11, 2023

Choose a reason for hiding this comment

fanghuaqi Oct 12, 2023

Choose a reason for hiding this comment

TommyMurphyTM1234 commented Oct 7, 2023 • edited Loading

JanMatCodasip commented Oct 12, 2023 • edited Loading

TommyMurphyTM1234 commented Oct 12, 2023 • edited Loading

fanghuaqi commented Oct 18, 2023

JanMatCodasip commented Oct 19, 2023

fanghuaqi commented Oct 19, 2023

TommyMurphyTM1234 commented Oct 19, 2023 • edited Loading

TommyMurphyTM1234 commented Oct 19, 2023 • edited Loading

timsifive commented Oct 19, 2023

timsifive commented Oct 19, 2023

fanghuaqi commented Oct 20, 2023

fanghuaqi Oct 20, 2023

Choose a reason for hiding this comment

JanMatCodasip commented Oct 20, 2023 • edited Loading

TommyMurphyTM1234 commented Oct 20, 2023

fanghuaqi commented Oct 20, 2023

TommyMurphyTM1234 commented Oct 20, 2023

timsifive commented Oct 20, 2023

TommyMurphyTM1234 commented Oct 7, 2023 •

edited

Loading

JanMatCodasip commented Oct 12, 2023 •

edited

Loading

TommyMurphyTM1234 commented Oct 12, 2023 •

edited

Loading

TommyMurphyTM1234 commented Oct 19, 2023 •

edited

Loading

TommyMurphyTM1234 commented Oct 19, 2023 •

edited

Loading

JanMatCodasip commented Oct 20, 2023 •

edited

Loading