Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1163943004:
Make SharedMemoryHandle a class on Mac. (Closed)
|
Created:
5 years, 6 months ago by erikchen Modified:
5 years, 6 months ago CC:
reveman, chromium-reviews, darin-cc_chromium.org, Daniele Castagna, erikwright+watch_chromium.org, gavinp+memory_chromium.org, jam Base URL:
https://chromium.googlesource.com/chromium/src.git@shared_memory_make_class3_base Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMac: Make SharedMemoryHandle its own class.
This is in prepration for allowing SharedMemoryHandle to be backed by both Mach
primitives and POSIX fds. Other than a slight change in the wire format for
SharedMemoryHandle on Mac, there are no other intended behavior changes.
This CL makes a new file shared_memory_handle.h to hold the now expanded logic
for SharedMemoryHandle. This CL forks the file shared_memory_posix.cc to
shared_memory_mac.cc, since the implementations are about to heavily diverge.
BUG=466437
NOPRESUBMIT=true
Committed: https://crrev.com/cc9f9127f979acd6eb22103656057a655a0e76a6
Cr-Commit-Position: refs/heads/master@{#335360}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : Rebase against top of tree. #Patch Set 6 : Fork shared_memory_posix to shared_memory_mac. #Patch Set 7 : Remove an unnecessary (and incorrect) include on windows. #Patch Set 8 : Use right include on windows. #
Total comments: 6
Patch Set 9 : Comments from rsesek. #Patch Set 10 : Fix NACL rewriting of IPC messages. #
Total comments: 11
Patch Set 11 : Rebase against top of tree. #Patch Set 12 : Comments from rsesek, round two. #Patch Set 13 : Fix compile error from rebase. #Patch Set 14 : Fix logic error. #
Total comments: 31
Patch Set 15 : Comments from tsepez. #
Total comments: 2
Patch Set 16 : Rebase against top of tree. #Patch Set 17 : Comments from piman. #Patch Set 18 : Rebase against top of tree. #Patch Set 19 : Fix up shared_memory_mac.cc #Patch Set 20 : Remove more unused headers that magically appeared from rebase. #
Messages
Total messages: 96 (44 generated)
The CQ bit was checked by erikchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1163943004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2015/06/03 22:13:12, commit-bot: I haz the power wrote: > Dry run: Try jobs failed on following builders: > linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) > ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) > mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) > mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) duh, test failures are because this CL relies on several minor refactors which haven't landed yet: https://codereview.chromium.org/1152273005/ https://codereview.chromium.org/1143323010/ https://codereview.chromium.org/1166443004/
On 2015/06/03 22:15:07, erikchen wrote: > On 2015/06/03 22:13:12, commit-bot: I haz the power wrote: > > Dry run: Try jobs failed on following builders: > > linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > > ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) > > ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) > > mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) > > mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) > > duh, test failures are because this CL relies on several minor refactors which > haven't landed yet: > > https://codereview.chromium.org/1152273005/ > https://codereview.chromium.org/1143323010/ > https://codereview.chromium.org/1166443004/ base_unittests passes locally. All targets compile locally (Mac).
erikchen@chromium.org changed reviewers: + rsesek@chromium.org
rsesek: Please review.
The CQ bit was checked by erikchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1163943004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by erikchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1163943004/80001
Patchset #6 (id:100001) has been deleted
The CQ bit was checked by erikchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1163943004/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
https://codereview.chromium.org/1163943004/diff/160001/base/memory/shared_mem... File base/memory/shared_memory_handle.h (right): https://codereview.chromium.org/1163943004/diff/160001/base/memory/shared_mem... base/memory/shared_memory_handle.h:34: POSIX_FD = 0, I think just POSIX and MACH are better names. https://codereview.chromium.org/1163943004/diff/160001/base/memory/shared_mem... base/memory/shared_memory_handle.h:63: uint32_t GetMechanismForWire() const; These names are kind of awkward. Why not just have SharedMemoryHandle pickle and depickle itself? Add WriteToPickle(Pickle*) and ReadFromPickle(PickleIterator*), and then none of the implementation details need to leak. https://codereview.chromium.org/1163943004/diff/160001/base/memory/shared_mem... File base/memory/shared_memory_handle_mac.cc (right): https://codereview.chromium.org/1163943004/diff/160001/base/memory/shared_mem... base/memory/shared_memory_handle_mac.cc:64: CHECK(mechanism_ == POSIX_FD); CHECK_EQ, and throughout.
rsesek: PTAL https://codereview.chromium.org/1163943004/diff/160001/base/memory/shared_mem... File base/memory/shared_memory_handle.h (right): https://codereview.chromium.org/1163943004/diff/160001/base/memory/shared_mem... base/memory/shared_memory_handle.h:34: POSIX_FD = 0, On 2015/06/05 19:27:14, Robert Sesek wrote: > I think just POSIX and MACH are better names. Done. https://codereview.chromium.org/1163943004/diff/160001/base/memory/shared_mem... base/memory/shared_memory_handle.h:63: uint32_t GetMechanismForWire() const; On 2015/06/05 19:27:14, Robert Sesek wrote: > These names are kind of awkward. Why not just have SharedMemoryHandle pickle and > depickle itself? Add WriteToPickle(Pickle*) and ReadFromPickle(PickleIterator*), > and then none of the implementation details need to leak. I moved the mechanism logic into this class, but the FD logic needs to be exposed since reading/writing attachments is a Chrome IPC addition onto base::Pickle. https://codereview.chromium.org/1163943004/diff/160001/base/memory/shared_mem... File base/memory/shared_memory_handle_mac.cc (right): https://codereview.chromium.org/1163943004/diff/160001/base/memory/shared_mem... base/memory/shared_memory_handle_mac.cc:64: CHECK(mechanism_ == POSIX_FD); On 2015/06/05 19:27:14, Robert Sesek wrote: > CHECK_EQ, and throughout. Done.
The CQ bit was checked by erikchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1163943004/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2015/06/06 01:54:39, commit-bot: I haz the power wrote: > Dry run: Try jobs failed on following builders: > chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) This CL is no longer ready for review. Removing rsesek@ from reviewers list.
erikchen@chromium.org changed reviewers: - rsesek@chromium.org
Patchset #10 (id:200001) has been deleted
The CQ bit was checked by erikchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1163943004/220001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Patchset #10 (id:220001) has been deleted
The CQ bit was checked by erikchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1163943004/240001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by erikchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1163943004/260001
The CQ bit was checked by erikchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1163943004/280001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #11 (id:260001) has been deleted
Patchset #10 (id:240001) has been deleted
erikchen@chromium.org changed reviewers: + rsesek@chromium.org
rsesek: PTAL
https://codereview.chromium.org/1163943004/diff/280001/base/memory/shared_mem... File base/memory/shared_memory_handle.h (right): https://codereview.chromium.org/1163943004/diff/280001/base/memory/shared_mem... base/memory/shared_memory_handle.h:34: enum Mechanism { "Mechanism" seems very verbose to me. Does "Type" sound OK to you? https://codereview.chromium.org/1163943004/diff/280001/base/memory/shared_mem... base/memory/shared_memory_handle.h:73: // Returns the POSIX fd backing the SharedMemoryHandle. Requires that the Is this necessary if there's also GetFileDescriptor()->fd ? https://codereview.chromium.org/1163943004/diff/280001/base/memory/shared_mem... base/memory/shared_memory_handle.h:86: // Duplicates the underlying OS resources. Move this up to be after or before teh assignment operator? https://codereview.chromium.org/1163943004/diff/280001/base/memory/shared_mem... File base/memory/shared_memory_handle_mac.cc (right): https://codereview.chromium.org/1163943004/diff/280001/base/memory/shared_mem... base/memory/shared_memory_handle_mac.cc:71: CHECK_EQ(mechanism_, POSIX); Can these be DCHECKs instead? This should only happen for developer error, right? https://codereview.chromium.org/1163943004/diff/280001/ipc/ipc_message_utils.cc File ipc/ipc_message_utils.cc (right): https://codereview.chromium.org/1163943004/diff/280001/ipc/ipc_message_utils.... ipc/ipc_message_utils.cc:529: ParamTraits<base::FileDescriptor>::Write(m, *p.GetFileDescriptor()); Hm… looking at this again, it's a bit odd to have things partially serialized in //base and partially in //ipc. Maybe we merge your original approach and what I suggested last round: Add SharedMemoryHandle::GetType() rather than having IsBackedByPOSIXFD() (and presumably IsBackedByMachObject()), but do the conversion to the wire format here, rather than exposing an oddly named method on the //base interface. You probably also want to static_assert<> that the enum type fits within the wire type. That way all the serialization and deserialization logic is one place, and it doesn't have to be directly exposed on the SharedMemoryHandle interface.
Patchset #12 (id:320001) has been deleted
The CQ bit was checked by erikchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1163943004/340001
rsesek: PTAL https://codereview.chromium.org/1163943004/diff/280001/base/memory/shared_mem... File base/memory/shared_memory_handle.h (right): https://codereview.chromium.org/1163943004/diff/280001/base/memory/shared_mem... base/memory/shared_memory_handle.h:34: enum Mechanism { On 2015/06/15 21:34:33, Robert Sesek wrote: > "Mechanism" seems very verbose to me. Does "Type" sound OK to you? I think "Type" is a poor choice for a type, since the word "type" is already heavily overloaded. That being said, I think debating this point costs more than the additional round-trip review latency, so I've changed the enum to be Type. https://codereview.chromium.org/1163943004/diff/280001/base/memory/shared_mem... base/memory/shared_memory_handle.h:73: // Returns the POSIX fd backing the SharedMemoryHandle. Requires that the On 2015/06/15 21:34:32, Robert Sesek wrote: > Is this necessary if there's also GetFileDescriptor()->fd ? Nope. I've removed this method. I prefer to keep the setter. (Theoretically, we could use the non-const GetFileDescriptor(), but I would like to use that method as little as possible, since exposing instance-internal pointers is not a good interface.) https://codereview.chromium.org/1163943004/diff/280001/base/memory/shared_mem... base/memory/shared_memory_handle.h:86: // Duplicates the underlying OS resources. On 2015/06/15 21:34:33, Robert Sesek wrote: > Move this up to be after or before teh assignment operator? Sure, done https://codereview.chromium.org/1163943004/diff/280001/base/memory/shared_mem... File base/memory/shared_memory_handle_mac.cc (right): https://codereview.chromium.org/1163943004/diff/280001/base/memory/shared_mem... base/memory/shared_memory_handle_mac.cc:71: CHECK_EQ(mechanism_, POSIX); On 2015/06/15 21:34:33, Robert Sesek wrote: > Can these be DCHECKs instead? This should only happen for developer error, > right? You're right, DCHECKs will work here. https://codereview.chromium.org/1163943004/diff/280001/ipc/ipc_message_utils.cc File ipc/ipc_message_utils.cc (right): https://codereview.chromium.org/1163943004/diff/280001/ipc/ipc_message_utils.... ipc/ipc_message_utils.cc:529: ParamTraits<base::FileDescriptor>::Write(m, *p.GetFileDescriptor()); On 2015/06/15 21:34:33, Robert Sesek wrote: > Hm… looking at this again, it's a bit odd to have things partially serialized in > //base and partially in //ipc. Maybe we merge your original approach and what I > suggested last round: > > Add SharedMemoryHandle::GetType() rather than having IsBackedByPOSIXFD() (and > presumably IsBackedByMachObject()), but do the conversion to the wire format > here, rather than exposing an oddly named method on the //base interface. You > probably also want to static_assert<> that the enum type fits within the wire > type. That way all the serialization and deserialization logic is one place, and > it doesn't have to be directly exposed on the SharedMemoryHandle interface. Added static asserts. Moved logic out of SharedMemoryHandle and into this class.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by erikchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1163943004/360001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
LGTM https://codereview.chromium.org/1163943004/diff/280001/base/memory/shared_mem... File base/memory/shared_memory_handle.h (right): https://codereview.chromium.org/1163943004/diff/280001/base/memory/shared_mem... base/memory/shared_memory_handle.h:34: enum Mechanism { On 2015/06/16 00:59:27, erikchen wrote: > On 2015/06/15 21:34:33, Robert Sesek wrote: > > "Mechanism" seems very verbose to me. Does "Type" sound OK to you? > > I think "Type" is a poor choice for a type, since the word "type" is already > heavily overloaded. That being said, I think debating this point costs more than > the additional round-trip review latency, so I've changed the enum to be Type. If it were just "Type" I'd agree, but since the fully qualified name is SharedMemoryHandle::Type, I think that's fairly descriptive.
erikchen@chromium.org changed reviewers: + thakis@chromium.org
thakis: Looking for an OWNER review of base/
https://codereview.chromium.org/1163943004/diff/380001/base/base.gypi File base/base.gypi (right): https://codereview.chromium.org/1163943004/diff/380001/base/base.gypi#newcode771 base/base.gypi:771: 'memory/shared_memory_posix.cc', should probably be renamed to shared_memory_linux.cc then? https://codereview.chromium.org/1163943004/diff/380001/base/memory/shared_mem... File base/memory/shared_memory_mac.cc (right): https://codereview.chromium.org/1163943004/diff/380001/base/memory/shared_mem... base/memory/shared_memory_mac.cc:392: bool SharedMemory::PrepareMapFile(ScopedFILE fp, ScopedFD readonly_fd) { There's a lot of code in this file that's just copied without any modifications. Do you have a preview of how much this file is going to change in follow-ups? https://codereview.chromium.org/1163943004/diff/380001/content/browser/render... File content/browser/renderer_host/media/video_capture_buffer_pool.cc (right): https://codereview.chromium.org/1163943004/diff/380001/content/browser/render... content/browser/renderer_host/media/video_capture_buffer_pool.cc:81: #endif maybe there could be some function for this direction too, given that it's needed in a few places?
thakis: PTAL https://codereview.chromium.org/1163943004/diff/380001/base/base.gypi File base/base.gypi (right): https://codereview.chromium.org/1163943004/diff/380001/base/base.gypi#newcode771 base/base.gypi:771: 'memory/shared_memory_posix.cc', On 2015/06/17 04:03:20, Nico wrote: > should probably be renamed to shared_memory_linux.cc then? I thought that Android is considered posix but not Linux in the Chrome code base? https://codereview.chromium.org/1163943004/diff/380001/base/memory/shared_mem... File base/memory/shared_memory_mac.cc (right): https://codereview.chromium.org/1163943004/diff/380001/base/memory/shared_mem... base/memory/shared_memory_mac.cc:392: bool SharedMemory::PrepareMapFile(ScopedFILE fp, ScopedFD readonly_fd) { On 2015/06/17 04:03:21, Nico wrote: > There's a lot of code in this file that's just copied without any modifications. > Do you have a preview of how much this file is going to change in follow-ups? I don't have a preview to show you, but pretty much everything is changing. In the short term, every function has to support both Mach and POSIX backed shared memory. In the long term, every function will only support Mach backed shared memory. https://codereview.chromium.org/1163943004/diff/380001/content/browser/render... File content/browser/renderer_host/media/video_capture_buffer_pool.cc (right): https://codereview.chromium.org/1163943004/diff/380001/content/browser/render... content/browser/renderer_host/media/video_capture_buffer_pool.cc:81: #endif On 2015/06/17 04:03:21, Nico wrote: > maybe there could be some function for this direction too, given that it's > needed in a few places? This change showed up in a recent rebase against top of tree, and I just wanted to get it compiling again so I could merge the change (and not have to deal with future merge problems). As far as I can tell, this function is only ever called on Linux. I was planning on chatting with the OWNER about this later in the development cycle of Mach shared memory. (This is temporary blocked on my Windows attachment broker IPC work).
lgtm https://codereview.chromium.org/1163943004/diff/380001/base/base.gypi File base/base.gypi (right): https://codereview.chromium.org/1163943004/diff/380001/base/base.gypi#newcode771 base/base.gypi:771: 'memory/shared_memory_posix.cc', On 2015/06/17 17:38:55, erikchen wrote: > On 2015/06/17 04:03:20, Nico wrote: > > should probably be renamed to shared_memory_linux.cc then? > > I thought that Android is considered posix but not Linux in the Chrome code > base? Right you are. Nevermind.
The CQ bit was checked by erikchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org Link to the patchset: https://codereview.chromium.org/1163943004/#ps380001 (title: "Fix logic error.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1163943004/380001
The CQ bit was unchecked by erikchen@chromium.org
On 2015/06/17 17:51:27, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/1163943004/380001 whoops, got too excited. Still need more OWNERs.
erikchen@chromium.org changed reviewers: + dalecurtis@chromium.org, piman@chromium.org, tsepez@chromium.org
piman: Looking for ppapi/proxy OWNER review. tsepez: Looking for ipc/ OWNER review. dalecurtis: Looking for a content/browser/renderer_host/media OWNER review.
media/ lgtm since this isn't used on mac yet, but +reveman, dcastagna
Also be sure to do a build with: export GYP_DEFINES="enable_ipc_fuzzer=1" gclient runhooks ninja -C out/Debug ipc_fuzzer_all Which builds some stuff only inferno@ uses (hence not part of the bots due to large link times). https://codereview.chromium.org/1163943004/diff/380001/base/memory/shared_mem... File base/memory/shared_memory_handle.h (right): https://codereview.chromium.org/1163943004/diff/380001/base/memory/shared_mem... base/memory/shared_memory_handle.h:45: // Constructs a SharedMemoryHandle backed by |file_descriptor|. nit: comment about who owns the FD. Must the caller close it themselves? How long must it remain valid? etc. https://codereview.chromium.org/1163943004/diff/380001/base/memory/shared_mem... base/memory/shared_memory_handle.h:69: void SetType(Type type); Can the type change after the SMH is created? I'd assume this is set to one or the other by ctors? https://codereview.chromium.org/1163943004/diff/380001/base/memory/shared_mem... base/memory/shared_memory_handle.h:81: Type type_; const Type type_; per above. https://codereview.chromium.org/1163943004/diff/380001/base/memory/shared_mem... base/memory/shared_memory_handle.h:84: #elif defined(OS_POSIX) nit: I might re-arrange this so that win/posix cases come first since they are small, then this one last. That way, when I hit this line, I don't have to remember. "Oh yeah, there's still another possibility". No matter. https://codereview.chromium.org/1163943004/diff/380001/base/memory/shared_mem... File base/memory/shared_memory_handle_mac.cc (right): https://codereview.chromium.org/1163943004/diff/380001/base/memory/shared_mem... base/memory/shared_memory_handle_mac.cc:7: #include <unistd.h> nit: system includes come first. https://codereview.chromium.org/1163943004/diff/380001/base/memory/shared_mem... base/memory/shared_memory_handle_mac.cc:31: type_ = handle.type_; Does this make sense? Do you want to dup the file handle? If I do shm2 = shm1; close(shm1.fd); shm2 becomes invalid despite being separate from shm1. operator= usually requires an if (this != &handle) test. https://codereview.chromium.org/1163943004/diff/380001/base/memory/shared_mem... base/memory/shared_memory_handle_mac.cc:42: } nit: typically avoid invoking operators directly, instead: return !(*this == handle); https://codereview.chromium.org/1163943004/diff/380001/ipc/ipc_message_utils.cc File ipc/ipc_message_utils.cc (right): https://codereview.chromium.org/1163943004/diff/380001/ipc/ipc_message_utils.... ipc/ipc_message_utils.cc:526: static_assert(sizeof(base::SharedMemoryHandle::Type) <= sizeof(int), nit: this assert can go into the class body itself. https://codereview.chromium.org/1163943004/diff/380001/ipc/ipc_message_utils.... ipc/ipc_message_utils.cc:538: static_assert(sizeof(base::SharedMemoryHandle::Type) <= sizeof(int), same here. https://codereview.chromium.org/1163943004/diff/380001/ipc/ipc_message_utils.... ipc/ipc_message_utils.cc:561: The pattern for these kinds of classes is to not assign them member by member, but instead use ctor + assignment as in: base::FileDescriptor fd; if (!read(m, iter, &fd)) return false; *r = base::SharedMemoryHandle(fd); Then you can avoid having to leak the FD address to the outside world.
reveman@chromium.org changed reviewers: + mcasas@chromium.org, reveman@chromium.org
+mcasas https://codereview.chromium.org/1163943004/diff/380001/content/browser/render... File content/browser/renderer_host/media/video_capture_buffer_pool.cc (right): https://codereview.chromium.org/1163943004/diff/380001/content/browser/render... content/browser/renderer_host/media/video_capture_buffer_pool.cc:110: return gmb_->GetHandle().type; Unrelated to this patch but the use of GetHandle() here and below is a layering violation. Users of GpuMemoryBuffers should not be calling GetHandle and be poking around in that struct. We might make this opaque and private to the IPC code in content/ that it's intended for and this code will then break.
https://codereview.chromium.org/1163943004/diff/380001/content/browser/render... File content/browser/renderer_host/media/video_capture_buffer_pool.cc (right): https://codereview.chromium.org/1163943004/diff/380001/content/browser/render... content/browser/renderer_host/media/video_capture_buffer_pool.cc:75: #if defined(OS_MACOSX) OS_MACOSX implies OS_POSIX, right? [1] [1] https://code.google.com/p/chromium/codesearch#chromium/src/build/build_config... https://codereview.chromium.org/1163943004/diff/380001/content/browser/render... content/browser/renderer_host/media/video_capture_buffer_pool.cc:110: return gmb_->GetHandle().type; On 2015/06/17 19:24:10, reveman wrote: > Unrelated to this patch but the use of GetHandle() here and below is a layering > violation. Users of GpuMemoryBuffers should not be calling GetHandle and be > poking around in that struct. We might make this opaque and private to the IPC > code in content/ that it's intended for and this code will then break. Currently this is just used for a DCHECK and some double-checking that we're in a platform where |gmb_| is a DmaBuf, and the latter can be done via #if defined(USE_OZONE).
Patchset #15 (id:400001) has been deleted
Patchset #15 (id:420001) has been deleted
On 2015/06/17 21:33:11, mcasas wrote: > https://codereview.chromium.org/1163943004/diff/380001/content/browser/render... > File content/browser/renderer_host/media/video_capture_buffer_pool.cc (right): > > https://codereview.chromium.org/1163943004/diff/380001/content/browser/render... > content/browser/renderer_host/media/video_capture_buffer_pool.cc:75: #if > defined(OS_MACOSX) > OS_MACOSX implies OS_POSIX, right? [1] > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/build/build_config... > > https://codereview.chromium.org/1163943004/diff/380001/content/browser/render... > content/browser/renderer_host/media/video_capture_buffer_pool.cc:110: return > gmb_->GetHandle().type; > On 2015/06/17 19:24:10, reveman wrote: > > Unrelated to this patch but the use of GetHandle() here and below is a > layering > > violation. Users of GpuMemoryBuffers should not be calling GetHandle and be > > poking around in that struct. We might make this opaque and private to the IPC > > code in content/ that it's intended for and this code will then break. > > Currently this is just used for a DCHECK and some > double-checking that we're in a platform where > |gmb_| is a DmaBuf, and the latter can be done > via #if defined(USE_OZONE). I've confirmed that ipc_fuzzer_all builds successfully on Linux with this patch. (why isn't there a trybot config for this?)
https://codereview.chromium.org/1163943004/diff/380001/base/memory/shared_mem... File base/memory/shared_memory_handle.h (right): https://codereview.chromium.org/1163943004/diff/380001/base/memory/shared_mem... base/memory/shared_memory_handle.h:45: // Constructs a SharedMemoryHandle backed by |file_descriptor|. On 2015/06/17 18:51:55, Tom Sepez wrote: > nit: comment about who owns the FD. Must the caller close it themselves? How > long must it remain valid? etc. I've added a long comment indicating the ownership semantics. The ownership semantics are copied from base::FileDescriptor (by necessity). Unfortunately, those aren't well defined because base::FileDescriptor is a struct and code that uses it frequently passes it around by assignment/copy rather than by reference. https://codereview.chromium.org/1163943004/diff/380001/base/memory/shared_mem... base/memory/shared_memory_handle.h:69: void SetType(Type type); On 2015/06/17 18:51:55, Tom Sepez wrote: > Can the type change after the SMH is created? I'd assume this is set to one or > the other by ctors? Yes, since code that uses base::SharedMemoryHandles passes it by assignment. That being said, there's no reason for this functionality to be exposed. I've removed the method. https://codereview.chromium.org/1163943004/diff/380001/base/memory/shared_mem... base/memory/shared_memory_handle.h:81: Type type_; On 2015/06/17 18:51:55, Tom Sepez wrote: > const Type type_; per above. That won't work, since SharedMemoryHandles are passed by assignment and copy. (For example, your suggestion for not exposing the internals of SharedMemoryHandle in ipc_message_utils is good, but itself relies on being able to assign (and change the type_) of SharedMemoryHandle). https://codereview.chromium.org/1163943004/diff/380001/base/memory/shared_mem... base/memory/shared_memory_handle.h:84: #elif defined(OS_POSIX) On 2015/06/17 18:51:55, Tom Sepez wrote: > nit: I might re-arrange this so that win/posix cases come first since they are > small, then this one last. That way, when I hit this line, I don't have to > remember. "Oh yeah, there's still another possibility". No matter. Good suggestion, done. https://codereview.chromium.org/1163943004/diff/380001/base/memory/shared_mem... File base/memory/shared_memory_handle_mac.cc (right): https://codereview.chromium.org/1163943004/diff/380001/base/memory/shared_mem... base/memory/shared_memory_handle_mac.cc:7: #include <unistd.h> On 2015/06/17 18:51:55, Tom Sepez wrote: > nit: system includes come first. In the base/ module (and I think most of chrome), the first include in an implementation file is the associated header. For example: https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/shared... https://codereview.chromium.org/1163943004/diff/380001/base/memory/shared_mem... base/memory/shared_memory_handle_mac.cc:31: type_ = handle.type_; On 2015/06/17 18:51:55, Tom Sepez wrote: > Does this make sense? Do you want to dup the file handle? If I do > shm2 = shm1; > close(shm1.fd); > shm2 becomes invalid despite being separate from shm1. > > operator= usually requires an if (this != &handle) test. Added the check to compare "this" and "&handle". I am intentionally making a shallow copy here. This means that the ownership semantics of this class are totally messed up. This can't be avoided in the short term, since base::SharedMemoryHandle is a drop-in replacement for base::FileDescriptor (on POSIX), which has the exact same, messed up ownership semantics. You're right - the example you give would result in an error. Unfortunately, it is incredibly easy to make that error today, and that can't be easily fixed without rewriting a lot of code. https://codereview.chromium.org/1163943004/diff/380001/base/memory/shared_mem... base/memory/shared_memory_handle_mac.cc:42: } On 2015/06/17 18:51:55, Tom Sepez wrote: > nit: typically avoid invoking operators directly, instead: > return !(*this == handle); Done. https://codereview.chromium.org/1163943004/diff/380001/content/browser/render... File content/browser/renderer_host/media/video_capture_buffer_pool.cc (right): https://codereview.chromium.org/1163943004/diff/380001/content/browser/render... content/browser/renderer_host/media/video_capture_buffer_pool.cc:75: #if defined(OS_MACOSX) On 2015/06/17 21:33:11, mcasas wrote: > OS_MACOSX implies OS_POSIX, right? [1] > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/build/build_config... I don't understand your question. Yes, OS_MACOSX implies OS_POSIX, but it's also possible to have OS_POSIX without OS_MACOSX (android, linux, chromeos, etc.) https://codereview.chromium.org/1163943004/diff/380001/ipc/ipc_message_utils.cc File ipc/ipc_message_utils.cc (right): https://codereview.chromium.org/1163943004/diff/380001/ipc/ipc_message_utils.... ipc/ipc_message_utils.cc:526: static_assert(sizeof(base::SharedMemoryHandle::Type) <= sizeof(int), On 2015/06/17 18:51:55, Tom Sepez wrote: > nit: this assert can go into the class body itself. Sure, done. I made a new type base::SharedMemoryHandle::TypeWireFormat to use instead of int on line 535. This ensures that the assert actually matches the code being used in this file. https://codereview.chromium.org/1163943004/diff/380001/ipc/ipc_message_utils.... ipc/ipc_message_utils.cc:538: static_assert(sizeof(base::SharedMemoryHandle::Type) <= sizeof(int), On 2015/06/17 18:51:55, Tom Sepez wrote: > same here. Done. https://codereview.chromium.org/1163943004/diff/380001/ipc/ipc_message_utils.... ipc/ipc_message_utils.cc:561: On 2015/06/17 18:51:55, Tom Sepez wrote: > The pattern for these kinds of classes is to not assign them member by member, > but instead use ctor + assignment as in: > > base::FileDescriptor fd; > if (!read(m, iter, &fd)) > return false; > *r = base::SharedMemoryHandle(fd); > > Then you can avoid having to leak the FD address to the outside world. Ah, good suggestion. Done.
lgtm
LGTM+nit https://codereview.chromium.org/1163943004/diff/440001/base/memory/shared_mem... File base/memory/shared_memory_handle.h (right): https://codereview.chromium.org/1163943004/diff/440001/base/memory/shared_mem... base/memory/shared_memory_handle.h:56: SharedMemoryHandle(const base::FileDescriptor& file_descriptor); nit: explicit
https://codereview.chromium.org/1163943004/diff/440001/base/memory/shared_mem... File base/memory/shared_memory_handle.h (right): https://codereview.chromium.org/1163943004/diff/440001/base/memory/shared_mem... base/memory/shared_memory_handle.h:56: SharedMemoryHandle(const base::FileDescriptor& file_descriptor); On 2015/06/19 01:52:45, piman (Very slow to review) wrote: > nit: explicit Done.
The CQ bit was checked by erikchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org, dalecurtis@chromium.org, thakis@chromium.org, piman@chromium.org, tsepez@chromium.org Link to the patchset: https://codereview.chromium.org/1163943004/#ps480001 (title: "Comments from piman.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1163943004/480001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #21 (id:560001) has been deleted
Patchset #20 (id:540001) has been deleted
The CQ bit was checked by erikchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org, rsesek@chromium.org, dalecurtis@chromium.org, tsepez@chromium.org, thakis@chromium.org Link to the patchset: https://codereview.chromium.org/1163943004/#ps580001 (title: "Remove more unused headers that magically appeared from rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1163943004/580001
Message was sent while issue was closed.
Committed patchset #20 (id:580001)
Message was sent while issue was closed.
Patchset 20 (id:??) landed as https://crrev.com/cc9f9127f979acd6eb22103656057a655a0e76a6 Cr-Commit-Position: refs/heads/master@{#335360}
Message was sent while issue was closed.
A revert of this CL (patchset #20 id:580001) has been created in https://codereview.chromium.org/1200473003/ by scottmg@chromium.org. The reason for reverting is: Possibly breaking Mac build: FAILED: cd ../../third_party/libyuv; export BUILT_PRODUCTS_DIR=/b/build/slave/GPU_Mac_Builder/build/src/out/Release; export CONFIGURATION=Release; export PRODUCT_NAME=libyuv_nacl; export SDKROOT=/Applications/Xcode5.1.1.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.6.sdk; export SRCROOT=/b/build/slave/GPU_Mac_Builder/build/src/out/Release/../../third_party/libyuv; export SOURCE_ROOT="${SRCROOT}"; export TARGET_BUILD_DIR=/b/build/slave/GPU_Mac_Builder/build/src/out/Release; export TEMP_DIR="${TMPDIR}";python ../../native_client/build/build_nexe.py --root ../.. --product-dir ../../out/Release/xyz --config-name Release -t ../../native_client/toolchain/ --arch pnacl --build newlib_plib --name ../../out/Release/gen/tc_pnacl_newlib/lib/libyuv_nacl.a --objdir ../../out/Release/obj/third_party/libyuv/libyuv_nacl.gen/pnacl_newlib-pnacl/libyuv_nacl "--include-dirs=../../out/Release/gen/tc_pnacl_newlib/include ../.. \"../../out/Release/gen\" include" "--compile_flags=-O2 -g -Wall -fdiagnostics-show-option -Werror -Wno-unused-function -Wno-char-subscripts -Wno-c++11-extensions -Wno-unnamed-type-template-args -Wno-extra-semi -Wno-unused-private-field -Wno-char-subscripts -Wno-unused-function \"-std=gnu++11\" " --gomadir /b/build/goma "--defines=\"__STDC_LIMIT_MACROS=1\" \"__STDC_FORMAT_MACROS=1\" \"_GNU_SOURCE=1\" \"_DEFAULT_SOURCE=1\" \"_BSD_SOURCE=1\" \"_POSIX_C_SOURCE=199506\" \"_XOPEN_SOURCE=600\" \"DYNAMIC_ANNOTATIONS_ENABLED=1\" \"DYNAMIC_ANNOTATIONS_PREFIX=NACL_\" \"NACL_BUILD_ARCH=x86\" V8_DEPRECATION_WARNINGS \"__ASSERT_MACROS_DEFINE_VERSIONS_WITHOUT_UNDERSCORE=0\" CHROMIUM_BUILD \"CR_CLANG_REVISION=239765-1\" \"USE_LIBJPEG_TURBO=1\" ENABLE_ONE_CLICK_SIGNIN ENABLE_PRE_SYNC_BACKUP \"ENABLE_REMOTING=1\" \"ENABLE_WEBRTC=1\" \"ENABLE_MEDIA_ROUTER=1\" USE_PROPRIETARY_CODECS ENABLE_PEPPER_CDMS ENABLE_CONFIGURATION_POLICY ENABLE_NOTIFICATIONS \"ENABLE_HIDPI=1\" SYSTEM_NATIVELY_SIGNALS_MEMORY_PRESSURE DONT_EMBED_BUILD_METADATA \"DCHECK_ALWAYS_ON=1\" \"ENABLE_TASK_MANAGER=1\" \"ENABLE_EXTENSIONS=1\" \"ENABLE_PLUGIN_INSTALLATION=1\" \"ENABLE_PLUGINS=1\" \"ENABLE_SESSION_SERVICE=1\" \"ENABLE_THEMES=1\" \"ENABLE_AUTOFILL_DIALOG=1\" \"ENABLE_BACKGROUND=1\" \"ENABLE_GOOGLE_NOW=1\" \"CLD_VERSION=2\" \"ENABLE_PRINTING=1\" \"ENABLE_BASIC_PRINTING=1\" \"ENABLE_PRINT_PREVIEW=1\" \"ENABLE_SPELLCHECK=1\" \"ENABLE_CAPTIVE_PORTAL_DETECTION=1\" \"ENABLE_APP_LIST=1\" \"ENABLE_SETTINGS_APP=1\" \"ENABLE_SUPERVISED_USERS=1\" \"ENABLE_SERVICE_DISCOVERY=1\" \"ENABLE_WIFI_BOOTSTRAPPING=1\" V8_USE_EXTERNAL_STARTUP_DATA FULL_SAFE_BROWSING SAFE_BROWSING_CSD SAFE_BROWSING_DB_LOCAL SAFE_BROWSING_SERVICE \"USE_LIBPCI=1\" \"USE_OPENSSL=1\"" "--link_flags=-B../../out/Release/gen/tc_pnacl_newlib/lib " "--source-list=../../out/gypfiles/third_party/libyuv/pnacl_newlib.libyuv_nacl.source_list.gypcmd" source/row_any.cc:13:10: fatal error: 'memory.h' file not found #include <memory.h> // for memcpy() ^ 1 error generated. FAILED with 1: /b/build/goma/gomacc ../../native_client/toolchain/mac_x86/pnacl_newlib/bin/pnacl-clang++ -c source/row_any.cc -o ../../out/Release/obj/third_party/libyuv/libyuv_nacl.gen/pnacl_newlib-pnacl/libyuv_nacl/row_any_67f422e1.o -MD -MF ../../out/Release/obj/third_party/libyuv/libyuv_nacl.gen/pnacl_newlib-pnacl/libyuv_nacl/row_any_67f422e1.o.d -O2 -g -Wall -fdiagnostics-show-option -Werror -Wno-unused-function -Wno-char-subscripts -Wno-c++11-extensions -Wno-unnamed-type-template-args -Wno-extra-semi -Wno-unused-private-field -Wno-char-subscripts -Wno-unused-function -std=gnu++11 -D__STDC_LIMIT_MACROS=1 -D__STDC_FORMAT_MACROS=1 -D_GNU_SOURCE=1 -D_DEFAULT_SOURCE=1 -D_BSD_SOURCE=1 -D_POSIX_C_SOURCE=199506 -D_XOPEN_SOURCE=600 -DDYNAMIC_ANNOTATIONS_ENABLED=1 -DDYNAMIC_ANNOTATIONS_PREFIX=NACL_ -DV8_DEPRECATION_WARNINGS -D__ASSERT_MACROS_DEFINE_VERSIONS_WITHOUT_UNDERSCORE=0 -DCHROMIUM_BUILD -DCR_CLANG_REVISION=239765-1 -DUSE_LIBJPEG_TURBO=1 -DENABLE_ONE_CLICK_SIGNIN -DENABLE_PRE_SYNC_BACKUP -DENABLE_REMOTING=1 -DENABLE_WEBRTC=1 -DENABLE_MEDIA_ROUTER=1 -DUSE_PROPRIETARY_CODECS -DENABLE_PEPPER_CDMS -DENABLE_CONFIGURATION_POLICY -DENABLE_NOTIFICATIONS -DENABLE_HIDPI=1 -DSYSTEM_NATIVELY_SIGNALS_MEMORY_PRESSURE -DDONT_EMBED_BUILD_METADATA -DDCHECK_ALWAYS_ON=1 -DENABLE_TASK_MANAGER=1 -DENABLE_EXTENSIONS=1 -DENABLE_PLUGIN_INSTALLATION=1 -DENABLE_PLUGINS=1 -DENABLE_SESSION_SERVICE=1 -DENABLE_THEMES=1 -DENABLE_AUTOFILL_DIALOG=1 -DENABLE_BACKGROUND=1 -DENABLE_GOOGLE_NOW=1 -DCLD_VERSION=2 -DENABLE_PRINTING=1 -DENABLE_BASIC_PRINTING=1 -DENABLE_PRINT_PREVIEW=1 -DENABLE_SPELLCHECK=1 -DENABLE_CAPTIVE_PORTAL_DETECTION=1 -DENABLE_APP_LIST=1 -DENABLE_SETTINGS_APP=1 -DENABLE_SUPERVISED_USERS=1 -DENABLE_SERVICE_DISCOVERY=1 -DENABLE_WIFI_BOOTSTRAPPING=1 -DV8_USE_EXTERNAL_STARTUP_DATA -DFULL_SAFE_BROWSING -DSAFE_BROWSING_CSD -DSAFE_BROWSING_DB_LOCAL -DSAFE_BROWSING_SERVICE -DUSE_LIBPCI=1 -DUSE_OPENSSL=1 -DNACL_WINDOWS=0 -DNACL_OSX=0 -DNACL_LINUX=0 -DNACL_ANDROID=0 -DNACL_BUILD_ARCH=pnacl -I../../out/Release/gen/tc_pnacl_newlib/include -I../.. -I../../out/Release/gen -Iinclude -DNDEBUG -std=gnu++0x -Wno-deprecated-register Compile options: ['-O2', '-g', '-Wall', '-fdiagnostics-show-option', '-Werror', '-Wno-unused-function', '-Wno-char-subscripts', '-Wno-c++11-extensions', '-Wno-unnamed-type-template-args', '-Wno-extra-semi', '-Wno-unused-private-field', '-Wno-char-subscripts', '-Wno-unused-function', '-std=gnu++11', '-D__STDC_LIMIT_MACROS=1', '-D__STDC_FORMAT_MACROS=1', '-D_GNU_SOURCE=1', '-D_DEFAULT_SOURCE=1', '-D_BSD_SOURCE=1', '-D_POSIX_C_SOURCE=199506', '-D_XOPEN_SOURCE=600', '-DDYNAMIC_ANNOTATIONS_ENABLED=1', '-DDYNAMIC_ANNOTATIONS_PREFIX=NACL_', '-DV8_DEPRECATION_WARNINGS', '-D__ASSERT_MACROS_DEFINE_VERSIONS_WITHOUT_UNDERSCORE=0', '-DCHROMIUM_BUILD', '-DCR_CLANG_REVISION=239765-1', '-DUSE_LIBJPEG_TURBO=1', '-DENABLE_ONE_CLICK_SIGNIN', '-DENABLE_PRE_SYNC_BACKUP', '-DENABLE_REMOTING=1', '-DENABLE_WEBRTC=1', '-DENABLE_MEDIA_ROUTER=1', '-DUSE_PROPRIETARY_CODECS', '-DENABLE_PEPPER_CDMS', '-DENABLE_CONFIGURATION_POLICY', '-DENABLE_NOTIFICATIONS', '-DENABLE_HIDPI=1', '-DSYSTEM_NATIVELY_SIGNALS_MEMORY_PRESSURE', '-DDONT_EMBED_BUILD_METADATA', '-DDCHECK_ALWAYS_ON=1', '-DENABLE_TASK_MANAGER=1', '-DENABLE_EXTENSIONS=1', '-DENABLE_PLUGIN_INSTALLATION=1', '-DENABLE_PLUGINS=1', '-DENABLE_SESSION_SERVICE=1', '-DENABLE_THEMES=1', '-DENABLE_AUTOFILL_DIALOG=1', '-DENABLE_BACKGROUND=1', '-DENABLE_GOOGLE_NOW=1', '-DCLD_VERSION=2', '-DENABLE_PRINTING=1', '-DENABLE_BASIC_PRINTING=1', '-DENABLE_PRINT_PREVIEW=1', '-DENABLE_SPELLCHECK=1', '-DENABLE_CAPTIVE_PORTAL_DETECTION=1', '-DENABLE_APP_LIST=1', '-DENABLE_SETTINGS_APP=1', '-DENABLE_SUPERVISED_USERS=1', '-DENABLE_SERVICE_DISCOVERY=1', '-DENABLE_WIFI_BOOTSTRAPPING=1', '-DV8_USE_EXTERNAL_STARTUP_DATA', '-DFULL_SAFE_BROWSING', '-DSAFE_BROWSING_CSD', '-DSAFE_BROWSING_DB_LOCAL', '-DSAFE_BROWSING_SERVICE', '-DUSE_LIBPCI=1', '-DUSE_OPENSSL=1', '-DNACL_WINDOWS=0', '-DNACL_OSX=0', '-DNACL_LINUX=0', '-DNACL_ANDROID=0', '-DNACL_BUILD_ARCH=pnacl', '-I../../out/Release/gen/tc_pnacl_newlib/include', '-I../..', '-I../../out/Release/gen', '-Iinclude', '-DNDEBUG'] Linker options: ['-B../../out/Release/gen/tc_pnacl_newlib/lib'] Traceback (most recent call last): File "../../native_client/build/build_nexe.py", line 849, in CompileProcess output_queue.put((filename, build.Compile(filename))) File "../../native_client/build/build_nexe.py", line 579, in Compile raise Error('FAILED with %d: %s' % (err, ' '.join(cmd_line))) Error: FAILED with 1: /b/build/goma/gomacc ../../native_client/toolchain/mac_x86/pnacl_newlib/bin/pnacl-clang++ -c source/row_any.cc -o ../../out/Release/obj/third_party/libyuv/libyuv_nacl.gen/pnacl_newlib-pnacl/libyuv_nacl/row_any_67f422e1.o -MD -MF ../../out/Release/obj/third_party/libyuv/libyuv_nacl.gen/pnacl_newlib-pnacl/libyuv_nacl/row_any_67f422e1.o.d -O2 -g -Wall -fdiagnostics-show-option -Werror -Wno-unused-function -Wno-char-subscripts -Wno-c++11-extensions -Wno-unnamed-type-template-args -Wno-extra-semi -Wno-unused-private-field -Wno-char-subscripts -Wno-unused-function -std=gnu++11 -D__STDC_LIMIT_MACROS=1 -D__STDC_FORMAT_MACROS=1 -D_GNU_SOURCE=1 -D_DEFAULT_SOURCE=1 -D_BSD_SOURCE=1 -D_POSIX_C_SOURCE=199506 -D_XOPEN_SOURCE=600 -DDYNAMIC_ANNOTATIONS_ENABLED=1 -DDYNAMIC_ANNOTATIONS_PREFIX=NACL_ -DV8_DEPRECATION_WARNINGS -D__ASSERT_MACROS_DEFINE_VERSIONS_WITHOUT_UNDERSCORE=0 -DCHROMIUM_BUILD -DCR_CLANG_REVISION=239765-1 -DUSE_LIBJPEG_TURBO=1 -DENABLE_ONE_CLICK_SIGNIN -DENABLE_PRE_SYNC_BACKUP -DENABLE_REMOTING=1 -DENABLE_WEBRTC=1 -DENABLE_MEDIA_ROUTER=1 -DUSE_PROPRIETARY_CODECS -DENABLE_PEPPER_CDMS -DENABLE_CONFIGURATION_POLICY -DENABLE_NOTIFICATIONS -DENABLE_HIDPI=1 -DSYSTEM_NATIVELY_SIGNALS_MEMORY_PRESSURE -DDONT_EMBED_BUILD_METADATA -DDCHECK_ALWAYS_ON=1 -DENABLE_TASK_MANAGER=1 -DENABLE_EXTENSIONS=1 -DENABLE_PLUGIN_INSTALLATION=1 -DENABLE_PLUGINS=1 -DENABLE_SESSION_SERVICE=1 -DENABLE_THEMES=1 -DENABLE_AUTOFILL_DIALOG=1 -DENABLE_BACKGROUND=1 -DENABLE_GOOGLE_NOW=1 -DCLD_VERSION=2 -DENABLE_PRINTING=1 -DENABLE_BASIC_PRINTING=1 -DENABLE_PRINT_PREVIEW=1 -DENABLE_SPELLCHECK=1 -DENABLE_CAPTIVE_PORTAL_DETECTION=1 -DENABLE_APP_LIST=1 -DENABLE_SETTINGS_APP=1 -DENABLE_SUPERVISED_USERS=1 -DENABLE_SERVICE_DISCOVERY=1 -DENABLE_WIFI_BOOTSTRAPPING=1 -DV8_USE_EXTERNAL_STARTUP_DATA -DFULL_SAFE_BROWSING -DSAFE_BROWSING_CSD -DSAFE_BROWSING_DB_LOCAL -DSAFE_BROWSING_SERVICE -DUSE_LIBPCI=1 -DUSE_OPENSSL=1 -DNACL_WINDOWS=0 -DNACL_OSX=0 -DNACL_LINUX=0 -DNACL_ANDROID=0 -DNACL_BUILD_ARCH=pnacl -I../../out/Release/gen/tc_pnacl_newlib/include -I../.. -I../../out/Release/gen -Iinclude -DNDEBUG -std=gnu++0x -Wno-deprecated-register ninja: build stopped: subcommand failed. . |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
