mojo: Recent unordered_map to base::flat_map conversion breaks GCC build |
||||
Issue descriptionhttps://chromium-review.googlesource.com/1000393 ("Mojo C++ Bindings: unordered_map => flat_map") is currently causing GCC to fail with an error that looks like this: g++ -MMD -MF obj/mojo/public/interfaces/bindings/tests/test_interfaces/test_unions.mojom.o.d -DV8_DEPRECATION_WARNINGS -DUSE_UDEV -DUSE_AURA=1 -DUSE_GLIB=1 -DUSE_NSS_CERTS=1 -DUSE_X11=1 -DFULL_SAFE_BROWSING -DSAFE_BROWSING_CSD -DSAFE_BROWSING_DB_LOCAL -DCHROMIUM_BUILD -DFIELDTRIAL_TESTING_ENABLED -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D_FORTIFY_SOURCE=2 -DCOMPONENT_BUILD -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 -DGLIB_VERSION_MAX_ALLOWED=GLIB_VERSION_2_32 -DGLIB_VERSION_MIN_REQUIRED=GLIB_VERSION_2_26 -DGOOGLE_PROTOBUF_NO_RTTI -DGOOGLE_PROTOBUF_NO_STATIC_INITIALIZER -DHAVE_PTHREAD -DPROTOBUF_USE_DLLS -I../.. -Igen -I../../third_party/protobuf/src -fno-strict-aliasing --param=ssp-buffer-size=4 -fstack-protector -Wno-builtin-macro-redefined -D__DATE__= -D__TIME__= -D__TIMESTAMP__= -funwind-tables -fPIC -pipe -B../../third_party/binutils/Linux_x64/Release/bin -pthread -m64 -march=x86-64 -Wall -Wno-unused-local-typedefs -Wno-maybe-uninitialized -Wno-deprecated-declarations -fno-delete-null-pointer-checks -Wno-comments -Wno-missing-field-initializers -Wno-unused-parameter -O2 -fno-ident -fdata-sections -ffunction-sections -fno-omit-frame-pointer -g0 -fvisibility=hidden -isystem../../../../../../usr/include/glib-2.0 -isystem../../../../../../usr/lib64/glib-2.0/include -std=gnu++14 -Wno-narrowing -fno-exceptions -fno-rtti -fvisibility-inlines-hidden -c gen/mojo/public/interfaces/bindings/tests/test_unions.mojom.cc -o obj/mojo/public/interfaces/bindings/tests/test_interfaces/test_unions.mojom.o gen/mojo/public/interfaces/bindings/tests/test_unions.mojom.cc: In member function ‘void mojo::test::ObjectUnion::set_f_map_int8(const base::flat_map<std::__cxx11::basic_string<char>, signed char>&)’: gen/mojo/public/interfaces/bindings/tests/test_unions.mojom.cc:455:30: error: use of deleted function ‘base::flat_map<std::__cxx11::basic_string<char>, signed char>::flat_map(const base::flat_map<std::__cxx11::basic_string<char>, signed char>&)’ std::move(f_map_int8)); ^ In file included from ../../mojo/public/cpp/bindings/clone_traits.h:11:0, from gen/mojo/public/interfaces/bindings/tests/test_unions.mojom.h:22, from gen/mojo/public/interfaces/bindings/tests/test_unions.mojom.cc:15: ../../base/containers/flat_map.h:152:7: note: ‘base::flat_map<std::__cxx11::basic_string<char>, signed char>::flat_map(const base::flat_map<std::__cxx11::basic_string<char>, signed char>&)’ is implicitly declared as deleted because ‘base::flat_map<std::__cxx11::basic_string<char>, signed char>’ declares a move constructor or move assignment operator class flat_map : public ::base::internal::flat_tree< ^~~~~~~~ I believe this is yet another instance of https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84782, which we worked around in the past with e.g. https://chromium-review.googlesource.com/c/chromium/src/+/944403. While part of it seems to be a GCC bug caused by flat_map inheriting flat_tree's constructors and assignment operators, it also highlights the fact that the generated mojo code is going through multiple functions taking const lvalue references (e.g. NewFMapInt8(), ObjectUnion::set_f_map_int8()) while passing these const lvalue references forward with std::move(). However, std::move() in this case is creating a const rvalue reference, which doesn't match the signature of a move constructor or move assignment operator and the compiler's actually falling back to copy operations. At the moment, I can think of two possible solutions: either re-adding the explicit constructors and operator=() overloads to base::flat_map that were removed in https://chromium-review.googlesource.com/c/chromium/src/+/705955 ("base::flat_map was missing defaults for duplication handling") or adjusting mojo to use move semantics correctly.
,
Apr 26 2018
,
Apr 26 2018
,
Apr 30 2018
//base folks: would you be OK with declaring flat_map's constructors and operator='s rather than inheriting them from flat_tree?
,
May 1 2018
It sounds okay, with comments explaining.
,
May 4 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1131840b370f71d4c60d2427254381505dd06571 commit 1131840b370f71d4c60d2427254381505dd06571 Author: Raphael Kubo da Costa <raphael.kubo.da.costa@intel.com> Date: Fri May 04 20:15:13 2018 flat_map: Readd constructors and assignment operator overloads. https://chromium-review.googlesource.com/c/chromium/src/+/705955 ("base::flat_map was missing defaults for duplication handling") removed them in favor of inheriting them from flat_tree. This can cause problems with GCC due to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84782, and the build has been failing with it since https://chromium-review.googlesource.com/1000393 ("Mojo C++ Bindings: unordered_map => flat_map") with a message like this: gen/mojo/public/interfaces/bindings/tests/test_unions.mojom.cc: In member function ‘void mojo::test::ObjectUnion::set_f_map_int8(const base::flat_map<std::__cxx11::basic_string<char>, signed char>&)’: gen/mojo/public/interfaces/bindings/tests/test_unions.mojom.cc:455:30: error: use of deleted function ‘base::flat_map<std::__cxx11::basic_string<char>, signed char>::flat_map(const base::flat_map<std::__cxx11::basic_string<char>, signed char>&)’ std::move(f_map_int8)); ^ In file included from ../../mojo/public/cpp/bindings/clone_traits.h:11:0, from gen/mojo/public/interfaces/bindings/tests/test_unions.mojom.h:22, from gen/mojo/public/interfaces/bindings/tests/test_unions.mojom.cc:15: ../../base/containers/flat_map.h:152:7: note: ‘base::flat_map<std::__cxx11::basic_string<char>, signed char>::flat_map(const base::flat_map<std::__cxx11::basic_string<char>, signed char>&)’ is implicitly declared as deleted because ‘base::flat_map<std::__cxx11::basic_string<char>, signed char>’ declares a move constructor or move assignment operator class flat_map : public ::base::internal::flat_tree< ^~~~~~~~ Work around it by replacing the inheritance statements with actual constructors and operator=() overloads again. This change does not completely revert https://chromium-review.googlesource.com/1000393, as the constructors maintain their new signature (so that they look like flat_tree's). Bug: 819294, 837221 Change-Id: I8f37a2c8aa269b3c1025cdc009518a7709e95947 Reviewed-on: https://chromium-review.googlesource.com/1038103 Commit-Queue: Raphael Kubo da Costa <raphael.kubo.da.costa@intel.com> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Reviewed-by: kylechar <kylechar@chromium.org> Cr-Commit-Position: refs/heads/master@{#556168} [modify] https://crrev.com/1131840b370f71d4c60d2427254381505dd06571/base/containers/flat_map.h
,
May 7 2018
|
||||
►
Sign in to add a comment |
||||
Comment 1 by roc...@chromium.org
, Apr 26 2018