Skip to content

Add verbatim builtin. #11123

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

Merged
merged 3 commits into from
Apr 26, 2021
Merged

Add verbatim builtin. #11123

merged 3 commits into from
Apr 26, 2021

Conversation

chriseth
Copy link
Contributor

@chriseth chriseth commented Mar 17, 2021

Fixes #10869

This seems to work end-to-end.
Needs:

  • tests
  • figure out how the argument can be conveniently encoded in hex instead of a regular ascii string.

@chriseth chriseth requested a review from axic March 17, 2021 18:38
@chriseth chriseth force-pushed the verbatimdata branch 3 times, most recently from 861cc49 to 34a260e Compare March 17, 2021 18:53
@chriseth chriseth force-pushed the verbatimdata branch 5 times, most recently from 8fe91a8 to de3bad9 Compare April 1, 2021 15:22
@chriseth
Copy link
Contributor Author

chriseth commented Apr 1, 2021

We might need to make the side-effects a bit worse, by the way: This could use msize and so on.

@chriseth chriseth force-pushed the verbatimdata branch 2 times, most recently from 94151ac to c56a1c5 Compare April 6, 2021 16:41
@chriseth chriseth changed the base branch from develop to allowHexStringLiterals April 6, 2021 16:41
@chriseth chriseth requested review from ekpyron and mijovic April 6, 2021 16:41
@chriseth chriseth force-pushed the allowHexStringLiterals branch from a96c2b4 to dc91ba7 Compare April 7, 2021 08:40
@mijovic
Copy link
Contributor

mijovic commented Apr 8, 2021

I think Changelog item should be added.
If verbatim can be used in inline assembly block, we should add such test.
Also, can we add some simple semantic test with inline assembly, of course with opcodes known to evm?

@chriseth chriseth force-pushed the allowHexStringLiterals branch 6 times, most recently from 2545e35 to f04adde Compare April 8, 2021 13:03
@chriseth
Copy link
Contributor Author

Added the check to the optimizer test.

@chriseth
Copy link
Contributor Author

@@ -302,6 +317,9 @@ BuiltinFunctionForEVM const* EVMDialect::builtin(YulString _name) const

bool EVMDialect::reservedIdentifier(YulString _name) const
{
if (m_objectAccess)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had to re-read the actual source code (not this PR). It seems for the other builtins, we have reserved them in Yul entirely, not just for the object version.

If we do it here, are we afraid some people use verbatim* in inline assembly? We should aim to reserve it in 0.9.0 at least, if so. (Or perhaps by that time verbatim will be supported in inline assembly?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused, what is the verdict? We lave it as-is and change it in 0.9.0?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes!

@chriseth
Copy link
Contributor Author

Re-checked the EVMASM optimizer:

  • Inliner is unaffected since it ignores all blocks that breaksCSEAnalysisBlock.
  • Jumpdest remover is unaffected since it removes unreferenced jumpdests, but verbatim is not supposed to jump to an actual jumpdest
  • peephole optimizer is unaffected. It might remove the verbatim if it is unreachable, documented that.
  • block deduplicator can combine identical verbatim bytecode blocks. Documented that.
  • CSE splits at verbatim blocks and thus is unaffected.
  • Constant Optimizer is unaffected.

@hrkrshnn
Copy link
Member

hrkrshnn commented Apr 22, 2021

A test to print the side effects test/libyul/functionSideEffects/verbatim.yul throws.

{
    function f() { verbatim_0i_0o("test") }
}

Here is a stack trace:

#0  0x00007ffff5ffb492 in __cxxabiv1::__cxa_throw (obj=0x1530910, tinfo=0x14e9788 <typeinfo for std::out_of_range>, dest=0x7ffff6011070 <std::out_of_range::~out_of_range()>)
    at ../../../../libstdc++-v3/libsupc++/eh_throw.cc:78
#1  0x00007ffff5ff23f8 in std::__throw_out_of_range (__s=0x102c79e "map::at") at ../../../../../libstdc++-v3/src/c++11/functexcept.cc:82
#2  0x0000000000d13d6f in std::map<solidity::yul::YulString, std::set<solidity::yul::YulString, std::less<solidity::yul::YulString>, std::allocator<solidity::yul::YulString> >, std::less<solidity::yul::YulString>, std::allocator<std::pair<solidity::yul::YulString const, std::set<solidity::yul::YulString, std::less<solidity::yul::YulString>, std::allocator<solidity::yul::YulString> > > > >::at (this=0x7fffffffd820, __k=...) at /usr/include/c++/10/bits/stl_map.h:550
#3  0x0000000000d73c19 in operator()<solidity::yul::SideEffectsPropagator::sideEffects(const solidity::yul::Dialect&, const solidity::yul::CallGraph&)::<lambda(solidity::yul::YulString, auto:24&&)>&>(solidity::yul::YulString, struct {...} &) (__closure=0x7fffffffd700, _function=..., _recurse=...)
    at /home/hari/s/r2/libyul/optimiser/Semantics.cpp:139
#4  0x0000000000d73ffe in solidity::yul::SideEffectsPropagator::sideEffects (_dialect=..., _directCallGraph=...) at /home/hari/s/r2/libyul/optimiser/Semantics.cpp:144
#5  0x00000000005384e6 in solidity::yul::test::FunctionSideEffects::run (this=0x1532b30, _stream=..., _linePrefix="  ", _formatted=true)
    at /home/hari/s/r2/test/libyul/FunctionSideEffects.cpp:94
#6  0x0000000000437bf4 in TestTool::process (this=0x7fffffffde00) at /home/hari/s/r2/test/tools/isoltest.cpp:172
#7  0x00000000004392ef in TestTool::processPath (

The problem is https://github.com/ethereum/solidity/blob/verbatimdata/libyul/optimiser/Semantics.cpp#L139. Not sure why it should get to that branch. Also don't have a direct example that throws. (Turns out that m_objectAccess is false and m_dialect->builtin("verbatim_...") returns nullptr. Maybe only an issue during testing?)

@chriseth
Copy link
Contributor Author

@hrkrshnn what is the command you run?

@chriseth
Copy link
Contributor Author

ah, `functionSideEffects' in isoltest?

@chriseth chriseth changed the base branch from develop to fixFunctionSideEffectsDialect April 22, 2021 12:41
@chriseth
Copy link
Contributor Author

Depends on #11292

Base automatically changed from fixFunctionSideEffectsDialect to develop April 22, 2021 12:55
@chriseth chriseth force-pushed the verbatimdata branch 2 times, most recently from fe8bdfb to 40c4fea Compare April 26, 2021 12:53
hrkrshnn
hrkrshnn previously approved these changes Apr 26, 2021
@hrkrshnn
Copy link
Member

Added a couple of tests #11316

@@ -162,6 +176,9 @@ class AssemblyItem
AssemblyItemType m_type;
Instruction m_instruction; ///< Only valid if m_type == Operation
std::shared_ptr<u256> m_data; ///< Only valid if m_type != Operation
/// If m_type == VerbatimBytecode, this holds number of arguments, number of
/// return variables and verbatim bytecode.
std::optional<std::tuple<size_t, size_t, bytes>> m_verbatimBytecode;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, this got increased to three members? I think it should really be documented what they are, I assume args,rets,data?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two lines up?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New builtin literalbytecode
6 participants