third_party/protobuf: Update to HEAD (83d681ee2c)

third_party/protobuf: Update to HEAD (83d681ee2c)

Rebase protobuf to current HEAD to pull the 3 following changes:
* 4c5d3ed Fix integer overflow in FastUInt32ToBufferLeft
* 4587a3f [arm/gcc] Don't rely on KUSER_HELPERS feature for atomics
* a5a2c1d generic atomicops: Use strong compare_exchange

This change has been made following steps described in
third_party/protobuf/README.chromium file, please refer to it
for further information.

BUG=599051, 673488
Committed: https://crrev.com/4fa2d9262fd91a4e613269f230cca34b95ca4af7
Cr-Commit-Position: refs/heads/master@{#439821}

[jbriance, 2016-11-10 14:51:41]

jbriance@cisco.com changed reviewers:
+ pkasting@chromium.org, xyzzyz@chromium.org

[Peter Kasting, 2016-11-10 20:42:05]

LGTM

https://codereview.chromium.org/2495533002/diff/1/third_party/protobuf/BUILD.gn
File third_party/protobuf/BUILD.gn (right):

https://codereview.chromium.org/2495533002/diff/1/third_party/protobuf/BUILD....
third_party/protobuf/BUILD.gn:55: "src/google/protobuf/has_bits.h",
This needs to be here (and not in protobuf_full) because it's transitively
included by some source file in this section, right?

https://codereview.chromium.org/2495533002/diff/1/third_party/protobuf/BUILD....
third_party/protobuf/BUILD.gn:301: "src/google/protobuf/util/package_info.h",
...and this is transitively included by some source file in this list?

https://codereview.chromium.org/2495533002/diff/1/third_party/protobuf/README...
File third_party/protobuf/README.chromium (right):

https://codereview.chromium.org/2495533002/diff/1/third_party/protobuf/README...
third_party/protobuf/README.chromium:6: Version: (intermediate version)
Should be 3.1.0 (basically, use whatever is in
protobuf/protoc-artifacts/pom.xml).

https://codereview.chromium.org/2495533002/diff/1/third_party/protobuf/README...
third_party/protobuf/README.chromium:26: For future releases, it will be worth
looking into which patches still need
Nit: Might want to note here that updated versions of the patches should then be
saved back to the patches dir (as you've done).

[jbriance, 2016-11-10 21:36:54]

Thank you for the review. I'm out of office until monday, I'll update the change
as soon as I get back to work.

https://codereview.chromium.org/2495533002/diff/1/third_party/protobuf/BUILD.gn
File third_party/protobuf/BUILD.gn (right):

https://codereview.chromium.org/2495533002/diff/1/third_party/protobuf/BUILD....
third_party/protobuf/BUILD.gn:55: "src/google/protobuf/has_bits.h",
On 2016/11/10 20:42:05, Peter Kasting wrote:
> This needs to be here (and not in protobuf_full) because it's transitively
> included by some source file in this section, right?

Oops, I read too quickly and missed this part. I'll check this carefully and
update the change if needed.

https://codereview.chromium.org/2495533002/diff/1/third_party/protobuf/BUILD....
third_party/protobuf/BUILD.gn:301: "src/google/protobuf/util/package_info.h",
On 2016/11/10 20:42:04, Peter Kasting wrote:
> ...and this is transitively included by some source file in this list?

Same here.

https://codereview.chromium.org/2495533002/diff/1/third_party/protobuf/README...
File third_party/protobuf/README.chromium (right):

https://codereview.chromium.org/2495533002/diff/1/third_party/protobuf/README...
third_party/protobuf/README.chromium:6: Version: (intermediate version)
On 2016/11/10 20:42:04, Peter Kasting wrote:
> Should be 3.1.0 (basically, use whatever is in
> protobuf/protoc-artifacts/pom.xml).

Ok, I'll update the change.

https://codereview.chromium.org/2495533002/diff/1/third_party/protobuf/README...
third_party/protobuf/README.chromium:26: For future releases, it will be worth
looking into which patches still need
On 2016/11/10 20:42:04, Peter Kasting wrote:
> Nit: Might want to note here that updated versions of the patches should then
be
> saved back to the patches dir (as you've done).

Ok, will do.

[jbriance, 2016-11-14 10:57:51]

Please take another look

https://codereview.chromium.org/2495533002/diff/1/third_party/protobuf/BUILD.gn
File third_party/protobuf/BUILD.gn (right):

https://codereview.chromium.org/2495533002/diff/1/third_party/protobuf/BUILD....
third_party/protobuf/BUILD.gn:301: "src/google/protobuf/util/package_info.h",
This one doesn't need to be declared at all

https://codereview.chromium.org/2495533002/diff/10001/third_party/protobuf/BU...
File third_party/protobuf/BUILD.gn (right):

https://codereview.chromium.org/2495533002/diff/10001/third_party/protobuf/BU...
third_party/protobuf/BUILD.gn:55: "src/google/protobuf/has_bits.h",
This one is included by "src/google/protobuf/generated_message_util.h"

[jbriance, 2016-11-14 10:58:14]

The CQ bit was checked by jbriance@cisco.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-14 10:58:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-14 11:21:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-14 11:21:50]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[jbriance, 2016-11-14 13:15:38]

The CQ bit was checked by jbriance@cisco.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-14 13:15:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-14 13:38:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-14 13:38:54]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Peter Kasting, 2016-11-14 18:36:05]

Rather than patches 5 and 11, can we send PRs for these upstream and get them
landed there, then pull the fixed version here?  It seems like they would be
willing to take these fixes quickly.  (I can ping people internally if you're
not getting rapid traction.)

[jbriance, 2016-11-14 21:06:25]

On 2016/11/14 18:36:05, Peter Kasting wrote:
> Rather than patches 5 and 11, can we send PRs for these upstream and get them
> landed there, then pull the fixed version here?  It seems like they would be
> willing to take these fixes quickly.  (I can ping people internally if you're
> not getting rapid traction.)

Okay, will do.

I'll also submit a small change for the following presubmit error:

  ** Presubmit ERRORS **
  third_party/protobuf/csharp/src/Google.Protobuf.Test/project.json could not be
parsed: Expecting property name: line 25 column 3 (char 482)

[jbriance, 2016-11-14 21:35:54]

