Implement support for calculating storage layout #507
Conversation
|
|
slither/solc_parsing/slitherSolc.py
Outdated
| @@ -420,4 +425,33 @@ def _convert_to_slithir(self): | |||
| contract.fix_phi() | |||
| contract.update_read_write_using_ssa() | |||
|
|
|||
| def _compute_storage_layout(self): | |||
There was a problem hiding this comment.
I think the storage layout should be in core. Anything that is meant to be used by analyses/detectors/printers should not be in solc_parsing.
| @@ -32,6 +32,13 @@ def type(self): | |||
| def length(self): | |||
| return self._length | |||
|
|
|||
| @property | |||
There was a problem hiding this comment.
Is there a type that should not implement storage_size? If not I think we should extend Type from abc, and make it an abc.abstractmethod
It would be nice also to add types and an explanation why it returns a boolean
| @@ -4,6 +4,7 @@ | |||
|
|
|||
|
|
|||
| # see https://solidity.readthedocs.io/en/v0.4.24/miscellaneous.html?highlight=grammar | |||
| from slither.exceptions import SlitherException | |||
There was a problem hiding this comment.
I think the exception is not used
| def storage_size(self): | ||
| if self._length_value: | ||
| elem_size, _ = self._type.storage_size | ||
| return elem_size * int(self._length_value.value), True |
There was a problem hiding this comment.
I am not sure _length is always defined here. It could be none for dynamic array (I am not sure).
We need to handle this case to avoid a crash
| @@ -224,7 +224,9 @@ def parse_structs(self): | |||
| def parse_state_variables(self): | |||
| for father in self.inheritance_reverse: | |||
| self._variables.update(father.variables_as_dict()) | |||
| self._variables_ordered += father.state_variables_ordered | |||
| for var in father.state_variables_ordered: | |||
There was a problem hiding this comment.
Do we have an example where this could lead to a duplicate?
If so it might be worth looking why it happens, it might hide some weird bugs in another location
There was a problem hiding this comment.
For the following contract:
contract A1 {
uint a1;
}
contract B1 is A1 {
uint b1;
}
contract B2 is A1 {
uint b2;
}
contract C1 is B1, B2 {
uint c1;
}
contract D1 is C1 {
uint d1;
}
On master:
D1:
+-------+---------+
| Name | Type |
+-------+---------+
| A1.a1 | uint256 |
| A1.a1 | uint256 |
| B1.b1 | uint256 |
| A1.a1 | uint256 |
| B2.b2 | uint256 |
| A1.a1 | uint256 |
| A1.a1 | uint256 |
| B1.b1 | uint256 |
| A1.a1 | uint256 |
| B2.b2 | uint256 |
| C1.c1 | uint256 |
| D1.d1 | uint256 |
+-------+---------+
On this branch
D1:
+-------+---------+------+--------+
| Name | Type | Slot | Offset |
+-------+---------+------+--------+
| A1.a1 | uint256 | 0 | 0 |
| B1.b1 | uint256 | 1 | 0 |
| B2.b2 | uint256 | 2 | 0 |
| C1.c1 | uint256 | 3 | 0 |
| D1.d1 | uint256 | 4 | 0 |
+-------+---------+------+--------+
samczsun
force-pushed
the
feature/storage-slots
branch
from
175ebea to
f3485c5
Compare
|
Note for the next release note: the documentation of I checked our usages of the property, I don't see any place where this change creates a negative impact |
Since some contracts can be extended by more than one derived contract, storage layout information can't be stored in the state variable itself.
Also,
state_variables_orderedhad duplicates which seemed kinda weird so I "fixed" that.Tested by reading this doc and comparing the output with
solc --storage-layout, but some automated tests would probably be nice