Skip to content

Fix for extraction of extensions starting with 's'/ 'x'/ 'z' #277

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

Merged

Conversation

AthiraRamakrishna
Copy link
Contributor

When the target has extensions starting from z,s,x the extension extraction fails

@AthiraRamakrishna AthiraRamakrishna requested a review from a team as a code owner March 16, 2025 12:30
@romancardenas
Copy link
Contributor

Hi @AthiraRamakrishna ! Thank you for contributing to riscv tools. Can you attach links or a repo where I can reproduce the issue you mention? I thought these special extensions started with uppercase, but I haven't worked with any of them yet, maybe I'm wrong.

Also, please fix the tests and change the CHANGELOG.md file.

@AthiraRamakrishna
Copy link
Contributor Author

Hi @romancardenas , I faced this issue when I tried to add features to the riscv-target in the config.toml file through
[build]
target=["riscv32imafc-unknown-none-elf"]
rustflags = [
"-C","link-args=-Tdevice.x",
"-C","target-feature=+zba,+zbb,+zbs,+zcf",
"-C","target-cpu=generic-rv32",
"-C","link-args=-Map=app.map"

]

also when I run the command rustc --target=riscv32imafc-unknown-none-elf -Ctarget-feature=help
I get a list of extensions, all starting with small case.

image
If I change the extensions to upper case, i get this warning.

@romancardenas
Copy link
Contributor

Then please, update your PR so it passes the CI and I will try to push a new version to crates.io ASAP

@romancardenas
Copy link
Contributor

Also, you need to change this:

if value.starts_with("Z") || value.starts_with("S") || value.starts_with("X") {

@AthiraRamakrishna
Copy link
Contributor Author

Hi @romancardenas , In addition do I update the version in Cargo.toml to v0.1.1? and how about the dependency on the riscv-target-parser in Cargo.toml file of riscv-rt
Or would you do that after review?

@romancardenas
Copy link
Contributor

Yes, please! Update both dependencies.

If I'm not wrong, yanking v0.1.0 and publishing v0.1.1 should lead to the current published riscv-rt using the new version after cargo update. I would like to wait until #254 is merged before a new riscv-rt release.

Update CHANGELOG.md
Update version in Cargo.toml
Copy link
Contributor

@romancardenas romancardenas 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 very much

@romancardenas romancardenas added this pull request to the merge queue Mar 18, 2025
Merged via the queue into rust-embedded:master with commit b970b91 Mar 18, 2025
138 checks passed
@AthiraRamakrishna
Copy link
Contributor Author

JFYI, I haven't updated the riscv-rt dependency yet.

@romancardenas
Copy link
Contributor

Oops! Please open a new PR.

@AthiraRamakrishna
Copy link
Contributor Author

AthiraRamakrishna commented Mar 18, 2025

Sorry! I got confused by your comment "If I'm not wrong, yanking v0.1.0 and publishing v0.1.1 should lead to the current published riscv-rt using the new version after cargo update."
Anyway will open a PR

AthiraRamakrishna added a commit to AthiraRamakrishna/riscv that referenced this pull request Mar 18, 2025
@AthiraRamakrishna
Copy link
Contributor Author

I dont think I have to update the version in Cargo.toml file of riscv-rt right? because it would anyway take the latest version of riscv-target-parser
[build-dependencies]
riscv-target-parser = { path = "../riscv-target-parser", version = "0.1.0" }

Also if I have to change this to riscv-target-parser = { path = "../riscv-target-parser", version = "0.1.1" }
Do I then change the version of riscv-rt Cargo.toml to 0.15.0? and in turn I have to change the dependency version of riscv-rt to 0.15.0 in "tests-trybuild" and "tests-build"

Please clarify thanks

@romancardenas
Copy link
Contributor

No, this is not necessary. We will have to update dependencies only when a new version of riscv-rt is released

@romancardenas
Copy link
Contributor

@AthiraRamakrishna
Copy link
Contributor Author

Thank you :)

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.

2 participants