Memory-safety annotation for inline assembly. #12620

ekpyron · 2022-02-03T11:02:51Z

We can also properly make inline assembly StructurallyDocumented, etc, but then we need to double-check that existing natspec-style comments don't cause any errors to keep this fully non-breaking.

libsolidity/ast/ASTJsonConverter.cpp

chriseth · 2022-02-03T11:38:42Z

Can you also define what exactly memory safety means?

ekpyron · 2022-02-03T11:40:04Z

Can you also define what exactly memory safety means?

Sure, this needs extensive documentation before I undraft it :-).
Mainly pushed it already to see if it works for the external test runs. For trident it does - but ENS still fails, I'm just trying to figure out whether that's because it pulls in dependencies with assembly that I didn't patch or because of something else.

ekpyron · 2022-02-03T11:45:08Z

Eventually we should also add better reports for all of this...
Ideally, I'd report an info message whenever any variables are moved to memory and if compilation fails due to stack too deep, but could not run stack-to-memory due to inline assembly, it should properly suggest adding those annotations... Ideally even with source locations :-).

But that's a bit of a hassle, since codegen so far is not meant to report stuff like that and we may need to store the source locations somewhere, etc., so I'd keep that separate for now...

test/libsolidity/ASTJSON/assembly/inline_assembly_memory_safe_parseOnly.json

ekpyron · 2022-02-03T13:03:07Z

libsolidity/analysis/DocStringTagParser.cpp

+{
+	if (!_assembly.documentation())
+		return true;
+	StructuredDocumentation documentation{-1, _assembly.location(), _assembly.documentation()};


The -1 node id is a bit creepy...

ekpyron · 2022-02-03T15:17:15Z

I just added a first rough draft for documenting this.

I'm wondering how restrictive we should define this - technically it's valid for stack-to-memory to access any memory that is known to be past the initial value of the free memory pointer...
Also we need to decide whether we want to specifically add the requirement for memory accesses to be bounded, i.e. the assumption for the redundant store eliminator (either as only ever allocating bounded amounts of memory, if we stay with only accessing allocated memory, or more freely as not accessing memory from unbounded offsets).

ekpyron · 2022-02-03T15:42:28Z

Test failures are just pending #12624 I think

cameel · 2022-02-03T17:50:06Z

#12624 is ready.

ekpyron · 2022-02-04T13:15:01Z

docs/assembly.rst

+All accessed memory (read or written) was properly allocated. This can either be memory allocated by yourself
+using a mechanism like the ``allocate`` function described above, or memory allocated by Solidity,
+e.g. memory within the bounds of a memory array you reference. The only exception to this is the scratch space
+between memory offset 0 and 64 mentioned above.


For this we still need to discuss what our actual requirements should really be, so I expect this to still change - for the rest of the docs, a review would be good already, though :-).

chriseth · 2022-02-09T15:20:03Z

Changelog.md

@@ -3,6 +3,7 @@
 Language Features:
 * General: Support ``ContractName.functionName`` for ``abi.encodeCall``, in addition to external function pointers.
 * General: Add equality-comparison operators for external function types.
+ * General: Allow annotating inline assembly as memory-safe to allow optimizations and stack limit evasion that rely on respecting Solidity's memory model.


This only applies to "via ir", does it?

In practice at least... in principle, we could use the information for old codegen as well, but we probably won't.

chriseth · 2022-02-09T15:23:19Z

docs/assembly.rst

+While we recommend to always respect Solidity's memory model, it is possible to use memory
+in an incompatible way from inline assembly. Therefore, moving stack variables to memory and additional


Suggested change

While we recommend to always respect Solidity's memory model, it is possible to use memory

in an incompatible way from inline assembly. Therefore, moving stack variables to memory and additional

While we recommend to always respect Solidity's memory model, inline assembly allows you to use memory

in an incompatible way. Therefore, moving stack variables to memory and additional

ekpyron · 2022-03-02T09:09:51Z

libsolidity/analysis/DocStringTagParser.cpp

+
+			set<string> valuesSeen;
+			set<string> duplicates;
+			for (auto const& value: values | ranges::views::filter(not_fn(&string::empty)))


Instead of the boost::split I could also split on the fly using ranges...

constexpr auto splitString = [](auto _pred) { return ranges::views::split_when(move(_pred)) | ranges::views::transform([](auto&& _rng){ return string_view(&*begin(_rng), static_cast<string_view::size_type>(ranges::distance(_rng))); }); }; for (auto value: tagValue.content | splitString(isWhiteSpace) | ranges::views::filter(not_fn(&string_view::empty)))

But maybe that's over the top...

libsolidity/analysis/DocStringTagParser.cpp

ekpyron · 2022-03-02T15:01:15Z

Updated the warnings. We should probably be clear that we can also merge the other one, before merging this, though. Although it doesn't matter as long as we manage for the same release I guess.

test/libsolidity/syntaxTests/inlineAssembly/natspec_multi.sol

ekpyron · 2022-03-02T15:43:31Z

test/libsolidity/syntaxTests/inlineAssembly/natspec_multi_swallowed.sol

+}
+// ----
+// Warning 6269: (177-188): Unexpected NatSpec tag "after" with value "bogus-value" in inline assembly.
+// Warning 6269: (177-188): Unexpected NatSpec tag "before" with value "@solidity a   memory-safe-assembly b    c           d" in inline assembly.


See #12732
But if we report the full value here, I'd say it's still ok for this purpose and we can fix it with #12732...

ekpyron force-pushed the assemblyAnnotation branch from 62de5d3 to 4539718 Compare February 3, 2022 11:13

ekpyron commented Feb 3, 2022

View reviewed changes

libsolidity/ast/ASTJsonConverter.cpp Outdated Show resolved Hide resolved

ekpyron force-pushed the assemblyAnnotation branch from bfd815c to a3e16d7 Compare February 3, 2022 11:59

ekpyron commented Feb 3, 2022

View reviewed changes

test/libsolidity/ASTJSON/assembly/inline_assembly_memory_safe_parseOnly.json Outdated Show resolved Hide resolved

ekpyron force-pushed the assemblyAnnotation branch 4 times, most recently from d177ebf to e9c8803 Compare February 3, 2022 12:24

ethereum deleted a comment from stackenbotten Feb 3, 2022

ekpyron commented Feb 3, 2022

View reviewed changes

ekpyron force-pushed the assemblyAnnotation branch 4 times, most recently from 77e0d2f to 2f37409 Compare February 3, 2022 15:14

ekpyron requested a review from chriseth February 3, 2022 15:24

ekpyron force-pushed the assemblyAnnotation branch from 2f37409 to 3d19b07 Compare February 3, 2022 15:26

ekpyron force-pushed the assemblyAnnotation branch from 3d19b07 to d38f89a Compare February 4, 2022 13:03

ekpyron commented Feb 4, 2022

View reviewed changes

ekpyron marked this pull request as ready for review February 4, 2022 13:15

chriseth reviewed Feb 9, 2022

View reviewed changes

ekpyron force-pushed the assemblyAnnotation branch from d4542f4 to 5a41d45 Compare March 1, 2022 13:03

ekpyron mentioned this pull request Mar 1, 2022

ABIEncoderV2 runs into stack too deep #10930

Closed

ekpyron force-pushed the assemblyAnnotation branch from 7de3b3c to 38a8612 Compare March 2, 2022 09:05

ekpyron commented Mar 2, 2022

View reviewed changes

ekpyron requested a review from chriseth March 2, 2022 09:15

chriseth reviewed Mar 2, 2022

View reviewed changes

libsolidity/analysis/DocStringTagParser.cpp Outdated Show resolved Hide resolved

chriseth reviewed Mar 2, 2022

View reviewed changes

libsolidity/analysis/DocStringTagParser.cpp Outdated Show resolved Hide resolved

chriseth previously approved these changes Mar 2, 2022

View reviewed changes

ekpyron dismissed chriseth’s stale review via d924581 March 2, 2022 14:59

ekpyron force-pushed the assemblyAnnotation branch from 38a8612 to d924581 Compare March 2, 2022 14:59

