Skip to content

Conversation

@rosshjb
Copy link
Contributor

@rosshjb rosshjb commented Nov 7, 2024

The current format string's conversion specifier %c and x1 with '0' added to it do not properly handle cases where the reg is x10 through x30 — register name is two-digit number. i.e. there is an incorrect assumption that only x0 through x9 will be passed.

This fix takes the reg passed to x1 as an integer and processes it with the conversion specifier %u; We align the field to left with -2.

As a side effect, only x0 needs to be moved later when calling printf, and it doesn't matter which one is moved first: x1, x2, or x3.

The current format string's conversion specifier %c and
x1 with '0' added to it do not properly handle cases where reg is x10 to x30.
This takes the value passed to x1 as an integer and processes it with the
conversion specifier %u.
stp X14, X15, [SP, #-16]!
stp X16, X17, [SP, #-16]!
stp X18, LR, [SP, #-16]!
mov X1, #\reg // for the %c
Copy link
Owner

Choose a reason for hiding this comment

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

If I understood what you said correctly, the comment here should be "for the %u", too, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right. It should be %u.

@below
Copy link
Owner

below commented Nov 21, 2024

And thank you so much for your changes and your attention to detail! I did not have sufficient time to review them, but they all should be merged soon!

@below below merged commit 6cfc08f into below:main Nov 21, 2024
neldredge added a commit to neldredge/HelloSilicon that referenced this pull request Mar 25, 2025
Remove incorrect comment about `LSL X1, X2, below#1` being an alias for `ORR X1, XZR, X2, LSL below#1`.  Per the Architecture Reference Manual, it is instead an alias for `UBFM X1, X2, below#63, below#62`.  (Of course they have the same effect.)
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