Here they are:
- https://github.com/google/protobuf/pull/2366
- https://github.com/google/protobuf/pull/2367
- https://github.com/google/protobuf/pull/2368

[jbriance, 2016-11-16 13:16:49]

The CQ bit was checked by jbriance@cisco.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-16 13:17:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jbriance, 2016-11-16 13:30:04]

Description was changed from

==========
third_party/protobuf: Update to HEAD (ce5160b83b)

Rebase protobuf to current HEAD to pull the 2 following changes:
* 4587a3f [arm/gcc] Don't rely on KUSER_HELPERS feature for atomics
* a5a2c1d generic atomicops: Use strong compare_exchange

This change has been made following steps described in
third_party/protobuf/README.chromium file, please refer to it
for further information.

BUG=599051
==========

to

==========
third_party/protobuf: Update to HEAD (6e93fa41c4)

Rebase protobuf to current HEAD to pull the 2 following changes:
* 4587a3f [arm/gcc] Don't rely on KUSER_HELPERS feature for atomics
* a5a2c1d generic atomicops: Use strong compare_exchange

This change has been made following steps described in
third_party/protobuf/README.chromium file, please refer to it
for further information.

BUG=599051
==========

[jbriance, 2016-11-16 15:25:28]

PTAL.

Please note that patch 0012-add-missing-include-for-INT-MAX.patch is not removed
because https://github.com/google/protobuf/pull/2367 is still pending.

[commit-bot: I haz the power, 2016-11-16 15:34:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-16 15:34:54]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jbriance, 2016-11-16 16:54:42]

FYI, I can reproduce the "linux_chromium_asan_rel_ng" try bot failure locally,
i.e. the
following command runs fine without my patch set and reports memory leaks with
it:

  ASAN_OPTIONS=detect_leaks=1 ./media_unittests --gtest_filter=CdmAdapter*

[Peter Kasting, 2016-11-17 00:58:22]

On 2016/11/16 16:54:42, jbriance wrote:
> FYI, I can reproduce the "linux_chromium_asan_rel_ng" try bot failure locally,
> i.e. the
> following command runs fine without my patch set and reports memory leaks with
> it:
> 
>   ASAN_OPTIONS=detect_leaks=1 ./media_unittests --gtest_filter=CdmAdapter*

Uh oh.  Does that mean we have a leak in the protobuf code?  I mean, if so, yay
that we caught this now because hopefully it's more fixable, but it's just
adding on blockers to getting your underlying problem solved.  Are these easy to
track down to something we can file as an upstream bug?

(BTW, I really appreciate how willing you've been to submit PRs upstream, pull
whole new versions, etc., so I really don't want to let the hurdles just keep
piling up.)

[jbriance, 2016-11-17 08:40:43]

Description was changed from

==========
third_party/protobuf: Update to HEAD (6e93fa41c4)

Rebase protobuf to current HEAD to pull the 2 following changes:
* 4587a3f [arm/gcc] Don't rely on KUSER_HELPERS feature for atomics
* a5a2c1d generic atomicops: Use strong compare_exchange

This change has been made following steps described in
third_party/protobuf/README.chromium file, please refer to it
for further information.

BUG=599051
==========

to

==========
third_party/protobuf: Update to HEAD (01d321c2d6)

Rebase protobuf to current HEAD to pull the 2 following changes:
* 4587a3f [arm/gcc] Don't rely on KUSER_HELPERS feature for atomics
* a5a2c1d generic atomicops: Use strong compare_exchange

This change has been made following steps described in
third_party/protobuf/README.chromium file, please refer to it
for further information.

BUG=599051
==========

[jbriance, 2016-11-17 12:23:12]

The CQ bit was checked by jbriance@cisco.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-17 12:23:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-17 14:39:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-17 14:39:16]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jbriance, 2016-11-17 17:18:27]

On 2016/11/17 00:58:22, Peter Kasting wrote:
> Uh oh.  Does that mean we have a leak in the protobuf code?  I mean, if so,
yay
> that we caught this now because hopefully it's more fixable, but it's just
> adding on blockers to getting your underlying problem solved.  Are these easy
to
> track down to something we can file as an upstream bug?

I think I will be able to find which protobuf commit introduces the regression
by
bisecting a bit.
 
> I really don't want to let the hurdles just keep piling up.

This is kind of a background task for me and I learn a lot by doing it, so this
is
not a problem at all. For instance, I wasn't aware of what ASAN was before
working
on this :)

[jbriance, 2016-11-17 17:43:53]

The good news is that I found the protobuf commit making the try bot
complaining: CdmAdapterTest.Initialize media_unittest is failing with protobuf
commit cc8ca5b whereas it is fine with its parent 337a028. So the culprit seems
to be
https://github.com/google/protobuf/commit/cc8ca5b6a5478b40546d4206392eb147145...

The bad news is that it's a huge commit (Integrate internal changes).

I'll investigate further and file a bug if relevant.

[Peter Kasting, 2016-11-17 19:30:09]

On 2016/11/17 17:43:53, jbriance wrote:
> The good news is that I found the protobuf commit making the try bot
> complaining: CdmAdapterTest.Initialize media_unittest is failing with protobuf
> commit cc8ca5b whereas it is fine with its parent 337a028. So the culprit
seems
> to be
>
https://github.com/google/protobuf/commit/cc8ca5b6a5478b40546d4206392eb147145...
> 
> The bad news is that it's a huge commit (Integrate internal changes).
> 
> I'll investigate further and file a bug if relevant.

OK.  Usually ASAN can tell you the allocation stack for the specific leaking
object, which can help track this down, but tracking that down may only be easy
for someone who knows the protobuf code well.  I would be aggressive with filing
bugs about this, as clearly this commit introduced some kind of leak even if
we're not sure of the nature of the leak yet.

[jbriance, 2016-11-17 23:23:52]

Protobuf bug filed: https://github.com/google/protobuf/issues/2385

[jbriance, 2016-11-18 15:12:58]

The CQ bit was checked by jbriance@cisco.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-18 15:13:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jbriance, 2016-11-18 17:33:01]

As I wrote in the filed bug, removing GOOGLE_PROTOBUF_NO_STATIC_INITIALIZER
define from BUILD.gn fixes the leak.

