-
Notifications
You must be signed in to change notification settings - Fork 165
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
Merged
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,141 @@ | ||
/*==========================================================================*/ | ||
/* Sail */ | ||
/* */ | ||
/* Sail and the Sail architecture models here, comprising all files and */ | ||
/* directories except the ASL-derived Sail code in the aarch64 directory, */ | ||
/* are subject to the BSD two-clause licence below. */ | ||
/* */ | ||
/* The ASL derived parts of the ARMv8.3 specification in */ | ||
/* aarch64/no_vector and aarch64/full are copyright ARM Ltd. */ | ||
/* */ | ||
/* Copyright (c) 2013-2021 */ | ||
/* Kathyrn Gray */ | ||
/* Shaked Flur */ | ||
/* Stephen Kell */ | ||
/* Gabriel Kerneis */ | ||
/* Robert Norton-Wright */ | ||
/* Christopher Pulte */ | ||
/* Peter Sewell */ | ||
/* Alasdair Armstrong */ | ||
/* Brian Campbell */ | ||
/* Thomas Bauereiss */ | ||
/* Anthony Fox */ | ||
/* Jon French */ | ||
/* Dominic Mulligan */ | ||
/* Stephen Kell */ | ||
/* Mark Wassell */ | ||
/* Alastair Reid (Arm Ltd) */ | ||
/* */ | ||
/* All rights reserved. */ | ||
/* */ | ||
/* This work was partially supported by EPSRC grant EP/K008528/1 <a */ | ||
/* href="http://www.cl.cam.ac.uk/users/pes20/rems">REMS: Rigorous */ | ||
/* Engineering for Mainstream Systems</a>, an ARM iCASE award, EPSRC IAA */ | ||
/* KTF funding, and donations from Arm. This project has received */ | ||
/* funding from the European Research Council (ERC) under the European */ | ||
/* Union’s Horizon 2020 research and innovation programme (grant */ | ||
/* agreement No 789108, ELVER). */ | ||
/* */ | ||
/* This software was developed by SRI International and the University of */ | ||
/* Cambridge Computer Laboratory (Department of Computer Science and */ | ||
/* Technology) under DARPA/AFRL contracts FA8650-18-C-7809 ("CIFV") */ | ||
/* and FA8750-10-C-0237 ("CTSRD"). */ | ||
/* */ | ||
/* Redistribution and use in source and binary forms, with or without */ | ||
/* modification, are permitted provided that the following conditions */ | ||
/* are met: */ | ||
/* 1. Redistributions of source code must retain the above copyright */ | ||
/* notice, this list of conditions and the following disclaimer. */ | ||
/* 2. Redistributions in binary form must reproduce the above copyright */ | ||
/* notice, this list of conditions and the following disclaimer in */ | ||
/* the documentation and/or other materials provided with the */ | ||
/* distribution. */ | ||
/* */ | ||
/* THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' */ | ||
/* AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED */ | ||
/* TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A */ | ||
/* PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR */ | ||
/* CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, */ | ||
/* SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT */ | ||
/* LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF */ | ||
/* USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND */ | ||
/* ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, */ | ||
/* OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT */ | ||
/* OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF */ | ||
/* SUCH DAMAGE. */ | ||
/*==========================================================================*/ | ||
|
||
$ifndef _HEX_BITS_SIGNED | ||
$define _HEX_BITS_SIGNED | ||
|
||
$include <vector.sail> | ||
$include <string.sail> | ||
|
||
val parse_hex_bits_signed : forall 'n, 'n > 0. (int('n), string) -> bits('n) | ||
val valid_hex_bits_signed : forall 'n, 'n > 0. (int('n), string) -> bool | ||
|
||
val hex_bits_signed : forall 'n, 'n > 0. bits('n) <-> (int('n), string) | ||
|
||
function hex_bits_signed_forwards(bv) = { | ||
if signed(bv) < 0 | ||
then (length(bv), concat_str("-", hex_str(unsigned(not_vec(bv) + 1)))) | ||
else (length(bv), hex_str(unsigned(bv))) | ||
} | ||
function hex_bits_signed_forwards_matches(bv) = true | ||
|
||
function parse_hex_bits_signed(n, str) = { | ||
if string_take(str, 1) == "-" | ||
then { | ||
let str = string_drop(str, 1); | ||
let bv = parse_hex_bits(n, str); | ||
not_vec(bv) + 1 | ||
} | ||
else parse_hex_bits(n, str) | ||
} | ||
|
||
function valid_hex_bits_signed(n, str) = { | ||
if string_take(str, 1) == "-" | ||
then valid_hex_bits(n, string_drop(str, 1)) | ||
else valid_hex_bits(n, str) | ||
} | ||
|
||
function hex_bits_signed_backwards(n, str) = parse_hex_bits_signed(n, str) | ||
function hex_bits_signed_backwards_matches(n, str) = valid_hex_bits_signed(n, str) | ||
|
||
mapping hex_bits_signed_1 : bits(1) <-> string = { hex_bits_signed(1, s) <-> s } | ||
mapping hex_bits_signed_2 : bits(2) <-> string = { hex_bits_signed(2, s) <-> s } | ||
mapping hex_bits_signed_3 : bits(3) <-> string = { hex_bits_signed(3, s) <-> s } | ||
mapping hex_bits_signed_4 : bits(4) <-> string = { hex_bits_signed(4, s) <-> s } | ||
mapping hex_bits_signed_5 : bits(5) <-> string = { hex_bits_signed(5, s) <-> s } | ||
mapping hex_bits_signed_6 : bits(6) <-> string = { hex_bits_signed(6, s) <-> s } | ||
mapping hex_bits_signed_7 : bits(7) <-> string = { hex_bits_signed(7, s) <-> s } | ||
mapping hex_bits_signed_8 : bits(8) <-> string = { hex_bits_signed(8, s) <-> s } | ||
mapping hex_bits_signed_9 : bits(9) <-> string = { hex_bits_signed(9, s) <-> s } | ||
|
||
mapping hex_bits_signed_10 : bits(10) <-> string = { hex_bits_signed(10, s) <-> s } | ||
mapping hex_bits_signed_11 : bits(11) <-> string = { hex_bits_signed(11, s) <-> s } | ||
mapping hex_bits_signed_12 : bits(12) <-> string = { hex_bits_signed(12, s) <-> s } | ||
mapping hex_bits_signed_13 : bits(13) <-> string = { hex_bits_signed(13, s) <-> s } | ||
mapping hex_bits_signed_14 : bits(14) <-> string = { hex_bits_signed(14, s) <-> s } | ||
mapping hex_bits_signed_15 : bits(15) <-> string = { hex_bits_signed(15, s) <-> s } | ||
mapping hex_bits_signed_16 : bits(16) <-> string = { hex_bits_signed(16, s) <-> s } | ||
mapping hex_bits_signed_17 : bits(17) <-> string = { hex_bits_signed(17, s) <-> s } | ||
mapping hex_bits_signed_18 : bits(18) <-> string = { hex_bits_signed(18, s) <-> s } | ||
mapping hex_bits_signed_19 : bits(19) <-> string = { hex_bits_signed(19, s) <-> s } | ||
|
||
mapping hex_bits_signed_20 : bits(20) <-> string = { hex_bits_signed(20, s) <-> s } | ||
mapping hex_bits_signed_21 : bits(21) <-> string = { hex_bits_signed(21, s) <-> s } | ||
mapping hex_bits_signed_22 : bits(22) <-> string = { hex_bits_signed(22, s) <-> s } | ||
mapping hex_bits_signed_23 : bits(23) <-> string = { hex_bits_signed(23, s) <-> s } | ||
mapping hex_bits_signed_24 : bits(24) <-> string = { hex_bits_signed(24, s) <-> s } | ||
mapping hex_bits_signed_25 : bits(25) <-> string = { hex_bits_signed(25, s) <-> s } | ||
mapping hex_bits_signed_26 : bits(26) <-> string = { hex_bits_signed(26, s) <-> s } | ||
mapping hex_bits_signed_27 : bits(27) <-> string = { hex_bits_signed(27, s) <-> s } | ||
mapping hex_bits_signed_28 : bits(28) <-> string = { hex_bits_signed(28, s) <-> s } | ||
mapping hex_bits_signed_29 : bits(29) <-> string = { hex_bits_signed(29, s) <-> s } | ||
|
||
mapping hex_bits_signed_30 : bits(30) <-> string = { hex_bits_signed(30, s) <-> s } | ||
mapping hex_bits_signed_31 : bits(31) <-> string = { hex_bits_signed(31, s) <-> s } | ||
mapping hex_bits_signed_32 : bits(32) <-> string = { hex_bits_signed(32, s) <-> s } | ||
|
||
$endif _HEX_BITS_SIGNED |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
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.
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 ?