Fix crashes on Linux/PPC64 ELFv1

[zeldin]

Instead of using "defined(_AIX)" or "V8_OS_AIX" as a test for when to use function descriptors, use "ABI_USES_FUNCTION_DESCRIPTORS" which exists precisely for this purpose. This fixes
the crashes on Linux/ppc64 ELFv1.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• commit message follows commit guidelines

deps: V8: fix tests for function descriptors on PPC64

Fixes: #29534

[devsnek]

This change will need to be made in upstream V8 before we land it here. Instructions for submitting changes to V8 can be found here: https://v8.dev/docs/contribute. I'm also happy to submit small changes if needed.

[kapouer]

Cool ! I'm going to test that asap.
EDIT: it went very well, test-ci-js pass with that patch backported to 12.18.0 on a debian ppc64 porter machine.

[mhdawson]

@miladfarca, @john-yan can you take a quick look at this? If you think it make sense maybe you can help get it landed upstream.

[miladfarca]

I'm assuming this change is specific for 64bit Linux as 32 bit does not have TOC and function descriptors.

Have v8 specific tests been executed on a Linux BE machine? We do not have such a machine so cannot test the above patch.

There are a number of other places where AIX macros are used which are not FD specific, here is an example:
https://github.com/v8/v8/blob/master/src/heap/cppgc/asm/ppc/push_registers_asm.cc#L20
BE Linux has a different descriptor section which is not covered by any f the ifdef's.
(If Im not mistaking above code isn't being used yet but under development for gc improvements).

We can definitely land this patch on V8 but we don't/can't monitor future changes to assure BE Linux compatibility.

[kapouer]

@miladfarca i know it's suboptimal but if it fails again on ppc64 i'll report it again.

[zeldin]

@miladfarca I did run make test on Linux BE as noted in the PR checklist. Is there anything else I can do to run more "v8 specific" tests?
Also: Would you like to have a machine where you can run such tests yourselves? If you are interested I can talk to Raptor CS about getting you access to a VM.

[mhdawson]

@zeldin you could run make test-v8, but I believe that would require you adding ninja and gn binaries in the right place so I'm not sure its practical. I think given we don't support it in the project anymore as long as make test has passed and you are comfortable its probably ok.

[mhdawson]

@zeldin we had BE machines but removed them once we dropped support for PPC BE. I'm wondering what's driving the interest? Our team is driving the PPC platform support at the V8 level and each platform requires time and effort. We dropped BE after we could no longer justify the effort based on what we thought was BE usage as well as the level of internal support for the work to keep it going.

[zeldin]

@mhdawson Running make test-v8 gave me:

________ running 'vpython v8/third_party/depot_tools/update_depot_tools_toggle.py --disable' in '.'
[E2020-06-16T23:47:49.533736+02:00 50266 0 annotate.go:241] goroutine 1:
[E2020-06-16T23:47:49.533798+02:00 50266 0 annotate.go:241] #0 vpython/pep425.go:258 - main.getPEP425CIPDTemplateForTag()
[E2020-06-16T23:47:49.533818+02:00 50266 0 annotate.go:241]   reason: failed to infer CIPD platform for tag [python:"cp27" abi:"cp27mu" platform:"linux_ppc64" ]

I have no idea why it needs to "infer CIPD platform", or why golang is involved in the first place. This seems overengineered...

[mhdawson]

@zeldin. It's from the V8 build tools, they install a lot of components etc. to build/run the tests and there won't be copies of the platform specific ones for PPCBE. (Hence my mention of having to install your own ninja and gn).

In any case while it is likely possible, its probably more work that is reasonable to get the tests to run.

[miladfarca]

Change is now merged with V8 master:
https://chromium-review.googlesource.com/c/v8/v8/+/2248202

It can now be merged with Nodejs master using these instructions:
https://github.com/nodejs/node/blob/master/doc/guides/maintaining-V8.md

[mmarchini]

@zeldin FYI I updated the PR title to reflect the commit subject, so that folks skimming through notifications or the PR list know what the PR is about without opening it. Hope you don't mind :)

[aduh95]

@zeldin The change is part of V8 8.5, and is part of #35415 (which should land in Node.js v15). I'll go ahead and close this out, thanks a lot for your contribution!

If you're interested to have this change land on v14 and/or v12, you can open backporting PRs using these instructions:
https://github.com/nodejs/node/blob/master/doc/guides/maintaining-V8.md