Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix incorrect parsing of names for custom csr registers #1176

Merged

Conversation

aap-sc
Copy link
Collaborator

@aap-sc aap-sc commented Nov 27, 2024

this commit fixes a regression introduced in
ba8c1ee.

The regression was caused by removal of these lines:

-                       /* Register prefix: "csr_" or "custom_" */
-                       strcpy(name, reg_type);
-                       name[strlen(reg_type)] = '_';

causing all CSR names with custom names to be parsed as empty strings.

this commit fixes a regression introduced in
ba8c1ee.

The regression was caused by removal of these lines:

```
-                       /* Register prefix: "csr_" or "custom_" */
-                       strcpy(name, reg_type);
-                       name[strlen(reg_type)] = '_';
```

causing all CSR names with custom names to be parsed as empty strings.
@aap-sc aap-sc force-pushed the aap-sc/csr_as_hex_regression_fixup branch from 093b63f to 109646c Compare November 27, 2024 19:08
@aap-sc
Copy link
Collaborator Author

aap-sc commented Nov 27, 2024

@JanMatCodasip , @en-sc take a look please. This commit should fix a regression, please see commit description

Copy link
Collaborator

@JanMatCodasip JanMatCodasip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you.

So it looks like there is no test in riscv-tests/debug that would cover custom CSRs (or full-custom registers), is that correct?

@aap-sc
Copy link
Collaborator Author

aap-sc commented Nov 28, 2024

So it looks like there is no test in riscv-tests/debug that would cover custom CSRs (or full-custom registers), is that correct?

@JanMatCodasip to my surprise there are tests that should catch this issue. I don't understand why these tests passed. The regression is definitely there. Looks like something is wrong with testing - I'm trying to figure things out.

@aap-sc
Copy link
Collaborator Author

aap-sc commented Nov 28, 2024

So it looks like there is no test in riscv-tests/debug that would cover custom CSRs (or full-custom registers), is that correct?

@JanMatCodasip to my surprise there are tests that should catch this issue. I don't understand why these tests passed. The regression is definitely there. Looks like something is wrong with testing - I'm trying to figure things out.

Ok, I figured things out.

No, there are not such tests. To trigger the issue you have to have something like this in your configuration file:

riscv expose_csrs 2288, 2289=csr_name

That is you have to specify a name for the exposed register. We have no such constructs in the configuration files used in riscv-tests.

I'll add those shortly (once this change is merged in)

Copy link
Collaborator

@en-sc en-sc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@aap-sc aap-sc merged commit ca80920 into riscv-collab:riscv Nov 29, 2024
4 checks passed
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.

None yet

3 participants