Disable RefPtr conversion constructors when underlying pointer types are not convertible
Categories
(Core :: MFBT, task)
Tracking
()
Tracking | Status | |
---|---|---|
firefox73 | --- | fixed |
People
(Reporter: sg, Assigned: sg)
References
Details
Attachments
(1 file)
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.
Assignee | ||
Comment 1•3 years ago
|
||
![]() |
||
Comment 2•3 years ago
|
||
(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?)
Assignee | ||
Comment 3•3 years ago
|
||
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•3 years ago
|
||
(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
.
Assignee | ||
Comment 5•3 years ago
|
||
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>();
Updated•3 years ago
|
Pushed by sgiesecke@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d561645c3554 Disable RefPtr conversion constructors when underlying pointer types are not convertible. r=froydnj
Comment 7•3 years ago
|
||
Backed out changeset d561645c3554 (Bug 1600096) for causing bustages in RefPtr.h
Backout link: https://hg.mozilla.org/integration/autoland/rev/0a4da12080809dc07c3c71cc27489f17348af7ec
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
Pushed by sgiesecke@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/41f4ca1d2e22 Disable RefPtr conversion constructors when underlying pointer types are not convertible. r=froydnj
Comment 9•3 years ago
|
||
bugherder |
Assignee | ||
Updated•3 years ago
|
Comment 10•3 years ago
|
||
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•3 years ago
|
||
(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.gz1: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•3 years ago
|
||
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 ofis_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•3 years ago
|
||
(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 ofis_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 reachmfbt/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•3 years ago
|
||
The full log shows the
-std=gnu++17
is defined and added, but to make it reachmfbt/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•3 years ago
|
||
(In reply to Rico Tzschichholz from comment #14)
The full log shows the
-std=gnu++17
is defined and added, but to make it reachmfbt/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 withlibstdc++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•3 years ago
|
||
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•3 years ago
|
||
(In reply to Rico Tzschichholz from comment #16)
The compiler is set in
.mozconfig
like thismk_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 bycheck_compiler()
intoolchain.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•3 years ago
|
||
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!
Description
•