-
Notifications
You must be signed in to change notification settings - Fork 6.3k
Memory-safety annotation for inline assembly. #12620
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
e6848ca
9bcfcc6
dfb7bf2
62a997a
ad13062
6b6e163
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,6 +30,7 @@ | |
| #include <liblangutil/Common.h> | ||
|
|
||
| #include <range/v3/algorithm/any_of.hpp> | ||
| #include <range/v3/view/filter.hpp> | ||
|
|
||
| #include <boost/algorithm/string.hpp> | ||
|
|
||
|
|
@@ -162,6 +163,71 @@ bool DocStringTagParser::visit(ErrorDefinition const& _error) | |
| return true; | ||
| } | ||
|
|
||
| bool DocStringTagParser::visit(InlineAssembly const& _assembly) | ||
| { | ||
| if (!_assembly.documentation()) | ||
| return true; | ||
| StructuredDocumentation documentation{-1, _assembly.location(), _assembly.documentation()}; | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
| ErrorList errors; | ||
| ErrorReporter errorReporter{errors}; | ||
| auto docTags = DocStringParser{documentation, errorReporter}.parse(); | ||
|
|
||
| if (!errors.empty()) | ||
| { | ||
| SecondarySourceLocation ssl; | ||
| for (auto const& error: errors) | ||
| if (error->comment()) | ||
| ssl.append( | ||
| *error->comment(), | ||
| _assembly.location() | ||
| ); | ||
|
Comment on lines
+177
to
+183
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Alternatives would be: use an empty source location - or not emit any of this at all and just leave it at the simple warning without reporting the |
||
| m_errorReporter.warning( | ||
chriseth marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| 7828_error, | ||
| _assembly.location(), | ||
| "Inline assembly has invalid NatSpec documentation.", | ||
| ssl | ||
| ); | ||
| } | ||
|
|
||
| for (auto const& [tagName, tagValue]: docTags) | ||
| { | ||
| if (tagName == "solidity") | ||
| { | ||
| vector<string> values; | ||
| boost::split(values, tagValue.content, isWhiteSpace); | ||
|
|
||
| set<string> valuesSeen; | ||
| set<string> duplicates; | ||
| for (auto const& value: values | ranges::views::filter(not_fn(&string::empty))) | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of the 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... |
||
| if (valuesSeen.insert(value).second) | ||
| { | ||
| if (value == "memory-safe-assembly") | ||
| _assembly.annotation().markedMemorySafe = true; | ||
| else | ||
| m_errorReporter.warning( | ||
| 8787_error, | ||
| _assembly.location(), | ||
| "Unexpected value for @solidity tag in inline assembly: " + value | ||
| ); | ||
| } | ||
| else if (duplicates.insert(value).second) | ||
| m_errorReporter.warning( | ||
| 4377_error, | ||
| _assembly.location(), | ||
| "Value for @solidity tag in inline assembly specified multiple times: " + value | ||
| ); | ||
| } | ||
| else | ||
| m_errorReporter.warning( | ||
| 6269_error, | ||
| _assembly.location(), | ||
| "Unexpected NatSpec tag \"" + tagName + "\" with value \"" + tagValue.content + "\" in inline assembly." | ||
| ); | ||
| } | ||
|
|
||
| return true; | ||
| } | ||
|
|
||
| void DocStringTagParser::checkParameters( | ||
| CallableDeclaration const& _callable, | ||
| StructurallyDocumented const& _node, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -160,8 +160,8 @@ class IRGenerationContext | |
|
|
||
| std::set<ContractDefinition const*, ASTNode::CompareByID>& subObjectsCreated() { return m_subObjects; } | ||
|
|
||
| bool inlineAssemblySeen() const { return m_inlineAssemblySeen; } | ||
| void setInlineAssemblySeen() { m_inlineAssemblySeen = true; } | ||
| bool memoryUnsafeInlineAssemblySeen() const { return m_memoryUnsafeInlineAssemblySeen; } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would be nice to have a type that can be set false->true but not true->false. |
||
| void setMemoryUnsafeInlineAssemblySeen() { m_memoryUnsafeInlineAssemblySeen = true; } | ||
|
|
||
| /// @returns the runtime ID to be used for the function in the dispatch routine | ||
| /// and for internal function pointers. | ||
|
|
@@ -202,8 +202,8 @@ class IRGenerationContext | |
| /// Whether to use checked or wrapping arithmetic. | ||
| Arithmetic m_arithmetic = Arithmetic::Checked; | ||
|
|
||
| /// Flag indicating whether any inline assembly block was seen. | ||
| bool m_inlineAssemblySeen = false; | ||
| /// Flag indicating whether any memory-unsafe inline assembly block was seen. | ||
| bool m_memoryUnsafeInlineAssemblySeen = false; | ||
|
|
||
| /// Function definitions queued for code generation. They're the Solidity functions whose calls | ||
| /// were discovered by the IR generator during AST traversal. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This only applies to "via ir", does it?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In practice at least... in principle, we could use the information for old codegen as well, but we probably won't.