Yul FunctionCallFinder uses FunctionName instead of strings as search key

[clonker]

description is in the title

[cameel]

Looks like this is a bugfix so we should check if the bug had any observable effects and, if so, cover them with regression tests.

Other than that just, some minor style tweaks and simplifications.

[cameel]

Oh, so this is a bugfix rather than just a refactor?

If I understand this correctly, the old code would run over all function calls and misdetect any function called memoryguard() as the actual builtin. And this could fool the compiler into thinking the code is memory-safe and doing potentially unsafe optimizations (inlining, stack evader).

Note that, unlike all other Yul builtins, memoryguard is currently not a reserved identifier. We don't allow defining functions named like builtins so you still can't name a function like this in pure Yul, but in inline assembly it's not a builtin. Fortunately looks like we do some name mangling on functions defined in inline assembly (it will end up as usr$memoryguard()), but I don't know if we can assume this code never runs on unmangled assembly - please check.

We should also add some memory guard tests covering inline assembly with functions/variables named like that. I don't think we have any right now.

Since we have not released in a while, looks like we'll get away without a changelog entry even if it turns out it did have some user-observable effects. This bug is probably still only on develop. I think it must have been #15520/#15347 where this slipped through and we have not released that yet.

[clonker]

Huh, a fix by accident then. I was not (actively) aware that memoryguard is not a reserved identifier. If it was broken, it was broken before the builtin AST changes as well if I am interpreting this correctly. There, it just checked if the yulstring name of any function call matches "memoryguard". I'll see whether it can run on unmangled asm.

[clonker]

Don't think it's possible with inline assembly:

solidity/libsolidity/codegen/ir/IRGeneratorForStatements.cpp

Lines 81 to 91 in 5d27b0f

	yul::YulName translateIdentifier(yul::YulName _name) override
	{
	// Strictly, the dialect used by inline assembly (m_dialect) could be different
	// from the Yul dialect we are compiling to. So we are assuming here that the builtin
	// functions are identical. This should not be a problem for now since everything
	// is EVM anyway.
	if (m_dialect.findBuiltin(_name.str()))
	return _name;
	else
	return yul::YulName{"usr$" + _name.str()};
	}

As soon as it is not recognized as builtin (which would be the case for memoryguard), it gets the prefix. The compact JSON output does not contain the mangling yet, it's only added intermittently.

Other than that, this exists: libsolidity/syntaxTests/inlineAssembly/clash_with_non_reserved_pure_yul_builtin.sol. We could of course add another test in which it is shown that this is indeed mangled.

[clonker]

I have added a cmdline test that prints ir output for a contract with memoryguard identifiers. Then we can see that it indeed gets the prefix in all cases.