The question is now: why this define was there in the first place and is it safe
to remove it ?

[commit-bot: I haz the power, 2016-11-18 17:35:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-18 17:35:05]

Dry run: This issue passed the CQ dry run.

[Peter Kasting, 2016-11-18 18:24:56]

On 2016/11/18 17:33:01, jbriance wrote:
> As I wrote in the filed bug, removing GOOGLE_PROTOBUF_NO_STATIC_INITIALIZER
> define from BUILD.gn fixes the leak.
> 
> The question is now: why this define was there in the first place and is it
safe
> to remove it ?

I don't know, but at a glance, it looks like it's about avoiding static
initializers, which are banned in the Google Style Guide.  I'd imagine we don't
want to remove it.

Perhaps the changed code adds objects which are expected to leak, and should
simply be annotated so ASAN knows this?

[jbriance, 2016-11-19 14:50:42]

I've updated https://github.com/google/protobuf/issues/2385

To sum up, I'm pretty sure that GOOGLE_PROTOBUF_NO_STATIC_INITIALIZER define
logic has been inverted by accident in protobuf.

I'll work on a fix in protobuf, then I'll update this CL to include it + restore
GOOGLE_PROTOBUF_NO_STATIC_INITIALIZER define in BUILD.gn

[jbriance, 2016-11-21 13:20:22]

Pull request created: https://github.com/google/protobuf/pull/2398

[jbriance, 2016-11-21 21:43:09]

Description was changed from

==========
third_party/protobuf: Update to HEAD (01d321c2d6)

Rebase protobuf to current HEAD to pull the 2 following changes:
* 4587a3f [arm/gcc] Don't rely on KUSER_HELPERS feature for atomics
* a5a2c1d generic atomicops: Use strong compare_exchange

This change has been made following steps described in
third_party/protobuf/README.chromium file, please refer to it
for further information.

BUG=599051
==========

to

==========
third_party/protobuf: Update to HEAD (b7632464b4)

Rebase protobuf to current HEAD to pull the 2 following changes:
* 4587a3f [arm/gcc] Don't rely on KUSER_HELPERS feature for atomics
* a5a2c1d generic atomicops: Use strong compare_exchange

This change has been made following steps described in
third_party/protobuf/README.chromium file, please refer to it
for further information.

BUG=599051
==========

[jbriance, 2016-11-21 21:46:37]

The CQ bit was checked by jbriance@cisco.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-21 21:47:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jbriance, 2016-11-21 22:47:30]

PTAL

[commit-bot: I haz the power, 2016-11-22 01:00:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-22 01:00:05]

Dry run: This issue passed the CQ dry run.

[Peter Kasting, 2016-11-22 20:28:23]

https://codereview.chromium.org/2495533002/diff/70001/third_party/protobuf/pa...
File
third_party/protobuf/patches/0008-uninline_getemptystring_and_getemptystringalreadyinited.patch
(right):

https://codereview.chromium.org/2495533002/diff/70001/third_party/protobuf/pa...
third_party/protobuf/patches/0008-uninline_getemptystring_and_getemptystringalreadyinited.patch:9:
+const ::std::string& GetEmptyStringAlreadyInited() {
How come you switched to deinlining this?  It seems like doing so ought not to
have much effect.

https://codereview.chromium.org/2495533002/diff/70001/third_party/protobuf/pa...
File third_party/protobuf/patches/0011-uninline_internal_default_instance.patch
(right):

https://codereview.chromium.org/2495533002/diff/70001/third_party/protobuf/pa...
third_party/protobuf/patches/0011-uninline_internal_default_instance.patch:12: -
     "  return &$classname$_default_instance_.get();\n"
Is this needed because the relevant _default_instance_ objects are not exported
correctly?  Or because the generated output file here doesn't #include the
proper headers?  Or for some other reason?

It seems like we ought not to need a Chromium-side patch for this sort of thing,
but I'm not 100% sure.

[jbriance, 2016-11-22 22:11:31]

Do you think it would be better to export the symbols instead of deinline in
these cases ?

https://codereview.chromium.org/2495533002/diff/70001/third_party/protobuf/pa...
File
third_party/protobuf/patches/0008-uninline_getemptystring_and_getemptystringalreadyinited.patch
(right):

https://codereview.chromium.org/2495533002/diff/70001/third_party/protobuf/pa...
third_party/protobuf/patches/0008-uninline_getemptystring_and_getemptystringalreadyinited.patch:9:
+const ::std::string& GetEmptyStringAlreadyInited() {
fixed_address_empty_string is not exported, and thus "is_component_build=true"
builds fail without deinlining (because of undefined reference to
'google::protobuf::internal::fixed_address_empty_string')

I just realize that I should have done this in the 0011 patch instead of this
one, because the comment in README.chromium is misleading.

https://codereview.chromium.org/2495533002/diff/70001/third_party/protobuf/pa...
File third_party/protobuf/patches/0011-uninline_internal_default_instance.patch
(right):

https://codereview.chromium.org/2495533002/diff/70001/third_party/protobuf/pa...
third_party/protobuf/patches/0011-uninline_internal_default_instance.patch:12: -
     "  return &$classname$_default_instance_.get();\n"
> Is this needed because the relevant _default_instance_ objects are not
> exported correctly?

Yes, when "is_component_build=true" I get link errors.

For instance with libcc.so:
obj/cc/cc/compositor_state_deserializer.o:../src/cc/blimp/compositor_state_deserializer.cc:function
cc::CompositorStateDeserializer::DeserializeCompositorUpdate(cc::proto::LayerTreeHost
const&): error: undefined reference to 'cc::proto::LayerTree_default_instance_'
obj/cc/cc/compositor_state_deserializer.o:../src/cc/blimp/compositor_state_deserializer.cc:function
cc::CompositorStateDeserializer::DeserializeCompositorUpdate(cc::proto::LayerTreeHost
const&): error: undefined reference to 'cc::proto::SkPictures_default_instance_'
obj/cc/cc/compositor_state_deserializer.o:../src/cc/blimp/compositor_state_deserializer.cc:function
cc::CompositorStateDeserializer::DeserializeCompositorUpdate(cc::proto::LayerTreeHost
const&): error: undefined reference to
'cc::proto::LayerUpdate_default_instance_'
[...]

[Peter Kasting, 2016-11-22 22:16:09]

On Tue, Nov 22, 2016 at 2:11 PM, <jbriance@cisco.com> wrote:

> Do you think it would be better to export the symbols instead of deinline
> in
> these cases ?
>

In general, yes, I would prefer to export symbols rather than deinline
their accesses.  This should have less of a change on behavior, and would
hopefully be a patch that's easier to maintain in the future (e.g. does not
break if more accesses to those variables are added).  It may be a patch
that can be submitted upstream as well, which is always a win.  (But I'm
not sure.)

PK

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jbriance, 2016-12-08 12:39:13]

