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

s390x: illegal instruction in unit tests #47064

Closed
AdamMajer opened this issue Mar 13, 2023 · 13 comments · Fixed by #49401
Closed

s390x: illegal instruction in unit tests #47064

AdamMajer opened this issue Mar 13, 2023 · 13 comments · Fixed by #49401
Labels
s390 Issues and PRs related to the s390 architecture.

Comments

@AdamMajer
Copy link
Contributor

Version

19.6.0

Platform

No response

Subsystem

v8

What steps will reproduce the bug?

No response

How often does it reproduce? Is there a required condition?

Every time

What is the expected behavior?

No errors

What do you see instead?

Compiling with mostly default options results in some unit tests failures in file system tests. All these ended up with <Builtins_MulHandler+868> on z13 while running just fine on z15 machine.

For example, when executing test/parallel/test-fs-cp.mjs or test/parallel/test-fs-stat-bigint.js

I've then added --v8-enable-object-print --v8-non-optimized-debug --v8-with-dchecks to the configure and rebuilt. During the build, the following error popped up

Program terminated with signal SIGILL, Illegal instruction.
#0  0x000002aa411276a8 in Builtins_MulHandler () at ../../deps/v8/src/builtins/cast.tq:72
72      ../../deps/v8/src/builtins/cast.tq: No such file or directory.
[Current thread is 1 (Thread 0x3ffa92f2840 (LWP 19049))]
(gdb) bt
#0  0x000002aa411276a8 in Builtins_MulHandler () at ../../deps/v8/src/builtins/cast.tq:72
#1  0x000002aa40f20d86 in Builtins_InterpreterEntryTrampoline () at ../../deps/v8/src/builtins/torque-internal.tq:236
PC not saved

which points to,

@export
macro IsHeapNumber(o: HeapObject): bool {
return Is(o);
}

And additionally adding --v8-enable-short-builtin-calls to the configure, I get an error in the macro assembler for s390 where unreachable code is apparently reached.

#
# Fatal error in ../deps/v8/src/codegen/s390/macro-assembler-s390.cc, line 500
# unreachable code
#
#
#
#FailureMessage Object: 0x3ffcf8fbbf0
 1: 0x2aa3ca2386a  [/home/abuild/rpmbuild/BUILD/node-v19.6.0/out/Release/node_mksnapshot]
 2: 0x2aa3db0125e V8_Fatal(char const*, int, char const*, ...) [/home/abuild/rpmbuild/BUILD/node-v19.6.0/out/Release/node_mksnapshot]
 3: 0x2aa3d612d38 v8::internal::TurboAssembler::Call(v8::internal::Handle<v8::internal::Code>, v8::internal::RelocInfo::Mode, v8::internal::Condition) [/home/abuild/rpmbuild/BUILD/node-v19.6.0/out/Release/node_mksnapshot]
 4: 0x2aa3d72d8f6 v8::internal::baseline::BaselineCompiler::Prologue() [/home/abuild/rpmbuild/BUILD/node-v19.6.0/out/Release/node_mksnapshot]
 5: 0x2aa3d734a42 v8::internal::baseline::BaselineCompiler::GenerateCode() [/home/abuild/rpmbuild/BUILD/node-v19.6.0/out/Release/node_mksnapshot]
 6: 0x2aa3d735624 v8::internal::GenerateBaselineCode(v8::internal::Isolate*, v8::internal::Handle<v8::internal::SharedFunctionInfo>) [/home/abuild/rpmbuild/BUILD/node-v19.6.0/out/Release/node_mksnapshot]
 7: 0x2aa3ccc70dc v8::internal::Compiler::CompileSharedWithBaseline(v8::internal::Isolate*, v8::internal::Handle<v8::internal::SharedFunctionInfo>, v8::internal::Compiler::ClearExceptionFlag, v8::internal::IsCompiledScope*) [/home/abuild/rpmbuild/BUILD/node-v19.6.0/out/Release/node_mksnapshot]
 8: 0x2aa3ccc99a2 v8::internal::Compiler::CompileBaseline(v8::internal::Isolate*, v8::internal::Handle<v8::internal::JSFunction>, v8::internal::Compiler::ClearExceptionFlag, v8::internal::IsCompiledScope*) [/home/abuild/rpmbuild/BUILD/node-v19.6.0/out/Release/node_mksnapshot]
 9: 0x2aa3d719d3c v8::internal::baseline::BaselineBatchCompiler::EnqueueFunction(v8::internal::Handle<v8::internal::JSFunction>) [/home/abuild/rpmbuild/BUILD/node-v19.6.0/out/Release/node_mksnapshot]
