Code Deduplication Creates Wrong Source Mappings

[veniger]

Description

When the optimizer does code deduplication, source maps for the resulting instructions map to one of the original source locations, instead of being cleared/"-1"ed

Environment

• Compiler version: Any, tested on 0.8.23
• Operating system: Tested on Linux fedora 6.5.12-200.fc38.x86_64

Steps to Reproduce

Compile the following code(with default optimizations) and observe when debugging the xincrement function, the SSTORE (and anything after the checked add) is mapped to the yincrement function, instead of -1 (unmapped)

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

contract DeduplicationBrokenSourceMaps
{
	uint256 public x;
	uint256 public y;
	
	function xincrement(uint256 val) public
	{   
	    x += val;
	}
	
	function yincrement(uint256 val) public
	{   
	    y += val;
	}
}

[cameel]

We discussed it on the design call today. There were several options we considered:

1. Simply clearing the ambiguous locations as suggested in the description
   • We're not convinced that it does not drop information that could still be very useful in some scenarios.
2. A compiler flag to switch these locations on and off
   • Too high complexity. There are just too many places in the codebase, often deeply nested, where this flag would have to be checked.
3. Adding extra information to the source map to mark entries where the compiler chose one location arbitrarily
   • Too high chance of it being a breaking change.
     • Previously such changes were done in breaking versions (e.g. modifier depth in 0.6.0)
     • The docs do not clearly state that the tuple might be extended and there is a real chance that some tools might break not expecting that.
   • If this could be done in a non-breaking way, it would actually be the preferred solution. It would be helpful for making decisions about ethdebug, by showing how prevalent such locations are.
   • The next breaking release will include ethdebug support so putting this feature off until such a release does not make sense.

Overall, considering the complication and doubts about it being breaking, we're inclined to rather wait for ethdebug to solve this.

[github-actions[bot]]

This issue has been marked as stale due to inactivity for the last 90 days. It will be automatically closed in 7 days.

[github-actions[bot]]

Hi everyone! This issue has been automatically 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.