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

Build error on SmartOS #239

Closed
Tracked by #44650
targos opened this issue Sep 15, 2022 · 9 comments
Closed
Tracked by #44650

Build error on SmartOS #239

targos opened this issue Sep 15, 2022 · 9 comments

Comments

@targos
Copy link
Member

targos commented Sep 15, 2022

https://ci.nodejs.org/job/node-test-commit-smartos/45420/nodes=smartos20-64/console

10:51:08 In file included from ../deps/v8/src/base/free_deleter.h:15,
10:51:08                  from ../deps/v8/src/base/debug/stack_trace_posix.cc:41:
10:51:08 ../deps/v8/src/base/platform/memory.h: In function 'std::size_t v8::base::MallocUsableSize(void*)':
10:51:08 ../deps/v8/src/base/platform/memory.h:118:10: error: 'malloc_usable_size' was not declared in this scope; did you mean 'MallocUsableSize'?
10:51:08   118 |   return malloc_usable_size(ptr);
10:51:08       |          ^~~~~~~~~~~~~~~~~~
10:51:08       |          MallocUsableSize
10:51:08 make[2]: *** [tools/v8_gypfiles/v8_libbase.target.mk:177: /home/iojs/build/workspace/node-test-commit-smartos/nodes/smartos20-64/out/Release/obj.target/v8_libbase/deps/v8/src/base/debug/stack_trace_posix.o] Error 1

@nodejs/platform-smartos

@bahamat
Copy link

bahamat commented Sep 16, 2022

My team will be taking a look at this.

@jperkin
Copy link

jperkin commented Sep 16, 2022

Hi @targos, I'm struggling to find where this is coming from - GitHub search across the entire nodejs and v8 organisations show no code matches for malloc_usable_size(). It's not something we have in SmartOS, and many other platforms don't have it either, so the fix should be pretty straight-forward, I just can't figure out where it's actually being used.

I'd be more than happy to submit a fix for it if you could help me get up to speed on which repositories that test job is pulling from. Thanks!

@targos
Copy link
Member Author

targos commented Sep 16, 2022

In the V8 repo: https://github.com/v8/v8/blob/50504b168d2c364d49e3332cbd934dc60a4bfad8/src/base/platform/memory.h#L117-L118

Introduced by https://chromium-review.googlesource.com/c/v8/v8/+/3858226

In this repo (canary is updated every day so the commit is ephemeral):

#else // POSIX.
return malloc_usable_size(ptr);

@jperkin
Copy link

jperkin commented Sep 16, 2022

Ah, ok, thanks! Looks like GitHub search just completely doesn't work.

The fix looks straight-forward (add V8_OS_SOLARIS to the exclusion list for V8_HAS_MALLOC_USABLE_SIZE on line 26, similar to AIX) but I assume I'll need to get this through the v8 contribution process first.

@targos
Copy link
Member Author

targos commented Sep 25, 2022

The fix looks straight-forward (add V8_OS_SOLARIS to the exclusion list for V8_HAS_MALLOC_USABLE_SIZE on line 26, similar to AIX) but I assume I'll need to get this through the v8 contribution process first.

I'm floating a patch for now in nodejs/node#44741 but let's keep this issue open until it's upstreamed.

@bnoordhuis
Copy link
Member

@bahamat the patch still needs to be upstreamed, see nodejs/node#45118.

@bahamat
Copy link

bahamat commented Oct 28, 2022

@bnoordhuis Ok, I'm looking into the CLA requirement. It's not clear to me that MNX is the most appropriate agent to contribute the patch, since we are not the author/copyright holder.

@bnoordhuis
Copy link
Member

@targos That means you'd have to upstream it?

@targos
Copy link
Member Author

targos commented Nov 14, 2022

