Skip to content

Conversation

fk-sc
Copy link
Contributor

@fk-sc fk-sc commented Oct 1, 2025

No description provided.

fk-sc added 2 commits October 1, 2025 15:16
This commit fixes comparison in scratch_reserve function

Change-Id: If994b333282bf706736b953227a9739e52f08139
Signed-off-by: Farid Khaydari <[email protected]>
…or alignment and bit operations

This commit refactors the scratch_reserve()
- Replacing hardcoded bit manipulation with BIT() macro
- Using GENMASK_ULL() instead of hardcoded masks for sign extension
- Applying ALIGN_UP() macro for address alignment calculations
- Using DIV_ROUND_UP() instead of manual division with addition

Change-Id: Ic40923ef7d9ac5ca0ffb313c9d4fc6d1457d6bbb
Signed-off-by: Farid Khaydari <[email protected]>
@fk-sc
Copy link
Contributor Author

fk-sc commented Oct 1, 2025

@JanMatCodasip, @MarekVCodasip, could you take a look? This change is required to make this Spike MR pass the testing

@fk-sc
Copy link
Contributor Author

fk-sc commented Oct 2, 2025

Failure example: gdbserver.py targets/RISC-V/spike32.py DebugChangeString for target with datasize greater than 2

@fk-sc
Copy link
Contributor Author

fk-sc commented Oct 3, 2025

There's a logic error in the comparisons in scratch_reserve(). The conditions are backwards - they check if the required space is greater than or equal to the available space, when they should check if it fits within the available space.

If riscv_xlen == 32 then fpr_read_progbuf calls internal_register_read64_progbuf_scratch function that calls scratch_reserve. And here we have our comparisons:

Current code (wrong):

// Option 1: data registers
if ((size_bytes + scratch->hart_address - info->dataaddr + 3) / 4 >=
		info->datasize) {
    // use data registers
}

// Option 2: progbuf  
if ((info->progbuf_writable == YNM_YES) &&
		((size_bytes + scratch->hart_address - info->progbuf_address + 3) / 4 >=
		info->progbufsize)) {
    // use progbuf
}

What happens:

  • With datasize=8 condition is false, so it skips using data registers even though there's plenty of space
  • With datasize=2 condition is true, so it uses data registers

The conditions should use <= instead of >=:

// Option 1: data registers
if ((size_bytes + scratch->hart_address - info->dataaddr + 3) / 4 <=
		info->datasize) {
    // fine for datasize > 2
}

// Option 2: progbuf  
if ((info->progbuf_writable == YNM_YES) &&
		((size_bytes + scratch->hart_address - info->progbuf_address + 3) / 4 <=
		info->progbufsize)) {
    // fine for larger progbuf
}

@TommyMurphyTM1234
Copy link
Collaborator

TommyMurphyTM1234 commented Oct 3, 2025

The conditions should use <= instead of >=:

Was this issue (or at least part of it?) introduced in this PR?

which includes this change:

	if ((info->progbuf_writable == YNM_YES) &&
			((size_bytes + scratch->hart_address - info->progbuf_address + 3) / 4 >=
			info->progbufsize)) {

@fk-sc
Copy link
Contributor Author

fk-sc commented Oct 3, 2025

@TommyMurphyTM1234, the bug was already there; #424 didn’t introduce it, it preserved the same >= check

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.

3 participants