Verify that bpf2bpf updates r10

[Alan-Jowett]

This pull request includes changes to the tests/call_local.data file to ensure that the stack is preserved during function calls. The changes involve writing to the stack before a function call and reading back the values after the call to check that they were not overwritten by the function.

Stack preservation checks:

• tests/call_local.data: Added lines to write to the stack before calling local func1 and read back the values after the call. This ensures that the values are preserved and not overwritten by func1.
• tests/call_local.data: Added lines to write to the stack in stdw [%r10-4], 0xCCBBAA99. This ensures that the stack writing operation does not overwrite the callee's values.

[coveralls]

coverage: 95.339%. remained the same
when pulling 34dffe2 on improve_local_call_test
into 3e528b4 on main.

[dthaler]

I don't think this PR should be merged as such as this is not an ISA requirement and this is (currently) an ISA conformance suite.

See discussion in microsoft/ebpf-for-windows#3506 (comment)

[dthaler]

I think this file tests the psABI which is not targeted for standardization, only documentation as informational, rather than the ISA which is targeted for standardization. Is bpf_conformance actually a standards conformance test suite? if so, this PR is inappropriate in my view. Or is it both standards conformance and also Linux psABI conformance? I'd recommend separating them, such as into separate test directories if so, and this change could be in the linux psabi tests directory rather than the isa tests directory.

[Alan-Jowett]

That makes sense.

[Alan-Jowett]

@dthaler I don't agree.

My position:
Just like when making calls to helper functions, when making bpf2bpf calls the assumption is that the local variables and the non-volatile registers are preserved. How exactly that is done is implementation defined, but [r10-8] in the callee and [r10-8] in the caller must either point to different locations or the run time must save the stack state and restore it.

Possible implementations:

1. r10 is adjusted by size of locals in outer.
2. r10 is adjusted to point to a brand new BPF stack.
3. The runtime saves the values stored in [r10-8] somewhere else and restored it after the call returns.

Assume the following program:

inner:
stdw [%r10-8], 2
exit
outer:
stdw [%r10-8], 1
call local inner
ldxw %r0, [%r10-8]
exit

What value should outer return? 1 or 2? I don't see that as being related to the Linux psABI, but rather a core issue of BPF ISA conformance.

[dthaler]

@dthaler I don't agree.

My position: Just like when making calls to helper functions, when making bpf2bpf calls the assumption is that the local variables and the non-volatile registers are preserved. How exactly that is done is implementation defined, but [r10-8] in the callee and [r10-8] in the caller must either point to different locations or the run time must save the stack state and restore it.

Possible implementations:

1. r10 is adjusted by size of locals in outer.
2. r10 is adjusted to point to a brand new BPF stack.
3. The runtime saves the values stored in [r10-8] somewhere else and restored it after the call returns.

Assume the following program:

inner:
stdw [%r10-8], 2
exit
outer:
stdw [%r10-8], 1
call local inner
ldxw %r0, [%r10-8]
exit

What value should outer return? 1 or 2? I don't see that as being related to the Linux psABI, but rather a core issue of BPF ISA conformance.

r10 is not defined as a stack pointer in the ISA.
It is defined as a stack pointer in the psABI.

So what should the following return?

inner:
stdw [%r1-8], 2
exit
outer:
stdw [%r1-8], 1
call local inner
ldxw %r0, [%r1-8]
exit

[Alan-Jowett]

Thanks. I didn't realize the BPF ISA doesn't reference stack at all. So should we remove all test that assume R10 is stack address?

[Alan-Jowett]

OK, I understand your point. Will close this PR.

[dthaler]

Thanks. I didn't realize the BPF ISA doesn't reference stack at all. So should we remove all test that assume R10 is stack address?

It's a good question. Right now my opinion is to keep tests for both the ISA and the Linux psABI but put the test files in separate directories and allow the conformance runner to be told which directory to use. That is, I expect tests against the psABI will be interesting once better documented, and tests that assume R10 is a stack address will be important there.

[dthaler]

BTW the very beginnings of a psABI document is at https://www.kernel.org/doc./html/latest/bpf/standardization/abi.html
Clearly most of it is missing.

As shown at https://datatracker.ietf.org/wg/bpf/about/ (fourth bullet), it should be published as an IETF RFC eventually, though as Informational [I] not a Proposed Standard [PS].