deps: support madvise(3C) across ALL illumos revisions

[danmcd]

In illumos, madvise(3C) now takes void * for its first argument post-illumos#14418, but uses caddr_t pre-illumos#14418. This fix will detect if the illumos is pre-or-post-illumos#14418 so builds can work either way.

Cf. https://illumos.org/issues/14418

[nodejs-github-bot]

Review requested:

• @nodejs/security-wg
• @nodejs/v8-update

[danmcd]

Currently testing on a post-illumos#14418 environment. The Jenkins agents for smartos actually use pre-illumos#14418 environments. This fix plans on working in both (which cannot happen today), which will allow me to revisit, perhaps, what a smartos or even an illumos of some other flavor (e.g. OmniOS or OpenIndiana) build agent might look like. Today's smartos agents are a bit convoluted, thanks in no small part to fallout from illumos#14418.

[danmcd]

This change should be a literal NOP for ANY OTHER BUILD PLATFORM, even Oracle Solaris, since the __illumos__ symbol is in use for cpp directives.

[danmcd]

Do I have to promote this to a non-draft PR to get Jenkins to run a build?

[github-actions]

Failed to start CI

   ⚠  No approving reviews found
   ✘  Refusing to run CI on potentially unsafe PR

https://github.com/nodejs/node/actions/runs/14914717962

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/66717/

[danmcd]

Thank you @cjihrig !

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/66723/

[danmcd]

This is good news and I've updated this PR to ready-for-review. The Node CI for SmartOS uses a pre-illumos#14418 illumos revision, and my local build uses a post-illumos#14418 revision. The same code now works before-and-after the illumos change in question. As mentioned earlier, this may allow less byzantine build agents for the Node CI.

The Node CI results are above, and my local build's test results were all successfull but one failure:

=== release test-child-process-stdio-reuse-readable-stdio ===                 
Path: parallel/test-child-process-stdio-reuse-readable-stdio
Command: out/Release/node --test-reporter=./test/common/test-error-reporter.js --test-reporter-destination=stdout /home/danmcd/node/test/parallel/test-child-process-stdio-reuse-readable-stdio.js
--- TIMEOUT ---

Which was the same one when I updated the V8 pointer-updates for illumos.

[mhdawson]

LGTM

[danmcd]

Thank you all for the feedback and confirmation on this. I hope to use the experience here as a way to get V8 itself to accept this and the VA48 behavior in their upstream-from-here repo. (I'll have to figure out the google tooling but that's my problem not yours.)

One more thing I'd like to reiterate: "As mentioned earlier, this may allow less byzantine build agents for the Node CI."

It also means, I think, that we could introduce a non-SmartOS illumos distro build agent if we wished. OmniOS just released its new LTS release (r151054), and that would make a FINE place (3 years support, 2 years primary lifetime) to drop a build agent that'll confirm greater-illumos stability. I'm now running a node build with this fix in place on my newly-updated home server running OmniOS. I will report back here with results.

[mhdawson]

@danmcd since this floats a change on V8 getting it upstreamed sooner than later would be good. Sometimes we ask that patches will land upstream before we will float them.

@targos any concerns with floating this one?

[danmcd]

@mhdawson AFAICT it'll be ideal to have this and the VA48 one land upstream in V8 before Node next pulls in its next V8 update. This is, as you point out, a place where @targos 's opinion should reign supreme.

[targos]

I think it's fine to float the patch as long as the path for upstreaming it is determined. There are two things to fix before this PR can land though:

• The commit message title should start with an imperative verb (and maybe avoid using a syntax that GitHub converts into a dead link)
• common.gypi should be updated by incrementing the v8_embedder_string

[danmcd]

@targos => A better title such as deps: support madvise() across ALL illumos revisions ?

I don't wanna push an update with v8_embedder_string fixes until I got the title right as well.

[danmcd]

Oh shoot, forgot my branch is directly linked to this PR.

[targos]

Thanks, LGTM!

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/66805/