Skip to content

Conversation

AlexandraKulyatskaya
Copy link

Add auto-selection of breakpoint size for RISC-V targets. The new length depends on C extension. If the C extension is used, the auto-selected length is 2 bytes. Otherwise, the default length is 4 bytes.

Change-Id: Ie3473514beace15b76714aa6d5441122cd3262aa

@aap-sc aap-sc requested review from JanMatCodasip and en-sc and removed request for JanMatCodasip July 3, 2025 14:39
@AlexandraKulyatskaya AlexandraKulyatskaya force-pushed the a.kulyatskaya/auto-selection-bp-size branch 2 times, most recently from 427b9ee to 3cf367c Compare July 3, 2025 15:16
Add auto-selection of breakpoint size for RISC-V targets. The new length
depends on C extension. If the C extension is used, the auto-selected
length is 2 bytes. Otherwise, the default length is 4 bytes.

Change-Id: Ie3473514beace15b76714aa6d5441122cd3262aa
Signed-off-by: Kulyatskaya Alexandra <[email protected]>
@AlexandraKulyatskaya AlexandraKulyatskaya force-pushed the a.kulyatskaya/auto-selection-bp-size branch from 3cf367c to e307df1 Compare July 3, 2025 15:26
@aap-sc aap-sc requested a review from JanMatCodasip July 3, 2025 16:43
@en-sc
Copy link
Collaborator

en-sc commented Jul 7, 2025

@AlexandraKulyatskaya, I'd suggest avoiding modifying the length of the breakpoint.

Consider instead introducing a new target_type method, something like get_default_breakpoint_length to be used in handle_bp_command_set() in case the user wants a default-length breakpoint.
IMHO, this will allow to avoid introducing special cases and simplify reasoning about breacpoint's lifecycle.

Please, note that in general get_default_breakpoint_length may fail if it requires communication with the target.

@MarekVCodasip
Copy link
Collaborator

MarekVCodasip commented Sep 5, 2025

I don't think this will work anyway as support of c.ebreak isn't guaranteed when "C" extension is supported #908 (comment)

@TommyMurphyTM1234
Copy link
Collaborator

TommyMurphyTM1234 commented Sep 5, 2025

I don't think this will work anyway as support of c.ebreak isn't guaranteed when "C" extension is supported #908 (comment)

Hi @MarekVCodasip - I think that my comment that you linked to may have been based on incomplete or inaccurate information at the time (similar to previous comments linked from there etc.). If you look at the latest ratified RISC-V Instruction Set Manual Volume I: Unprivileged ISA then page 171 of chapter 27 has this as part of the C extension description:

2025-09-05 11 53 40

So it seems that c.break IS part of the C extension after all?

2025-09-05 11 56 34

@MarekVCodasip
Copy link
Collaborator

Hi @MarekVCodasip - I think that my comment that you linked to may have been based on incomplete or inaccurate information at the time (similar to previous comments linked from there etc.). If you look at the latest ratified RISC-V Instruction Set Manual Volume I: Unprivileged ISA then page 171 has this as part of the C extension description:
2025-09-05 11 53 40

So it seems that c.break IS part of the C extension after all?

Thanks for the info, I just remember that this was an issue back then as I tried to implement code to reject software breakpoints of size 2 and it had to be reverted.

@TommyMurphyTM1234
Copy link
Collaborator

TommyMurphyTM1234 commented Sep 5, 2025

Thanks for the info, I just remember that this was an issue back then as I tried to implement code to reject software breakpoints of size 2 and it had to be reverted.

I think that some of the confusion may have arisen from the C extension (or sub-extensions?) specifications omitting to mention c.break at some stage (maybe while still in draft form or an earlier ratified spec that was subsequently updated?) and it only being incorporated into the C extension specification and then incorporation of that extension specification into the main Unprivileged ISA spec? To be honest I am often confused by the way that RISC-V extension specification is done and how RISC-V International makes the relevant information available and changed resulting in a plethora of broken links... :-(

@AlexandraKulyatskaya
Copy link
Author

Reopened as #1288

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants