Store both Yul and Solidity locations in debug data #11998

cameel · 2021-09-20T16:31:23Z

Fixes #11932.

ICE is gone but still not all errors from ICE when the file name in @use-src annotation does not match the name of the file passed to --strict-assembly #11932 (comment) have locations. This might still be using the original location in places where IR location should be used.
Make sure nothing is broken and maybe add some tests if the current ones did not detect all the breakage.
Some of the updated lines should be using the locationOf() helper. I only started adding it halfway through.

libyul/AST.h

chriseth · 2021-09-20T20:21:37Z

I have the impression that AsmParser needs some more changes, but I might be wrong.

We have three modes for AsmParser:

native yul mode - irLocation and originalLocation should always point to the locations generated from the scanner ignoring the annotations
location override: This is used for snippets of yul code generated from solidity code in the old codegen. It probably makes sense for both to point at the solidity source location. We could also make the ir location point at the scanner location, but we probably don't have the actual source anymore at the point where we generate a potential parser error.
from comments: ir location points at scanner, original location is taken from comments

It might very well be that your code already accomplishes the above. If this is the case, it might be worth refactoring it at a later point in time to make the distinction clearer - especially if it is possible in 2) for the ir location to point at the scanner locations - in this case we can make it always point at the scanner locations.

cameel · 2021-09-20T20:40:02Z

I'll take a deeper look at that tomorrow. For now in a few places I wasn't really sure which location to take but just went with it to get it to a state where it compiles, with the intention to do one more pass through the code after that. Judging by test failures, I must have chosen wrong in some places :)

cameel · 2021-09-21T14:32:44Z

libyul/AsmParser.h

 	langutil::SourceLocation currentLocation() const override
 	{
-		if (m_useSourceLocationFrom == UseSourceLocationFrom::Scanner)
-			return ParserBase::currentLocation();
-		return m_locationOverride;
+		if (m_useSourceLocationFrom == UseSourceLocationFrom::LocationOverride)
+			return m_locationOverride;
+
+		return ParserBase::currentLocation();
 	}


ICE is gone but still not all errors from #11932 (comment) have locations. This might still be using the original location in places where IR location should be used.

There was actually an extra bug here. Location override was overriding current location even when location from comments was selected. This resulted in empty location in errors reported from AsmParser.cpp. See test/libyul/yulSyntaxTests/invalid/literals_on_stack_disallowed_with_debug_info.yul.

cameel · 2021-09-21T14:44:27Z

libyul/AsmParser.cpp

 		case UseSourceLocationFrom::LocationOverride:
-			return DebugData::create(m_locationOverride);
+			return DebugData::create(m_locationOverride, m_locationOverride);


location override: This is used for snippets of yul code generated from solidity code in the old codegen. It probably makes sense for both to point at the solidity source location.