ekpyron requested a review from chriseth March 2, 2022 15:00

ekpyron commented Mar 2, 2022

View reviewed changes

test/libsolidity/syntaxTests/inlineAssembly/natspec_multi.sol Outdated Show resolved Hide resolved

ekpyron force-pushed the assemblyAnnotation branch from d924581 to 3730c58 Compare March 2, 2022 15:37

ekpyron added 6 commits March 2, 2022 16:42

Allow annotating inline assembly as memory-safe.

e6848ca

Inline assembly without memory effects is implicitly memory safe.

9bcfcc6

Report memoryguard in stack too deep error.

dfb7bf2

Documentation.

62a997a

Patch external tests with a safe inline assembly annotation.

ad13062

Tests.

6b6e163

ekpyron force-pushed the assemblyAnnotation branch from 3730c58 to 6b6e163 Compare March 2, 2022 15:42

ekpyron commented Mar 2, 2022

View reviewed changes

Cryptomooner111 approved these changes Mar 2, 2022

View reviewed changes

This comment was marked as spam.

Sign in to view

chriseth merged commit 4f19d68 into develop Mar 7, 2022

chriseth deleted the assemblyAnnotation branch March 7, 2022 11:48

wchargin mentioned this pull request Mar 10, 2022

Clarify that bytes values do not need to be aligned #12576

Closed

ekpyron mentioned this pull request Mar 11, 2022

Make use of inline assembly dialect for memory safety #11417

Closed

This was referenced Mar 25, 2024

Mask computed address in Create2 and Clones libraries OpenZeppelin/openzeppelin-contracts#4941

Merged

Consider changing memory-safe NatSpec annotations to assembly('memory-safe') dialect string OpenZeppelin/openzeppelin-contracts#4971

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Memory-safety annotation for inline assembly. #12620

Memory-safety annotation for inline assembly. #12620

ekpyron commented Feb 3, 2022 •

edited

Loading

chriseth commented Feb 3, 2022

ekpyron commented Feb 3, 2022 •

edited

Loading

ekpyron commented Feb 3, 2022

ekpyron Feb 3, 2022

ekpyron commented Feb 3, 2022 •

edited

Loading

ekpyron commented Feb 3, 2022 •

edited

Loading

cameel commented Feb 3, 2022

ekpyron Feb 4, 2022

chriseth Feb 9, 2022

ekpyron Feb 9, 2022 •

edited

Loading

chriseth Feb 9, 2022

ekpyron Mar 2, 2022

ekpyron commented Mar 2, 2022

ekpyron Mar 2, 2022

This comment was marked as spam.

		While we recommend to always respect Solidity's memory model, it is possible to use memory
		in an incompatible way from inline assembly. Therefore, moving stack variables to memory and additional

Memory-safety annotation for inline assembly. #12620

Memory-safety annotation for inline assembly. #12620

Conversation

ekpyron commented Feb 3, 2022 • edited Loading

chriseth commented Feb 3, 2022

ekpyron commented Feb 3, 2022 • edited Loading

ekpyron commented Feb 3, 2022

ekpyron Feb 3, 2022

Choose a reason for hiding this comment

ekpyron commented Feb 3, 2022 • edited Loading

ekpyron commented Feb 3, 2022 • edited Loading

cameel commented Feb 3, 2022

ekpyron Feb 4, 2022

Choose a reason for hiding this comment

chriseth Feb 9, 2022

Choose a reason for hiding this comment

ekpyron Feb 9, 2022 • edited Loading

Choose a reason for hiding this comment

chriseth Feb 9, 2022

Choose a reason for hiding this comment

ekpyron Mar 2, 2022

Choose a reason for hiding this comment

ekpyron commented Mar 2, 2022

ekpyron Mar 2, 2022

Choose a reason for hiding this comment

This comment was marked as spam.

ekpyron commented Feb 3, 2022 •

edited

Loading

ekpyron commented Feb 3, 2022 •

edited

Loading

ekpyron commented Feb 3, 2022 •

edited

Loading

ekpyron commented Feb 3, 2022 •

edited

Loading

ekpyron Feb 9, 2022 •

edited

Loading