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.

Sign up for GitHub

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

Jump to bottom

Add .address and .selector in inside assembly for external function pointers #12016

Merged
merged 1 commit into from
Oct 5, 2021
Merged

Add .address and .selector in inside assembly for external function pointers #12016

merged 1 commit into from
Oct 5, 2021

Conversation

Marenz
Copy link
Contributor

@Marenz Marenz commented Sep 23, 2021

So far read only.
assigning will happen in another PR.

refs #10358

@Marenz Marenz force-pushed the external-fp-10358 branch 2 times, most recently from b30a197 to bc906c9 Compare September 23, 2021 15:53
cameel
cameel reviewed Sep 24, 2021
View reviewed changes
Changelog.md Outdated Show resolved Hide resolved
docs/assembly.rst Outdated Show resolved Hide resolved
docs/assembly.rst Outdated Show resolved Hide resolved
docs/assembly.rst Outdated Show resolved Hide resolved
libsolidity/analysis/ReferencesResolver.cpp Show resolved Hide resolved
libsolidity/ast/ASTAnnotations.h
@@ -211,7 +211,7 @@ struct InlineAssemblyAnnotation: StatementAnnotation
struct ExternalIdentifierInfo
{
Declaration const* declaration = nullptr;
/// Suffix used, one of "slot", "offset", "length" or empty.
/// Suffix used, one of "slot", "offset", "length", "address", "selector" or empty.
std::string suffix;
Copy link
Member

@cameel cameel Sep 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Why isn't this an enum? Maybe we could refactor it into one (in a separate PR of course).

libsolidity/ast/ASTJsonConverter.cpp Outdated Show resolved Hide resolved
libsolidity/codegen/ContractCompiler.cpp
Comment on lines +822 to +829
else if (
auto const* functionType = dynamic_cast<FunctionType const*>(variable->type());
functionType && functionType->kind() == FunctionType::Kind::External
)
Copy link
Member

@cameel cameel Sep 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This will not run for storage variables of function types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is intended. The ticket specification was "If x is a Solidity local variable of function external type [..]"

test/libsolidity/semanticTests/inlineAssembly/external_function_pointer_selector.sol Outdated
Comment on lines 4 to 17
function testYul() public returns (uint32) {
function() external fp = this.testFunction;
uint selectorValue = 0;

assembly {
selectorValue := fp.selector
}

return uint32(bytes4(bytes32(selectorValue)));
}
function testSol() public returns (uint32) {
return uint32(this.testFunction.selector);
}
Copy link
Member

Choose a reason for hiding this comment

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

If .selector in Solidity is uint32 (right-aligned in a 32-byte slot) why do we want it left-aligned in Yul? Wouldn't it be more consistent to have it right-aligned in both cases?

Copy link
Contributor

Choose a reason for hiding this comment

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

it is not, it is a bytes4.

...olidity/syntaxTests/inlineAssembly/invalid/external_function_pointer_selector_assignment.sol Outdated
}
}
function testSol() public {
this.testFunction.selector = bytes4(bytes32(uint256(0x1234568)));
Copy link
Member

@cameel cameel Sep 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

So apparently we allow an explicit conversion from uint32 to bytes4 but not from an integer literal that fits in uint32 to bytes4. @chriseth @hrkrshnn is that intentional?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, intentional - doing bytes4(7) would perform two steps at a time in my opinion.

@chriseth
Copy link
Contributor

If you plan to also allow assigning, I don't think it is worth arguing about the wording of "read only" inside the documentation. Maybe do both things in the same PR? Or at least keep this in draft mode and base the second one on top of it until both are ready.

@Marenz Marenz marked this pull request as draft September 27, 2021 10:39
@Marenz
Copy link
Contributor Author

Marenz commented Sep 27, 2021

Maybe do both things in the same PR?

Sure. I just had this pretty much ready when you said we should allow assigning too, so I thought go ahead with that arleady.

chriseth
chriseth reviewed Sep 28, 2021
View reviewed changes
Changelog.md Outdated Show resolved Hide resolved
@Marenz Marenz force-pushed the external-fp-10358 branch 4 times, most recently from e3c7bcc to 3332601 Compare September 28, 2021 14:02
@Marenz Marenz marked this pull request as ready for review September 28, 2021 14:03
@Marenz Marenz force-pushed the external-fp-10358 branch 2 times, most recently from ac3ae9d to ad81cad Compare September 28, 2021 14:56
@Marenz Marenz mentioned this pull request Sep 28, 2021
Closed
@Marenz Marenz force-pushed the external-fp-10358 branch 4 times, most recently from 5ae4e0c to 81d5208 Compare September 30, 2021 16:18
chriseth
chriseth reviewed Oct 4, 2021
View reviewed changes
docs/assembly.rst Outdated Show resolved Hide resolved
@Marenz Marenz force-pushed the external-fp-10358 branch from 81d5208 to 6d83ca9 Compare October 4, 2021 14:19
@Marenz Marenz force-pushed the external-fp-10358 branch from 6d83ca9 to a8d0312 Compare October 4, 2021 14:59
@Marenz Marenz force-pushed the external-fp-10358 branch from a8d0312 to 98dd783 Compare October 4, 2021 16:06
@chriseth chriseth merged commit ecfcca1 into develop Oct 5, 2021
@chriseth chriseth deleted the external-fp-10358 branch October 5, 2021 12:49
@chriseth chriseth mentioned this pull request Oct 5, 2021
Closed
@Xanewok Xanewok mentioned this pull request Apr 24, 2024
Merged
github-merge-queue bot pushed a commit to NomicFoundation/slang that referenced this pull request Apr 25, 2024
@Xanewok
Support .address access in Yul paths (#945)
e8f80d8 
Found with the Sanctuary run on Arbitrum:
https://github.com/NomicFoundation/slang/actions/runs/8803528884
This fixes the last bug in Arbitrum data set:
https://github.com/Xanewok/slang/actions/runs/8813993801

Implemented in ethereum/solidity#12016 (0.8.10),
grammar updated in ethereum/solidity#12545.

This matches the upstream grammar, however:
- this is more relaxed and parses code that would be rejected by
validation, i.e. `<built-in> := ...`
- we do get a bit of noise, since most of the paths will never contain
the built-in.

I've also changed `YulidentifierPath` to `YulPath` to match upstream and
since it's not only an identifier anymore.

This also includes a patch level bump - while technically it's breaking
the CST shape, I'd consider this a (hopefully last?) grammar bug fix
that might justify the patch level.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cameel cameel cameel left review comments

@chriseth chriseth chriseth approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Marenz @chriseth @cameel