Protobuf pull request https://github.com/google/protobuf/pull/2407 has landed.

Once the two following ones are landed, it should be fine:
- https://github.com/google/protobuf/pull/2461
- https://github.com/google/protobuf/pull/2462

[jbriance, 2016-12-09 08:36:35]

On 2016/12/08 12:39:13, jbriance wrote:
> Protobuf pull request https://github.com/google/protobuf/pull/2407 has landed.
> 
> Once the two following ones are landed, it should be fine:
> - https://github.com/google/protobuf/pull/2461
> - https://github.com/google/protobuf/pull/2462

All PRs have landed, but Chromium presubmit pointed out another thing to do :
https://github.com/google/protobuf/pull/2471

[jbriance, 2016-12-13 08:34:10]

Description was changed from

==========
third_party/protobuf: Update to HEAD (b7632464b4)

Rebase protobuf to current HEAD to pull the 2 following changes:
* 4587a3f [arm/gcc] Don't rely on KUSER_HELPERS feature for atomics
* a5a2c1d generic atomicops: Use strong compare_exchange

This change has been made following steps described in
third_party/protobuf/README.chromium file, please refer to it
for further information.

BUG=599051
==========

to

==========
third_party/protobuf: Update to HEAD (eb455ce1c9)

Rebase protobuf to current HEAD to pull the 2 following changes:
* 4587a3f [arm/gcc] Don't rely on KUSER_HELPERS feature for atomics
* a5a2c1d generic atomicops: Use strong compare_exchange

This change has been made following steps described in
third_party/protobuf/README.chromium file, please refer to it
for further information.

BUG=599051
==========

[jbriance, 2016-12-13 08:34:57]

The CQ bit was checked by jbriance@cisco.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-13 08:35:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-13 08:59:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-13 08:59:28]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jbriance, 2016-12-13 09:16:13]

Seems that another PR is needed here:
https://github.com/google/protobuf/pull/2493

[jbriance, 2016-12-13 10:50:58]

The CQ bit was checked by jbriance@cisco.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-13 10:51:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-13 11:16:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-13 11:16:19]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jbriance, 2016-12-13 14:11:20]

The CQ bit was checked by jbriance@cisco.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-13 14:11:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-13 14:34:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-13 14:34:52]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[xyzzyz, 2016-12-13 19:37:55]

FYI: we'd also like to get this merged and land in Chromium.
https://github.com/google/protobuf/pull/2496 . See
https://bugs.chromium.org/p/chromium/issues/detail?id=673488

[xyzzyz, 2016-12-13 23:23:37]

On 2016/12/13 19:37:55, xyzzyz wrote:
> FYI: we'd also like to get this merged and land in Chromium.
> https://github.com/google/protobuf/pull/2496 . See
> https://bugs.chromium.org/p/chromium/issues/detail?id=673488

https://github.com/google/protobuf/pull/2496 is now merged.

[jbriance, 2016-12-14 08:04:24]

Description was changed from

==========
third_party/protobuf: Update to HEAD (eb455ce1c9)

Rebase protobuf to current HEAD to pull the 2 following changes:
* 4587a3f [arm/gcc] Don't rely on KUSER_HELPERS feature for atomics
* a5a2c1d generic atomicops: Use strong compare_exchange

This change has been made following steps described in
third_party/protobuf/README.chromium file, please refer to it
for further information.

BUG=599051
==========

to

==========
third_party/protobuf: Update to HEAD (f983302ca7)

Rebase protobuf to current HEAD to pull the 3 following changes:
* 4c5d3ed Fix integer overflow in FastUInt32ToBufferLeft
* 4587a3f [arm/gcc] Don't rely on KUSER_HELPERS feature for atomics
* a5a2c1d generic atomicops: Use strong compare_exchange

This change has been made following steps described in
third_party/protobuf/README.chromium file, please refer to it
for further information.

BUG=599051,673488
==========

[jbriance, 2016-12-14 08:05:02]

The CQ bit was checked by jbriance@cisco.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-14 08:05:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jbriance, 2016-12-14 08:06:35]

On 2016/12/13 23:23:37, xyzzyz wrote:
> https://github.com/google/protobuf/pull/2496 is now merged.

Thanks, it is part of patch set 12.
I've also updated the commit message accordingly.

[commit-bot: I haz the power, 2016-12-14 09:26:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-14 09:26:48]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[jbriance, 2016-12-14 15:32:38]

I'll need some help, to fix windows and android trybot issues seen with patch
set 12.

1) I do not have a win environment, and I do not understand where the proto
files used to generate chrome_settings.pb.h and cloud_policy.pb.h come from:

  [8860/41451] CXX
obj/components/policy/chrome_settings_proto/chrome_settings.pb.obj
  FAILED: obj/components/policy/chrome_settings_proto/chrome_settings.pb.obj 
  ninja -t msvc -e environment.x86 -- E:\b\c\cipd\goma/gomacc.exe
"E:\b\depot_tools\win_toolchain\vs_files\d3cb0e37bdd120ad0ac4650b674b09e81be45616\VC\bin\amd64_x86/cl.exe"
/nologo /showIncludes /FC
@obj/components/policy/chrome_settings_proto/chrome_settings.pb.obj.rsp /c
gen/components/policy/proto/chrome_settings.pb.cc
/Foobj/components/policy/chrome_settings_proto/chrome_settings.pb.obj
/Fd"obj/components/policy/chrome_settings_proto_cc.pdb"
 
e:\b\c\b\win\src\out\debug\gen\components\policy\proto\chrome_settings.pb.h(127):
error C2220: warning treated as error - no 'object' file generated
 
