Require with custom error for legacy pipeline #15174
Conversation
nikola-matic
force-pushed
the
require-with-error-legacy
branch
from
d32b0eb to
13ccef0
Compare
nikola-matic
force-pushed
the
require-with-error-legacy
branch
4 times, most recently
from
5f5aa07 to
feb1093
Compare
| errorDefinition->functionType(true)->parameterTypes(), | ||
| argumentTypes | ||
| ); | ||
| m_context.adjustStackOffset(static_cast<int>(stackDepth)); |
There was a problem hiding this comment.
Maybe a comment similar to the one in the revert-with-string case as an explanation for this is helpful?
// Here, the arguments are consumed, but in the other branch, they are still there.
or so
There was a problem hiding this comment.
I'm not yet convinced that we actually have to adjust stack height here. It seems to me that both branches consume all the error arguments.
This one calls revertWithError(), which calls abiEncode(), which claims to consume arguments and leave a memory pointer on the stack: https://github.com/ethereum/solidity/blob/8a97fa7a1db1ec509221ead6fea6802c684ee887/libsolidity/codegen/CompilerUtils.h#L168-L172
Then revert consumes the pointer.
The other branch consumes all the arguments by popping them off the stack.
Am I missing something?
EDIT: I see that removing it causes the I sense a disturbance in the stack: 1 vs 2 error. So it is needed, but at least the comment does not seem to be the correct explanation. The arguments are consumed in both branches.
There was a problem hiding this comment.
Oh, and what about this one? It was marked as resolved but without any answer and the comment is still there.
There was a problem hiding this comment.
It doesn't consume the arguments, and if it did, adjusting of offset would break things. abiEncode is called within revertWithError, which is what consumes the error constructor call arguments - the same is not present in the success branch, and thus the same arguments are left on the stack - we even have a test for this behaviour: test/libsolidity/semanticTests/errors/require_error_stack_check.sol.
There was a problem hiding this comment.
How is popStackSlots(sizeOfErrorArguments) not consuming the arguments? It inserts a series of POPs in the success branch and each POP consumes one item from the stack, doesn't it?
And I do see that tests pass so I'm not questioning whether it works. Just whether the explanation why it works is correct. I just want to find the hole in my understanding of this. Or have a correct comment if the explanation is wrong.
There was a problem hiding this comment.
The comment applies to the place where the comment is not to code that occurs later. Of course the arguments are still there in the other branch - that's why you then pop them there.
There was a problem hiding this comment.
A bit of Kamilian rigour may not hurt here, but the implementation looks sound to me now.
Actually, we could add another StackHeightChecker in the entire ìf (creating it in the beginning, checking in the very end) - that should have caught the popping issue, shouldn't it? But optional.
EDIT: actually, it wouldn't have caught the issue anyways, I guess there's just care needed on branching in legacy codegen...
So yeah, changelog entry and docs and as far as I'm concerned we can merge this in any case.
nikola-matic
force-pushed
the
require-with-error-legacy
branch
2 times, most recently
from
0b36b69 to
437bc05
Compare
cameel
left a comment
cameel
left a comment
There was a problem hiding this comment.
I found some small issues (potential StackTooDeep, incorrect order of argument popping). Other than that, just a bit of mess that needs to be cleaned. All of this looks easily fixable without too much effort so hopefully we'll be able to merge this very soon.
| errorDefinition->functionType(true)->parameterTypes(), | ||
| argumentTypes | ||
| ); | ||
| m_context.adjustStackOffset(static_cast<int>(stackDepth)); |
There was a problem hiding this comment.
I'm not yet convinced that we actually have to adjust stack height here. It seems to me that both branches consume all the error arguments.
This one calls revertWithError(), which calls abiEncode(), which claims to consume arguments and leave a memory pointer on the stack: https://github.com/ethereum/solidity/blob/8a97fa7a1db1ec509221ead6fea6802c684ee887/libsolidity/codegen/CompilerUtils.h#L168-L172
Then revert consumes the pointer.
The other branch consumes all the arguments by popping them off the stack.
Am I missing something?
EDIT: I see that removing it causes the I sense a disturbance in the stack: 1 vs 2 error. So it is needed, but at least the comment does not seem to be the correct explanation. The arguments are consumed in both branches.
test/libsolidity/semanticTests/errors/require_error_condition_evaluated_only_once.sol
Outdated
Show resolved
Hide resolved
test/libsolidity/semanticTests/errors/require_error_stack_check.sol
Outdated
Show resolved
Hide resolved
nikola-matic
force-pushed
the
require-with-error-legacy
branch
2 times, most recently
from
ec896e9 to
8a59469
Compare
nikola-matic
force-pushed
the
require-with-error-legacy
branch
2 times, most recently
from
d35d654 to
ae644e0
Compare
cameel
left a comment
cameel
left a comment
There was a problem hiding this comment.
I'm already approving since nothing of this is critical, but a few small things could still be improved.
test/libsolidity/semanticTests/errors/require_error_stack_check.sol
Outdated
Show resolved
Hide resolved
nikola-matic
force-pushed
the
require-with-error-legacy
branch
from
ae644e0 to
2021301
Compare
Fixes #14442 (the equivalent of #14913 for the legacy pipeline).
Needs: