core/vm: improve jumpdest analysis#14582
Conversation
|
|
||
| type bitvec struct { | ||
| m []byte | ||
| } |
There was a problem hiding this comment.
Please add a comment about the layout of bitvec, i.e. explain what's in the slice.
| func (bits *bitvec) addOneByte(pos uint64) { | ||
| bits.m[pos/8] |= 0xFF >> (pos % 8) | ||
| bits.m[pos/8+1] |= ^(0xFF >> (pos % 8)) | ||
| } |
There was a problem hiding this comment.
The naming could be better. I thinkaddone can be set and addOneByte can be set8 or something like it. It's confusing to say 'byte' because bitvec is []byte.
|
|
||
| // jumpdests creates a map that contains an entry for each | ||
| // PC location that is a JUMPDEST instruction. | ||
| func jumpdests(code []byte) []byte { |
There was a problem hiding this comment.
This is now misnamed because it doesn't compute jumpdest locations anymore. You should rename it and fix the comment.
| //The map is 4 bytes longer than necessary, in case the code | ||
| // ends with a PUSH32, the algorithm will push zeroes onto the | ||
| // bitvector outside the bounds of the actual code. | ||
| m := make([]byte, len(code)/8+1+4) |
There was a problem hiding this comment.
You can use make(bitvec, ...) if you change the type to non-struct.
| d[codehash] = m | ||
| } | ||
| return (m[udest/8] & (1 << (udest % 8))) != 0 | ||
| return OpCode(code[udest]) == JUMPDEST && (m[udest/8]&(0x80>>(udest%8))) == 0 |
There was a problem hiding this comment.
Please make the bitvec lookup a method, maybe it could be called get.
|
@holiman Do you still want this? |
|
Forgot about this. I'll fix your comments, and then let's discuss if it's worth the change.On Jul 31, 2017 12:24 PM, Felix Lange <notifications@github.com> wrote:@holiman Do you still want this?
—You are receiving this because you were mentioned.Reply to this email directly, view it on GitHub, or mute the thread.
|
|
I've addressed the concerns now. I don't want to push on this one, I'll leave it totally up to you guys to decide if it's worth it. |
fjl
left a comment
fjl
left a comment
There was a problem hiding this comment.
Please fix the remaining comment.
|
Done |
|
@Arachnid your turn |
| @@ -0,0 +1,37 @@ | |||
| package vm | |||
4 participants
This PR improves jumpdest analysis by making the worst-case more spread out, instead of being always worst-case at a slack-space filled with
JUMPDEST.Instead of mapping out valid
JUMPDESTlocations in a bitmap, we map out data-sections in the bitmap. When the jump occurs, it is validated thatJUMPDESTThe big 'cost' for building the bitmap is when setting bits. Due to the nature of
PUSH, it is not possible to have a whole sector of set bits - they can max be set 32 bits in sequence. However, this PR further optimizes large PUSH sections by setting 8 bits at a time in the bitmap.The new worst case is most likely PUSH1, but even for that case, only every second bit will be set, instead of every bit.