e:\b\c\b\win\src\out\debug\gen\components\policy\proto\chrome_settings.pb.h(127):
warning C4273: 'enterprise_management::_BooleanPolicyProto_default_instance_':
inconsistent dll linkage
  e:\b\c\b\win\src\out\debug\gen\components\policy\proto\cloud_policy.pb.h(35):
note: see previous definition of '_BooleanPolicyProto_default_instance_'
 
e:\b\c\b\win\src\out\debug\gen\components\policy\proto\chrome_settings.pb.h(163):
warning C4273: 'enterprise_management::_CloudPolicySettings_default_instance_':
inconsistent dll linkage
  e:\b\c\b\win\src\out\debug\gen\components\policy\proto\cloud_policy.pb.h(38):
note: see previous definition of '_CloudPolicySettings_default_instance_'
 
e:\b\c\b\win\src\out\debug\gen\components\policy\proto\chrome_settings.pb.h(508):
warning C4273: 'enterprise_management::_IntegerPolicyProto_default_instance_':
inconsistent dll linkage
  e:\b\c\b\win\src\out\debug\gen\components\policy\proto\cloud_policy.pb.h(41):
note: see previous definition of '_IntegerPolicyProto_default_instance_'
 
e:\b\c\b\win\src\out\debug\gen\components\policy\proto\chrome_settings.pb.h(607):
warning C4273: 'enterprise_management::_PolicyOptions_default_instance_':
inconsistent dll linkage
  e:\b\c\b\win\src\out\debug\gen\components\policy\proto\cloud_policy.pb.h(44):
note: see previous definition of '_PolicyOptions_default_instance_'
 
e:\b\c\b\win\src\out\debug\gen\components\policy\proto\chrome_settings.pb.h(823):
warning C4273: 'enterprise_management::_StringList_default_instance_':
inconsistent dll linkage
  e:\b\c\b\win\src\out\debug\gen\components\policy\proto\cloud_policy.pb.h(47):
note: see previous definition of '_StringList_default_instance_'
 
e:\b\c\b\win\src\out\debug\gen\components\policy\proto\chrome_settings.pb.h(826):
warning C4273:
'enterprise_management::_StringListPolicyProto_default_instance_': inconsistent
dll linkage
  e:\b\c\b\win\src\out\debug\gen\components\policy\proto\cloud_policy.pb.h(50):
note: see previous definition of '_StringListPolicyProto_default_instance_'
 
e:\b\c\b\win\src\out\debug\gen\components\policy\proto\chrome_settings.pb.h(829):
warning C4273: 'enterprise_management::_StringPolicyProto_default_instance_':
inconsistent dll linkage
  e:\b\c\b\win\src\out\debug\gen\components\policy\proto\cloud_policy.pb.h(53):
note: see previous definition of '_StringPolicyProto_default_instance_'


2) I don't have an android environment either, and the linux_android_rel_ng
trybot reports failures in component_unittests:

  [UNKNOWN] PrecacheFetcherTest.PrecacheResourceSelection:
  [ RUN      ] PrecacheFetcherTest.PrecacheResourceSelection
  [libprotobuf FATAL
../../third_party/protobuf/src/google/protobuf/stubs/hash.h:188] Should never be
called.[ CRASHED      ]
  [UNKNOWN] PrecacheFetcherTest.PrecacheResourceSelectionMissingBitset:
  [ RUN      ] PrecacheFetcherTest.PrecacheResourceSelectionMissingBitset
  [libprotobuf FATAL
../../third_party/protobuf/src/google/protobuf/stubs/hash.h:188] Should never be
called.[ CRASHED      ]
  [UNKNOWN] TranslateRankerTest.CalculateScore:
  [ RUN      ] TranslateRankerTest.CalculateScore
  [libprotobuf FATAL
../../third_party/protobuf/src/google/protobuf/stubs/hash.h:202] Should never be
called.[ CRASHED      ]
  [UNKNOWN] TranslateRankerTest.LoggingDisabled:
  [ RUN      ] TranslateRankerTest.LoggingDisabled
  [libprotobuf FATAL
../../third_party/protobuf/src/google/protobuf/stubs/hash.h:202] Should never be
called.[ CRASHED      ]
  [UNKNOWN] TranslateRankerTest.RecordAndFlushEvents:
  [ RUN      ] TranslateRankerTest.RecordAndFlushEvents
  [libprotobuf FATAL
../../third_party/protobuf/src/google/protobuf/stubs/hash.h:202] Should never be
called.[ CRASHED      ]
  [UNKNOWN] TranslateRankerTest.ShouldOfferTranslation:
  [ RUN      ] TranslateRankerTest.ShouldOfferTranslation
  [libprotobuf FATAL
../../third_party/protobuf/src/google/protobuf/stubs/hash.h:202] Should never be
called.[ CRASHED      ]

[Peter Kasting, 2016-12-14 20:21:53]

On Wed, Dec 14, 2016 at 7:32 AM, <jbriance@cisco.com> wrote:

> 1) I do not have a win environment, and I do not understand where the proto
> files used to generate chrome_settings.pb.h and cloud_policy.pb.h come from
>

Looking up the names:

https://cs.chromium.org/search/?q=chrome_settings.proto&sq=package:chromium&t...
https://cs.chromium.org/search/?q=cloud_policy.proto&sq=package:chromium&type=cs

Looks like these are both autogenerated
by src/components/policy/tools/generate_policy_source.py.

However, just looking at the proto files themselves, I don't see anything
obviously weird.  Both use the same syntax, optimize_for, etc.

One file "import"s the other.  Your error messages suggest to me that
something in protoc changed so that it doesn't give the same linkage to
symbols from the original proto as when that proto is imported elsewhere.

Looking at the old version of one of the generated headers sitting on my
drive, this looks like it has something to do with inlining and static
initializers?  Maybe with one of the patches we apply locally to deinline
things?  I'd look for "default_instance" in the sources, especially in our
local patches.  We probably aren't tagging something with the right DLL
linkage macros in the generated code we output somewhere?

Let me know if this still isn't enough to help and you need me to try and
patch things in locally and debug.

PK

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jbriance, 2016-12-14 21:27:07]

Thanks for your detailed answer :-)

Also, about the 2nd issue (android trybot), I feel fairly optimistic about
https://github.com/google/protobuf/pull/2495

