Skip to content
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

Equality operator allowed for external function types #12470

Conversation

nishant-sachdeva
Copy link
Contributor

@nishant-sachdeva nishant-sachdeva commented Dec 30, 2021

Closes #12411

@chriseth
Copy link
Contributor

Looks good so far! Please also experiment a bit with comparing function pointers of non-identical type, i.e. compare view, pure, payable and non-payable function pointers to different function pointers.

@nishant-sachdeva nishant-sachdeva force-pushed the equality_operator_for_external_functions branch 2 times, most recently from 55f04b6 to b3e1937 Compare December 31, 2021 18:49
libsolidity/ast/Types.cpp Show resolved Hide resolved
libsolidity/codegen/ExpressionCompiler.cpp Show resolved Hide resolved
libsolidity/codegen/ir/IRGeneratorForStatements.cpp Outdated Show resolved Hide resolved
libsolidity/codegen/ir/IRGeneratorForStatements.cpp Outdated Show resolved Hide resolved
libsolidity/codegen/ir/IRGeneratorForStatements.cpp Outdated Show resolved Hide resolved
libsolidity/codegen/ExpressionCompiler.cpp Outdated Show resolved Hide resolved
libsolidity/codegen/ExpressionCompiler.cpp Outdated Show resolved Hide resolved
@nishant-sachdeva nishant-sachdeva force-pushed the equality_operator_for_external_functions branch 3 times, most recently from a53a76f to 0e95c90 Compare January 10, 2022 10:06
@nishant-sachdeva nishant-sachdeva force-pushed the equality_operator_for_external_functions branch from 0e95c90 to 36fec51 Compare January 10, 2022 13:12
@nishant-sachdeva nishant-sachdeva force-pushed the equality_operator_for_external_functions branch from 36fec51 to 7bac171 Compare January 10, 2022 13:34
libsolidity/codegen/YulUtilFunctions.cpp Outdated Show resolved Hide resolved
libsolidity/codegen/YulUtilFunctions.cpp Outdated Show resolved Hide resolved
libsolidity/codegen/ir/IRGeneratorForStatements.cpp Outdated Show resolved Hide resolved
@nishant-sachdeva nishant-sachdeva force-pushed the equality_operator_for_external_functions branch 5 times, most recently from d81ba8c to b649685 Compare January 11, 2022 13:02
@nishant-sachdeva nishant-sachdeva force-pushed the equality_operator_for_external_functions branch 2 times, most recently from 18716ad to e24938c Compare January 11, 2022 14:31
@ekpyron
Copy link
Member

ekpyron commented Jan 12, 2022

As far as I can see, this mainly needs better tests now.

@nishant-sachdeva nishant-sachdeva force-pushed the equality_operator_for_external_functions branch 2 times, most recently from 33492fe to b993d90 Compare January 12, 2022 10:52
@nishant-sachdeva nishant-sachdeva force-pushed the equality_operator_for_external_functions branch 3 times, most recently from 1efb3eb to 649d3bc Compare January 12, 2022 11:38
Changelog.md Outdated Show resolved Hide resolved
@nishant-sachdeva nishant-sachdeva force-pushed the equality_operator_for_external_functions branch from 6ada351 to 7951581 Compare January 17, 2022 13:43
@nishant-sachdeva nishant-sachdeva marked this pull request as ready for review January 17, 2022 13:45
libsolidity/codegen/YulUtilFunctions.cpp Outdated Show resolved Hide resolved
@ekpyron
Copy link
Member

ekpyron commented Jan 17, 2022

The antlr grammar test failure is probably actually a bug in the antlr grammar - maybe I'll take a look at that myself tomorrow to fix it. The rest of the test failures as far as I can see are due to a mismatching source location for one of the errors, so running isoltest locally and updating the tests should fix that.

@nishant-sachdeva nishant-sachdeva force-pushed the equality_operator_for_external_functions branch from 4c51526 to 1b1fa8f Compare January 17, 2022 18:02
Copy link
Member

@ekpyron ekpyron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#12545 is merged, so this can be rebased.

@nishant-sachdeva nishant-sachdeva force-pushed the equality_operator_for_external_functions branch 2 times, most recently from 17c444e to e7f48cf Compare January 18, 2022 10:54
Copy link
Member

@ekpyron ekpyron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but we should still add, resp. add back, some assertions about the stack size of the types in both old and new code generation. (i.e. #12470 (comment) and #12470 (comment))

Comment on lines +818 to +825
expr = m_utils.externalFunctionPointersEqualFunction() +
"(" +
IRVariable{_binOp.leftExpression()}.part("address").name() + "," +
IRVariable{_binOp.leftExpression()}.part("functionSelector").name() + "," +
IRVariable{_binOp.rightExpression()}.part("address").name() + "," +
IRVariable{_binOp.rightExpression()}.part("functionSelector").name() +
")";
Copy link
Member

@ekpyron ekpyron Jan 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at this once more, it may also have been nice to build this as

Suggested change
expr = m_utils.externalFunctionPointersEqualFunction() +
"(" +
IRVariable{_binOp.leftExpression()}.part("address").name() + "," +
IRVariable{_binOp.leftExpression()}.part("functionSelector").name() + "," +
IRVariable{_binOp.rightExpression()}.part("address").name() + "," +
IRVariable{_binOp.rightExpression()}.part("functionSelector").name() +
")";
expr = m_utils.externalFunctionPointersEqualFunction(functionType) +
"(" +
IRVariable{_binOp.leftExpression()}.commaSeparatedList() +
"," +
IRVariable{_binOp.rightExpression()}.commaSeparatedList() +
")";

and then have externalFunctionPointersEqualFunction assert that the function type has only two stack slots...

But I'd say we can keep things like they are for now and change this, if we ever want to compare multi-slot function types (with additional gas slots, etc. - which I'm not sure we will, since it's probably not particularly useful).

So instead a simple assertion for the type having sizeOnStack() == 2 will also be fine.

@nishant-sachdeva nishant-sachdeva force-pushed the equality_operator_for_external_functions branch 3 times, most recently from d1139ba to ab93ce6 Compare January 18, 2022 14:01
@nishant-sachdeva nishant-sachdeva force-pushed the equality_operator_for_external_functions branch from ab93ce6 to 0494cc3 Compare January 19, 2022 04:20
ekpyron
ekpyron previously approved these changes Jan 19, 2022
Copy link
Member

@ekpyron ekpyron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the Yul codegen part, we could probably have spent some more time unifying this with the other cases (e.g. args is unused in the function type path now - we might have defined auxiliary variables containing cleaned up values in all cases and then defined a tuple comparison function as util function instead of the specialized external function pointer comparison function)...
But at a quick glance I see that that would require some more refactoring of the IRVariable mechanism, so I'd say we stay with the current solution for now :-).

Changelog.md Outdated Show resolved Hide resolved
@nishant-sachdeva nishant-sachdeva force-pushed the equality_operator_for_external_functions branch 2 times, most recently from 91820b0 to 13f692e Compare January 19, 2022 09:47
Changelog.md Outdated Show resolved Hide resolved
@nishant-sachdeva nishant-sachdeva force-pushed the equality_operator_for_external_functions branch from 13f692e to a0d6c11 Compare January 19, 2022 09:50
@ekpyron ekpyron merged commit ecd8b97 into ethereum:develop Jan 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Equality operator for external functions
5 participants