Fix inconsistent negative subassembly indices between different sizeof(size_t)

[clonker]

When computing object ids for referencing subassemblies, these ids are currently determined as negative DFS enumeration based on size_t:

solidity/libevmasm/Assembly.cpp

Line 1825 in a652292

size_t objectId = std::numeric_limits<size_t>::max() - m_subPaths.size();

This PR changes this to uint64_t, so that assembly text - in particular PUSH #[$] instructions - is consistent over different type sizes of size_t.

Since these indices are stored in a map and later referenced for exporting and importing, as well as (currently) numeric_limits<size_t>::max() is used to indicate the root object or an empty state which is used in, eg, various asserts, I found it helpful to wrap the uint64_t into a struct so that these places become apparent and dealing with the sizes is explicit. This caused a whole avalanche of changes, as such object ids (or more generally: SubAssemblyIDs) are used in many places. In particular, also yul::Object::subIds are subject to the type.

We were lacking a cmdline test that has non-zero PUSH #[$]es, so I added one. Since emscripten builds do not execute them, we have no direct regression test by this but I have added one to the solc-js tests. While doing that I realized that it is currently not possible (to me at least :P) to request ASM json output via standard json interface if the language is Yul.

Fixes #15953

[r0qs]

While doing that I realized that it is currently not possible (to me at least :P) to request ASM json output via standard json interface if the language is Yul.

Isn't that the evm.assembly option?

{
	"language": "Yul",
	"sources":
	{
		"C":
		{
			"content": "{ let x := mload(0) sstore(add(x, 0), 0) }"
		}
	},
	"settings":
	{
		"outputSelection":
		{
			"*": { "*": ["evm.assembly"], "": [ "*" ] }
		}
	}
}

[clonker]

Isn't that the evm.assembly option?

The equivalent that is showing the PUSH #[$]es would be evm.legacyAssembly

[cameel]

Oh, it's the first time I see assemblies referred to as subroutines. It really stretches the definition. From Wikipedia:

In computer programming, a function (also procedure, method, subroutine, routine, or subprogram) is a callable unit[1] of software logic that has a well-defined interface and behavior and can be invoked multiple times.

It's going to become very confusing with EOF, where we have both functions (code sections) and assemblies (containers). I'd by default interpret "subroutine" to mean to former (there was even a competing "simple subroutines" EIP). We should really rename this at some point.

[cameel]

Ugh, I thought I understood what's happening and then I ran into this. WTF are we doing? It just makes my head hurt. If I understand correctly:

• Assembly keeps track of chunks of data and subassemblies separately.
  • Assemblies are stored in m_subs vector.
  • Data is stored by hash in m_data.
    • Except for metadata, which is singled out and stored in m_auxiliaryData.
• EthAssemblyAdapter holds the assembly and independently keeps track of all the appended data chunks in m_dataHashBySubId.
  • It gives them fake IDs in the upper half of the subassembly ID space
• When a Yul object is being compiled, EVMObjectCompiler adds subassemblies and data chunks via the adapter, receiving both real and fake IDs.
  • It stores the IDs in BuiltinContext, each one associated with a Yul node by name.
  • This is done only for children of the object. Anything nested deeper does not get an ID.
• BuiltinContext is passed to Yul code transform and later to builtins defined in EVMDialect.
  • Builtins like datasize then use BuiltinContext to map Yul node names to sub IDs.
    • If the name is present in BuiltinContext, they take the ID stored there.
    • If not, they assume the name is a dotted path and convert it to a sequence of IDs.
      • BTW, this is a pretty questionable assumption: #13794, #15540.
    • In either case the sequence is passed into EthAssemblyAdapter::appendDataSize().
      • It assumes that if the first ID in the sequence is present in m_dataHashBySubId, it must be a fake ID assigned to a data chunk.
      • Otherwise it converts the path to a sub ID using Assembly::encodeSubPath() and assumes it must be a subassembly.
        • There seems to be an assumption that Yul Object tree structure matches the Assembly tree, which I'm not sure is true. For example we sometimes we move things (e.g. long Yul strings) to data chunks and the transition happens in evmasm optimizer, which means that they do not have corresponding data nodes at Yul level.
      • I actually don't understand how paths to nested data are handled. I don't think they're are added to BuiltinContext, but then how do they not end up treated as assemblies by appendDataSize()?

