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

"origin" locations in Yul AST #12005

Closed
wants to merge 4 commits into from
Closed

Conversation

cameel
Copy link
Member

@cameel cameel commented Sep 22, 2021

Originally requested in #11998 (comment).
Depends on #11998.
Related to #11043.

  • Problem: the location does not always follow the format described by our docs on Source Mappings.

@cameel cameel requested a review from chriseth September 22, 2021 11:55
@cameel cameel self-assigned this Sep 22, 2021
Comment on lines +215 to +222
ret["src"] = formatLocationWithFileIndex(_debugData->nativeLocation, m_sourceIndex);
if (_debugData->originLocation.isValid())
{
if (_debugData->nativeLocation.sourceName == _debugData->originLocation.sourceName)
ret["origin"] = formatLocationWithFileIndex(_debugData->originLocation, m_sourceIndex);
else
ret["origin"] = formatLocationWithFileName(_debugData->originLocation);
}
Copy link
Member Author

Choose a reason for hiding this comment

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

The problem with this PR is that we don't have indexes for the original Solidity files when compiling Yul.

This does work because currently we do not support outputting Yul AST (#9590) but will break when we implement it so I don't think this is a good solution.

@cameel cameel force-pushed the origin-location-in-yul-ast-json branch 4 times, most recently from 28b4092 to 2e3b316 Compare September 22, 2021 13:20
Base automatically changed from separate-yul-and-sol-locations-in-debug-info to develop September 22, 2021 13:27
@chriseth
Copy link
Contributor

I think we need to provide the source-name-to-index-mapping to the asm json converter in the same way we do it for the ASTJsonConverter - does this not work?

@cameel
Copy link
Member Author

cameel commented Sep 22, 2021

But that will work only if we have access to Solidity sources. When we're importing inline assembly AST we do, but this is the case where "origin" is superfluous anyway because it matches "src". "origin" is only useful if we allow importing AST of a pure-Yul file and in that case we don't.

@cameel cameel force-pushed the origin-location-in-yul-ast-json branch from 2e3b316 to fff703e Compare September 22, 2021 15:47
@leonardoalt
Copy link
Member

But that will work only if we have access to Solidity sources. When we're importing inline assembly AST we do, but this is the case where "origin" is superfluous anyway because it matches "src". "origin" is only useful if we allow importing AST of a pure-Yul file and in that case we don't.

@cameel was this solved now?

@cameel
Copy link
Member Author

cameel commented Apr 4, 2022

Not really. I think this PR does not really make sense without implementing Yul AST output first (#9590). These origin locations will never show up for inline assembly.

Originally I wanted to implement #9590 and then put this PR on top of that. But I got into other PRs and didn't really have time for this one. I could close it but @chriseth seems to think this should actually work. I didn't want to close it without clarifying that first.

@cameel
Copy link
Member Author

cameel commented Jul 6, 2022

I'm going to close this. I definitely won't be working on this in the near future and like I said above, I'm not even sure if this PR even makes sense - it's only needed for Yul ASTs and we don't support printing those at this point.

@cameel cameel closed this Jul 6, 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.

3 participants