Update to libv8-node 17.x Take 2

[seanmakesgames]

Did a rebase of the changes in #232 -- will be taking a look at the segfaults in locale switching introduced after 16.10 Should make 18.x #261 easier to upgrade to.

[lloeki]

@seanmakesgames you probably want to cherry pick a couple of commits from #261 such as: https://github.com/rubyjs/mini_racer/pull/261/commits/10c4e4c6bbef2ebedd09ebb17bffe7bc23593e57 https://github.com/rubyjs/mini_racer/pull/261/commits/d4ff19e35dc5d59cca745d72b5682c976fa6a2da https://github.com/rubyjs/mini_racer/pull/261/commits/03aa36a15fe6b2b3d003817fa9d25871b0469882

[seanmakesgames]

Thanks @lloeki -

By the way, there are now a HUGE amount of compiler warnings on my machine (most of them look like this). Are others seeing them?

../../../../ext/mini_racer_extension/mini_racer_extension.cc:756:39: warning: '(' and '{' tokens introducing statement expression appear in different macro expansion contexts [-Wcompound-token-split-by-macro]

[lloeki]

The only ones I got when building mini_racer are:

compiling ../../../../ext/mini_racer_extension/mini_racer_extension.cc
cc1plus: warning: command line option '-Wimplicit-int' is valid for C/ObjC but not for C++
cc1plus: warning: unrecognized command line option '-Wno-self-assign'
cc1plus: warning: unrecognized command line option '-Wno-constant-logical-operand'
cc1plus: warning: unrecognized command line option '-Wno-parentheses-equality'
linking shared-object mini_racer_extension.so
make: warning:  Clock skew detected.  Your build may be incomplete.
cd -
/usr/bin/make install target_prefix=
make: Warning: File 'mini_racer_extension.so' has modification time 0.41 s in the future

Although when building libv8 I get a huuuuge amount of these with gcc:

#17 73.81 g++: warning: switch '-msign-return-address=all' is no longer supported

[seanmakesgames]

After a bit of finagling; I have repro'd the segfault in the default codespace by switching to ruby 3.0 in rvm. Ruby 3.1 was failing on:

undefined method `require_paths' for nil:NilClass (NoMethodError) ext_path = Gem.loaded_specs['mini_racer'].require_paths

Not sure what to do to fix that.

If you're curious and want to do some debugging too, you can spin up a codespace.

[seanmakesgames]

The only ones I got when building mini_racer are

must be my machine then. Thanks :)

[seanmakesgames]

@lloeki What I want to do is test libv8 directly without any of the mini_racer code and/or ruby. Would be great to rule that code out as a source of the issue. Likely this means writing a c/++ embedder, which would be great to have in the test suite in libv8. I think we are using mini_racer at the moment as the test workflow of libv8.

[seanmakesgames]

My current thought here is that there are some c++ files that moved around in v8 in such a way that it all still compiles, but our libv8 construction and collation process are missing them.

It is weird that it behaves differently between locales and it behaves differently between mac & linux.

As a surprise to probably no-one, running the command in a browser window for the locale works fine.

[lloeki]

Likely this means writing a c/++ embedder, which would be great to have in the test suite in libv8. I think we are using mini_racer at the moment as the test workflow of libv8.

Wow that would be fantastic.

I had this idea too as this would save me from cloning mini_racer when testing libv8-node (see Makefile test target and Makefile.docker test/linux and test/linux-musl targets).

In addition it would help with these kind of issues in discriminating whether it's libv8 or mini_racer having an issue.

[seanmakesgames]

I'm starting with a codespace at the moment (ubuntu+ruby3.0) running a rake compile which I assume will take a million years. My guess is that it will repro the issue still.

Do you have a preferred or favorite c++ unit test framework? From my quick reading, https://github.com/catchorg/Catch2 seems to be a good fit for us.

[lloeki]

As a surprise to probably no-one, running the command in a browser window for the locale works fine.

Note that due to how the build system is done there's no libv8_monolith.a target neither in libv8 nor in node, and the other intermediate targets proved insufficient back then, so we're building the full node (which doesn't add much in terms of time) so you can try things out there as well:

$ src/node-v16.19,0/out/Release/node
Welcome to Node.js 16.19.0.
Type ".help" for more information.
> 

(Yes I'm building and publishing 16.19.0 right now, which is affected as well)

It is weird that it behaves differently between locales

Could be explained by some C pointer thing, use after free, or something like that.

it behaves differently between mac & linux.

Could be explained by underlying locale stuff using a different OS thing (e.g what if the locales are missing from locale.gen or something?)

There are too much unknowns to decide.

[seanmakesgames]

There are too much unknowns to decide

Haha. indeed!

$ src/node-v16.19,0/out/Release/node

This is great. Can I use this in a context with a pre-built libv8 binary? or does it have to be in the libv8 repo after being compiled?

[lloeki]

Do you have a preferred or favorite c++ unit test framework?

I don't. The only thing I'm vaguely familiar with is whatever is used by https://github.com/datadog/libddwaf.

[seanmakesgames]

whatever is used by https://github.com/datadog/libddwaf.

looks like google test. https://github.com/DataDog/libddwaf/blob/master/tests/test.h#L23

[lloeki]

This is great. Can I use this in a context with a pre-built libv8 binary? or does it have to be in the libv8 repo after being compiled?

It's an artifact from the build but it's not packaged. Also it's probably statically linked against libv8 so I don't think you can swap another precompiled libv8 without a pile of hacks.

This should have you

git clone https://github.com/rubyjs/libv8-node
cd libv8-node
git checkout node-16 # or node-17 or node-18
make test # I use that on darwin, might work on linux but it would be subject to your environment
make test/linux # I use that for a consistent linux environment, it should produce an image that contains all the things

[seanmakesgames]

Shoot. I didn't check out the specific branch. I'm on default branch- but luckily it's 17.3 and should repro :)

[seanmakesgames]

@Fayti1703 has agreed to help out with writing some c embed code for libv8 --

@lloeki What's the branch strategy / where should we make this PR to add a c++ test in libv8? Should we start in node-17? node-16? the default branch?

[lloeki]

master should target latest node (which should be node-19, although I did not work on that)

node-16, node-17, node-18 should be "stable-ish" branches targetting each version. I usually cherry pick changes that make sense between these branches.

In this specific case I would have this added to node-16 as a base branch since it's the one that's closest to things that used to work + the one that is the current mini_racer release (and once we have things in good order, apply the changes on each branch)

[seanmakesgames]

In this specific case I would have this added to node-16

ok sweet. then we can move it around wherever it needs to go.

[seanmakesgames]

Alright- we are poking around on this branch to make the test: https://github.com/seanmakesgames/libv8-node/tree/add_test

I've got a codespace building libv8 on ubuntu again (on node-16, because the master branch doesn't have the most up-to-date make stuff and wasn't building node)

I'll be taking a look at this again later today. Thanks for the support! I'm sure we'll figure this out (or come up with a suitable solution)

[lloeki]

Thanks so much @seanmakesgames @Fayti1703!

[lloeki]

BTW I just pushed the last of the 16.19.0.0 gem so that one should be complete now.

[seanmakesgames]

16.19.0.0 fails github actions though right? because they run against the mini_racer tests that will segfault- So that version is just pushed up for our testing purposes, correct?

[lloeki]

Correct, it does fail in the same way as 17.x and 18.x

[seanmakesgames]

@Fayti1703 has confirmed that it does not reproduce in the node binary that's built, so that's great news. means we can probably fix the issue and that it exists inside libv8-node somewhere