Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrong offset key is reported in E_WARNING message with function JIT #12672

Closed
pfustc opened this issue Nov 15, 2023 · 8 comments
Closed

Wrong offset key is reported in E_WARNING message with function JIT #12672

pfustc opened this issue Nov 15, 2023 · 8 comments

Comments

@pfustc
Copy link
Contributor

pfustc commented Nov 15, 2023

Description

Hi @dstogov, below bug is found with CALL VM, function JIT and release build on my Mac (M1, AArch64). So far, I haven't reproduced it on Linux AArch64.

The following code:

<?php
function test() {
  $a[6] = 0;
  echo $a[5];
}
test();
?>

with php.ini:

opcache.enable_cli=1
opcache.jit=function
opcache.jit_buffer_size=16M
opcache.jit_hot_func=1
opcache.jit_hot_loop=1
opcache.jit_hot_return=1
opcache.jit_hot_side_exit=1
zend.assertions=1

Resulted in this output:

Warning: Undefined array key 6097925520 in /Users/penli01/work/www/index.php on line 4

But I expected this output instead:

Warning: Undefined array key 5 in /Users/penli01/work/www/index.php on line 4

Wrong offset key is reported in the warning message when using function JIT. Tracing JIT is OK based on my test.

PHP Version

master @ 2ca142e

Operating System

MacOS 14.1.1

@dstogov
Copy link
Member

dstogov commented Nov 16, 2023

I guess the bug is related to code generated by zend_jit_undefined_offset_stub().

Could you please run php -d opcache.jit_debug=0x8 and check the assembler code for JIT$$undefined_offset.

