Merge pull request #12663 from ethereum/assemblyAnnotationDialectString

chriseth · web-flow · commit 57e012da9855 · 2022-03-07T13:03:08.000+01:00
Assembly annotation as memory-safe using assembly flags.
diff --git a/docs/assembly.rst b/docs/assembly.rst
@@ -297,8 +297,7 @@ model as follows:
 
 .. code-block:: solidity
 
-    /// @solidity memory-safe-assembly
-    assembly {
+    assembly ("memory-safe") {
         ...
     }
 
@@ -327,8 +326,7 @@ But the following is:
 
 .. code-block:: solidity
 
-    /// @solidity memory-safe-assembly
-    assembly {
+    assembly ("memory-safe") {
       let p := mload(0x40)
       returndatacopy(p, 0, returndatasize())
       revert(p, returndatasize())
@@ -341,8 +339,7 @@ If the memory operations use a length of zero, it is also fine to just use any o
 
 .. code-block:: solidity
 
-    /// @solidity memory-safe-assembly
-    assembly {
+    assembly ("memory-safe") {
       revert(0, 0)
     }
 
@@ -364,3 +361,16 @@ in memory is automatically considered memory-safe and does not need to be annota
     It is your responsibility to make sure that the assembly actually satisfies the memory model. If you annotate
     an assembly block as memory-safe, but violate one of the memory assumptions, this **will** lead to incorrect and
     undefined behaviour that cannot easily be discovered by testing.
+
+In case you are developing a library that is meant to be compatible across multiple versions
+of solidity, you can use a special comment to annotate an assembly block as memory-safe:
+
+.. code-block:: solidity
+
+    /// @solidity memory-safe-assembly
+    assembly {
+        ...
+    }
+
+Note that we will disallow the annotation via comment in a future breaking release, so if you are not concerned with
+backwards-compatibility with older compiler versions, prefer using the dialect string.
diff --git a/docs/grammar/SolidityLexer.g4 b/docs/grammar/SolidityLexer.g4
@@ -251,6 +251,12 @@ mode AssemblyBlockMode;
 AssemblyDialect: '"evmasm"';
 AssemblyLBrace: '{' -> popMode, pushMode(YulMode);
 
+AssemblyFlagString: '"' DoubleQuotedStringCharacter+ '"';
+
+AssemblyBlockLParen: '(';
+AssemblyBlockRParen: ')';
+AssemblyBlockComma: ',';
+
 AssemblyBlockWS: [ \t\r\n\u000C]+ -> skip ;
 AssemblyBlockCOMMENT: '/*' .*? '*/' -> channel(HIDDEN) ;
 AssemblyBlockLINE_COMMENT: '//' ~[\r\n]* -> channel(HIDDEN) ;
diff --git a/docs/grammar/SolidityParser.g4 b/docs/grammar/SolidityParser.g4
@@ -476,7 +476,13 @@ revertStatement: Revert expression callArgumentList Semicolon;
  * The contents of an inline assembly block use a separate scanner/lexer, i.e. the set of keywords and
  * allowed identifiers is different inside an inline assembly block.
  */
-assemblyStatement: Assembly AssemblyDialect? AssemblyLBrace yulStatement* YulRBrace;
+assemblyStatement: Assembly AssemblyDialect? assemblyFlags? AssemblyLBrace yulStatement* YulRBrace;
+
+/**
+ * Assembly flags.
+ * Comma-separated list of double-quoted strings as flags.
+ */
+assemblyFlags: AssemblyBlockLParen AssemblyFlagString (AssemblyBlockComma AssemblyFlagString)* AssemblyBlockRParen;
 
 //@doc:inline
 variableDeclarationList: variableDeclarations+=variableDeclaration (Comma variableDeclarations+=variableDeclaration)*;
diff --git a/libsolidity/analysis/DocStringTagParser.cpp b/libsolidity/analysis/DocStringTagParser.cpp
@@ -202,7 +202,17 @@ bool DocStringTagParser::visit(InlineAssembly const& _assembly)
 				if (valuesSeen.insert(value).second)
 				{
 					if (value == "memory-safe-assembly")
+					{
+						if (_assembly.annotation().markedMemorySafe)
+							m_errorReporter.warning(
+								8544_error,
+								_assembly.location(),
+								"Inline assembly marked as memory safe using both a NatSpec tag and an assembly flag. "
+								"If you are not concerned with backwards compatibility, only use the assembly flag, "
+								"otherwise only use the NatSpec tag."
+							);
 						_assembly.annotation().markedMemorySafe = true;
+					}
 					else
 						m_errorReporter.warning(
 							8787_error,
diff --git a/libsolidity/analysis/SyntaxChecker.cpp b/libsolidity/analysis/SyntaxChecker.cpp
@@ -334,6 +334,27 @@ bool SyntaxChecker::visit(UnaryOperation const& _operation)
 
 bool SyntaxChecker::visit(InlineAssembly const& _inlineAssembly)
 {
+	if (_inlineAssembly.flags())
+		for (auto flag: *_inlineAssembly.flags())
+		{
+			if (*flag == "memory-safe")
+			{
+				if (_inlineAssembly.annotation().markedMemorySafe)
+					m_errorReporter.syntaxError(
+						7026_error,
+						_inlineAssembly.location(),
+						"Inline assembly marked memory-safe multiple times."
+					);
+				_inlineAssembly.annotation().markedMemorySafe = true;
+			}
+			else
+				m_errorReporter.warning(
+					4430_error,
+					_inlineAssembly.location(),
+					"Unknown inline assembly flag: \"" + *flag + "\""
+				);
+		}
+
 	if (!m_useYulOptimizer)
 		return false;
 
diff --git a/libsolidity/ast/AST.h b/libsolidity/ast/AST.h
@@ -1463,19 +1463,26 @@ class InlineAssembly: public Statement
 		SourceLocation const& _location,
 		ASTPointer<ASTString> const& _docString,
 		yul::Dialect const& _dialect,
+		ASTPointer<std::vector<ASTPointer<ASTString>>> _flags,
 		std::shared_ptr<yul::Block> _operations
 	):
-		Statement(_id, _location, _docString), m_dialect(_dialect), m_operations(std::move(_operations)) {}
+		Statement(_id, _location, _docString),
+		m_dialect(_dialect),
+		m_flags(move(_flags)),
+		m_operations(std::move(_operations))
+	{}
 	void accept(ASTVisitor& _visitor) override;
 	void accept(ASTConstVisitor& _visitor) const override;
 
 	yul::Dialect const& dialect() const { return m_dialect; }
 	yul::Block const& operations() const { return *m_operations; }
+	ASTPointer<std::vector<ASTPointer<ASTString>>> const& flags() const { return m_flags; }
 
 	InlineAssemblyAnnotation& annotation() const override;
 
 private:
 	yul::Dialect const& m_dialect;
+	ASTPointer<std::vector<ASTPointer<ASTString>>> m_flags;
 	std::shared_ptr<yul::Block> m_operations;
 };
 
diff --git a/libsolidity/ast/ASTJsonConverter.cpp b/libsolidity/ast/ASTJsonConverter.cpp
@@ -600,11 +600,23 @@ bool ASTJsonConverter::visit(InlineAssembly const& _node)
 	for (Json::Value& it: externalReferences | ranges::views::values)
 		externalReferencesJson.append(std::move(it));
 
-	setJsonNode(_node, "InlineAssembly", {
+	std::vector<pair<string, Json::Value>> attributes = {
 		make_pair("AST", Json::Value(yul::AsmJsonConverter(sourceIndexFromLocation(_node.location()))(_node.operations()))),
 		make_pair("externalReferences", std::move(externalReferencesJson)),
 		make_pair("evmVersion", dynamic_cast<solidity::yul::EVMDialect const&>(_node.dialect()).evmVersion().name())
-	});
+	};
+
+	if (_node.flags())
+	{
+		Json::Value flags(Json::arrayValue);
+		for (auto const& flag: *_node.flags())
+			if (flag)
+				flags.append(*flag);
+			else
+				flags.append(Json::nullValue);
+		attributes.emplace_back(make_pair("flags", move(flags)));
+	}
+	setJsonNode(_node, "InlineAssembly", move(attributes));
 
 	return false;
 }
diff --git a/libsolidity/ast/ASTJsonImporter.cpp b/libsolidity/ast/ASTJsonImporter.cpp
@@ -626,11 +626,24 @@ ASTPointer<InlineAssembly> ASTJsonImporter::createInlineAssembly(Json::Value con
 	astAssert(m_evmVersion == evmVersion, "Imported tree evm version differs from configured evm version!");
 
 	yul::Dialect const& dialect = yul::EVMDialect::strictAssemblyForEVM(evmVersion.value());
+	ASTPointer<vector<ASTPointer<ASTString>>> flags;
+	if (_node.isMember("flags"))
+	{
+		flags = make_shared<vector<ASTPointer<ASTString>>>();
+		Json::Value const& flagsNode = _node["flags"];
+		astAssert(flagsNode.isArray(), "Assembly flags must be an array.");
+		for (Json::ArrayIndex i = 0; i < flagsNode.size(); ++i)
+		{
+			astAssert(flagsNode[i].isString(), "Assembly flag must be a string.");
+			flags->emplace_back(make_shared<ASTString>(flagsNode[i].asString()));
+		}
+	}
 	shared_ptr<yul::Block> operations = make_shared<yul::Block>(yul::AsmJsonImporter(m_sourceNames).createBlock(member(_node, "AST")));
 	return createASTNode<InlineAssembly>(
 		_node,
 		nullOrASTString(_node, "documentation"),
 		dialect,
+		move(flags),
 		operations
 	);
 }
diff --git a/libsolidity/parsing/Parser.cpp b/libsolidity/parsing/Parser.cpp
@@ -1321,13 +1321,28 @@ ASTPointer<InlineAssembly> Parser::parseInlineAssembly(ASTPointer<ASTString> con
 		advance();
 	}
 
+	ASTPointer<vector<ASTPointer<ASTString>>> flags;
+	if (m_scanner->currentToken() == Token::LParen)
+	{
+		flags = make_shared<vector<ASTPointer<ASTString>>>();
+		do
+		{
+			advance();
+			expectToken(Token::StringLiteral, false);
+			flags->emplace_back(make_shared<ASTString>(m_scanner->currentLiteral()));
+			advance();
+		}
+		while (m_scanner->currentToken() == Token::Comma);
+		expectToken(Token::RParen);
+	}
+
 	yul::Parser asmParser(m_errorReporter, dialect);
 	shared_ptr<yul::Block> block = asmParser.parseInline(m_scanner);
 	if (block == nullptr)
 		BOOST_THROW_EXCEPTION(FatalError());
 
 	location.end = nativeLocationOf(*block).end;
-	return make_shared<InlineAssembly>(nextID(), location, _docString, dialect, block);
+	return make_shared<InlineAssembly>(nextID(), location, _docString, dialect, move(flags), block);
 }
 
 ASTPointer<IfStatement> Parser::parseIfStatement(ASTPointer<ASTString> const& _docString)
diff --git a/test/libsolidity/memoryGuardTests/comment/constructor_safe_deploy_unsafe.sol b/test/libsolidity/memoryGuardTests/comment/constructor_safe_deploy_unsafe.sol
diff --git a/test/libsolidity/memoryGuardTests/comment/constructor_unsafe_deploy_safe.sol b/test/libsolidity/memoryGuardTests/comment/constructor_unsafe_deploy_safe.sol
diff --git a/test/libsolidity/memoryGuardTests/comment/free_function.sol b/test/libsolidity/memoryGuardTests/comment/free_function.sol
diff --git a/test/libsolidity/memoryGuardTests/comment/multi_annotation.sol b/test/libsolidity/memoryGuardTests/comment/multi_annotation.sol
diff --git a/test/libsolidity/memoryGuardTests/comment/multiple_contracts.sol b/test/libsolidity/memoryGuardTests/comment/multiple_contracts.sol
diff --git a/test/libsolidity/memoryGuardTests/comment/safe_and_unmarked_empty.sol b/test/libsolidity/memoryGuardTests/comment/safe_and_unmarked_empty.sol
diff --git a/test/libsolidity/memoryGuardTests/comment/safe_and_unmarked_unsafe.sol b/test/libsolidity/memoryGuardTests/comment/safe_and_unmarked_unsafe.sol
diff --git a/test/libsolidity/memoryGuardTests/dialectString/constructor_safe_deploy_unsafe.sol b/test/libsolidity/memoryGuardTests/dialectString/constructor_safe_deploy_unsafe.sol
@@ -0,0 +1,17 @@
+contract C {
+    constructor() {
+        uint256 x;
+        assembly { x := 0 }
+        f();
+    }
+    function f() internal pure {
+        assembly "evmasm" ("memory-safe") { mstore(0, 0) }
+        assembly ("memory-safe") { mstore(0, 0) }
+    }
+    function g() public pure {
+        assembly { mstore(0, 0) }
+    }
+}
+// ----
+// :C(creation) true
+// :C(runtime) false
diff --git a/test/libsolidity/memoryGuardTests/dialectString/constructor_unsafe_deploy_safe.sol b/test/libsolidity/memoryGuardTests/dialectString/constructor_unsafe_deploy_safe.sol
@@ -0,0 +1,16 @@
+contract C {
+    constructor() {
+        uint256 x;
+        assembly { x := 0 }
+        f();
+    }
+    function f() internal pure {
+        assembly { mstore(0, 0) }
+    }
+    function g() public pure {
+        assembly "evmasm" ("memory-safe") { mstore(0, 0) }
+    }
+}
+// ----
+// :C(creation) false
+// :C(runtime) true
diff --git a/test/libsolidity/memoryGuardTests/dialectString/free_function.sol b/test/libsolidity/memoryGuardTests/dialectString/free_function.sol
@@ -0,0 +1,28 @@
+function safe() pure returns (uint256 x) {
+    assembly { x := 42 }
+    assembly "evmasm" ("memory-safe") { mstore(0, 0) }
+}
+function unsafe() pure returns (uint256 x) {
+    assembly { pop(mload(0)) }
+}
+contract C {
+    constructor() {
+        unsafe();
+    }
+    function f() public pure {
+        safe();
+    }
+}
+contract D {
+    constructor() {
+        safe();
+    }
+    function f() public pure {
+        unsafe();
+    }
+}
+// ----
+// :C(creation) false
+// :C(runtime) true
+// :D(creation) true
+// :D(runtime) false
diff --git a/test/libsolidity/memoryGuardTests/dialectString/multiple_contracts.sol b/test/libsolidity/memoryGuardTests/dialectString/multiple_contracts.sol
@@ -0,0 +1,23 @@
+contract C {
+    constructor(uint256 x) {
+        assembly { x := 4 }
+        assembly "evmasm" ("memory-safe") { mstore(0, 0) }
+    }
+    function f() public pure {
+        assembly { mstore(0,0) }
+    }
+}
+contract D {
+    constructor() {
+        assembly { mstore(0,0) }
+    }
+    function f(uint256 x) public pure {
+        assembly { x := 4 }
+        assembly ("memory-safe") { mstore(0, 0) }
+    }
+}
+// ----
+// :C(creation) true
+// :C(runtime) false
+// :D(creation) false
+// :D(runtime) true
diff --git a/test/libsolidity/memoryGuardTests/dialectString/safe_and_unmarked_empty.sol b/test/libsolidity/memoryGuardTests/dialectString/safe_and_unmarked_empty.sol
@@ -0,0 +1,9 @@
+contract C {
+    function f() external pure {
+        assembly "evmasm" ("memory-safe") {}
+        assembly {}
+    }
+}
+// ----
+// :C(creation) true
+// :C(runtime) true
diff --git a/test/libsolidity/memoryGuardTests/dialectString/safe_and_unmarked_unsafe.sol b/test/libsolidity/memoryGuardTests/dialectString/safe_and_unmarked_unsafe.sol
@@ -0,0 +1,9 @@
+contract C {
+    function f() external pure {
+        assembly "evmasm" ("memory-safe") {}
+        assembly { mstore(0,0) }
+    }
+}
+// ----
+// :C(creation) true
+// :C(runtime) false
diff --git a/test/libsolidity/memoryGuardTests/dialectString/stub.sol b/test/libsolidity/memoryGuardTests/dialectString/stub.sol
@@ -0,0 +1,4 @@
+contract C {}
+// ----
+// :C(creation) true
+// :C(runtime) true
diff --git a/test/libsolidity/syntaxTests/inlineAssembly/assembly_dialect_duplicate_option.sol b/test/libsolidity/syntaxTests/inlineAssembly/assembly_dialect_duplicate_option.sol
@@ -0,0 +1,5 @@
+function f() pure {
+    assembly "evmasm" ("memory-safe", "memory-safe") {}
+}
+// ----
+// SyntaxError 7026: (24-75): Inline assembly marked memory-safe multiple times.
diff --git a/test/libsolidity/syntaxTests/inlineAssembly/assembly_dialect_invalid_options.sol b/test/libsolidity/syntaxTests/inlineAssembly/assembly_dialect_invalid_options.sol
@@ -0,0 +1,8 @@
+function f() pure {
+    assembly "evmasm" ("a", "b", "c", "c") {}
+}
+// ----
+// Warning 4430: (24-65): Unknown inline assembly flag: "a"
+// Warning 4430: (24-65): Unknown inline assembly flag: "b"
+// Warning 4430: (24-65): Unknown inline assembly flag: "c"
+// Warning 4430: (24-65): Unknown inline assembly flag: "c"
diff --git a/test/libsolidity/syntaxTests/inlineAssembly/assembly_dialect_leading_space.sol b/test/libsolidity/syntaxTests/inlineAssembly/assembly_dialect_leading_space.sol
@@ -0,0 +1,5 @@
+function f() pure {
+    assembly " evmasm" {}
+}
+// ----
+// ParserError 4531: (33-42): Only "evmasm" supported.
diff --git a/test/libsolidity/syntaxTests/inlineAssembly/assembly_duplicate_option.sol b/test/libsolidity/syntaxTests/inlineAssembly/assembly_duplicate_option.sol
@@ -0,0 +1,5 @@
+function f() pure {
+    assembly ("memory-safe", "memory-safe") {}
+}
+// ----
+// SyntaxError 7026: (24-66): Inline assembly marked memory-safe multiple times.
diff --git a/test/libsolidity/syntaxTests/inlineAssembly/assembly_empty_option_list.sol b/test/libsolidity/syntaxTests/inlineAssembly/assembly_empty_option_list.sol
@@ -0,0 +1,5 @@
+function f() pure {
+    assembly () {}
+}
+// ----
+// ParserError 2314: (34-35): Expected 'StringLiteral' but got ')'
diff --git a/test/libsolidity/syntaxTests/inlineAssembly/assembly_empty_option_list_dialect.sol b/test/libsolidity/syntaxTests/inlineAssembly/assembly_empty_option_list_dialect.sol
@@ -0,0 +1,5 @@
+function f() pure {
+    assembly "evmasm" () {}
+}
+// ----
+// ParserError 2314: (43-44): Expected 'StringLiteral' but got ')'
diff --git a/test/libsolidity/syntaxTests/inlineAssembly/assembly_flags_delimiter.sol b/test/libsolidity/syntaxTests/inlineAssembly/assembly_flags_delimiter.sol
@@ -0,0 +1,5 @@
+function f() pure {
+	assembly ("a" "b") {}
+}
+// ----
+// ParserError 2314: (35-38): Expected ')' but got 'StringLiteral'
diff --git a/test/libsolidity/syntaxTests/inlineAssembly/assembly_invalid_options.sol b/test/libsolidity/syntaxTests/inlineAssembly/assembly_invalid_options.sol
@@ -0,0 +1,8 @@
+function f() pure {
+    assembly ("a", "b", "c", "c") {}
+}
+// ----
+// Warning 4430: (24-56): Unknown inline assembly flag: "a"
+// Warning 4430: (24-56): Unknown inline assembly flag: "b"
+// Warning 4430: (24-56): Unknown inline assembly flag: "c"
+// Warning 4430: (24-56): Unknown inline assembly flag: "c"
diff --git a/test/libsolidity/syntaxTests/inlineAssembly/memory_safe_dialect_string_and_comment.sol b/test/libsolidity/syntaxTests/inlineAssembly/memory_safe_dialect_string_and_comment.sol
diff --git a/test/libsolidity/syntaxTests/inlineAssembly/memory_safe_in_dialect_string.sol b/test/libsolidity/syntaxTests/inlineAssembly/memory_safe_in_dialect_string.sol
