1679736 - Visual Studio 2017 fails to compile SM headers because of StructGCPolicy static assert

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect, P3)

Description

[wraitii]

Attachment: FixMSVCRootedVoid.diff

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14_6) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/14.0 Safari/605.1.15

Steps to reproduce:

Compiling a project with MSVC 2017 (and likely later) that includes <js/RootingAPI.h> fails, tripping the static_assert in StructGCPolicy<T> that T cannot be a pointer (added in #1459577)

From what I could tell, the problem is that Rooted includes a RootedTraceable<void*>, and MSVC instantiates the RootedTraceable<void*>::trace method, which then trips the static assert as that defaults to StructGCPolicy.
I think MSVC is actually correct in instantiating that method given this can be exported, so might be called by unseen code. Wouldn't swear it though, and couldn't reproduce on clang.

I fixed it by specializing that case in RootingAPI.h to ignore the GC, as there seems to be a similar hack for MapTypeToRootKind<void*>. See attached patch.

The gjs project has a similar fix here: https://gitlab.gnome.org/GNOME/gjs/-/commit/5bb84516689d562f6065f1859148371df2a6b0bcn, (thanks Philip Chimento for the note)

(See also 1614243, the second bug reported there (about dll export) is actually not fixed either)

Actual results:

Static_assert trips and things don't compile.

Expected results:

Static assert doesn't trip and things compile.

Comment 1

[Jon Coppeard (:jonco)]

(In reply to wraitii from comment #0)
Thanks for the report.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: Bug 1679736 - Add void* to GCIgnorePolicy r?sfink

RootingAPI.h uses Rooted<void*> which in turn makes use of GCPolicy<void*>. On
MSVC 17 this triggers a static assert because this defaults to
StructGCPolicy<T> which asserts that T is not a pointer type. I don't know why
this don't fail with clang too.

Comment 3

[Steve Fink [:sfink] [:s:]]

Huh. I tried compiling a standalone file:

#include "RootingAPI.h"
static JS::Rooted<void*> blargus(nullptr);

and it failed in the same way with both gcc and clang. But this:

#include "RootingAPI.h"
int main() {
  JS::Rooted<JSObject*> blargus((JSContext*)nullptr);
  blargus.trace(nullptr, "test");
  return 0;
}

does not error out with gcc or clang.

I tried looking at where Rooted<void*> is getting instantiated.

• several reinterpret_cast<Rooted<void*>*>(...) casts. It never dereferences the resulting pointer, so I don't think it should need to instantiate?
• Rooted<void*>* and Rooted<void*>** to manage the position in the stack. Again, just pointers.
• similar for PersistentRooted<void*> (function parameters and reinterpret_cast)

so my guess is that MSVC instantiates for one of the above reasons but gcc and clang don't.

I was looking into this because it bothers me a little that Rooted<void*> isn't a compile error. I'd kind of rather it were. But we're using it to avoid aliasing problems, and I'm never very good about working out the rules there. I'm wondering if we could make a struct RootListEntryType {} and use Rooted<RootListEntryType*>* instead, so that we can forbid Rooted<void*>?

Comment 4

[Steve Fink [:sfink] [:s:]]

Attachment: Bug 1679736 - Prohibit Rooted<void*>

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0e35830f4a25
Prohibit Rooted<void*> r=jonco

Comment 6

[Bogdan Tara[:bogdan_tara | bogdant]]

Backed out changeset 0e35830f4a25 (bug 1679736) for rust failure.

Push with failure: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&selectedTaskRun=I5MdVZ48S6GFrWnQ23PK6w.0&fromchange=2985d50c1c350f570e558dcd002af60a7a87dcb6&searchStr=linux%2Cx64%2Cdebug%2Cspidermonkey%2Cbuilds%2Cspidermonkey-sm-rust-bindings-linux64%2Fdebug%2Crust&tochange=3ad1dbbf981dfd2d7cccc6e79fe52f918311f5ae

Backout link: https://hg.mozilla.org/integration/autoland/rev/3ad1dbbf981dfd2d7cccc6e79fe52f918311f5ae

Failure log: https://treeherder.mozilla.org/logviewer?job_id=323397194&repo=autoland&lineNumber=635

[task 2020-12-03T04:43:44.265Z]   --> js/rust/src/conversions.rs:91:5
[task 2020-12-03T04:43:44.265Z]    |
[task 2020-12-03T04:43:44.265Z] 91 |     #[inline]
[task 2020-12-03T04:43:44.265Z]    |     ^^^^^^^^^
[task 2020-12-03T04:43:44.265Z] 
[task 2020-12-03T04:43:44.285Z] warning: use of deprecated function `std::mem::uninitialized`: use `mem::MaybeUninit` instead
[task 2020-12-03T04:43:44.285Z]     --> /builds/worker/checkouts/gecko/target/debug/build/js-7326033596ab23ce/out/jsapi_debug.rs:8134:37
[task 2020-12-03T04:43:44.285Z]      |
[task 2020-12-03T04:43:44.285Z] 8134 |             let mut __bindgen_tmp = ::std::mem::uninitialized();
[task 2020-12-03T04:43:44.285Z]      |                                     ^^^^^^^^^^^^^^^^^^^^^^^^^
[task 2020-12-03T04:43:44.285Z] 
[task 2020-12-03T04:43:44.285Z] warning: use of deprecated function `std::mem::uninitialized`: use `mem::MaybeUninit` instead
[task 2020-12-03T04:43:44.285Z]     --> /builds/worker/checkouts/gecko/target/debug/build/js-7326033596ab23ce/out/jsapi_debug.rs:9124:37
[task 2020-12-03T04:43:44.285Z]      |
[task 2020-12-03T04:43:44.285Z] 9124 |             let mut __bindgen_tmp = ::std::mem::uninitialized();
[task 2020-12-03T04:43:44.285Z]      |                                     ^^^^^^^^^^^^^^^^^^^^^^^^^
[task 2020-12-03T04:43:44.285Z] 
[task 2020-12-03T04:43:44.285Z] warning: use of deprecated function `std::mem::uninitialized`: use `mem::MaybeUninit` instead
[task 2020-12-03T04:43:44.285Z]     --> /builds/worker/checkouts/gecko/target/debug/build/js-7326033596ab23ce/out/jsapi_debug.rs:9130:37
[task 2020-12-03T04:43:44.285Z]      |
[task 2020-12-03T04:43:44.285Z] 9130 |             let mut __bindgen_tmp = ::std::mem::uninitialized();
[task 2020-12-03T04:43:44.285Z]      |                                     ^^^^^^^^^^^^^^^^^^^^^^^^^
[task 2020-12-03T04:43:44.285Z] 
[task 2020-12-03T04:43:44.285Z] warning: use of deprecated function `std::mem::uninitialized`: use `mem::MaybeUninit` instead
[task 2020-12-03T04:43:44.285Z]      --> /builds/worker/checkouts/gecko/target/debug/build/js-7326033596ab23ce/out/jsapi_debug.rs:12140:37
[task 2020-12-03T04:43:44.285Z]       |
[task 2020-12-03T04:43:44.285Z] 12140 |             let mut __bindgen_tmp = ::std::mem::uninitialized();
[task 2020-12-03T04:43:44.285Z]       |                                     ^^^^^^^^^^^^^^^^^^^^^^^^^
[task 2020-12-03T04:43:44.285Z] 
[task 2020-12-03T04:43:44.291Z] warning: use of deprecated function `std::mem::uninitialized`: use `mem::MaybeUninit` instead
[task 2020-12-03T04:43:44.291Z]     --> /builds/worker/checkouts/gecko/target/debug/build/js-7326033596ab23ce/out/jsapi_debug.rs:1081:41
[task 2020-12-03T04:43:44.291Z]      |
[task 2020-12-03T04:43:44.291Z] 1081 |                 let mut __bindgen_tmp = ::std::mem::uninitialized();
[task 2020-12-03T04:43:44.291Z]      |                                         ^^^^^^^^^^^^^^^^^^^^^^^^^
[task 2020-12-03T04:43:44.291Z] 
[task 2020-12-03T04:43:44.298Z] warning: use of deprecated function `std::mem::uninitialized`: use `mem::MaybeUninit` instead
[task 2020-12-03T04:43:44.299Z]     --> /builds/worker/checkouts/gecko/target/debug/build/js-7326033596ab23ce/out/jsapi_debug.rs:3403:41
[task 2020-12-03T04:43:44.299Z]      |
[task 2020-12-03T04:43:44.299Z] 3403 |                 let mut __bindgen_tmp = ::std::mem::uninitialized();
[task 2020-12-03T04:43:44.299Z]      |                                         ^^^^^^^^^^^^^^^^^^^^^^^^^
[task 2020-12-03T04:43:44.299Z] 
[task 2020-12-03T04:43:44.299Z] warning: use of deprecated function `std::mem::uninitialized`: use `mem::MaybeUninit` instead
[task 2020-12-03T04:43:44.299Z]     --> /builds/worker/checkouts/gecko/target/debug/build/js-7326033596ab23ce/out/jsapi_debug.rs:3686:41
[task 2020-12-03T04:43:44.299Z]      |
[task 2020-12-03T04:43:44.299Z] 3686 |                 let mut __bindgen_tmp = ::std::mem::uninitialized();
[task 2020-12-03T04:43:44.299Z]      |                                         ^^^^^^^^^^^^^^^^^^^^^^^^^
[task 2020-12-03T04:43:44.299Z] 
[task 2020-12-03T04:43:44.488Z] error[E0308]: mismatched types
[task 2020-12-03T04:43:44.488Z]    --> js/rust/src/rust.rs:382:22
[task 2020-12-03T04:43:44.489Z]     |
[task 2020-12-03T04:43:44.489Z] 382 |         self.stack = Self::get_root_stack(cx);
[task 2020-12-03T04:43:44.489Z]     |                      ^^^^^^^^^^^^^^^^^^^^^^^^ expected struct `RootListEntry`, found enum `c_void`
[task 2020-12-03T04:43:44.489Z]     |
[task 2020-12-03T04:43:44.489Z]     = note: expected raw pointer `*mut *mut root::JS::Rooted<*mut RootListEntry>`
[task 2020-12-03T04:43:44.489Z]                found raw pointer `*mut *mut root::JS::Rooted<*mut c_void>`
[task 2020-12-03T04:43:44.489Z] 
[task 2020-12-03T04:43:44.852Z] error: aborting due to previous error; 27 warnings emitted
[task 2020-12-03T04:43:44.852Z] 
[task 2020-12-03T04:43:44.852Z] For more information about this error, try `rustc --explain E0308`.
[task 2020-12-03T04:43:44.856Z] error: could not compile `js`
[task 2020-12-03T04:43:44.856Z] 
[task 2020-12-03T04:43:44.856Z] Caused by:
[task 2020-12-03T04:43:44.857Z]   process didn't exit successfully: `/builds/worker/fetches/rustc/bin/rustc --crate-name js js/rust/src/lib.rs --error-format=json --json=diagnostic-rendered-ansi --crate-type lib --emit=dep-info,metadata,link -C opt-level=1 -C embed-bitcode=no -C debuginfo=2 -C debug-assertions=on --cfg 'feature="debugmozjs"' -C metadata=44cf8390f213d5d8 -C extra-filename=-44cf8390f213d5d8 --out-dir /builds/worker/checkouts/gecko/target/debug/deps -C incremental=/builds/worker/checkouts/gecko/target/debug/incremental -L dependency=/builds/worker/checkouts/gecko/target/debug/deps --extern lazy_static=/builds/worker/checkouts/gecko/target/debug/deps/liblazy_static-be04180fbe80739b.rmeta --extern libc=/builds/worker/checkouts/gecko/target/debug/deps/liblibc-534c2bfc29234e51.rmeta --extern log=/builds/worker/checkouts/gecko/target/debug/deps/liblog-6f4ba50bd18f035c.rmeta --extern mozjs_sys=/builds/worker/checkouts/gecko/target/debug/deps/libmozjs_sys-22108e2bc459b72c.rmeta --extern num_traits=/builds/worker/checkouts/gecko/target/debug/deps/libnum_traits-bde488b92192e017.rmeta -L native=/builds/worker/checkouts/gecko/target/debug/build/js-dd59fbd9ffae6ec7/out/lib -l static=jsglue -L native=/builds/worker/checkouts/gecko/target/debug/build/mozjs_sys-13fb300f8ab7f4fd/out/js/src/build -L native=/builds/worker/checkouts/gecko/target/debug/build/mozjs_sys-13fb300f8ab7f4fd/out/js/src -L native=/builds/worker/checkouts/gecko/target/debug/build/mozjs_sys-13fb300f8ab7f4fd/out/x86_64-unknown-linux-gnu/debug -L native=/builds/worker/checkouts/gecko/target/debug/build/mozjs_sys-13fb300f8ab7f4fd/out/dist/bin -L native=/usr/lib/x86_64-linux-gnu` (exit code: 1)
[task 2020-12-03T04:43:44.857Z] warning: build failed, waiting for other jobs to finish...
[task 2020-12-03T04:43:44.935Z] error[E0308]: mismatched types
[task 2020-12-03T04:43:44.935Z]    --> js/rust/src/rust.rs:382:22
[task 2020-12-03T04:43:44.935Z]     |
[task 2020-12-03T04:43:44.935Z] 382 |         self.stack = Self::get_root_stack(cx);
[task 2020-12-03T04:43:44.935Z]     |                      ^^^^^^^^^^^^^^^^^^^^^^^^ expected struct `RootListEntry`, found enum `c_void`
[task 2020-12-03T04:43:44.935Z]     |
[task 2020-12-03T04:43:44.935Z]     = note: expected raw pointer `*mut *mut root::JS::Rooted<*mut RootListEntry>`
[task 2020-12-03T04:43:44.935Z]                found raw pointer `*mut *mut root::JS::Rooted<*mut c_void>`
[task 2020-12-03T04:43:44.935Z] 
[task 2020-12-03T04:43:45.811Z] error: aborting due to previous error; 27 warnings emitted
[task 2020-12-03T04:43:45.811Z] 
[task 2020-12-03T04:43:45.811Z] For more information about this error, try `rustc --explain E0308`.
[task 2020-12-03T04:43:45.957Z] error: build failed
[taskcluster 2020-12-03 04:43:46.267Z] === Task Finished ===
[taskcluster 2020-12-03 04:43:46.390Z] Artifact "public/build" not found at "/builds/worker/artifacts/"
[taskcluster 2020-12-03 04:43:46.550Z] Unsuccessful task run with exit code: 101 completed in 297.49 seconds

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a68d3b2e8748
Prohibit Rooted<void*> r=jonco

Comment 8

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/a68d3b2e8748