10: 0x2aa3cdfc9f6 v8::internal::TieringManager::OnInterruptTick(v8::internal::Handle<v8::internal::JSFunction>, v8::internal::CodeKind) [/home/abuild/rpmbuild/BUILD/node-v19.6.0/out/Release/node_mksnapshot]
11: 0x2aa3d950cc4  [/home/abuild/rpmbuild/BUILD/node-v19.6.0/out/Release/node_mksnapshot]
12: 0x2aa3d95166c v8::internal::Runtime_BytecodeBudgetInterrupt_Ignition(int, unsigned long*, v8::internal::Isolate*) [/home/abuild/rpmbuild/BUILD/node-v19.6.0/out/Release/node_mksnapshot]
13: 0x2aa3c7cc610  [/home/abuild/rpmbuild/BUILD/node-v19.6.0/out/Release/node_mksnapshot]

Additional information

Looking at the logs, the failures in the unit tests started with 19.3.0 and 19.1.0 was last version where everything worked. This seems to point to v8 update then as possible culprit.

#45230

@AdamMajer
Copy link
Contributor Author

In mostly default compilation case, looking at the disassembly of the core file, it points to this instruction.

   0x000002aa4023b0a2 <+834>:   jg      0x2aa4023b0c0 <Builtins_MulHandler+864>
   0x000002aa4023b0a8 <+840>:   lg      %r5,15(%r2)
   0x000002aa4023b0ae <+846>:   tmll    %r4,1
   0x000002aa4023b0b2 <+850>:   jge     0x2aa4023b0c0 <Builtins_MulHandler+864>
   0x000002aa4023b0b8 <+856>:   lcgr    %r4,%r5
   0x000002aa4023b0bc <+860>:   lgr     %r5,%r4
   0x000002aa4023b0c0 <+864>:   mgrk    %r0,%r8,%r5
=> 0x000002aa4023b0c4 <+868>:   lgr     %r4,%r1
   0x000002aa4023b0c8 <+872>:   srag    %r1,%r1,63
   0x000002aa4023b0ce <+878>:   clgr    %r0,%r1
   0x000002aa4023b0d2 <+882>:   jgne    0x2aa4023b488 <Builtins_MulHandler+1832>
   0x000002aa4023b0d8 <+888>:   ltgr    %r0,%r4
   0x000002aa4023b0dc <+892>:   jge     0x2aa4023b15c <Builtins_MulHandler+1020>
   0x000002aa4023b0e2 <+898>:   lay     %r5,52552(%r10)
   0x000002aa4023b0e8 <+904>:   lg      %r2,0(%r5)


r0             0x2                 2                                                           
r1             0x22                34                                                          
r2             0x456a9e8e31        298141519409                                                
r3             0x85994415b1        573802026417                                
r4             0x2                 2           
r5             0x3b9aca00          1000000000  
r6             0x39                57                                                          
r7             0x1                 1           
r8             0x640f08df          1678706911                                                  
r9             0x0                 0                                                           
r10            0x2aa1a1e5070       2929605890160                                               
r11            0x3ffd0cfce70       4397254823536                                               
r12            0xc0                192                                                         
r13            0x85994415b1        573802026417    
r14            0x2aa17b871e6       2929565659622                                               
r15            0x3ffd0cfce28       4397254823464                               

@richardlau
Copy link
Member

cc @nodejs/platform-s390

@miladfarca
Copy link
Contributor

miladfarca commented Mar 13, 2023

Thank you for reporting the issue, it's related to MacroAssembler::MulHighS64 using mgrk which doesn't exist on z13, will need to check if there is a workaround.

@AdamMajer
Copy link
Contributor Author

Looks like this was introduced in V8 by https://chromium-review.googlesource.com/c/v8/v8/+/3930898

@VoltrexKeyva VoltrexKeyva added the s390 Issues and PRs related to the s390 architecture. label Mar 13, 2023
@john-yan
Copy link

@AdamMajer Hello, would you please verify if this v8 patch work for you? https://chromium-review.googlesource.com/c/v8/v8/+/4334353

@AdamMajer
Copy link
Contributor Author

@AdamMajer Hello, would you please verify if this v8 patch work for you? https://chromium-review.googlesource.com/c/v8/v8/+/4334353

Sorry for lateness of the reply. Checking now.

@AdamMajer
Copy link
Contributor Author

It looks like it still crashes in same unit tests. I will check which code path it's using there.

@AdamMajer
Copy link
Contributor Author

Finally, I'm looking at this again. Building now with 20.1.0 where the v8 patch is now included,

   0x00000000026a74d6 <+822>:   jge     0x26a74e4 <Builtins_MulHandler+836>                    
   0x00000000026a74dc <+828>:   lcgr    %r4,%r5                                                
   0x00000000026a74e0 <+832>:   lgr     %r5,%r4                                                
   0x00000000026a74e4 <+836>:   mgrk    %r0,%r8,%r5                                            
=> 0x00000000026a74e8 <+840>:   lgr     %r4,%r1                                                
   0x00000000026a74ec <+844>:   srag    %r1,%r1,63                                             
   0x00000000026a74f2 <+850>:   clgr    %r0,%r1                                                

This looks like it's coming from

src/compiler/backend/s390/code-generator-s390.cc:1706

@miladfarca
Copy link
Contributor

