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

Investigate source mappings for reverts after optimization. Resp. how to deal with the optimizer collapsing source locations. #10715

Closed
ekpyron opened this issue Jan 5, 2021 · 3 comments
Labels
closed due inactivity The issue/PR was automatically closed due to inactivity. optimizer protocol design 🔮 Potential changes to ABI, meta data, standard JSON stale The issue/PR was marked as stale because it has been open for too long.

Comments

@ekpyron
Copy link
Member

ekpyron commented Jan 5, 2021

Reported by @haltman-at on gitter:

hey, according to the Brownie people there's an issue in Solidity 0.8.0 where it will generate incorrect source mappings for reverts: eth-brownie/brownie#907 (they don't seem to have filed a Solidity issue for it) However I can't seem to reproduce this issue myself. Does anyone know if this is real?
if it is, I want to suggest that the relevant overlapping code should either lie in a generated source, or else simply be unmapped, so that this problem doesn't happen. but, I can't reproduce it, so I don't even know if it's real

IIUC the issue is that the optimizer collapses source locations - not sure if we can do much about this, unless we allow a piece of bytecode to point to multiple source locations in the source mappings.

@ekpyron ekpyron changed the title Investigate source mappings for reverts after optimization. Investigate source mappings for reverts after optimization. Resp. how to deal with the optimizer collapsing source locations. Jan 5, 2021
@haltman-at
Copy link
Contributor

Yes, so, to be clear, the problem only happens with the optimizer turned on (I didn't understand this at first which was why I wasn't sure it was real), which makes it arguably not a bug, but it is annoying.

For obvious reasons, I don't think making one piece of bytecode point to two source locations makes any sense. However -- to repeat what I said on Gitter -- if it would be possible to instead just mark that code as unmapped, or for it to live in a generated source, then it seems to me that would also get rid of the problem of misleading stacktraces?

@ekpyron
Copy link
Member Author

ekpyron commented Jan 6, 2021

Well, pointing to several source locations would be the most accurate, especially since this may happen a lot more often and for any kind of user-code as well once we switch to Sol->Yul codegen... but it may of course not be very practical... and also probably not very useful unless a jump would also mark into which set of those multiple source locations it would jump (if that is even always possible)...
For now it should probably just be marked as generated source, but this will haunt us back in the future with Yul-IR-codegen in general, so maybe worth thinking about how we want to deal with this in general in the future...

@cameel cameel added optimizer protocol design 🔮 Potential changes to ABI, meta data, standard JSON labels Jan 7, 2021
@NunoFilipeSantos NunoFilipeSantos added the stale The issue/PR was marked as stale because it has been open for too long. label Nov 25, 2022
@github-actions
Copy link

github-actions bot commented Feb 5, 2023

Hi everyone! This issue has been closed due to inactivity.
If you think this issue is still relevant in the latest Solidity version and you have something to contribute, feel free to reopen.
However, unless the issue is a concrete proposal that can be implemented, we recommend starting a language discussion on the forum instead.

@github-actions github-actions bot added the closed due inactivity The issue/PR was automatically closed due to inactivity. label Feb 5, 2023
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Feb 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed due inactivity The issue/PR was automatically closed due to inactivity. optimizer protocol design 🔮 Potential changes to ABI, meta data, standard JSON stale The issue/PR was marked as stale because it has been open for too long.
Projects
None yet
Development

No branches or pull requests

4 participants