@targos targos closed this as completed Nov 15, 2022
targos added a commit to targos/node that referenced this issue Nov 15, 2022
Original commit message:

    [base] Fix SmartOS build

    That platform doesn't have `malloc_usable_size`.

    Refs: nodejs/node-v8#239
    Change-Id: I011dd8449d02b27219a32cba00132cd068069f50
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4026402
    Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
    Commit-Queue: Michaël Zasso <mic.besace@gmail.com>
    Cr-Commit-Position: refs/heads/main@{#84256}

Refs: v8/v8@f1c888e
targos added a commit to targos/node that referenced this issue Nov 18, 2022
Original commit message:

    [base] Fix SmartOS build

    That platform doesn't have `malloc_usable_size`.

    Refs: nodejs/node-v8#239
    Change-Id: I011dd8449d02b27219a32cba00132cd068069f50
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4026402
    Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
    Commit-Queue: Michaël Zasso <mic.besace@gmail.com>
    Cr-Commit-Position: refs/heads/main@{#84256}

Refs: v8/v8@f1c888e
targos added a commit to targos/node that referenced this issue Nov 18, 2022
Original commit message:

    [base] Fix SmartOS build

    That platform doesn't have `malloc_usable_size`.

    Refs: nodejs/node-v8#239
    Change-Id: I011dd8449d02b27219a32cba00132cd068069f50
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4026402
    Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
    Commit-Queue: Michaël Zasso <mic.besace@gmail.com>
    Cr-Commit-Position: refs/heads/main@{#84256}

Refs: v8/v8@f1c888e
nodejs-github-bot pushed a commit to nodejs/node that referenced this issue Nov 19, 2022
Original commit message:

    [base] Fix SmartOS build

    That platform doesn't have `malloc_usable_size`.

    Refs: nodejs/node-v8#239
    Change-Id: I011dd8449d02b27219a32cba00132cd068069f50
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4026402
    Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
    Commit-Queue: Michaël Zasso <mic.besace@gmail.com>
    Cr-Commit-Position: refs/heads/main@{#84256}

Refs: v8/v8@f1c888e
PR-URL: #45230
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
ruyadorno pushed a commit to nodejs/node that referenced this issue Nov 21, 2022
Original commit message:

    [base] Fix SmartOS build

    That platform doesn't have `malloc_usable_size`.

    Refs: nodejs/node-v8#239
    Change-Id: I011dd8449d02b27219a32cba00132cd068069f50
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4026402
    Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
    Commit-Queue: Michaël Zasso <mic.besace@gmail.com>
    Cr-Commit-Position: refs/heads/main@{#84256}

Refs: v8/v8@f1c888e
PR-URL: #45230
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
anonrig pushed a commit to anonrig/node that referenced this issue Nov 23, 2022
Original commit message:

    [base] Fix SmartOS build

    That platform doesn't have `malloc_usable_size`.

    Refs: nodejs/node-v8#239
    Change-Id: I011dd8449d02b27219a32cba00132cd068069f50
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4026402
    Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
    Commit-Queue: Michaël Zasso <mic.besace@gmail.com>
    Cr-Commit-Position: refs/heads/main@{#84256}

Refs: v8/v8@f1c888e
PR-URL: nodejs#45230
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>

# Conflicts:
#	common.gypi
marco-ippolito pushed a commit to marco-ippolito/node that referenced this issue Nov 23, 2022
Original commit message:

    [base] Fix SmartOS build

    That platform doesn't have `malloc_usable_size`.

    Refs: nodejs/node-v8#239
    Change-Id: I011dd8449d02b27219a32cba00132cd068069f50
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4026402
    Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
    Commit-Queue: Michaël Zasso <mic.besace@gmail.com>
    Cr-Commit-Position: refs/heads/main@{#84256}

Refs: v8/v8@f1c888e
PR-URL: nodejs#45230
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
anonrig pushed a commit to anonrig/node that referenced this issue Nov 26, 2022
Original commit message:

    [base] Fix SmartOS build

    That platform doesn't have `malloc_usable_size`.

    Refs: nodejs/node-v8#239
    Change-Id: I011dd8449d02b27219a32cba00132cd068069f50
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4026402
    Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
    Commit-Queue: Michaël Zasso <mic.besace@gmail.com>
    Cr-Commit-Position: refs/heads/main@{#84256}

Refs: v8/v8@f1c888e
PR-URL: nodejs#45230
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>

# Conflicts:
#	common.gypi
nodejs-github-bot pushed a commit to nodejs/node that referenced this issue Nov 27, 2022
Original commit message:

    [base] Fix SmartOS build

    That platform doesn't have `malloc_usable_size`.

    Refs: nodejs/node-v8#239
    Change-Id: I011dd8449d02b27219a32cba00132cd068069f50
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4026402
    Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
    Commit-Queue: Michaël Zasso <mic.besace@gmail.com>
    Cr-Commit-Position: refs/heads/main@{#84256}

Refs: v8/v8@f1c888e
PR-URL: #45230
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>

# Conflicts:
#	common.gypi
PR-URL: #45579
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
ErickWendel pushed a commit to ErickWendel/node that referenced this issue Nov 30, 2022
Original commit message:

    [base] Fix SmartOS build

    That platform doesn't have `malloc_usable_size`.

    Refs: nodejs/node-v8#239
    Change-Id: I011dd8449d02b27219a32cba00132cd068069f50
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4026402
    Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
    Commit-Queue: Michaël Zasso <mic.besace@gmail.com>
    Cr-Commit-Position: refs/heads/main@{#84256}

Refs: v8/v8@f1c888e
PR-URL: nodejs#45230
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>

# Conflicts:
#	common.gypi
PR-URL: nodejs#45579
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants