Skip to content

Do not use named function labels if function names are not unique. #12139

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

Merged
merged 1 commit into from
Oct 25, 2021

Conversation

chriseth
Copy link
Contributor

@chriseth chriseth commented Oct 14, 2021

Fixes #12090
Fixes #12149
Fixes #12158

@chriseth chriseth requested review from cameel and Marenz October 14, 2021 14:20
@chriseth chriseth force-pushed the nonamedlabelsifnotunique branch from 8ae7765 to f0c9a0d Compare October 14, 2021 14:34
Marenz
Marenz previously requested changes Oct 14, 2021
Copy link
Contributor

@Marenz Marenz left a comment

Choose a reason for hiding this comment

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

LGTM, just a stray space

@@ -0,0 +1 @@
--strict-assembly
Copy link
Member

Choose a reason for hiding this comment

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

I don't think the annotations add much in this particular test. Might actually be more readable without them. Maybe it would be better to disable them?

Suggested change
--strict-assembly
--strict-assembly --debug-info none

@cameel
Copy link
Member

cameel commented Oct 17, 2021

Does this PR fix also a similar case I found in inline assembly #12090 (comment)?

EDIT: Reported as a separate issue (#12158).

@@ -0,0 +1 @@
--experimental-via-ir --combined-json function-debug-runtime
Copy link
Member

Choose a reason for hiding this comment

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

Output would be more readable if it was not all on a single line:

Suggested change
--experimental-via-ir --combined-json function-debug-runtime
--experimental-via-ir --combined-json function-debug-runtime --pretty-json --json-indent 4

Comment on lines 1 to 8
contract C {
function f() public pure returns (uint r) {
assembly { function f() -> x { x := 1 } r := f() }
}
function g() public pure returns (uint r) {
assembly { function f() -> x { x := 2 } r := f() }
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
contract C {
function f() public pure returns (uint r) {
assembly { function f() -> x { x := 1 } r := f() }
}
function g() public pure returns (uint r) {
assembly { function f() -> x { x := 2 } r := f() }
}
}
contract C {
function f() public pure returns (uint r) {
assembly { function f() -> x { x := 1 } r := f() }
}
function g() public pure returns (uint r) {
assembly { function f() -> x { x := 2 } r := f() }
}
}

Also in function_name_clash.sol below.

Comment on lines +1 to +8
contract C {
function f() public pure returns (uint r) {
assembly { function f() -> x { x := 1 } r := f() }
}
function g() public pure returns (uint r) {
assembly { function f() -> x { x := 2 } r := f() }
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Does this PR also fix #12158 or only #12149? If it solves #12158, I'd add the test case from there because it triggers a different assertion.

@chriseth
Copy link
Contributor Author

Incorporated #12149 into the test.

cameel
cameel previously approved these changes Oct 19, 2021
Copy link
Member

@cameel cameel left a comment

Choose a reason for hiding this comment

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

There are still some open comments that could be applied to improve the command-line test output but overall the fix seems correct and does the job even in the current form so I'm approving.

@cameel cameel dismissed Marenz’s stale review October 19, 2021 15:17

I think the comments were addressed.

@chriseth
Copy link
Contributor Author

Rebased, simplified commandline tests and updated the "fixes".

@chriseth chriseth merged commit e6e30f8 into develop Oct 25, 2021
@chriseth chriseth deleted the nonamedlabelsifnotunique branch October 25, 2021 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants