Do not allocate memory on reverts with errors of small static encoding size.

[ekpyron]

I guess lack of micro-optimization like this is what drives people to aggressively resort to inline assembly like in this case seen in #13149

[ekpyron]

ir-optimize-evm+yul

project	bytecode_size	deployment_gas	method_gas
brink	-0.1% ✅
colony	0%
elementfi	0%
ens	0%	-0.71% ✅	0%
gnosis
gp2	0%	+0%	+0%
perpetual-pools	0%	-0%	+0.01% ❌
prb-math	-0.98% ✅	-0.97% ✅	0%
uniswap	0%	-0%	-0%
yield_liquidator	0%	0%	0%
zeppelin	-0.02% ✅	-0.02% ✅	-0%

This actually saves 1% code size on prb-math...

[ekpyron]

Looking and thinking through the possible cases here again, this might still be problematic for this case:

error E(bytes32 x);
contract C {
    bytes s;
    function f() public {
        revert E(bytes32(s));
    }
}

since the abi encoding from storage bytes to fixed bytes may involve temporary hasing in scartch space - given that, I'll postpone merging this until after the release pending a closer look.

[cameel]

Everything seems fine here.

Only needs to be rebased and have the changelog entry moved to the new version.

[cameel]

So, just to make sure I understand why this is memory safe:

• It's ok to overwrite free mem pointer because we're reverting and not allocating anything before then.
• It's ok to overwrite the zero slot since the condition above excludes any empty dynamic arrays passed in as arguments.
• It's not ok to overwrite the area where variables moved to memory might be because one of them might be <pos> or <end>.

Is that correct?

[ekpyron]

Yes, all that's true - but the restriction of types is in the current version not strong enough to exclude all uses of the scratch space during the encoding step, so that still needs fixing (restricting to value types for both the types of the arguments and the types of the error signature should do that - pending a closer look, though).

[cameel]

Ah, wait, I also need to think through the cases where the scratch space would be overwritten. So maybe not ok yet (but at least I did not notice any problems directly in the implementation).

[ekpyron]

Yeah, it's not fine as it stands - but I actually wanted to use this PR for a "teach how to review codegen for correctness" session, so I haven't fixed it yet - but not sure we'll have the time for it.
I'll draft it for now in any case.

[cameel]

Looking and thinking through the possible cases here again, this might still be problematic for this case:

Anything involving type conversions seems safe because the conversion is performed before the revert function (at least on 0.8.20):

/// @src 0:97:98  "s"
let _1_slot := 0x00
let expr_12_slot := _1_slot
/// @src 0:89:99  "bytes32(s)"
let expr_13 := convert_bytes_to_fixedbytes_from_t_bytes_storage_to_t_bytes32(expr_12_slot)
/// @src 0:87:100  "E(bytes32(s))"
{
    let _2 := allocate_unbounded()
    mstore(_2, 8203157970705126157628513801238240545114148153221041971593256941893860720640)
    let _3 := abi_encode_tuple_t_bytes32__to_t_bytes32__fromStack(add(_2, 4) , expr_13)
    revert(_2, sub(_3, _2))
}

So the only problems could come from the encoding. With parameters being restricted to value types I guess this excludes any possibility of it messing with the scratch space?

[ekpyron]

It's possible that the current version is already safe since we don't have implicit conversion from bytes to fixed bytes and the conversion will thereby happen before the revert snipped, yes. It's safer to restrict further in any case.

Actually, I'm looking forward to doing this in-language, then we could just encode the safety of stuff like this in the type system.

[cameel]

Ok, maybe better to be too careful. In any case, I tried a bunch of cases but did not came up with anything that could break it. For example:

• We have implicit conversion from string literals to bytes32 but limited to 32-byte strings so we're covered.
• I also played with calldata or storage values but to access a value type it must be in an array/slice/struct/mapping and you must use indexing or member access, which happens before encoding.
• We don't have pointers to value types so that's out too (and you'd have to artificially make it point at scratch space to mess with this mechanism anyway).
• Selectors are hashed at compilation time so not an issue either.

Just in case I rechecked which types are marked as value types in Types.h and the only one that looks suspicious is InaccessibleDynamic, but I doubt we have to worry about that one.

Looking at ABIFunctions::tupleEncoder() and the encoding helpers it calls, but it seems that with value types we're skipping all the cases where something weird could happen. Though to be honest, this is a sprawling set of functions and I don't know all its nook and crannies so there could always be something obscure and counterintuitive that I missed.

[ekpyron]

Yeah - that's why my plan was to screenshare on this PR and walk through what to look for and think through during such a review - maybe we'll still find time for it eventually, let's see :-). But yeah, especially with restricting to value types for the argument types as well should be safe.

[chriseth]

We could merge a version in case there are no arguments at all.

[cameel]

Would be good to get a fresh set of benchmarks from external tests. I'd expect the PR to have a lot more impact now that it's on top of #15058.

[cameel]

The code looks fine now, but review fixes to be squashed.

While at it, would be nice to update the test expectations in the first commit, because they seem out of date. I was hoping to see the effect on gas in the diff, but that commit seems to have been generated even before #14843 so it's useless. It should be updated or squashed.

Finally, what's the impact on external tests? I really expected there to be more of it with recent changes.

[nikola-matic]

Finally, what's the impact on external tests? I really expected there to be more of it with recent changes.

I wouldn't be surprised if it had very little impact, as I'd assume people would have been reverting small errors via assembly (which is why we even implemented this optimization).

[cameel]

Is this really the cost before optimization as the description says? i don't see it being updated in the other commit so if this was the case, CI would be failing.

[nikola-matic]

No, it was my bad, I ended up fixing up the first commit instead of the second and then squashed. This was the before and after:

  Running via Yul: 
  Expected result:
  // f() -> FAILURE, hex"92bbf6e8"
  // gas irOptimized: 222533
  // gas irOptimized code: 43200
  // gas legacy: 233746
  // gas legacy code: 37400
  // gas legacyOptimized: 224864
  // gas legacyOptimized code: 34200

  Obtained result:
  // f() -> FAILURE, hex"92bbf6e8"
  // gas irOptimized: 221918 [-615 (+0%)]
  // gas irOptimized code: 42800 [-400 (+0%)]
  // gas legacy: 233746
  // gas legacy code: 37400
  // gas legacyOptimized: 224864
  // gas legacyOptimized code: 34200

[cameel]

ok. That's a nice improvement.

But you should still at least update the commit description if you're not going to fix it in the commit. Or just squash it, because if it's wrong, it's worse than useless.

[nikola-matic]

Agreed. I'll just squash everything into one comment, and leave this comment here in case someone is curious and stumbles upon it.