Closed Bug 1600096 Opened 2 years ago Closed 2 years ago

Disable RefPtr conversion constructors when underlying pointer types are not convertible

Categories

(Core :: MFBT, task)

task
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla73
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.

(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?)

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>.

(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.

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>();
Attachment #9112317 - Attachment description: Bug 1600096 - Disable RefPtr conversion constructors when underlying pointer types are not convertible. r=froydnj → Bug 1600096 - Disable RefPtr conversion constructors when underlying pointer types are not convertible. r=#dom-workers-and-storage
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

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

Flags: needinfo?(sgiesecke)
Depends on: 1602514
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
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla73
Flags: needinfo?(sgiesecke)

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            ^

(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.

Flags: needinfo?(ricotz)

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.

Flags: needinfo?(ricotz)

(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.

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)

(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.

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.

(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!

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

You need to log in before you can comment on or make changes to this bug.