I think that m_locationOverride for both is the most consistent. If I use current location for nativeLocation instead, Parser will be reporting errors at m_locationOverride but AsmAnalyzer (which, I'm assuming gets locations from nativeLocation in the AST) will be be reporting them at the current location.

We could also make the ir location point at the scanner location, but we probably don't have the actual source anymore at the point where we generate a potential parser error.

Errors are processed and reported from within CompilerContext::appendInlineAssembly() which still has the original snippet. To check that I introduced an error to one of the generated sources and there were no failing assertions and messages seemed to be pointing at the correct location in the snippet - even when nativeLocation was set to the current location rather than the override.

But there's still the problem with inconsistent location in errors I mentioned above. We can change that but we should then also change error messages to consistently use one kind of locations.

It probably doesn't matter too much, because these will all lead to internal compiler errors anyway.

cameel · 2021-09-21T14:46:38Z

libsolidity/analysis/ControlFlowBuilder.cpp

 {
 	solAssert(m_currentNode && m_inlineAssembly, "");
-	m_currentNode->location = langutil::SourceLocation::smallestCovering(m_currentNode->location, locationOf(_statement));
+	m_currentNode->location = langutil::SourceLocation::smallestCovering(m_currentNode->location, nativeLocationOf(_statement));


Changing this to originLocationOf() does not break any tests so I think we don't have coverage for this in case where Yul source has debug info annotations.

Just to check: nativeLocationOf(_statement) always returns a "proper" source location, no matter in which mode the parser was used, right?

What I'm a bit worried about is the possibility of the location being the same when we use appendAssembly in the old code generator and supply two different snippets (that would not have a different name) but two yul ast items have the same native locations inside these snippets.

IIRC, location is not only used for informational purposes, but to check equality in the source - is this the case here?

Ah wait, this is only about inline assembly here (and not compiler-generated assembly for helper routines), so native location and origin location should be the same anyway. Maybe assert that?

Just to check: nativeLocationOf(_statement) always returns a "proper" source location, no matter in which mode the parser was used, right?

Yes.

IIRC, location is not only used for informational purposes, but to check equality in the source - is this the case here?

Yeah, it looks like it's using it to disambiguate the stuff it adds to variableOccurrences.

What I'm a bit worried about is the possibility of the location being the same when we use appendAssembly in the old code generator and supply two different snippets (that would not have a different name) but two yul ast items have the same native locations inside these snippets.

I don't really understand this. We're not storing snippets in debug info or passing them around. Or do you mean the inline assembly snippets we compile with the location override? I'm also not sure what the consequences of these locations being the same would be.

Ah wait, this is only about inline assembly here (and not compiler-generated assembly for helper routines), so native location and origin location should be the same anyway. Maybe assert that?

Ah, you're right. It's in libsolidity so it must be. Assertions added.

libsolidity/analysis/ReferencesResolver.cpp

cameel · 2021-09-21T14:49:52Z

libsolidity/ast/ASTJsonConverter.cpp

-Json::Value ASTJsonConverter::inlineAssemblyIdentifierToJson(pair<yul::Identifier const* ,InlineAssemblyAnnotation::ExternalIdentifierInfo> _info) const
+Json::Value ASTJsonConverter::inlineAssemblyIdentifierToJson(pair<yul::Identifier const*, InlineAssemblyAnnotation::ExternalIdentifierInfo> _info) const
 {
 	Json::Value tuple(Json::objectValue);
-	tuple["src"] = sourceLocationToString(_info.first->debugData->location);
+	tuple["src"] = sourceLocationToString(nativeLocationOf(*_info.first));


Changing this to originLocationOf() does not break any tests so but I guess this is because we cannot have debug info annotations in inline assembly?

chriseth · 2021-09-21T15:33:15Z

libyul/AsmJsonConverter.cpp

 {

 Json::Value AsmJsonConverter::operator()(Block const& _node) const
 {


Do we want to store both locations in the asm ast?

Sounds like something that could be useful to tools. I think it should be properly introduced in a standalone PR though. We need to agree on the name (maybe "origin"?) and decide if we want other debug info here, like AST ID. Also needs some tests.

I think this should be in the same PR - we are changing the value of "location" in the case of code that has @use-src.
origin sounds good.

we are changing the value of "location" in the case of code that has @use-src.

I wanted to say that we are not because there are no changes in test output, but you're right, there should have been some changes. So either something's wrong or, more likely, we do not have coverage for this.

Anyway, I'm out of time now so I'll be doing that change tomorrow. I'll do it in the same PR if I manage to before the release but I still think a separate PR would be more convenient - this one is pretty much a bugfix for the fact that these locations changed in 0.8.6 and is basically finished.

Ok, I'm fine with either, but it seems that there is a problem in the ast export and re-import anyway...

Right. I think that the problem is that the origin location is not preserved in AST nodes in inline assembly when you export and reimport the AST (https://github.com/ethereum/solidity/pull/11998/files#r713769635). So it's exactly what you pointed out but it's not enough to add it to JSON, we also need to import that information back.

I'm going to remove the assertions from this PR and add them in the one that extends the AST instead.

Of course, anything you add to the json export also has to be added to the json import :)

I knew there was a catch somewhere :P

Anyway, I'm going to actually push one more change. I'll make the importer use the "src" location for both native and origin location - I'm not not sure if adding "origin" actually makes sense since it has to include file index and I only have one when compiling inline assembly - and there it's the same as "src" anyway.

So either something's wrong or, more likely, we do not have coverage for this.

And this turns out to be because without #9590 there's no way to output the Yul AST (other than for inline assembly or generated sources and we can't have debug info annotations there). So naturally it does not get tested.

libyul/AsmParser.cpp

libyul/AsmParser.h

chriseth · 2021-09-22T09:00:50Z

commandline tests are failing.

cameel · 2021-09-22T09:03:43Z

Yeah, I'm working on that.

cameel · 2021-09-22T09:38:17Z

libsolidity/analysis/ReferencesResolver.cpp

 		m_errorReporter.declarationError(
 			4718_error,
-			_identifier.debugData->location,
+			nativeLocationOf(_identifier),
 			"Multiple matching identifiers. Resolving overloaded identifiers is not supported."
 		);


The assertion I added for this is actually failing during AST export and reimport of syntaxTests/inlineAssembly/storage_reference_old_shadow.sol:

contract C { uint[] x; fallback() external { uint y_slot = 2; uint y_offset = 3; uint[] storage y = x; assembly { pop(y_slot) pop(y_offset) } y[0] = 2; } } // ----

This is why CLI tests are failing. BTW the error handling for AST import is swallowing errors at multiple levels (both in solc and in the test script), which makes it harder to debug than it should be.

…cation and AST ID

…on from comments is selected

cameel requested a review from chriseth September 20, 2021 16:31

cameel self-assigned this Sep 20, 2021

chriseth reviewed Sep 20, 2021

View reviewed changes

libyul/AST.h Outdated Show resolved Hide resolved

chriseth reviewed Sep 20, 2021

View reviewed changes

libyul/AST.h Outdated Show resolved Hide resolved

cameel force-pushed the separate-yul-and-sol-locations-in-debug-info branch from 6fcae57 to 54ffd68 Compare September 21, 2021 14:29

cameel commented Sep 21, 2021

View reviewed changes

libsolidity/analysis/ReferencesResolver.cpp Show resolved Hide resolved

cameel commented Sep 21, 2021

View reviewed changes

cameel marked this pull request as ready for review September 21, 2021 14:54

cameel mentioned this pull request Sep 21, 2021

Command-line test for --asm-json #12001

Merged

chriseth reviewed Sep 21, 2021

View reviewed changes

libyul/AsmParser.cpp Outdated Show resolved Hide resolved

chriseth reviewed Sep 21, 2021

View reviewed changes

libyul/AsmParser.cpp Outdated Show resolved Hide resolved

chriseth reviewed Sep 21, 2021

View reviewed changes

libyul/AsmParser.h Outdated Show resolved Hide resolved

cameel force-pushed the separate-yul-and-sol-locations-in-debug-info branch from 54ffd68 to 9e6280e Compare September 21, 2021 16:03

cameel commented Sep 22, 2021

View reviewed changes

cameel force-pushed the separate-yul-and-sol-locations-in-debug-info branch from 9e6280e to 03d5b71 Compare September 22, 2021 10:21

chriseth previously approved these changes Sep 22, 2021

View reviewed changes

cameel added 5 commits September 22, 2021 13:11

Yul syntax tests showing broken behavior

84ca67c

AsmParser: Replace m_debugDataOverride with separate variables for lo…

9c1d40d

…cation and AST ID

Separate DebugData fields for Solidity and Yul source locations

ce4420f

AsmParser: Don't use locationOverride as current location when locati…

d23754e

…on from comments is selected

Fill out originLocation with nativeLocation when importing Yul AST

fc7e8c5

cameel dismissed chriseth’s stale review via fc7e8c5 September 22, 2021 11:18

cameel force-pushed the separate-yul-and-sol-locations-in-debug-info branch from 03d5b71 to fc7e8c5 Compare September 22, 2021 11:18

cameel mentioned this pull request Sep 22, 2021

"origin" locations in Yul AST #12005

Closed

1 task

chriseth approved these changes Sep 22, 2021

View reviewed changes

chriseth merged commit bcdaa0c into develop Sep 22, 2021

chriseth deleted the separate-yul-and-sol-locations-in-debug-info branch September 22, 2021 13:27

cameel mentioned this pull request Nov 10, 2021

Fix crash when using @use-src while compiling yul -> ewasm #12193

Merged

Store both Yul and Solidity locations in debug data #11998

Store both Yul and Solidity locations in debug data #11998

Uh oh!

Conversation

cameel commented Sep 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chriseth commented Sep 20, 2021

Uh oh!

cameel commented Sep 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cameel Sep 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chriseth commented Sep 22, 2021

Uh oh!

cameel commented Sep 22, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cameel commented Sep 20, 2021 •

edited

Loading

cameel commented Sep 20, 2021 •

edited

Loading

cameel Sep 21, 2021 •

edited

Loading