You may also try to trace it in debugger (I'm not sure if GDBJIT interface works on Mac).

$ gdb --args sapi/cli/php -d opcache.jit=function -d opcache.jit_debug=0x100 test.php 
(gdb) b 'JIT$$undefined_offset'
Function "JIT$$undefined_offset" not defined.
Make breakpoint pending on future shared library load? (y or [n]) y
Breakpoint 1 ('JIT$$undefined_offset') pending.
(gdb) r
Breakpoint 1, JIT$$undefined_offset () at unknown:1
(gdb) disassemble 
Dump of assembler code for function JIT$$undefined_offset:
=> 0x0000000010a00620 <+0>:	sub    $0x8,%rsp
   0x0000000010a00624 <+4>:	mov    (%r14),%rax
   0x0000000010a00627 <+7>:	mov    0x10(%rax),%ecx
   0x0000000010a0062a <+10>:	lea    0x8(%rcx,%r14,1),%rcx
   0x0000000010a0062f <+15>:	movl   $0x1,(%rcx)
   0x0000000010a00635 <+21>:	cmpb   $0x1,0x1e(%rax)
   0x0000000010a00639 <+25>:	jne    0x10a0065e <JIT$$undefined_offset+62>
   0x0000000010a0063b <+27>:	movslq 0xc(%rax),%rcx
   0x0000000010a0063f <+31>:	add    %rcx,%rax
   0x0000000010a00642 <+34>:	mov    (%rax),%rdx
   0x0000000010a00645 <+37>:	mov    $0x2,%edi
   0x0000000010a0064a <+42>:	movabs $0x7fffe1b31fe0,%rsi
   0x0000000010a00654 <+52>:	call   0x22712bf <zend_error>
   0x0000000010a00659 <+57>:	add    $0x8,%rsp
   0x0000000010a0065d <+61>:	ret
   0x0000000010a0065e <+62>:	mov    0xc(%rax),%ecx
   0x0000000010a00661 <+65>:	lea    (%r14,%rcx,1),%rax
   0x0000000010a00665 <+69>:	jmp    0x10a00642 <JIT$$undefined_offset+34>
(gdb) si
...

@dstogov
Copy link
Member

dstogov commented Nov 16, 2023

Somehow the third argument to zend_error should be passed incorrectly or may be "variable number of arguments" are handled incorrectly. May be some difference in ABI on Mac and Lunux, or a general bug related to AArch64 ABI.

@pfustc
Copy link
Contributor Author

pfustc commented Nov 21, 2023

I guess the bug is related to code generated by zend_jit_undefined_offset_stub().

Right! Thanks for your analysis.

Could you please run php -d opcache.jit_debug=0x8 and check the assembler code for JIT$$undefined_offset.
You may also try to trace it in debugger (I'm not sure if GDBJIT interface works on Mac).

Unfortunately, there is no GDB available on AArch64 Mac. I tried with LLDB but it can not break at JIT$$undefined_offset. By inserting a brk #0 instruction in this JIT-ed stub, I get below disassembly.

(lldb) x/30i $pc-8
    0x109838820: 0xa9bf7bfd   unknown     stp    x29, x30, [sp, #-0x10]!
    0x109838824: 0x910003fd   unknown     mov    x29, sp
->  0x109838828: 0xd4200000   unknown     brk    #0
    0x10983882c: 0xf9400360   unknown     ldr    x0, [x27]
    0x109838830: 0xb9401001   unknown     ldr    w1, [x0, #0x10]
    0x109838834: 0x2a0103e1   unknown     mov    w1, w1
    0x109838838: 0x8b1b0021   unknown     add    x1, x1, x27
    0x10983883c: 0x52800022   unknown     mov    w2, #0x1
    0x109838840: 0xb9000822   unknown     str    w2, [x1, #0x8]
    0x109838844: 0x39407801   unknown     ldrb   w1, [x0, #0x1e]
    0x109838848: 0x7100043f   unknown     cmp    w1, #0x1
    0x10983884c: 0x540001e1   unknown     b.ne   0x109838888
    0x109838850: 0xb9400c01   unknown     ldr    w1, [x0, #0xc]
    0x109838854: 0x93407c21   unknown     sxtw   x1, w1
    0x109838858: 0x8b000020   unknown     add    x0, x1, x0
    0x10983885c: 0xf9400002   unknown     ldr    x2, [x0]
    0x109838860: 0xd2800040   unknown     mov    x0, #0x2
    0x109838864: 0xd29b2901   unknown     mov    x1, #0xd948
    0x109838868: 0xf2a03001   unknown     movk   x1, #0x180, lsl #16
    0x10983886c: 0xf2c00021   unknown     movk   x1, #0x1, lsl #32
    0x109838870: 0xd29a4a91   unknown     mov    x17, #0xd254
    0x109838874: 0xf2a00491   unknown     movk   x17, #0x24, lsl #16
    0x109838878: 0xf2c00031   unknown     movk   x17, #0x1, lsl #32
    0x10983887c: 0xd63f0220   unknown     blr    x17
    0x109838880: 0xa8c17bfd   unknown     ldp    x29, x30, [sp], #0x10
    0x109838884: 0xd65f03c0   unknown     ret
    0x109838888: 0xb9400c00   unknown     ldr    w0, [x0, #0xc]
    0x10983888c: 0x2a0003e0   unknown     mov    w0, w0
    0x109838890: 0x8b000360   unknown     add    x0, x27, x0
    0x109838894: 0x17fffff2   unknown     b      0x10983885c

The problem is at passing arguments to zend_error() In above JIT-ed code, we use x2 to pass the undefined offset value, but that is wrong. As zend_error() is a variadic function, on AArch64 Linux, we can pass some arguments in registers according to AAPCS64. But on AArch64 MacOS, we should pass variadic arguments through the stack (see Apple's doc).

I see my colleague has fixed the same issue (#7023) in the old JIT before. But this becomes harder to fix as the IR level is introduced. Perhaps we need to add some new IRs for va_list fisrt? WDYT?

@dstogov
Copy link
Member

dstogov commented Nov 21, 2023

@pfustc thanks for for the analyses and the references to the doc and the old patch. I suspected something like this.
I'll take care about the fix, but it'll take a wile.
I'm currently work on implementing va_start(), va_arg() support and this is related.

It would be great, if you could try to build https://github.com/dstogov/ir on Mac/AArch64, run make test and post the list of the failed tests. make TARGET=aarch64 BUILD_DIR=build_aarch64 CC=clang BUILD_CC=clang test

@pfustc
Copy link
Contributor Author

pfustc commented Nov 21, 2023

It would be great, if you could try to build https://github.com/dstogov/ir on Mac/AArch64, run make test and post the list of the failed tests. make TARGET=aarch64 BUILD_DIR=build_aarch64 CC=clang BUILD_CC=clang test

I just tried building and running the tests on both Linux/AArch64 and MacOS/AArch64 with a few Makefile changes.

All tests pass on Linux/AArch64.

There are 20 failures on MacOS/AArch64. (see below)

-------------------------------
Test Sumary
-------------------------------
Total: 1005
Passed: 369
Skipped: 616
Expected fail: 0
Failed: 20
-------------------------------
FAILED TESTS
-------------------------------
001: Argument Passing [./tests/debug.aarch64/args_001.irt]
002: Argument Passing [./tests/debug.aarch64/args_002.irt]
Simple CALL -O0 [./tests/debug.aarch64/call-O0.irt]
Simple CALL [./tests/debug.aarch64/call.irt]
CALL with parallel argument passing [./tests/debug.aarch64/call2.irt]
Simple CALL with ALLOCA [./tests/debug.aarch64/call_alloca.irt]
Simple CALL with VADDR [./tests/debug.aarch64/call_vaddr.irt]
003: Parameter Loading and argument passing [./tests/debug.aarch64/params_003.irt]
Fib [./tests/debug.aarch64/regset-fib.irt]
Fib2 [./tests/debug.aarch64/regset-fib2.irt]
FibI [./tests/debug.aarch64/regset-fibi.irt]
Simple CALL [./tests/debug.aarch64/tailcall_001.irt]
FibI [./tests/fibi_min.irt]
CTLZ 001: [./tests/run/ctlz_001.irt]
CTPOP 001: [./tests/run/ctpop_001.irt]
CTTZ 001: [./tests/run/cttz_001.irt]
Floating Point number compariosn (001: CMP edge cases) [./tests/run/fcmp_001.irt]
Floating Point number compariosn (002: CMP+COND edge cases) [./tests/run/fcmp_002.irt]
Floating Point number compariosn (003: CMP+IF edge cases) [./tests/run/fcmp_003.irt]
Floating Point number compariosn (004: CMP+IF+swap edge cases) [./tests/run/fcmp_004.irt]
-------------------------------
make: *** [test] Error 1

Some of the failed tests have function calls (calling convention related). Some seem to be caused by the difference between GCC and LLVM. And there may be other bugs.

I just created a PR (#12740) of adding code-generation for IR_TRAP on AArch64. This may help debugging the JIT-ed code on MacOS/AArch64 where GDB is not available. Could you help integrate that?

BTW: I'm seeing a typo in above test name (compariosn vs. comparison)

@pfustc
Copy link
Contributor Author

pfustc commented Nov 22, 2023

I have run above 20 failed IR testes on MacOS/AArch64 again one by one and looked into the .out log. All of them are caused by the same assertion failure.

Assertion failed: (ctx && ctx->code_buffer), function ir_add_veneer, file ir_aarch64.dasc, line 5636.

The assertion failure is caused by ctx->code_buffer being NULL in ir_add_veneer().

static int ir_add_veneer(dasm_State *Dst, void *buffer, uint32_t ins, int *b, uint32_t *cp, ptrdiff_t offset)
{
    ir_ctx *ctx = ir_current_ctx;
    const void *addr, *veneer = NULL;
    ptrdiff_t na;
    int n, m;

    IR_ASSERT(ctx && ctx->code_buffer);

@dstogov
Copy link
Member

dstogov commented Nov 22, 2023

Thanks for checking this.
I'll try to fix the veneers support for IR soon.
Now they are supported only when IR is used as a part of PHP JIT.
The fix for variable arguments passing on Mac requires significant changes and will take time.

@dstogov dstogov closed this as completed Nov 22, 2023
@dstogov dstogov reopened this Nov 22, 2023
@dstogov
Copy link
Member

dstogov commented Nov 28, 2023

This should be fixed in PHP JIT via fc1b467
The vararg support for Mac/AArch64 in IR Framework is not fixed yet.
See dstogov/ir#55

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants