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

[DO NOT MERGE] Remove source locations on Yul optimizer function deduplication for testing. #15894

Closed
wants to merge 1 commit into from

Conversation

ekpyron
Copy link
Member

@ekpyron ekpyron commented Feb 26, 2025

@veniger As we talked about, this is a quick and dirty branch that removes source locations whenever functions are deduplicated by the Yul optimizer.
I'm a bit sceptical, whether this is really what we want (and it may still actually be the libevmasm optimizer that's doing the deduplication in the relevant cases), but I'd be curious to hear, if this actually changes the cases you're struggling with - if not, it may be interesting to post the sources for such a case, s.t. we can look into it in more detail.

Although, weirdly, I just see in some tests that - while it actually just removes source locations - in fact seems to sometimes add some - I guess since we'll fall back to previous ones, I'll still need to look into that to avoid that.

@ekpyron ekpyron force-pushed the sourceMappingsOnDeduplication branch from df471eb to 1034ccb Compare February 26, 2025 19:08
@veniger
Copy link
Contributor

veniger commented Mar 4, 2025

I think i created a minimal example that shows the problems here. If you revert with Err1 some instructions get mapped to line 26 and some get mapped to line 49. I can demonstrate this on the next call if needed.

In this case I'm not even sure that the removal of deduplicated source locations would be enough, we would need to store multiple source locations, since no instruction here can be used to distinguish between the reverts (except falling back to the AST and figuring it out that way, which wouldn't be viable for larger contracts)

settings: compiler version 0.8.26, via-ir enabled, --optimize-steps 30

// SPDX-License-Identifier: GPL-3.0
pragma solidity >=0.8.16 <0.9.0;

contract Example0
{
    error Err0(uint256 a, address addr);
    error Err1(uint256 a, uint256 b);
    error Err2(uint256 a, uint256 b);

    uint256 public x;

    function set(uint256 nx) public
    {
        x = nx;
    }

    function test0(uint256 a) public
    {
        if(a >= 0 && a < 10)
        {
            revert Err0(a + x, msg.sender);
        }
        
        if(a >= 10 && a < 20)
        {
            revert Err1(x, a);
        }
        
        if(a >= 20)
        {
            revert Err2(x, a);
        }
        
        x = a;

    }

    function test1(uint256 a, uint256 b) public
    {
        uint256 c = a + b;

        if(c >= 0 && c < 10)
        {
            revert Err0(a + x, msg.sender);
        }
        
        if(c >= 10 && c < 20)
        {
            revert Err1(x, a);
        }
        
        if(c >= 20)
        {
            revert Err2(x, a);
        }
        
        x = c;

    }


}

Copy link

This pull request is stale because it has been open for 14 days with no activity.
It will be closed in 7 days unless the stale label is removed.

@github-actions github-actions bot added the stale The issue/PR was marked as stale because it has been open for too long. label Mar 19, 2025
Copy link

This pull request was closed due to a lack of activity for 7 days after it was stale.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-due-inactivity stale The issue/PR was marked as stale because it has been open for too long.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants