ABIEncoderV2: Implement calldata structs without dynamically encoded members. #5936

ekpyron · 2019-02-05T19:31:53Z

Part of #1603.

codecov · 2019-02-05T20:05:16Z

Codecov Report

❗ No coverage uploaded for pull request base (develop@9460701). Click here to learn what that means.
The diff coverage is 93.45%.

@@            Coverage Diff             @@
##             develop    #5936   +/-   ##
==========================================
  Coverage           ?   88.37%           
==========================================
  Files              ?      359           
  Lines              ?    34309           
  Branches           ?     4064           
==========================================
  Hits               ?    30320           
  Misses             ?     2616           
  Partials           ?     1373

Flag	Coverage Δ
#all	`88.37% <93.45%> (?)`
#syntax	`27.93% <12.14%> (?)`

chriseth · 2019-02-05T20:09:19Z

libsolidity/ast/Types.h

@@ -827,6 +827,8 @@ class StructType: public ReferenceType
 	std::string richIdentifier() const override;
 	bool operator==(Type const& _other) const override;
 	unsigned calldataEncodedSize(bool _padded) const override;
+	// Offset of member in calldata. Does not work for recursive or dynamically encoded types.
+	unsigned calldataOffsetOfMember(std::string const& _name) const;


Wouldn't this better fit closer to memoryOffsetOfMember and storageOffsetOfMember?

chriseth · 2019-02-05T20:10:30Z

libsolidity/codegen/ABIFunctions.cpp

+	{
+		if (structType->dataStoredIn(DataLocation::CallData))
+		{
+			solAssert(!_fromMemory, "");


Can it happen that dataStoredIn(DataLocation::Memory) is true, but _fromMemory is false?

I think so - if we want to decode something in calldata into memory, I think.

libsolidity/codegen/ABIFunctions.cpp

libsolidity/codegen/CompilerUtils.cpp

chriseth · 2019-02-05T20:21:51Z

libsolidity/codegen/CompilerUtils.cpp

+				m_context << Instruction::DUP1;
+				m_context << typeOnStack.calldataEncodedSize(true);
+				m_context << Instruction::ADD;
+				abiDecode({targetType.shared_from_this()}, false);


Please check that this properly allocates memory.

Ah, but this might need calldata size checks, at least for dynamically-sized structs.

Yep - adjusted this (and the same for the other nested abiDecode call) in a new commit - should be cheaper with CALLDATASIZE and SUB anyways.

chriseth · 2019-02-05T20:22:33Z

libsolidity/codegen/ExpressionCompiler.cpp

+		{
+			solUnimplementedAssert(!type.isDynamicallyEncoded(), "");
+			m_context << type.calldataOffsetOfMember(member) << Instruction::ADD;
+			if (_memberAccess.annotation().type->isValueType())


Does this work for arrays?

Please also add explicit assertions for every non-value type this is designed to work with.

For every non-value type it should be fine to just return the calldata offset - I think the TypeChecker shouldn't allow anything that will actually try to further decode, if this is an array and if so the the actual decoding of the calldata array should have the unimplemented assertion.

Still better to assert all types we know are working, so that it automatically breaks if we introduce new types.

libsolidity/codegen/ExpressionCompiler.cpp

test/libsolidity/syntaxTests/structs/calldata_assign.sol

libsolidity/codegen/ExpressionCompiler.cpp

chriseth · 2019-02-07T12:10:03Z

libsolidity/codegen/ExpressionCompiler.cpp

+				m_context << Instruction::DUP1;
+				m_context << type.calldataEncodedSize(true);
+				m_context << Instruction::ADD;
+				CompilerUtils(m_context).abiDecode({_memberAccess.annotation().type}, false);


For uint256, this will turn into an assembly function call where the function only consists of a single calldataload, which cannot be inlined by the current optimizer. This is quite inefficient. Should we make a small exception for types where storageBytes is 32 and call loadFromMemoryDynamic as above? Actually loadFromMemoryDynamic also performs range checks. So another solution would be to update CompilerUtils::convertType( together with #5815

storageBytes is probably right, but feels a bit off... calldataEncodedSize(false) should do it as well, shouldn't it? And I'm wondering whether this actually needs to range-check that we're inside calldatasize or whether we can just use calldataload directly if calldataEncodedSize(false)==32... probably enough for the statically encoded case, right?

We do not need any calldatasize-range checks here for any value type. Using calldataEncodedSize is better, yes. And you can just use calldataload, yes :)

Or... we could just always call CompilerUtils(m_context).loadFromMemoryDynamic(*_memberAccess.annotation().type, true, false, false) here, that should do it and degenerate to a single calldataload whenever possible - I guess that's what you meant with updating CompilerUtils::convertType( together with #5815, so that (at least in this case) loadFromMemoryDynamic will validate instead of clean?

Yes, exactly.

chriseth · 2019-02-07T12:10:56Z

Are calldata size checks properly enforced here? I think you already answered that question when I meant something else ;)

ekpyron · 2019-02-07T12:17:06Z

@chriseth Actually I thought they were, but now I'm not sure anymore, I'll recheck and make sure!

chriseth · 2019-02-07T12:43:23Z

If we limit the scope of this PR to statically encoded structs, the only things left to do in my opinion are:

try to get uint member access a bit more efficient.
ssert that member access for non-value types is either a struct or an array.

chriseth · 2019-02-07T13:34:46Z

libsolidity/codegen/ExpressionCompiler.cpp

+			if (_memberAccess.annotation().type->isValueType())
+			{
+				solAssert(_memberAccess.annotation().type->calldataEncodedSize(false) > 0, "");
+				CompilerUtils(m_context).loadFromMemoryDynamic(*_memberAccess.annotation().type, true, false, false);


Suggested change

CompilerUtils(m_context).loadFromMemoryDynamic(*_memberAccess.annotation().type, true, false, false);

CompilerUtils(m_context).loadFromMemoryDynamic(*_memberAccess.annotation().type, true, true, false);

(struct members are always padded)

I don't think so - this will end up in CompilerUtils.cpp#L1261 and will make sure everything with calldataEncodedSize(false) != 32 will be cleaned (or at some point validated I guess).

I was in fact wrong, it will trigger the shifting in loadFromMemoryHelper - which we may in fact not want here. I'll check and recheck with some more tests, but I think you're right in that it should be true here.

chriseth · 2019-02-07T13:37:29Z

libsolidity/codegen/ExpressionCompiler.cpp

+				CompilerUtils(m_context).loadFromMemoryDynamic(*_memberAccess.annotation().type, true, false, false);
+			}
+			else
+				solUnimplementedAssert(


Suggested change

solUnimplementedAssert(

solAssert(

Are you planning on implemented anything else apart from arrays and structs? :)

This can only happen for something that doesn't exist yet anyways and if we add something that could fail here, then handling that is unimplemented, right :-)? At least that was the idea - I can change to an assert, though, I don't mind either :-).

chriseth

Needs tests for calldata structs with short calldata and dirty higher order bits.

ekpyron · 2019-02-07T16:01:43Z

test/libsolidity/SolidityEndToEndTest.cpp

+    // double check that the valid case goes through
+	ABI_CHECK(callContractFunction("f((uint256,uint256))", u256(1), u256(2)), encodeArgs(0x44));
+
+	ABI_CHECK(callContractFunctionNoEncoding("f((uint256,uint256))", bytes(63,0)), encodeArgs());


@chriseth Sorry, I force pushed your comment away - but this is missing one byte, that's why it should revert.

chriseth · 2019-02-07T16:16:29Z

Looks good, please squash!

ekpyron · 2019-02-07T16:46:15Z

Still needs more tests, I think.

chriseth · 2019-02-07T16:47:42Z

Hm, perhaps a struct containing an external function pointer - those are always a little nasty.

ekpyron · 2019-02-07T16:56:00Z

Also abi.decode lacks tests (for calldata structs) so far, resp. doesn't support calldata structs, but should.

ekpyron · 2019-02-11T10:23:11Z

Added another test case for function pointers. I think it makes more sense to keep abi.decode as is for now (i.e. always copy to memory) and change it, when we also have calldata arrays, internal calladata calls and local calldata variables.

Everything should be addressed.

…members.

ekpyron · 2019-02-11T14:53:53Z

~~Do not merge yet - I may have missed one case in the TypeChecker - just verifying.~~ Nevermind, should be fine.

ekpyron requested a review from erak February 5, 2019 19:33

chriseth reviewed Feb 5, 2019

View reviewed changes

libsolidity/codegen/ABIFunctions.cpp Show resolved Hide resolved

chriseth reviewed Feb 5, 2019

View reviewed changes

libsolidity/codegen/ABIFunctions.cpp Show resolved Hide resolved

chriseth suggested changes Feb 5, 2019

View reviewed changes

ekpyron force-pushed the calldataStructsV2 branch from d25da98 to 3c7816d Compare February 6, 2019 16:14

ekpyron commented Feb 7, 2019

View reviewed changes

libsolidity/codegen/ExpressionCompiler.cpp Outdated Show resolved Hide resolved

chriseth reviewed Feb 7, 2019

View reviewed changes

chriseth previously requested changes Feb 7, 2019

View reviewed changes

ekpyron mentioned this pull request Feb 7, 2019

Strict abi decoder (validate incoming data instead of cleaning it) #5815

Merged

3 tasks

ekpyron force-pushed the calldataStructsV2 branch from b92b120 to b2169e2 Compare February 7, 2019 16:00

ekpyron commented Feb 7, 2019

View reviewed changes

ekpyron force-pushed the calldataStructsV2 branch from b2169e2 to ed256af Compare February 7, 2019 16:45

ekpyron force-pushed the calldataStructsV2 branch from ed256af to c5d6a86 Compare February 7, 2019 17:46

ekpyron changed the title ~~[WIP] ABIEncoderV2: Implement calldata structs without dynamically encoded members.~~ ABIEncoderV2: Implement calldata structs without dynamically encoded members. Feb 11, 2019

ekpyron force-pushed the calldataStructsV2 branch from 6b04f98 to 962fd4c Compare February 11, 2019 10:20

ABIEncoderV2: Implement calldata structs without dynamically encoded …

0e4912a

…members.

ekpyron force-pushed the calldataStructsV2 branch from 962fd4c to 0e4912a Compare February 11, 2019 14:47

chriseth approved these changes Feb 11, 2019

View reviewed changes

ekpyron changed the title ~~ABIEncoderV2: Implement calldata structs without dynamically encoded members.~~ [WIP] ABIEncoderV2: Implement calldata structs without dynamically encoded members. Feb 11, 2019

ekpyron changed the title ~~[WIP] ABIEncoderV2: Implement calldata structs without dynamically encoded members.~~ ABIEncoderV2: Implement calldata structs without dynamically encoded members. Feb 11, 2019

chriseth merged commit 92cb6cb into develop Feb 11, 2019

ekpyron deleted the calldataStructsV2 branch February 11, 2019 15:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ABIEncoderV2: Implement calldata structs without dynamically encoded members. #5936

ABIEncoderV2: Implement calldata structs without dynamically encoded members. #5936

ekpyron commented Feb 5, 2019

codecov bot commented Feb 5, 2019 •

edited

Loading

chriseth Feb 5, 2019

chriseth Feb 5, 2019

ekpyron Feb 6, 2019

chriseth Feb 5, 2019

chriseth Feb 7, 2019

ekpyron Feb 7, 2019

chriseth Feb 5, 2019

chriseth Feb 5, 2019

ekpyron Feb 6, 2019

chriseth Feb 7, 2019

chriseth Feb 7, 2019

ekpyron Feb 7, 2019

chriseth Feb 7, 2019

ekpyron Feb 7, 2019

chriseth Feb 7, 2019

chriseth commented Feb 7, 2019

ekpyron commented Feb 7, 2019

chriseth commented Feb 7, 2019

chriseth Feb 7, 2019

ekpyron Feb 7, 2019

ekpyron Feb 7, 2019

chriseth Feb 7, 2019

ekpyron Feb 7, 2019

chriseth left a comment

ekpyron Feb 7, 2019

chriseth commented Feb 7, 2019

ekpyron commented Feb 7, 2019

chriseth commented Feb 7, 2019

ekpyron commented Feb 7, 2019 •

edited

Loading

ekpyron commented Feb 11, 2019

ekpyron commented Feb 11, 2019 •

edited

Loading

	CompilerUtils(m_context).loadFromMemoryDynamic(*_memberAccess.annotation().type, true, false, false);
	CompilerUtils(m_context).loadFromMemoryDynamic(*_memberAccess.annotation().type, true, true, false);

ABIEncoderV2: Implement calldata structs without dynamically encoded members. #5936

ABIEncoderV2: Implement calldata structs without dynamically encoded members. #5936

Conversation

ekpyron commented Feb 5, 2019

codecov bot commented Feb 5, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriseth commented Feb 7, 2019

ekpyron commented Feb 7, 2019

chriseth commented Feb 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriseth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriseth commented Feb 7, 2019

ekpyron commented Feb 7, 2019

chriseth commented Feb 7, 2019

ekpyron commented Feb 7, 2019 • edited Loading

ekpyron commented Feb 11, 2019

ekpyron commented Feb 11, 2019 • edited Loading

codecov bot commented Feb 5, 2019 •

edited

Loading

ekpyron commented Feb 7, 2019 •

edited

Loading

ekpyron commented Feb 11, 2019 •

edited

Loading