1600096 - Disable RefPtr conversion constructors when underlying pointer types are not convertible

Status: RESOLVED FIXED

(Core :: MFBT, task)

Description

[Simon Giesecke [:sg] [he/him]]

Disable the RefPtr conversion constructors if the underlying pointer types are not convertible such that, e.g., you can write something like

    struct Foo {}; struct Bar : Foo {};
    const auto f = condition ? MakeRefPtr<Bar>() : MakeRefPtr<Foo>();

Without disabling, the conversion is ambiguous.

Comment 1

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1600096 - Disable RefPtr conversion constructors when underlying pointer types are not convertible. r=#dom-workers-and-storage

Comment 2

[Nathan Froyd [:froydnj]]

(In reply to Simon Giesecke [:sg] [he/him] from comment #0)

Disable the RefPtr conversion constructors if the underlying pointer types are not convertible such that, e.g., you can write something like

    struct Foo {}; struct Bar : Foo {};
    const auto f = condition ? MakeRefPtr<Bar>() : MakeRefPtr<Foo>();

Without disabling, the conversion is ambiguous.

I am not sure we actually want to encourage people to write this, because it's not obvious what f is in general. (ISTR something about temporaries being formed for the individual MakeRefPtr calls as well as the entirety of the ?: operator that required the copy constructor to be invoked, thus making this sort of code inefficient, but maybe that restriction has gone away?)

Comment 3

[Simon Giesecke [:sg] [he/him]]

What are you concerned specifically about here? auto or the ?:?

I am not sure if I get where a copy constructor would be invoked here. You can do the same with a std::unique_ptr, e.g., which is not copyable at all:
const auto f = condition ? std::make_unique<Bar>() : std::make_unique<Foo>();
or see https://cppinsights.io/lnk?code=I2luY2x1ZGUgPG1lbW9yeT4KCnN0cnVjdCBGb28ge307CnN0cnVjdCBCYXIgOiBGb28ge307CgphdXRvIGZvbyhib29sIGNvbmQpIHsKICByZXR1cm4gY29uZCA/IHN0ZDo6bWFrZV91bmlxdWU8Rm9vPigpIDogc3RkOjptYWtlX3VuaXF1ZTxCYXI+KCk7Cn0=&std=cpp2a&rev=1.0

The type of f is unambiguously RefPtr<Foo>.

Comment 4

[Nathan Froyd [:froydnj]]

(In reply to Simon Giesecke [:sg] [he/him] from comment #3)

What are you concerned specifically about here? auto or the ?:?

Enabling the use of this code with auto.

Comment 5

[Simon Giesecke [:sg] [he/him]]

auto not the essential point here, I merely wrote that by habit. The same modifications are necessary to allow writing:

const RefPtr<Foo> f = condition ? MakeRefPtr<Bar>() : MakeRefPtr<Foo>();

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d561645c3554
Disable RefPtr conversion constructors when underlying pointer types are not convertible. r=froydnj

Comment 7

[Alexandru Michis [:malexandru]]

Backed out changeset d561645c3554 (Bug 1600096) for causing bustages in RefPtr.h

Backout link: https://hg.mozilla.org/integration/autoland/rev/0a4da12080809dc07c3c71cc27489f17348af7ec

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&collapsedPushes=598800&group_state=expanded&searchStr=linux%2Cx64%2Cdebug%2Cspidermonkey%2Cbuilds%2Cspidermonkey-sm-rust-bindings-linux64%2Fdebug%2Csm%28rust%29&tochange=0a4da12080809dc07c3c71cc27489f17348af7ec&fromchange=bf906a905a2635f08d055733627564a7f98b358b&selectedJob=280297638

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=280297638&repo=autoland&lineNumber=242

[task 2019-12-09T15:49:18.844Z] Caused by:
[task 2019-12-09T15:49:18.844Z] process didn't exit successfully: /builds/worker/workspace/build/src/target/debug/build/js-bf233af2d68c7af1/build-script-build (exit code: 101)
[task 2019-12-09T15:49:18.844Z] --- stderr
[task 2019-12-09T15:49:18.844Z] /builds/worker/workspace/build/src/target/debug/build/mozjs_sys-1fe9b0a8f12e1dd8/out/dist/include/mozilla/RefPtr.h:114:46: error: no member named 'is_convertible_v' in namespace 'std'
[task 2019-12-09T15:49:18.844Z] /builds/worker/workspace/build/src/target/debug/build/mozjs_sys-1fe9b0a8f12e1dd8/out/dist/include/mozilla/RefPtr.h:114:63: error: 'I' does not refer to a value
[task 2019-12-09T15:49:18.844Z] /builds/worker/workspace/build/src/target/debug/build/mozjs_sys-1fe9b0a8f12e1dd8/out/dist/include/mozilla/RefPtr.h:113:22: note: declared here
[task 2019-12-09T15:49:18.844Z] /builds/worker/workspace/build/src/target/debug/build/mozjs_sys-1fe9b0a8f12e1dd8/out/dist/include/mozilla/RefPtr.h:114:65: error: expected expression
[task 2019-12-09T15:49:18.844Z] /builds/worker/workspace/build/src/target/debug/build/mozjs_sys-1fe9b0a8f12e1dd8/out/dist/include/mozilla/RefPtr.h:115:16: error: expected ',' or '>' in template-parameter-list
[task 2019-12-09T15:49:18.844Z] /builds/worker/workspace/build/src/target/debug/build/mozjs_sys-1fe9b0a8f12e1dd8/out/dist/include/mozilla/RefPtr.h:120:23: error: expected member name or ';' after declaration specifiers
[task 2019-12-09T15:49:18.844Z] /builds/worker/workspace/build/src/target/debug/build/mozjs_sys-1fe9b0a8f12e1dd8/out/dist/include/mozilla/RefPtr.h:114:46: error: no member named 'is_convertible_v' in namespace 'std', err: true
[task 2019-12-09T15:49:18.844Z] /builds/worker/workspace/build/src/target/debug/build/mozjs_sys-1fe9b0a8f12e1dd8/out/dist/include/mozilla/RefPtr.h:114:63: error: 'I' does not refer to a value, err: true
[task 2019-12-09T15:49:18.844Z] /builds/worker/workspace/build/src/target/debug/build/mozjs_sys-1fe9b0a8f12e1dd8/out/dist/include/mozilla/RefPtr.h:114:65: error: expected expression, err: true
[task 2019-12-09T15:49:18.844Z] /builds/worker/workspace/build/src/target/debug/build/mozjs_sys-1fe9b0a8f12e1dd8/out/dist/include/mozilla/RefPtr.h:115:16: error: expected ',' or '>' in template-parameter-list, err: true
[task 2019-12-09T15:49:18.844Z] /builds/worker/workspace/build/src/target/debug/build/mozjs_sys-1fe9b0a8f12e1dd8/out/dist/include/mozilla/RefPtr.h:120:23: error: expected member name or ';' after declaration specifiers, err: true
[task 2019-12-09T15:49:18.844Z] thread 'main' panicked at 'Should generate JSAPI bindings OK: ()', src/libcore/result.rs:1165:5
[task 2019-12-09T15:49:18.844Z] stack backtrace:
[task 2019-12-09T15:49:18.844Z] 0: backtrace::backtrace::libunwind::trace
[task 2019-12-09T15:49:18.844Z] at /cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.37/src/backtrace/libunwind.rs:88
[task 2019-12-09T15:49:18.844Z] 1: backtrace::backtrace::trace_unsynchronized
[task 2019-12-09T15:49:18.844Z] at /cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.37/src/backtrace/mod.rs:66

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/41f4ca1d2e22
Disable RefPtr conversion constructors when underlying pointer types are not convertible. r=froydnj

Comment 9

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/41f4ca1d2e22

Comment 10

[Rico Tzschichholz]

Looks like this still causes build failures.
https://launchpadlibrarian.net/455512183/buildlog_ubuntu-focal-amd64.firefox-trunk_73.0~a1~hg20191213r506826-0ubuntu0.20.04.1~umd1_BUILDING.txt.gz

1:15.95 In file included from Unified_cpp_mfbt_tests_gtest0.cpp:11:
 1:15.96 In file included from /<<PKGBUILDDIR>>/mfbt/tests/gtest/TestLinkedList.cpp:9:
 1:15.96 In file included from /<<PKGBUILDDIR>>/obj-x86_64-linux-gnu/dist/include/mozilla/LinkedList.h:71:
 1:15.96 /<<PKGBUILDDIR>>/obj-x86_64-linux-gnu/dist/include/mozilla/RefPtr.h:116:46: error: no template named 'is_convertible_v' in namespace 'std'; did you mean 'is_convertible'?
 1:15.96             typename = std::enable_if_t<std::is_convertible_v<I*, T*>>>
 1:15.96                                         ~~~~~^~~~~~~~~~~~~~~~
 1:15.96                                              is_convertible
 1:15.96 /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/type_traits:1335:12: note: 'is_convertible' declared here
 1:15.96     struct is_convertible
 1:15.96            ^

Comment 11

[Nathan Froyd [:froydnj]]

(In reply to Rico Tzschichholz from comment #10)

Looks like this still causes build failures.
https://launchpadlibrarian.net/455512183/buildlog_ubuntu-focal-amd64.firefox-trunk_73.0~a1~hg20191213r506826-0ubuntu0.20.04.1~umd1_BUILDING.txt.gz

1:15.95 In file included from Unified_cpp_mfbt_tests_gtest0.cpp:11:
 1:15.96 In file included from /<<PKGBUILDDIR>>/mfbt/tests/gtest/TestLinkedList.cpp:9:
 1:15.96 In file included from /<<PKGBUILDDIR>>/obj-x86_64-linux-gnu/dist/include/mozilla/LinkedList.h:71:
 1:15.96 /<<PKGBUILDDIR>>/obj-x86_64-linux-gnu/dist/include/mozilla/RefPtr.h:116:46: error: no template named 'is_convertible_v' in namespace 'std'; did you mean 'is_convertible'?
 1:15.96             typename = std::enable_if_t<std::is_convertible_v<I*, T*>>>
 1:15.96                                         ~~~~~^~~~~~~~~~~~~~~~
 1:15.96                                              is_convertible
 1:15.96 /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/type_traits:1335:12: note: 'is_convertible' declared here
 1:15.96     struct is_convertible
 1:15.96            ^

Can you file a separate bug for this, and in the process of doing so, can you verify that Ubuntu's build is actually using -std=gnu++17? I can see the definition of is_convertible_v in GCC 7.40 and 9.2.0 headers, so somehow your build must not be picking up that flag.

Comment 12

[Rico Tzschichholz]

Can you file a separate bug for this, and in the process of doing so, can you verify that Ubuntu's build is actually using -std=gnu++17? I can see the definition of is_convertible_v in GCC 7.40 and 9.2.0 headers, so somehow your build must not be picking up that flag.

The full log shows the -std=gnu++17 is defined and added, but to make it reach mfbt/tests would be the responsibility of the mozilla's buildsystem?

Obviously reverting https://hg.mozilla.org/mozilla-central/rev/41f4ca1d2e22 makes mfbt build again.

Comment 13

[Nathan Froyd [:froydnj]]

(In reply to Rico Tzschichholz from comment #12)

Can you file a separate bug for this, and in the process of doing so, can you verify that Ubuntu's build is actually using -std=gnu++17? I can see the definition of is_convertible_v in GCC 7.40 and 9.2.0 headers, so somehow your build must not be picking up that flag.

The full log shows the -std=gnu++17 is defined and added, but to make it reach mfbt/tests would be the responsibility of the mozilla's buildsystem?

Sure, the question is whether there's some bug going on, perhaps specific to GCC 9, that is causing problems. I can't tell whether the particular file that you're seeing problems with is actually getting compiled with -std=gnu++17. I can see that it is getting compiled with -std=gnu++17 in our automation:

[task 2019-12-13T09:59:35.436Z] 09:59:35     INFO -  /builds/worker/fetches/sccache/sccache /builds/worker/fetches/clang/bin/clang++ -std=gnu++17 -o Unified_cpp_mfbt_tests_gtest0.o -c  -I/builds/worker/workspace/build/src/obj-firefox/dist/stl_wrappers -I/builds/worker/workspace/build/src/obj-firefox/dist/system_wrappers -include /builds/worker/workspace/build/src/config/gcc_hidden.h -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fstack-protector-strong -DNDEBUG=1 -DTRIMMED=1 -DSTATIC_EXPORTABLE_JS_API -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -I/builds/worker/workspace/build/src/mfbt/tests/gtest -I/builds/worker/workspace/build/src/obj-firefox/mfbt/tests/gtest -I/builds/worker/workspace/build/src/obj-firefox/dist/include -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nspr -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nss -fPIC -DMOZILLA_CLIENT -include /builds/worker/workspace/build/src/obj-firefox/mozilla-config.h -Qunused-arguments -Qunused-arguments -Wall -Wbitfield-enum-conversion -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wshadow-field-in-constructor-modified -Wsign-compare -Wtype-limits -Wunreachable-code -Wunreachable-code-return -Wwrite-strings -Wno-invalid-offsetof -Wclass-varargs -Wfloat-overflow-conversion -Wfloat-zero-conversion -Wloop-analysis -Wc++2a-compat -Wcomma -Wimplicit-fallthrough -Werror=non-literal-null-conversion -Wstring-conversion -Wtautological-overlap-compare -Wtautological-unsigned-enum-zero-compare -Wtautological-unsigned-zero-compare -Wno-error=tautological-type-limit-compare -Wno-inline-new-delete -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=backend-plugin -Wno-error=return-std-move -Wno-error=atomic-alignment -Wformat -Wformat-security -Wno-gnu-zero-variadic-macro-arguments -Wno-unknown-warning-option -Wno-return-type-c-linkage -D_GLIBCXX_USE_CXX11_ABI=0 -fno-sized-deallocation -fno-aligned-new -fcrash-diagnostics-dir=/builds/worker/artifacts -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -pthread -pipe -g -Xclang -load -Xclang /builds/worker/workspace/build/src/obj-firefox/build/clang-plugin/libclang-plugin.so -Xclang -add-plugin -Xclang moz-check -O2 -fno-omit-frame-pointer -funwind-tables -Werror  -MD -MP -MF .deps/Unified_cpp_mfbt_tests_gtest0.o.pp   Unified_cpp_mfbt_tests_gtest0.cpp

so I'd be curious to know if the same flag is being passed in the build you're doing and if the flag is getting passed, why you're still seeing these errors.

Comment 14

[Rico Tzschichholz]

The full log shows the -std=gnu++17 is defined and added, but to make it reach mfbt/tests would be the responsibility of the mozilla's buildsystem?

Sure, the question is whether there's some bug going on, perhaps specific to GCC 9, that is causing problems. I can't tell whether the particular file that you're seeing problems with is actually getting compiled with -std=gnu++17. I can see that it is getting compiled with -std=gnu++17 in our automation:

This is happening with all supported ubuntu series. Using clang (8 and 9) for compilation with libstdc++6 headers from gcc (5.4.0, 8.3.0 and 9.2.1)

Comment 15

[Nathan Froyd [:froydnj]]

(In reply to Rico Tzschichholz from comment #14)

The full log shows the -std=gnu++17 is defined and added, but to make it reach mfbt/tests would be the responsibility of the mozilla's buildsystem?

Sure, the question is whether there's some bug going on, perhaps specific to GCC 9, that is causing problems. I can't tell whether the particular file that you're seeing problems with is actually getting compiled with -std=gnu++17. I can see that it is getting compiled with -std=gnu++17 in our automation:

This is happening with all supported ubuntu series. Using clang (8 and 9) for compilation with libstdc++6 headers from gcc (5.4.0, 8.3.0 and 9.2.1)

The headers are from libstdc++ 6 despite the path being /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/type_traits ?

In any event, I think we don't support libstdc++ < 7 anymore since we moved to C++17. We do need to get better at failing the build much earlier as a result of that.

Comment 16

[Rico Tzschichholz]

The compiler is set in .mozconfig like this

mk_add_options "export CC=clang-9"
mk_add_options "export CXX=clang++-9"

which seems to be culprit and -std=gnu++17 is not added by check_compiler() in toolchain.configure.

Not exporting the wanted compiler version and leaving its discovery to the buildsystem does work out, but we require to set it for older series.

Comment 17

[Nathan Froyd [:froydnj]]

(In reply to Rico Tzschichholz from comment #16)

The compiler is set in .mozconfig like this

mk_add_options "export CC=clang-9"
mk_add_options "export CXX=clang++-9"

which seems to be culprit and -std=gnu++17 is not added by check_compiler() in toolchain.configure.

Not exporting the wanted compiler version and leaving its discovery to the buildsystem does work out, but we require to set it for older series.

I find this difficult to believe, since we compile with clang 9 in automation, with an explicit export CC/export CXX, and have no problem having -std=gnu++17 added.

I'm not sure what's different about your setup, and comment 16 and comment 12 are saying different things: the former says that -std=gnu++17 is not added, whereas the latter says that it is. It's pretty hard to get a good idea about what's going on with conflicting and incomplete information.

Please file a different bug and please provide full information about your setup: compiler version, C++ standard library version, full build logs, and config.status from the object directory. Thanks!

Comment 18

[Rico Tzschichholz]

Please file a different bug and please provide full information about your setup: compiler version, C++ standard library version, full build logs, and config.status from the object directory. Thanks!

https://bugzilla.mozilla.org/show_bug.cgi?id=1603860