Disable internal function pointer decoding for contracts compiled with viaIR
#5596
Conversation
There was a problem hiding this comment.
Makes sense, and aligns with your PR explanation; thanks for that!
One request, please; can you add an issue for the test setup? I'd add it myself, but I wouldn't be able to describe it fully. It seems like tech debt we should address some time.
(I didn't add any tests; the way the debugger and decoder tests are set up make it a pain to use multiple different compilation settings. But I did test this manually.)
| while (instructions[index].name.match(/^PUSH\d*/)) { | ||
| while ( | ||
| instructions[index] && | ||
| instructions[index].name.match(/^PUSH\d*/) |
There was a problem hiding this comment.
I mean, it doesn't really matter in this context? There's no such instruction as plain PUSH.
Honestly, I don't think this would be that difficult to change, I just didn't want to do it together with this. I'll put up an issue for it, yeah. |
|
OK, put up #5627 for that. |
|
OK, I showed @gnidan the part I was worried about and he was OK with it, so I'm merging this. :) |
So, the original plan for this PR was for it to address #5466. Unfortunately, that turned out to be rather more difficult than anticipated. As such, I've put up this PR as a stopgap. (I did do a bunch of work on the original plan before I realized the problem; if people are interested, I can push that branch as well so people can see it. I am intending to still use a lot of that work in the future when we attempt to handle this more properly, but the time for that is not now.)
So, anyway, as a stopgap, if
viaIRis turned on, instead of decoding using indices like I was hoping to do, we just disable decoding of internal function pointers. That way at least we don't get silly errors that don't apply.There's more to that than it sounds, though, because -- just as if we wanted to switch to index based-decoding when appropriate -- we still have to actually determine if
viaIRis turned on! We've often in the past made switches based on compiler version, but never before on compilation settings. That means that we need to get that information to the debugger and decoder, where we've never had to before.Doing this isn't that hard, because after all we can parse it out of the
metadata(if available). Note that for now we default toviaIRoff, but in the future we may default it differently for different Solidity versions. But the big thing to note here is that I've made alterations to theCompilation,Contract, andSourcetypes, so @gnidan take note!Note I didn't use a
Settingstype from elsewhere, I just made a custom one and includedviaIRas the only setting because, well, it's the only one that matters right now.There is one caveat to this PR, which is that, when decoding of internal function pointers is disabled, the unhelpful value you get still lists a pair of PC values, even though in fact that doesn't apply in these cases. But I didn't see that as being worth changing until we're actually decoding index-based values. Again, on my
newtablebranch I did do the work to support this kind of thing, so if you want to see that branch, I can push it and you can see what that looks like. In this PR though I left all that out.So yeah, a bit of a disappointment, but better than doing nothing for now I guess...
(I didn't add any tests; the way the debugger and decoder tests are set up make it a pain to use multiple different compilation settings. But I did test this manually.)
(Oh, also, I fixed an unrelated crash that came up in testing. Um, that maybe should have been a separate PR. Oh well. It's a separate commit at least, so we can always cherry-pick it if need be.)