[jbriance, 2016-12-14 22:32:00]

The CQ bit was checked by jbriance@cisco.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-14 22:32:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jbriance, 2016-12-14 22:37:04]

Description was changed from

==========
third_party/protobuf: Update to HEAD (f983302ca7)

Rebase protobuf to current HEAD to pull the 3 following changes:
* 4c5d3ed Fix integer overflow in FastUInt32ToBufferLeft
* 4587a3f [arm/gcc] Don't rely on KUSER_HELPERS feature for atomics
* a5a2c1d generic atomicops: Use strong compare_exchange

This change has been made following steps described in
third_party/protobuf/README.chromium file, please refer to it
for further information.

BUG=599051,673488
==========

to

==========
third_party/protobuf: Update to HEAD (83d681ee2c)

Rebase protobuf to current HEAD to pull the 3 following changes:
* 4c5d3ed Fix integer overflow in FastUInt32ToBufferLeft
* 4587a3f [arm/gcc] Don't rely on KUSER_HELPERS feature for atomics
* a5a2c1d generic atomicops: Use strong compare_exchange

This change has been made following steps described in
third_party/protobuf/README.chromium file, please refer to it
for further information.

BUG=599051,673488
==========

[jbriance, 2016-12-14 22:38:59]

jbriance@cisco.com changed reviewers:
+ pastarmovj@chromium.org

[jbriance, 2016-12-14 22:39:04]

+ pastarmovj for components/policy/BUILD.gn review

[commit-bot: I haz the power, 2016-12-14 22:46:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-14 22:46:25]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)

[jbriance, 2016-12-14 23:57:20]

The CQ bit was checked by jbriance@cisco.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-14 23:58:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-15 01:11:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-15 01:11:59]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[jbriance, 2016-12-15 07:09:02]

The CQ bit was checked by jbriance@cisco.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-15 07:09:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-15 08:45:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-15 08:45:58]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[jbriance, 2016-12-15 09:07:19]

The CQ bit was checked by jbriance@cisco.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-15 09:07:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-15 10:34:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-15 10:34:43]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[jbriance, 2016-12-15 13:39:51]

The CQ bit was checked by jbriance@cisco.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-15 13:40:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-15 14:40:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-15 14:40:48]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[jbriance, 2016-12-15 15:21:02]

Mmm, patch set 16 was not a good idea. The previous one was better :/

[jbriance, 2016-12-15 17:52:27]

The CQ bit was checked by jbriance@cisco.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-15 17:53:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-15 18:59:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-15 18:59:10]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[jbriance, 2016-12-15 22:04:08]

The CQ bit was checked by jbriance@cisco.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-15 22:05:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jbriance, 2016-12-15 23:18:36]

The CQ bit was checked by jbriance@cisco.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-15 23:19:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-16 01:55:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-16 01:55:51]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[jbriance, 2016-12-16 08:34:17]

The CQ bit was checked by jbriance@cisco.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-16 08:34:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jbriance, 2016-12-16 10:00:29]

The CQ bit was checked by jbriance@cisco.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-16 10:00:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jbriance, 2016-12-16 11:11:13]

The CQ bit was checked by jbriance@cisco.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-16 11:11:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jbriance, 2016-12-16 12:48:59]

The CQ bit was checked by jbriance@cisco.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-16 12:49:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-16 13:53:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-16 13:53:54]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[jbriance, 2016-12-16 16:16:09]

The CQ bit was checked by jbriance@cisco.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-16 16:16:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jbriance, 2016-12-16 17:32:36]

I can't get rid of the following link error for "browser_tests.exe" (win_clang
and win_chromium_compile_dbg_ng try bots).
Without a win environment, and because of my (very) limited knowledge in this
area (chrome_settings proto, cloud policy proto and browser tests), it's hard
for me to make progress.
Do you have guys any hint or advice to share ? :)


  FAILED: browser_tests.exe browser_tests.exe.pdb 
  E:/b/depot_tools/python276_bin/python.exe
../../build/toolchain/win/tool_wrapper.py link-wrapper environment.x64 False
link.exe /nologo /OUT:./browser_tests.exe /PDB:./browser_tests.exe.pdb
@./browser_tests.exe.rsp
 
cloud_policy_proto_generated_compile.dll.lib(cloud_policy_proto_generated_compile.dll)
: error LNK2005: "public: void __cdecl
enterprise_management::StringList::add_entries(char const *)"
(?add_entries@StringList@enterprise_management@@QEAAXPEBD@Z) already defined in
chrome_settings.pb.obj
 
cloud_policy_proto_generated_compile.dll.lib(cloud_policy_proto_generated_compile.dll)
: error LNK2005: "public: void __cdecl
enterprise_management::PolicyOptions::set_mode(enum
enterprise_management::PolicyOptions_PolicyMode)"
(?set_mode@PolicyOptions@enterprise_management@@QEAAXW4PolicyOptions_PolicyMode@2@@Z)
already defined in chrome_settings.pb.obj
 
cloud_policy_proto_generated_compile.dll.lib(cloud_policy_proto_generated_compile.dll)
: error LNK2005: "public: __cdecl
enterprise_management::CloudPolicySettings::CloudPolicySettings(void)"
(??0CloudPolicySettings@enterprise_management@@QEAA@XZ) already defined in
cloud_policy.pb.obj
 
cloud_policy_proto_generated_compile.dll.lib(cloud_policy_proto_generated_compile.dll)
: error LNK2005: "public: class enterprise_management::StringPolicyProto *
__cdecl
enterprise_management::CloudPolicySettings::mutable_homepagelocation(void)"
(?mutable_homepagelocation@CloudPolicySettings@enterprise_management@@QEAAPEAVStringPolicyProto@2@XZ)
already defined in chrome_settings.pb.obj
 
cloud_policy_proto_generated_compile.dll.lib(cloud_policy_proto_generated_compile.dll)
: error LNK2005: "public: void __cdecl
enterprise_management::StringPolicyProto::set_value(char const *)"
(?set_value@StringPolicyProto@enterprise_management@@QEAAXPEBD@Z) already
defined in chrome_settings.pb.obj
 
cloud_policy_proto_generated_compile.dll.lib(cloud_policy_proto_generated_compile.dll)
: error LNK2005: "public: class enterprise_management::BooleanPolicyProto *
__cdecl
enterprise_management::CloudPolicySettings::mutable_showhomebutton(void)"
(?mutable_showhomebutton@CloudPolicySettings@enterprise_management@@QEAAPEAVBooleanPolicyProto@2@XZ)
already defined in chrome_settings.pb.obj
 
cloud_policy_proto_generated_compile.dll.lib(cloud_policy_proto_generated_compile.dll)
: error LNK2005: "public: void __cdecl
enterprise_management::BooleanPolicyProto::set_value(bool)"
(?set_value@BooleanPolicyProto@enterprise_management@@QEAAX_N@Z) already defined
in chrome_settings.pb.obj
 
cloud_policy_proto_generated_compile.dll.lib(cloud_policy_proto_generated_compile.dll)
: error LNK2005: "public: class enterprise_management::IntegerPolicyProto *
__cdecl
enterprise_management::CloudPolicySettings::mutable_restoreonstartup(void)"
(?mutable_restoreonstartup@CloudPolicySettings@enterprise_management@@QEAAPEAVIntegerPolicyProto@2@XZ)
already defined in chrome_settings.pb.obj
 
cloud_policy_proto_generated_compile.dll.lib(cloud_policy_proto_generated_compile.dll)
: error LNK2005: "public: void __cdecl
enterprise_management::IntegerPolicyProto::set_value(__int64)"
(?set_value@IntegerPolicyProto@enterprise_management@@QEAAX_J@Z) already defined
in chrome_settings.pb.obj
 
cloud_policy_proto_generated_compile.dll.lib(cloud_policy_proto_generated_compile.dll)
: error LNK2005: "public: class enterprise_management::StringListPolicyProto *
__cdecl
enterprise_management::CloudPolicySettings::mutable_disabledschemes(void)"
(?mutable_disabledschemes@CloudPolicySettings@enterprise_management@@QEAAPEAVStringListPolicyProto@2@XZ)
already defined in chrome_settings.pb.obj
 
cloud_policy_proto_generated_compile.dll.lib(cloud_policy_proto_generated_compile.dll)
: error LNK2005: "public: class enterprise_management::StringList * __cdecl
enterprise_management::StringListPolicyProto::mutable_value(void)"
(?mutable_value@StringListPolicyProto@enterprise_management@@QEAAPEAVStringList@2@XZ)
already defined in chrome_settings.pb.obj
 
cloud_policy_proto_generated_compile.dll.lib(cloud_policy_proto_generated_compile.dll)
: error LNK2005: "public: class enterprise_management::BooleanPolicyProto *
__cdecl
enterprise_management::CloudPolicySettings::mutable_searchsuggestenabled(void)"
(?mutable_searchsuggestenabled@CloudPolicySettings@enterprise_management@@QEAAPEAVBooleanPolicyProto@2@XZ)
already defined in chrome_settings.pb.obj
 
cloud_policy_proto_generated_compile.dll.lib(cloud_policy_proto_generated_compile.dll)
: error LNK2005: "public: class enterprise_management::PolicyOptions * __cdecl
enterprise_management::BooleanPolicyProto::mutable_policy_options(void)"
(?mutable_policy_options@BooleanPolicyProto@enterprise_management@@QEAAPEAVPolicyOptions@2@XZ)
already defined in chrome_settings.pb.obj
 
cloud_policy_proto_generated_compile.dll.lib(cloud_policy_proto_generated_compile.dll)
: error LNK2005: "public: class enterprise_management::BooleanPolicyProto *
__cdecl enterprise_management::CloudPolicySettings::mutable_syncdisabled(void)"
(?mutable_syncdisabled@CloudPolicySettings@enterprise_management@@QEAAPEAVBooleanPolicyProto@2@XZ)
already defined in chrome_settings.pb.obj
 
cloud_policy_proto_generated_compile.dll.lib(cloud_policy_proto_generated_compile.dll)
: error LNK2005: "public: virtual __cdecl
enterprise_management::CloudPolicySettings::~CloudPolicySettings(void)"
(??1CloudPolicySettings@enterprise_management@@UEAA@XZ) already defined in
cloud_policy.pb.obj

[commit-bot: I haz the power, 2016-12-16 19:07:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-16 19:07:08]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Peter Kasting, 2016-12-17 04:03:28]

A bunch of class member definitions are being multiply defined, which is very
strange.  If those had been generated in some kind of class declaration, they'd
have been implicitly inlined.  So presumably protoc is actually dumping the
definitions into the output.  Can you look at the generated
chrome_settings.pb.cc and cloud_policy.pb.cc files on your non-Windows machine
to see what they contain w.r.t. these functions?  I'd first try to figure out if
this is a Clang-vs.-MSVC difference vs. a "different .cc files on different
platforms" issue by seeing if the files on Linux look suspiciously like they
multiply-define these same symbols.

[jbriance, 2016-12-18 22:51:54]

Thanks for the help Peter.
I finally managed to setup a win environment on my home PC (it's actually easier
than I thought it would be) and I think I'm now close to fix the issue.
I'll update a new patch set soon.

[jbriance, 2016-12-18 23:04:50]

The CQ bit was checked by jbriance@cisco.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-18 23:05:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-19 00:11:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-19 00:11:31]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jbriance, 2016-12-19 06:58:32]

The CQ bit was checked by jbriance@cisco.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-19 06:58:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-19 08:42:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-19 08:42:37]

Dry run: This issue passed the CQ dry run.

[jbriance, 2016-12-19 08:49:47]

Patch set 25 is a good candidate for CQ (I should have setup a win environment
earlier...).

Yulian, please review components/policy/BUILD.gn file
Peter, please take another look at the rest of the change

[pastarmovj, 2016-12-19 12:59:57]

lgtm

[Peter Kasting, 2016-12-20 02:51:34]

LGTM for the changes themselves.

Do you know what the binary size impact of this CL will be?  My main concern is
ensuring it doesn't jump significantly, as that was a big issue during the
protobuf 2 -> protobuf 3 upgrade process.

Casual evidence that suggests this is OK (e.g. build locally with and without
this patch, in any mode, and look at your local binary size; if it doesn't
change much, we're probably fine) is good enough for me here.

[jbriance, 2016-12-20 13:56:37]

On 2016/12/20 02:51:34, Peter Kasting wrote:
> Do you know what the binary size impact of this CL will be?

* Base src revision: 7ad32cbf574e3d263b64f1888520b9158798d41b
* Four different configurations:
  - Release (is_component_build = false / is_debug = false)
  - Release comp (is_component_build = true / is_debug = false)
  - Debug (is_component_build = false / is_debug = true)
  - Debug comp (is_component_build = true / is_debug = true)
* Target: chrome:chrome (Linux x64 build)


               +---------------------------------------------------------+
	       |                      chrome binary                      |
               +---------------+-------------------+---------------------+
	       | Base revision | With patch set 25 |        Delta        |
+--------------+---------------+-------------------+---------------------+
| Release      |    172 408 Kb |        172 300 Kb | -   108 Kb (-0.06%) |
| Release comp |     58 024 Kb |         57 968 Kb | -    56 Kb (-0.10%) |
| Debug        |  2 554 176 Kb |      2 558 576 Kb | + 4 400 Kb (+0.17%) |
| Debug comp   |    891 256 Kb |        894 520 Kb | + 3 264 Kb (+0.37%) |
+--------------+---------------+-------------------+---------------------+


               +---------------------------------------------------------+
	       |               libcontent.so (comp builds)               |
               +---------------+-------------------+---------------------+
	       | Base revision | With patch set 25 |        Delta        |
+--------------+---------------+-------------------+---------------------+
| Release comp |     34 660 Kb |         34 656 Kb | -     4 Kb (-0.01%) |
| Debug comp   |    485 272 Kb |        485 996 Kb | +   724 Kb (+0.15%) |
+--------------+---------------+-------------------+---------------------+


I'm sending patch set 25 to CQ, as binary size does not change much (release
builds seem even to be slightly smaller).

[jbriance, 2016-12-20 13:58:13]

The CQ bit was checked by jbriance@cisco.com

[commit-bot: I haz the power, 2016-12-20 13:58:41]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-20 16:49:51]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1482242293348390,
"parent_rev": "885e8d4927f4a2baea2ee1db969e72e7c525ead3", "commit_rev":
"0276337ad57f5ed7020cd70f63be9bbc5ab256dc"}

[commit-bot: I haz the power, 2016-12-20 16:50:26]

Description was changed from

==========
third_party/protobuf: Update to HEAD (83d681ee2c)

Rebase protobuf to current HEAD to pull the 3 following changes:
* 4c5d3ed Fix integer overflow in FastUInt32ToBufferLeft
* 4587a3f [arm/gcc] Don't rely on KUSER_HELPERS feature for atomics
* a5a2c1d generic atomicops: Use strong compare_exchange

This change has been made following steps described in
third_party/protobuf/README.chromium file, please refer to it
for further information.

BUG=599051,673488
==========

to

==========
third_party/protobuf: Update to HEAD (83d681ee2c)

Rebase protobuf to current HEAD to pull the 3 following changes:
* 4c5d3ed Fix integer overflow in FastUInt32ToBufferLeft
* 4587a3f [arm/gcc] Don't rely on KUSER_HELPERS feature for atomics
* a5a2c1d generic atomicops: Use strong compare_exchange

This change has been made following steps described in
third_party/protobuf/README.chromium file, please refer to it
for further information.

BUG=599051,673488

Review-Url: https://codereview.chromium.org/2495533002
==========

[commit-bot: I haz the power, 2016-12-20 16:50:32]

Committed patchset #25 (id:240001)

[commit-bot: I haz the power, 2016-12-20 16:52:58]

Description was changed from

==========
third_party/protobuf: Update to HEAD (83d681ee2c)

Rebase protobuf to current HEAD to pull the 3 following changes:
* 4c5d3ed Fix integer overflow in FastUInt32ToBufferLeft
* 4587a3f [arm/gcc] Don't rely on KUSER_HELPERS feature for atomics
* a5a2c1d generic atomicops: Use strong compare_exchange

This change has been made following steps described in
third_party/protobuf/README.chromium file, please refer to it
for further information.

BUG=599051,673488

Review-Url: https://codereview.chromium.org/2495533002
==========

to

==========
third_party/protobuf: Update to HEAD (83d681ee2c)

Rebase protobuf to current HEAD to pull the 3 following changes:
* 4c5d3ed Fix integer overflow in FastUInt32ToBufferLeft
* 4587a3f [arm/gcc] Don't rely on KUSER_HELPERS feature for atomics
* a5a2c1d generic atomicops: Use strong compare_exchange

This change has been made following steps described in
third_party/protobuf/README.chromium file, please refer to it
for further information.

BUG=599051,673488

Committed: https://crrev.com/4fa2d9262fd91a4e613269f230cca34b95ca4af7
Cr-Commit-Position: refs/heads/master@{#439821}
==========

[commit-bot: I haz the power, 2016-12-20 16:53:04]

Patchset 25 (id:??) landed as
https://crrev.com/4fa2d9262fd91a4e613269f230cca34b95ca4af7
Cr-Commit-Position: refs/heads/master@{#439821}

[Peter Kasting, 2016-12-20 20:14:57]

FYI, this got reverted due to MSAN breakage (use of uninitialized value).  See
https://codereview.chromium.org/2590803003/ for more details.

I'm not 100% sure how to reproduce this locally (never had to try).  If the info
on that CL and from the linked logs is insufficient to track this down, let me
know, and I'll try and find a knowledgeable memory person to help.

[jbriance, 2016-12-20 22:15:01]

On 2016/12/20 20:14:57, Peter Kasting wrote:
> FYI, this got reverted due to MSAN breakage (use of uninitialized value).
> See https://codereview.chromium.org/2590803003/ for more details.

Thanks for pointing this out :)

> I'm not 100% sure how to reproduce this locally (never had to try).  If the
> info on that CL and from the linked logs is insufficient to track this down,
> let me know, and I'll try and find a knowledgeable memory person to help.

I'm able to reproduce locally (I'm more comfortable with Linux build env)
and I've sent a PR that is fixing the issue in my local env:
https://github.com/google/protobuf/pull/2523

[jbriance, 2016-12-23 09:42:53]

Follow-up: https://codereview.chromium.org/2599263002/