fix: llvm/stack/array_allocation_variable_index.ll

[Oppen]

store_value takes index first, value after, but the main function was passing cell_1, cell_2 which is value, index.
Swap the arguments.

[hedgar2017]

store_value takes index first, value after, but the main function was passing cell_1, cell_2 which is value, index. Swap the arguments.

Hey @Oppen, does it not affect the test result?

[Oppen]

store_value takes index first, value after, but the main function was passing cell_1, cell_2 which is value, index. Swap the arguments.

Hey @Oppen, does it not affect the test result?

Not on vm2. On lambdaclass/era_vm it was failing without this fix due to an out of bounds, as it checks absolute stores and read don't try to access values above the current SP and the swapped argument led to a rather big number (which I'm not sure if we should, so maybe our VM also has a bug there).

[Oppen]

Sorry, not vm2, but the default one. I'm not sure which one it is, but I ran the tester without feature flags.

[hedgar2017]

Sorry, not vm2, but the default one. I'm not sure which one it is, but I ran the tester without feature flags.

Interesting. It has never failed in our CI.
What is your environment? Commit hashes of era-compiler-tester, era-compiler-solidity, and llvm?

[hedgar2017]

Sorry, not vm2, but the default one. I'm not sure which one it is, but I ran the tester without feature flags.

We haven't tried nor integrated lambdaclass/era_vm yet. Could you send me a link to it?
If we merge this, it will break the CI with the existing VM.

[hedgar2017]

Sorry, not vm2, but the default one. I'm not sure which one it is, but I ran the tester without feature flags.

We haven't tried nor integrated lambdaclass/era_vm yet. Could you send me a link to it? If we merge this, it will break the CI with the existing VM.

To verify this, could you kindly create a PR to https://github.com/matter-labs/era-compiler-tester with this submodule?

[Oppen]

Sorry, not vm2, but the default one. I'm not sure which one it is, but I ran the tester without feature flags.

Interesting. It has never failed in our CI. What is your environment? Commit hashes of era-compiler-tester, era-compiler-solidity, and llvm?

Sorry, not vm2, but the default one. I'm not sure which one it is, but I ran the tester without feature flags.

We haven't tried nor integrated lambdaclass/era_vm yet. Could you send me a link to it? If we merge this, it will break the CI with the existing VM.

Sorry, not vm2, but the default one. I'm not sure which one it is, but I ran the tester without feature flags.

We haven't tried nor integrated lambdaclass/era_vm yet. Could you send me a link to it? If we merge this, it will break the CI with the existing VM.

To verify this, could you kindly create a PR to https://github.com/matter-labs/era-compiler-tester with this submodule?

I know, we're working on that in a branch. We're using a modified era-compiler-tester to iron out bugs in our implementation. This is said branch on our fork.
Our changes are applied on top of 8815ef5abc5b6545578b67889b7df96a4f8025b8 from main branch.
llvm: 40b694e3348c5c10b9b429aafe0a22950656f963
contracts: 45d2c6de6366fbb162c38c2f846f5383320876e5
solidity: ce4be6e1e611cf094d9227c6c5dd843c163081a2

I'll go ahead and make the PR to the tester.
EDIT: matter-labs/era-compiler-tester#81

[hedgar2017]

@Oppen thanks, understood. Running now.

[Oppen]

BTW, I meant to ask about it. While I think this test was buggy, I'm not positive that our VM isn't bugged as well for not supporting the other order of arguments.
Is absolute addressing supposed to be able to modify and read any stack address (between the [0..2^16) range) and our check for < sp is incorrect, or the check is right and the bug was on the other VM for not checking that? I haven't found a definite answer on the spec.
In our case it returns an exception due to the failed bounds check.

[hedgar2017]

@Oppen have you guys followed this spec in implementation?
https://matter-labs.github.io/eravm-spec/spec.html

[Oppen]

@Oppen have you guys followed this spec in implementation? https://matter-labs.github.io/eravm-spec/spec.html

I can't tell for sure which document took precedence (I joined that project when it was relatively advanced), this spec or the Primer doc, but we do use it to disambiguate at least. Same for vm2, we may look at that code when in doubt.

[Oppen]

In particular, this is the bit for which my interpretation leads to adding the check:

Topmost frame of the callstack, whether internal or external, contains the stack pointer (SP) value cf_sp; which points to the address after the topmost element of the stack. That means that the topmost element of the stack is located in word number (SP−1)(SP−1) on the associated stack page.

Data stack grows towards greater addresses. In other words, pushing to stack increases stack pointer value, and popping elements decreases stack pointer.

I interpret that as SP marking the accessible parts of the stack, because of the "the topmost element" bit not making sense to me if accessing stuff after it is legal.
On the other hand, the spec doesn't explicitly mention bounds checks and vm2 does not perform them.

[hedgar2017]

Hey @sayon - could you take a look?
cc @StanislavBreadless @joonazan

[joonazan]

@Oppen @hedgar2017 vm2 is the best spec that we currently have. All writing on the VM is outdated or plain wrong. vm2 is equivalent to zk_evm in the most minute details.

And no, the stack pointer doesn't limit what is accessible. There are addressing modes that can be used to write to any part of the stack directly.

[Oppen]

And no, the stack pointer doesn't limit what is accessible. There are addressing modes that can be used to write to any part of the stack directly.

Noted. I'll fix our VM then. Could the test still be reviewed? I still think the intent in the name and later load points to the arguments being swapped. It would be a good idea to add a test that intentionally tests any address in the stack is writable, too.

[hedgar2017]

And no, the stack pointer doesn't limit what is accessible. There are addressing modes that can be used to write to any part of the stack directly.

Noted. I'll fix our VM then. Could the test still be reviewed? I still think the intent in the name and later load points to the arguments being swapped. It would be a good idea to add a test that intentionally tests any address in the stack is writable, too.

Sure, we'll merge it!
Feel free to add any tests you see appropriate. We'll appreciate it!