Overall, looks like we have several kinds of sub IDs sharing the same ID space, without any checks for conflicts (just assuming the space is large enough that they won't happen):

• Normal IDs assigned to subassemblies, starting at 0.
  • EOF ContainerIDs are a subset of this and are limited to uint8_t.
• Empty ID at max().
• Fake data IDs starting at max() / 2.
• "Negative" IDs representing deeply nested paths, starting at max() and going down.

I think we should assign a non-overlapping region to each of them and have asserts enforcing that.

The way we assign IDs should also be more prominently documented. For example m_subPaths does not even say that the "negative" IDs mean.

[clonker]

Sounds like a good idea to define non-overlapping regions. That way each of the regions can also be independently and cleanly documented. Perhaps in a follow up?
Regarding documenting subpaths and negative IDs: @r0qs had written something up about it and iirc wanted to add it in some form to the documentation.

[r0qs]

Sounds like a good idea to define non-overlapping regions. That way each of the regions can also be independently and cleanly documented. Perhaps in a follow up? Regarding documenting subpaths and negative IDs: @r0qs had written something up about it and iirc wanted to add it in some form to the documentation.

Yes, it is here: https://github.com/ethereum/solidity/wiki/Assembly-Indices

[clonker]

I have added a comment to m_subPaths that the subassembly IDs referenced there occupy the negative index space. I think a more thorough documentation of things should occur once we have defined non-overlapping regions. Then it's also straightforward where to put the documentation.

[cameel]

@r0qs I see that the doc does not mention the fake data IDs. Not sure if they can ever show up in assembly output, but even if they can't, they still affect it, because we can never safely allocate other kinds of IDs in the region they take up. So they effectively limit the range of those other kinds IDs.

[r0qs]

Indeed, that was not the initial intention of such documentation. However, I can extend it to cover your suggestion of defining non-overlapping regions for IDs.

[cameel]

We use size_t for tag ID but that should really be uint64_t as well. Would be nice to change that too at some point.

The tag+sub pair would also be better off as a proper struct with methods for converting to/from u256. We wouldn't then have to hard-code all those masks and 64s all over the place.

[clonker]

Agree, it would also make it easier to reason about the whole program and data flow.

[cameel]

Main things needed here are to replace validations with asserts and adjust tests a bit. Other than that mostly minor stuff.

This should be enough for the fix, but it needs to be followed by a PR that properly splits the index space into regions (#15955 (comment)) and documents it. And introduce a type for the tag ID (#15955 (comment)).

[cameel]

Suggested change

	for (SubAssemblyID subID {0}; subID.value < m_subs.size(); ++subID.value)
	for (SubAssemblyID subID{0}; subID.value < m_subs.size(); ++subID.value)

Also in AssemblyItem::splitForeignPushTag().

[clonker]

I really wish we could just settle on one style that has clang format support (or find a close enough approximation of the current style) and be done with it :) the amount of time and energy things like this eat up is unreasonable

[cameel]

I think that this one as well is just a general assumption rather than something that we expect to fail at runtime.

[cameel]

These tags were actually always confusing to me, because I could not see in what situation we'd ever jump into another subassembly, so I dug into this now to find out. Turns out we never do. This mechanism seems to exist only to support internal function pointers in legacy codegen. Since we support storage variables of such pointer types and storage can be accessed both at runtime and at creation time, we actually combine two pointers in the slot: to a copy of the function that exists in the creation assembly and to one in the runtime assembly. This not new to me, but I only now realized that to support this we have to be able to refer to tags from the runtime assembly inside the creation assembly. I think that's the only situation where they'll appear.

Here's what it looks like in practice:

contract C {
    function() fptr = f;
    function f() internal {}
}

EVM assembly:
  mstore(0x40, 0x80)
  or(tag_0_2, shl(0x20, tag_1))
  0x00
  ...
  sstore
  ...
tag_1:
  jump  // out
  ...
stop

sub_0: assembly {
    ...
    tag_2:
      jump      // out
    ...
}

Note that there's no tag named tag_0_2. It refers to tag_2 in sub_0. I don't think we ever insert such tags into the assembly item stream (and that's what the assertThrow() is guarding against). We only push them on the stack using PushTag, but this does not trigger the assert.

Such tags could, of course, appear in assembly import - we should have a validation against them there - but here it should be only an assert.

[cameel]

Based on my investigation, I think we could limit it to just PushTag:

Suggested change

	solAssert(m_type == PushTag || m_type == Tag || m_type == RelativeJump || m_type == ConditionalRelativeJump);
	solAssert(m_type == PushTag);

And similarly in setPushTagSubIdAndTag() and toSubAssemblyTag().

Though maybe it's better done in a separate PR. There could be other places like this and it would be good to also have assembly import validations against it.

[clonker]

Makes sense, thanks for investigating this!

[cameel]

Why not just import SubassemblyID.h and use the type directly?

[clonker]

I did it this way because they are used as argument to an method in AbstractAssembly - which expects AbstractAssembly::SubID. We could of course get rid of that typedef entirely.

[cameel]

Suggested change

	function containsTarget(obj, target) {
	function containsAssemblyItem(assemblyJSON, item) {

Also, I think it would be best to limit the search to the legacyAssembly subobject. It seems that now you're searching the whole object and there could be random stuff there. It would also make this check easier to understand - it took me a moment to understand what's happening here, because this is not at all what I expected.

[cameel]

Also as a general remark for the future - we should at some point create a CI job that runs the emscripten binary with those tests that provide Standard JSON input/output. It would make this whole JS mess unnecessary...

[clonker]

Also, I think it would be best to limit the search to the legacyAssembly subobject. It seems that now you're searching the whole object and there could be random stuff there. It would also make this check easier to understand - it took me a moment to understand what's happening here, because this is not at all what I expected.

Fair enough, what did you expect then? In my mind it was fairly straightforward to search the result json for some target object and it also seemed reasonably unlikely to me to find the target in our specific case anywhere else in the output. It is not big deal in any case, I am changing it to only consider the legacyAssembly subobject.

Also as a general remark for the future - we should at some point create a CI job that runs the emscripten binary with those tests that provide Standard JSON input/output. It would make this whole JS mess unnecessary...

Yes please!!

[cameel]

This is not just the interface:

Suggested change

	* Commandline Interface: Fix possible inconsistency in subassembly IDs between target architectures in `--asm-json` output.
	* Assembler: Fix different IDs being assigned to subassemblies nested more than one level away, resulting in inconsistent `--asm-json` output between target architectures.

[cameel]

Also, this needs to be moved to 0.8.31 now.

[clonker]

This is not just the interface:

Well yeah, although assuming the ID space isn't exhausted, it does behave consistently inside of the assembler. I don't mind either way so let's go with your phrasing. Although I would minimally amend it:

* Assembler: Fix not using a fixed-width type for IDs being assigned to subassemblies nested more than one level away, resulting in inconsistent --asm-json output between target architectures.