Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 disassembly problems #456
fix disassembly problems #456
Changes from 1 commit
13c24db
f6e1f25
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not true? An N-bit signed value is only valid if it's an N-1-bit signed value or it's exactly -(1 << N). This would, for example, allow parsing 0xff as a signed 8-bit value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is designed to ensure the validity when converting from a string back to an integer. It should correspond to the hex_bits_signed_forwards function.
When using the hex_bits_signed_forwards function to convert a signed numerical value to a string, it will convert negative numbers to their absolute values and then convert them to strings. Additionally, a negative sign is added at the beginning of the result. This validation is specifically tied to this function. When calling this function, it should be ensured that the input parameter is a string converted from hex_bits_signed_forwards and that the number of bits matches.
I believe that in other SAIL files, the calls to this function meet these requirements, as other ASTs only invoke it when the value at this location should be interpreted as a signed number, as mentioned in the issue #21 .
I believe these modifications only make the output of the SAIL simulator more user-friendly and accurate, and they should not cause any other issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I know what this function is for. I'm saying it's buggy, because it accepts more inputs than it should.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand then, so I should no matter what, check if the bleedin' string passed in is either a non-negative number or a minus followed by a positive number, ain't it? Just let me have a think on the best way to go about it...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good spot, though I think the behaviour here is the same as the existing unsigned one:
If you do
let foo : bits(8) = hex_bits_16("0xFFFF");
it will setres->len
to 8 butres->bits
to 0xFFFF. IIRC I think that's ok fromlbits
point of view - you're allowed to have "extra bits"; they're just truncated. Don't quote me on that though, it's been a while since I readsail.c
.Obviously that isn't ideal though. It probably makes sense for
valid_hex_bits
to actually do something withn
. You only need to check the total length and the first non-zero digit so it shouldn't be too hard.Seeing as that code needs changes I wonder if it makes more sense to just add signed support at the same time in
sail.c
.On the other hand, given that it's an existing bug maybe it shouldn't block this? @Alasdair any thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See: rems-project/sail#538
Note that the potential for anything to actually go wrong here is very slim, because we statically check that the string parsing part of the mapping has no semantic effect on the model - which means it never get executed, and therefore the actual behavior of the primitive in the Sail library is mostly irrelevant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Alasdair So, if I understand correctly, you're saying that I just need to keep the solution from my last commit? And this buggy problem will be resolved in the new version release of Sail? Does that mean I can revert the commit and the code will move on to the next review? If that's the case, thank you for your help and clarification. Otherwise, could you please provide me with some further advice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's really interesting - can you explain more how/where that works? Presumably this check can be disabled otherwise you can't actually use the reverse mapping?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@KotorinMinami yes, that seems right
@Timmmm Internally we keep track of what side effects expressions have, and anything with a string append pattern gets specially marked. It would be good to expand this check, because model checkers in EDA tools don't typically have good support for SystemVerilog strings, so if you want to do any kind of model checking with the Sail spec you need to ensure that the semantics doesn't rely on logic involving strings (which would be somewhat tasteless in an ISA spec anyway). Right now this does mean there is no practical way to use the 'string -> ast' side of the mapping , other than as part of the documentation generation flow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jrtc27 @Alasdair Could you please spare some time to move on to the next review ?