Merge pull request #15209 from clonker/asm_parser_use_scanner

clonker · web-flow · commit 8d788fb6cefb · 2024-07-01T19:29:26.000+02:00
AsmParser: parse source comment using scanner instead of regex
diff --git a/.circleci/parallel_bytecode_report.sh b/.circleci/parallel_bytecode_report.sh
@@ -44,9 +44,6 @@ cd test-cases/
 echo "Preparing input files"
 python3 ../scripts/isolate_tests.py ../test/
 
-# FIXME: These cases crash because of https://github.com/ethereum/solidity/issues/13583
-rm ./*_bytecode_too_large_*.sol ./*_combined_too_large_*.sol
-
 if [[ $binary_type == native || $binary_type == "osx_intel" ]]; then
     interface=$(echo -e "standard-json\ncli" | circleci tests split)
     echo "Selected interface: ${interface}"
diff --git a/Changelog.md b/Changelog.md
@@ -3,6 +3,7 @@
 Language Features:
  * Accept declarations of state variables with ``transient`` data location (parser support only, no code generation yet).
  * Make ``require(bool, Error)`` available when using the legacy pipeline.
+ * Yul: Parsing rules for source location comments have been relaxed: Whitespace between the location components as well as single-quoted code snippets are now allowed.
 
 
 Compiler Features:
@@ -19,6 +20,7 @@ Bugfixes:
  * SMTChecker: Fix internal compiler error when reporting proved targets for BMC engine.
  * TypeChecker: Fix segfault when assigning nested tuple to tuple.
  * Yul Optimizer: Name simplification could lead to forbidden identifiers with a leading and/or trailing dot, e.g., ``x._`` would get simplified into ``x.``.
+ * Yul Parser: Fix segfault when parsing very long location comments.
 
 
 ### 0.8.26 (2024-05-21)
diff --git a/liblangutil/Scanner.cpp b/liblangutil/Scanner.cpp
@@ -826,31 +826,48 @@ Token Scanner::scanString(bool const _isUnicode)
 	char const quote = m_char;
 	advance();  // consume quote
 	LiteralScope literal(this, LITERAL_TYPE_STRING);
-	while (m_char != quote && !isSourcePastEndOfInput() && !isUnicodeLinebreak())
+	// for source location comments we allow multiline string literals
+	while (m_char != quote && !isSourcePastEndOfInput() && (!isUnicodeLinebreak() || m_kind == ScannerKind::SpecialComment))
 	{
 		char c = m_char;
 		advance();
-		if (c == '\\')
+
+		if (m_kind == ScannerKind::SpecialComment)
 		{
-			if (isSourcePastEndOfInput() || !scanEscape())
-				return setError(ScannerError::IllegalEscapeSequence);
+			if (c == '\\')
+			{
+				if (isSourcePastEndOfInput())
+					return setError(ScannerError::IllegalEscapeSequence);
+				advance();
+			}
+			else
+				addLiteralChar(c);
 		}
 		else
 		{
-			// Report error on non-printable characters in string literals, however
-			// allow anything for unicode string literals, because their validity will
-			// be verified later (in the syntax checker).
-			//
-			// We are using a manual range and not isprint() to avoid
-			// any potential complications with locale.
-			if (!_isUnicode && (static_cast<unsigned>(c) <= 0x1f || static_cast<unsigned>(c) >= 0x7f))
+			if (c == '\\')
 			{
-				if (m_kind == ScannerKind::Yul)
-					return setError(ScannerError::IllegalCharacterInString);
-				return setError(ScannerError::UnicodeCharacterInNonUnicodeString);
+				if (isSourcePastEndOfInput() || !scanEscape())
+					return setError(ScannerError::IllegalEscapeSequence);
+			}
+			else
+			{
+				// Report error on non-printable characters in string literals, however
+				// allow anything for unicode string literals, because their validity will
+				// be verified later (in the syntax checker).
+				//
+				// We are using a manual range and not isprint() to avoid
+				// any potential complications with locale.
+				if (!_isUnicode && (static_cast<unsigned>(c) <= 0x1f || static_cast<unsigned>(c) >= 0x7f))
+				{
+					if (m_kind == ScannerKind::Yul)
+						return setError(ScannerError::IllegalCharacterInString);
+					return setError(ScannerError::UnicodeCharacterInNonUnicodeString);
+				}
+				addLiteralChar(c);
 			}
-			addLiteralChar(c);
 		}
+
 	}
 	if (m_char != quote)
 		return setError(ScannerError::IllegalStringEndQuote);
@@ -1023,6 +1040,9 @@ std::tuple<Token, unsigned, unsigned> Scanner::scanIdentifierOrKeyword()
 	auto const token = TokenTraits::fromIdentifierOrKeyword(m_tokens[NextNext].literal);
 	switch (m_kind)
 	{
+	case ScannerKind::SpecialComment:
+		// there are no keywords in special comments
+		return std::make_tuple(Token::Identifier, 0, 0);
 	case ScannerKind::Solidity:
 		// Turn experimental Solidity keywords that are not keywords in legacy Solidity into identifiers.
 		if (TokenTraits::isExperimentalSolidityOnlyKeyword(std::get<0>(token)))
diff --git a/liblangutil/Scanner.h b/liblangutil/Scanner.h
@@ -62,15 +62,12 @@
 namespace solidity::langutil
 {
 
-class AstRawString;
-class AstValueFactory;
-class ParserRecorder;
-
 enum class ScannerKind
 {
 	Solidity,
 	Yul,
-	ExperimentalSolidity
+	ExperimentalSolidity,
+	SpecialComment
 };
 
 enum class ScannerError
@@ -102,11 +99,13 @@ class Scanner
 {
 	friend class LiteralScope;
 public:
-	explicit Scanner(CharStream& _source):
+	explicit Scanner(CharStream& _source, ScannerKind _kind = ScannerKind::Solidity):
 		m_source(_source),
 		m_sourceName{std::make_shared<std::string>(_source.name())}
 	{
 		reset();
+		if (_kind != ScannerKind::Solidity)
+			setScannerMode(_kind);
 	}
 
 	/// Resets scanner to the start of input.
diff --git a/libyul/AsmParser.cpp b/libyul/AsmParser.cpp
@@ -28,11 +28,10 @@
 #include <liblangutil/ErrorReporter.h>
 #include <liblangutil/Exceptions.h>
 #include <liblangutil/Scanner.h>
+#include <liblangutil/Common.h>
 #include <libsolutil/Common.h>
 #include <libsolutil/Visitor.h>
 
-#include <range/v3/view/subrange.hpp>
-
 #include <boost/algorithm/string.hpp>
 
 #include <algorithm>
@@ -53,7 +52,7 @@ std::optional<int> toInt(std::string const& _value)
 	{
 		return stoi(_value);
 	}
-	catch (...)
+	catch (std::out_of_range const&)
 	{
 		return std::nullopt;
 	}
@@ -193,13 +192,41 @@ std::optional<std::pair<std::string_view, SourceLocation>> Parser::parseSrcComme
 	langutil::SourceLocation const& _commentLocation
 )
 {
-	static std::regex const argsRegex = std::regex(
-		R"~~(^(-1|\d+):(-1|\d+):(-1|\d+)(?:\s+|$))~~"  // index and location, e.g.: 1:234:-1
-		R"~~(("(?:[^"\\]|\\.)*"?)?)~~",                // optional code snippet, e.g.: "string memory s = \"abc\";..."
-		std::regex_constants::ECMAScript | std::regex_constants::optimize
-	);
-	std::match_results<std::string_view::const_iterator> match;
-	if (!regex_search(_arguments.cbegin(), _arguments.cend(), match, argsRegex))
+	CharStream argumentStream(std::string(_arguments), "");
+	Scanner scanner(argumentStream, ScannerKind::SpecialComment);
+
+	std::string_view tail{_arguments.substr(_arguments.size())};
+	auto const parseLocationComponent = [](Scanner& _scanner, bool expectTrailingColon) -> std::optional<std::string>
+	{
+		bool negative = false;
+		if (_scanner.currentToken() == Token::Sub)
+		{
+			negative = true;
+			_scanner.next();
+		}
+		if (_scanner.currentToken() != Token::Number)
+			return std::nullopt;
+		if (expectTrailingColon && _scanner.peekNextToken() != Token::Colon)
+			return std::nullopt;
+		if (!isValidDecimal(_scanner.currentLiteral()))
+			return std::nullopt;
+		std::string decimal = (negative ? "-" : "") + _scanner.currentLiteral();
+		_scanner.next();
+		if (expectTrailingColon)
+			_scanner.next();
+		return decimal;
+	};
+	std::optional<std::string> rawSourceIndex = parseLocationComponent(scanner, true);
+	std::optional<std::string> rawStart = parseLocationComponent(scanner, true);
+	std::optional<std::string> rawEnd = parseLocationComponent(scanner, false);
+
+	size_t const snippetStart = static_cast<size_t>(scanner.currentLocation().start);
+	bool const locationScannedSuccessfully = rawSourceIndex && rawStart && rawEnd;
+	bool const locationIsWhitespaceSeparated =
+		scanner.peekNextToken() == Token::EOS ||
+		(snippetStart > 0 && langutil::isWhiteSpace(_arguments[snippetStart - 1]));
+
+	if (!locationScannedSuccessfully || !locationIsWhitespaceSeparated)
 	{
 		m_errorReporter.syntaxError(
 			8387_error,
@@ -209,13 +236,16 @@ std::optional<std::pair<std::string_view, SourceLocation>> Parser::parseSrcComme
 		return std::nullopt;
 	}
 
-	solAssert(match.size() == 5, "");
-	std::string_view tail = _arguments.substr(static_cast<size_t>(match.position() + match.length()));
+	// captures error cases `"test` (illegal end quote) and `"test\` (illegal escape sequence / dangling backslash)
+	bool const illegalLiteral = scanner.currentToken() == Token::Illegal && (scanner.currentError() == ScannerError::IllegalStringEndQuote || scanner.currentError() == ScannerError::IllegalEscapeSequence);
+	if (scanner.currentToken() == Token::StringLiteral || illegalLiteral)
+		tail = _arguments.substr(static_cast<size_t>(scanner.currentLocation().end));
+	else
+		tail = _arguments.substr(static_cast<size_t>(scanner.currentLocation().start));
 
-	if (match[4].matched && (
-		!boost::algorithm::ends_with(match[4].str(), "\"") ||
-		boost::algorithm::ends_with(match[4].str(), "\\\"")
-	))
+	// Other scanner errors may occur if there is no string literal which follows
+	// (f.ex. IllegalHexDigit, IllegalCommentTerminator), but these are ignored
+	if (illegalLiteral)
 	{
 		m_errorReporter.syntaxError(
 			1544_error,
@@ -225,30 +255,34 @@ std::optional<std::pair<std::string_view, SourceLocation>> Parser::parseSrcComme
 		return {{tail, SourceLocation{}}};
 	}
 
-	std::optional<int> const sourceIndex = toInt(match[1].str());
-	std::optional<int> const start = toInt(match[2].str());
-	std::optional<int> const end = toInt(match[3].str());
+	std::optional<int> const sourceIndex = toInt(*rawSourceIndex);
+	std::optional<int> const start = toInt(*rawStart);
+	std::optional<int> const end = toInt(*rawEnd);
 
-	if (!sourceIndex.has_value() || !start.has_value() || !end.has_value())
+	if (
+		!sourceIndex.has_value() || *sourceIndex < -1 ||
+		!start.has_value() || *start < -1 ||
+		!end.has_value() || *end < -1
+	)
 		m_errorReporter.syntaxError(
 			6367_error,
 			_commentLocation,
 			"Invalid value in source location mapping. "
 			"Expected non-negative integer values or -1 for source index and location."
 		);
 	else if (sourceIndex == -1)
-		return {{tail, SourceLocation{start.value(), end.value(), nullptr}}};
-	else if (!(sourceIndex >= 0 && m_sourceNames->count(static_cast<unsigned>(sourceIndex.value()))))
+		return {{tail, SourceLocation{*start, *end, nullptr}}};
+	else if (!(sourceIndex >= 0 && m_sourceNames->count(static_cast<unsigned>(*sourceIndex))))
 		m_errorReporter.syntaxError(
 			2674_error,
 			_commentLocation,
 			"Invalid source mapping. Source index not defined via @use-src."
 		);
 	else
 	{
-		std::shared_ptr<std::string const> sourceName = m_sourceNames->at(static_cast<unsigned>(sourceIndex.value()));
+		std::shared_ptr<std::string const> sourceName = m_sourceNames->at(static_cast<unsigned>(*sourceIndex));
 		solAssert(sourceName, "");
-		return {{tail, SourceLocation{start.value(), end.value(), std::move(sourceName)}}};
+		return {{tail, SourceLocation{*start, *end, std::move(sourceName)}}};
 	}
 	return {{tail, SourceLocation{}}};
 }
diff --git a/test/liblangutil/Scanner.cpp b/test/liblangutil/Scanner.cpp
@@ -1026,6 +1026,52 @@ BOOST_AUTO_TEST_CASE(yul_function_with_whitespace)
 	BOOST_CHECK_EQUAL(scanner.next(), Token::EOS);
 }
 
+BOOST_AUTO_TEST_CASE(special_comment_with_invalid_escapes)
+{
+	std::string input(R"("test\x\f\g\u\g\a\u\g\a12\uö\xyoof")");
+	std::string expectedOutput(R"(test12öyoof)");
+	CharStream stream(input, "");
+	Scanner scanner(stream, ScannerKind::SpecialComment);
+	BOOST_REQUIRE(scanner.currentToken() == Token::StringLiteral);
+	BOOST_REQUIRE(scanner.currentLiteral() == expectedOutput);
+}
+
+BOOST_AUTO_TEST_CASE(special_comment_with_valid_and_invalid_escapes)
+{
+	std::string input(R"("test\n\x61\t\u01A9test\f")");
+	std::string expectedOutput(R"(test6101A9test)");
+	CharStream stream(input, "");
+	Scanner scanner(stream, ScannerKind::SpecialComment);
+	BOOST_REQUIRE(scanner.currentToken() == Token::StringLiteral);
+	BOOST_REQUIRE(scanner.currentLiteral() == expectedOutput);
+}
+
+BOOST_AUTO_TEST_CASE(special_comment_with_unterminated_escape_sequence_at_eos)
+{
+	CharStream stream(R"("test\)", "");
+	std::string expectedOutput(R"(test6101A9test)");
+	Scanner scanner(stream, ScannerKind::SpecialComment);
+	BOOST_REQUIRE(scanner.currentToken() == Token::Illegal);
+	BOOST_REQUIRE(scanner.currentError() == ScannerError::IllegalEscapeSequence);
+}
+
+BOOST_AUTO_TEST_CASE(special_comment_with_escaped_quotes)
+{
+	CharStream stream(R"("test\\\"")", "");
+	std::string expectedOutput(R"(test)");
+	Scanner scanner(stream, ScannerKind::SpecialComment);
+	BOOST_REQUIRE(scanner.currentToken() == Token::StringLiteral);
+	BOOST_REQUIRE(scanner.currentLiteral() == expectedOutput);
+}
+
+BOOST_AUTO_TEST_CASE(special_comment_with_unterminated_string)
+{
+	CharStream stream(R"("test)", "");
+	Scanner scanner(stream, ScannerKind::SpecialComment);
+	BOOST_REQUIRE(scanner.currentToken() == Token::Illegal);
+	BOOST_REQUIRE(scanner.currentError() == ScannerError::IllegalStringEndQuote);
+}
+
 BOOST_AUTO_TEST_SUITE_END()
 
 } // end namespaces
diff --git a/test/libyul/Parser.cpp b/test/libyul/Parser.cpp

Original file line number	Diff line number	Diff line change
`@@ -62,15 +62,12 @@`
`62`	`62`	`namespace solidity::langutil`
`63`	`63`	`{`
`64`	`64`
`65`		`-class AstRawString;`
`66`		`-class AstValueFactory;`
`67`		`-class ParserRecorder;`
`68`		`-`
`69`	`65`	`enum class ScannerKind`
`70`	`66`	`{`
`71`	`67`	`Solidity,`
`72`	`68`	`Yul,`
`73`		`- ExperimentalSolidity`
	`69`	`+ ExperimentalSolidity,`
	`70`	`+ SpecialComment`
`74`	`71`	`};`
`75`	`72`
`76`	`73`	`enum class ScannerError`
`@@ -102,11 +99,13 @@ class Scanner`
`102`	`99`	`{`
`103`	`100`	`friend class LiteralScope;`
`104`	`101`	`public:`
`105`		`- explicit Scanner(CharStream& _source):`
	`102`	`+ explicit Scanner(CharStream& _source, ScannerKind _kind = ScannerKind::Solidity):`
`106`	`103`	`m_source(_source),`
`107`	`104`	`m_sourceName{std::make_shared<std::string>(_source.name())}`
`108`	`105`	`{`
`109`	`106`	`reset();`
	`107`	`+ if (_kind != ScannerKind::Solidity)`
	`108`	`+ setScannerMode(_kind);`
`110`	`109`	`}`
`111`	`110`
`112`	`111`	`/// Resets scanner to the start of input.`