New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 868272 link

Starred by 1 user
Status: WontFix
Owner:
yukishiino@chromium.org
Closed: Jul 27
Cc:
█ kabusm@google.com
█ orsibatiz@google.com
peria@chromium.org
lfg@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Linking error in generated `callback` bindings.

Project Member Reported by mkwst@chromium.org, Jul 27 
https://chromium-review.googlesource.com/c/chromium/src/+/1152741 is a somewhat minified version of the bug.

It attempts to add a callback like:

    callback TrustedTestCallback = DOMString (DOMString string);

And then to reference that callback from an existing interface:

    interface TrustedHTML {
      static DOMString trustedTest(TrustedTestCallback callback);
    }

It has a trivial implementation of that method in the interface's C++ implementation, and compiles correctly.

It fails to link with the following error locally:

```
FAILED: libblink_core.so libblink_core.so.TOC 
python "../../build/toolchain/gcc_solink_wrapper.py" --readelf="readelf" --nm="nm" --sofile="./libblink_core.so" --tocfile="./libblink_core.so.TOC" --output="./libblink_core.so"  -- ../../third_party/llvm-build/Release+Asserts/bin/clang++ -shared -Wl,--fatal-warnings -fPIC -Wl,-z,noexecstack -Wl,-z,now -Wl,-z,relro -Wl,-z,defs -Wl,--as-needed -fuse-ld=lld -Wl,--icf=all -Wl,--color-diagnostics -m64 -Werror -Wl,-O2 -Wl,--gc-sections -nostdlib++ --sysroot=../../build/linux/debian_sid_amd64-sysroot -L../../build/linux/debian_sid_amd64-sysroot/usr/local/lib/x86_64-linux-gnu -Wl,-rpath-link=../../build/linux/debian_sid_amd64-sysroot/usr/local/lib/x86_64-linux-gnu -L../../build/linux/debian_sid_amd64-sysroot/lib/x86_64-linux-gnu -Wl,-rpath-link=../../build/linux/debian_sid_amd64-sysroot/lib/x86_64-linux-gnu -L../../build/linux/debian_sid_amd64-sysroot/usr/lib/x86_64-linux-gnu -Wl,-rpath-link=../../build/linux/debian_sid_amd64-sysroot/usr/lib/x86_64-linux-gnu -rdynamic -o "./libblink_core.so" -Wl,-soname="libblink_core.so" @"./libblink_core.so.rsp"
/usr/local/google/home/mkwst/chromium/src/out/Default/../../third_party/llvm-build/Release+Asserts/bin/ld.lld: error: undefined symbol: vtable for blink::V8TrustedTestCallback
>>> referenced by v8_trusted_html.cc
>>>               obj/third_party/blink/renderer/bindings/core/v8/bindings_core_impl/v8_trusted_html.o:(blink::V8TrustedHTML::trustedTestMethodCallback(v8::FunctionCallbackInfo<v8::Value> const&))
```

Kentaro suggested that yukishiino@ might have thoughts? :)

Comment 1 by mkwst@chromium.org, Jul 27 

The windows bot has a different error:

```
FAILED: headless_unittests.exe headless_unittests.exe.pdb 
ninja -t msvc -e environment.x86 -- ../../third_party/llvm-build/Release+Asserts/bin/lld-link.exe /nologo /OUT:./headless_unittests.exe /PDB:./headless_unittests.exe.pdb @./headless_unittests.exe.rsp
../../third_party/llvm-build/Release+Asserts/bin\lld-link.exe: error: undefined symbol: ?NameInHeapSnapshot@V8TrustedTestCallback@blink@@UBEPBDXZ
>>> referenced by bindings_core_impl.lib(v8_trusted_html.obj):(??_7V8TrustedTestCallback@blink@@6B@)
```

So far as I can tell, `V8TrustedTestCallback::NameInHeapSnapshot` is correctly overridden. I wonder if the `cc` file isn't getting pulled in correctly during linking?

Comment 2 by mkwst@chromium.org, Jul 27 

Changing the templates to inline that function in the `.h` file does make it link locally for me. Which is very strange.

Comment 3 by mkwst@chromium.org, Jul 27 

https://chromium-review.googlesource.com/c/chromium/src/+/1152811 inlines the function. It seems like there must be a more elegant solution, but that patch seems to work.

Comment 4 by raphael....@intel.com, Jul 27 

AFAICS by applying https://chromium-review.googlesource.com/c/chromium/src/+/1152741 locally, you just need to add v8_trusted_test_callback.{cc,h} to generated_core_callback_function_files in //third_party/blink/renderer/bindings/core/v8/BUILD.gn.

I guess inlining worked around the issue because you could then just rely on the header without actually pulling the .o into the right target.

Comment 5 by mkwst@chromium.org, Jul 27

Status: WontFix (was: Untriaged)
Ah ha! Thank you. That's the right fix indeed. Sorry for the noise!
►

Sign in to add a comment