Skip to content

Fix v8::External::Value calls for the new ExternalPointerTypeTag API#201

Merged
agracio merged 1 commit into
agracio:masterfrom
gdavidkov:fix/external-value-tag-api
May 6, 2026
Merged

Fix v8::External::Value calls for the new ExternalPointerTypeTag API#201
agracio merged 1 commit into
agracio:masterfrom
gdavidkov:fix/external-value-tag-api

Conversation

@gdavidkov
Copy link
Copy Markdown
Contributor

Recent V8 versions (currently shipped by Chromium/Electron; not yet in released Node.js) added an ExternalPointerTypeTag parameter to v8::External::Value():

  void* Value(ExternalPointerTypeTag tag) const;   // V8 with sandbox tagging
  void* Value() const;                             // earlier V8

The old zero-arg overload was removed, so edge-js fails to compile against Electron 42's V8 with errors like:

  src/dotnet/clrfunc.cpp(15): error C2660:
    'v8::External::Value': function does not take 0 arguments

The companion fix for nan's own callsites (which edge-js depends on) is in nodejs/nan#1015; both fixes are needed for end-to-end builds against Electron 42.

Recent V8 versions (currently shipped by Chromium/Electron; not yet in
released Node.js) added an ExternalPointerTypeTag parameter to
v8::External::Value():

  void* Value(ExternalPointerTypeTag tag) const;   // V8 with sandbox tagging
  void* Value() const;                             // earlier V8

The old zero-arg overload was removed, so edge-js fails to compile against
Electron 42's V8 with errors like:

  src/dotnet/clrfunc.cpp(15): error C2660:
    'v8::External::Value': function does not take 0 arguments

Six callsites of the form
  (SomeWrap*)(correlator->Value())
in src/{CoreCLREmbedding,mono,dotnet}/{coreclrfunc,clrfunc,
coreclrnodejsfuncinvokecontext,nodejsfuncinvokecontext}.cpp are guarded on
the V8_EXTERNAL_POINTER_TAG_COUNT macro (defined in <v8-internal.h>
alongside the new signature) and pass v8::kExternalPointerTypeTagDefault
when the new API is in scope. The else branch reproduces the prior source
line-for-line, so the change is a no-op on every released Node.js V8 today.

The companion fix for nan's own callsites (which edge-js depends on) is in
nodejs/nan#1015; both fixes are needed for end-to-end builds against
Electron 42.
@agracio agracio merged commit 31d1a29 into agracio:master May 6, 2026
57 checks passed
@agracio
Copy link
Copy Markdown
Owner

agracio commented May 6, 2026

Appreciate quick PR

@agracio
Copy link
Copy Markdown
Owner

agracio commented May 6, 2026

Unfortunately it does not resolve Electron 42 issue.

@agracio
Copy link
Copy Markdown
Owner

agracio commented May 6, 2026

After merging changes from https://github.com/gdavidkov/nan/tree/refs/heads/fix/external-value-v8-13 there is still an issue with node-gyp\Cache\42.0.0\include\node\cppgc\heap.h

  utils.cpp
C:\Users\user\AppData\Local\node-gyp\Cache\42.0.0\include\node\cppgc\heap.h(40,37): error C3861: '__builtin_frame_address': identifier not found [C:\Users\user\Dev\electron-edge-js\build\edge_nativeclr.vcxproj]
  (compiling source file '../src/dotnet/utils.cpp')

  v8synchronizationcontext.cpp
C:\Users\user\AppData\Local\node-gyp\Cache\42.0.0\include\node\cppgc\heap.h(40,37): error C3861: '__builtin_frame_address': identifier not found [C:\Users\user\Dev\electron-edge-js\build\edge_coreclr.vcxproj]
  (compiling source file '../src/common/v8synchronizationcontext.cpp')

heap.h

/**
 * A marker that captures the current stack start address.
 */
class V8_EXPORT StackStartMarker {
 public:
  StackStartMarker() : stack_start_(__builtin_frame_address(0)) {}
  void* stack_start() const { return stack_start_; }

 private:
  void* stack_start_;
};

This code is not present in v8 14.6 heap.h, looks like a new addition in v8 14.8

@agracio
Copy link
Copy Markdown
Owner

agracio commented May 9, 2026

@gdavidkov any ideas how to resolve it.
Were you able to build electron-edge-js 42 on you PC?

@gdavidkov
Copy link
Copy Markdown
Contributor Author

It's a workaround, not a proper fix — I patched the headers.

/ Shared cppgc/heap.h MSVC patcher.
//
// V8 13 dropped the MSVC fallback in <cppgc/heap.h>: the StackStartMarker
// constructor calls __builtin_frame_address(0) unconditionally, which is a
// GCC/Clang builtin MSVC does not recognize.  Restore the MSVC branch using
// _AddressOfReturnAddress() (the same intrinsic V8 itself used to ship)
// in every cached copy of the header so MSVC-built native addons that
// transitively include cppgc headers (e.g. electron-edge-js) compile.
//
// Two cache directories must be patched on Windows:
//   - %LOCALAPPDATA%\node-gyp\Cache\<v>\  (used by direct node-gyp builds)
//   - %USERPROFILE%\.electron-gyp\<v>\    (used by @electron/rebuild during
//     packaging via electron-builder)
//
// Idempotent: detects the already-patched marker and skips.

const fs = require("fs");
const path = require("path");
require("colors");

function patchCppgcHeapAt(headerPath) {
    if (!fs.existsSync(headerPath)) {
        console.log(`cppgc/heap.h not present at ${headerPath} - skipping`.bold.yellow);
        return;
    }

    let content = fs.readFileSync(headerPath, "utf8");
    if (content.includes("_AddressOfReturnAddress")) {
        console.log(`cppgc/heap.h already patched for MSVC at ${headerPath}`.bold.green);
        return;
    }

    const original = "StackStartMarker() : stack_start_(__builtin_frame_address(0)) {}";
    if (!content.includes(original)) {
        console.log(`cppgc/heap.h does not contain expected StackStartMarker line at ${headerPath} - skipping`.bold.yellow);
        return;
    }
    const replacement =
        "#if defined(_MSC_VER) && !defined(__clang__)\n" +
        "  StackStartMarker() : stack_start_(_AddressOfReturnAddress()) {}\n" +
        "#else\n" +
        "  StackStartMarker() : stack_start_(__builtin_frame_address(0)) {}\n" +
        "#endif";
    content = content.replace(original, replacement);

    // _AddressOfReturnAddress is declared in <intrin.h>; cppgc/heap.h does
    // not include it on its own.  Insert the include + intrinsic pragma
    // right after v8config.h so the MSVC branch above resolves cleanly.
    const includeAnchor = '#include "v8config.h"  // NOLINT(build/include_directory)';
    if (content.includes(includeAnchor)) {
        const includeBlock = includeAnchor +
            "\n\n#if defined(_MSC_VER) && !defined(__clang__)\n" +
            "#include <intrin.h>\n" +
            "#pragma intrinsic(_AddressOfReturnAddress)\n" +
            "#endif";
        content = content.replace(includeAnchor, includeBlock);
    }

    fs.writeFileSync(headerPath, content, "utf8");
    console.log(`Patched cppgc/heap.h for MSVC at ${headerPath}`.bold.green);
}

function knownCachePaths(electronVersion) {
    const candidates = [];
    if (process.env.LOCALAPPDATA) {
        candidates.push(path.join(
            process.env.LOCALAPPDATA, "node-gyp", "Cache", electronVersion,
            "include", "node", "cppgc", "heap.h"));
    }
    if (process.env.USERPROFILE) {
        candidates.push(path.join(
            process.env.USERPROFILE, ".electron-gyp", electronVersion,
            "include", "node", "cppgc", "heap.h"));
    }
    return candidates;
}

function patchAllKnownCaches(electronVersion) {
    const candidates = knownCachePaths(electronVersion);
    if (candidates.length === 0) {
        console.log("Neither LOCALAPPDATA nor USERPROFILE set - skipping cppgc/heap.h MSVC patch".bold.yellow);
        return;
    }
    for (const headerPath of candidates) {
        patchCppgcHeapAt(headerPath);
    }
}

@agracio
Copy link
Copy Markdown
Owner

agracio commented May 10, 2026

Released.

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

Successfully merging this pull request may close these issues.

2 participants