Hi Adam,
This CL is still WIP but once landed should solve this on z13: https://chromium-review.googlesource.com/c/v8/v8/+/4521297

@AdamMajer
Copy link
Contributor Author

This CL is still WIP but once landed should solve this on z13: https://chromium-review.googlesource.com/c/v8/v8/+/4521297

Looks like this fixes the issue. I'll get back tomorrow if something pops up when all the tests run. Thanks!

@miladfarca
Copy link
Contributor

Glad to hear, thanks for confirming.

@AdamMajer
Copy link
Contributor Author

All tests now pass on z13 and also z15. Thank you for fixing this!

@miladfarca
Copy link
Contributor

Not a problem, thanks again for confirming.

AdamMajer added a commit to AdamMajer/node that referenced this issue Aug 29, 2023
Original commit message:

    Fix usage of MulHighS64 on <= z13

    mgrk is only available with MISC_INSTR_EXT2 installed.
    A runtime call needs to be made if it's unavailable.
    We also need to make sure caller saved registers are saved
    accordingly before making a call.

    Change-Id: If7ac06eef57cc3db059c2640b77c80de3b16fced
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4521297
    Reviewed-by: Junliang Yan <junyan@redhat.com>
    Commit-Queue: Milad Farazmand <mfarazma@redhat.com>
    Cr-Commit-Position: refs/heads/main@{#87675}

Fixes: nodejs#47064
AdamMajer added a commit to AdamMajer/node that referenced this issue Aug 29, 2023
Original commit message:

    Fix usage of MulHighS64 on <= z13

    mgrk is only available with MISC_INSTR_EXT2 installed.
    A runtime call needs to be made if it's unavailable.
    We also need to make sure caller saved registers are saved
    accordingly before making a call.

    Change-Id: If7ac06eef57cc3db059c2640b77c80de3b16fced
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4521297
    Reviewed-by: Junliang Yan <junyan@redhat.com>
    Commit-Queue: Milad Farazmand <mfarazma@redhat.com>
    Cr-Commit-Position: refs/heads/main@{#87675}

Fixes: nodejs#47064
AdamMajer added a commit to AdamMajer/node that referenced this issue Aug 29, 2023
Original commit message:

    Fix usage of MulHighS64 on <= z13

    mgrk is only available with MISC_INSTR_EXT2 installed.
    A runtime call needs to be made if it's unavailable.
    We also need to make sure caller saved registers are saved
    accordingly before making a call.

    Change-Id: If7ac06eef57cc3db059c2640b77c80de3b16fced
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4521297
    Reviewed-by: Junliang Yan <junyan@redhat.com>
    Commit-Queue: Milad Farazmand <mfarazma@redhat.com>
    Cr-Commit-Position: refs/heads/main@{#87675}

Fixes: nodejs#47064
nodejs-github-bot pushed a commit that referenced this issue Aug 31, 2023
Original commit message:

    Fix usage of MulHighS64 on <= z13

    mgrk is only available with MISC_INSTR_EXT2 installed.
    A runtime call needs to be made if it's unavailable.
    We also need to make sure caller saved registers are saved
    accordingly before making a call.

    Change-Id: If7ac06eef57cc3db059c2640b77c80de3b16fced
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4521297
    Reviewed-by: Junliang Yan <junyan@redhat.com>
    Commit-Queue: Milad Farazmand <mfarazma@redhat.com>
    Cr-Commit-Position: refs/heads/main@{#87675}

Fixes: #47064
PR-URL: #49401
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
UlisesGascon pushed a commit that referenced this issue Sep 10, 2023
Original commit message:

    Fix usage of MulHighS64 on <= z13

    mgrk is only available with MISC_INSTR_EXT2 installed.
    A runtime call needs to be made if it's unavailable.
    We also need to make sure caller saved registers are saved
    accordingly before making a call.

    Change-Id: If7ac06eef57cc3db059c2640b77c80de3b16fced
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4521297
    Reviewed-by: Junliang Yan <junyan@redhat.com>
    Commit-Queue: Milad Farazmand <mfarazma@redhat.com>
    Cr-Commit-Position: refs/heads/main@{#87675}

Fixes: #47064
PR-URL: #49401
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
alexfernandez pushed a commit to alexfernandez/node that referenced this issue Nov 1, 2023
Original commit message:

    Fix usage of MulHighS64 on <= z13

    mgrk is only available with MISC_INSTR_EXT2 installed.
    A runtime call needs to be made if it's unavailable.
    We also need to make sure caller saved registers are saved
    accordingly before making a call.

    Change-Id: If7ac06eef57cc3db059c2640b77c80de3b16fced
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4521297
    Reviewed-by: Junliang Yan <junyan@redhat.com>
    Commit-Queue: Milad Farazmand <mfarazma@redhat.com>
    Cr-Commit-Position: refs/heads/main@{#87675}

Fixes: nodejs#47064
PR-URL: nodejs#49401
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s390 Issues and PRs related to the s390 architecture.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants