New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 723090 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: May 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: ----
Type: ----

Blocking:
issue 722827



Sign in to add a comment

compile failure on chromium.chromiumos/ChromiumOS x86-generic with dcheck_always_on:true

Project Member Reported by dmazz...@chromium.org, May 16 2017

Issue description

compile failure on chromium.chromiumos/ChromiumOS x86-generic Compile

Builders failed on: 
- ChromiumOS x86-generic Compile: 
  https://build.chromium.org/p/chromium.chromiumos/builders/ChromiumOS%20x86-generic%20Compile

https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.chromiumos%2FChromiumOS_x86-generic_Compile%2F35656%2F%2B%2Frecipes%2Fsteps%2Fcompile%2F0%2Fstdout

../../media/capture/content/video_capture_oracle.cc: In member function 'void media::VideoCaptureOracle::SetSourceSize(const gfx::Size&)':
../../media/capture/content/video_capture_oracle.cc:390:56: error: assuming signed overflow does not occur when assuming that (X - c) <= X is always true [-Werror=strict-overflow]
   return (frame_number >= 0 && next_frame_number_ >= 0 &&


Possible also dcheck-related?


 
Blockedon: 722827
Project Member

Comment 2 by bugdroid1@chromium.org, May 16 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8cc767cb0d1c65af6d5a36858c9633c929b7e4cd

commit 8cc767cb0d1c65af6d5a36858c9633c929b7e4cd
Author: findit-for-me <findit-for-me@appspot.gserviceaccount.com>
Date: Tue May 16 23:14:54 2017

Revert of Enable DCHECKs by default in non-official builds. (patchset #3 id:40001 of https://codereview.chromium.org/2886803002/ )

Reason for revert:

Findit (https://goo.gl/kROfz5) identified CL at revision 472195 as the
culprit for failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3ItbWVyRAsSDVdmU3VzcGVjdGVkQ0wiMWNocm9taXVtLzg3NWEyZmE0M2NhYWIwZWU4MmVmZTY4ZmI1YWRiMzU3MzkyNDM0YWQM

Original issue's description:
> Enable DCHECKs by default in non-official builds.
>
> BUG= 722827 
> TBR=sdefresne@chromium.org for ios/
>
> Review-Url: https://codereview.chromium.org/2886803002
> Cr-Commit-Position: refs/heads/master@{#472195}
> Committed: https://chromium.googlesource.com/chromium/src/+/875a2fa43caab0ee82efe68fb5adb357392434ad

TBR=brettw@chromium.org,thakis@chromium.org,sdefresne@chromium.org,gab@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 722827 , 723090 , 723049 

Review-Url: https://codereview.chromium.org/2892473002
Cr-Commit-Position: refs/heads/master@{#472242}

[modify] https://crrev.com/8cc767cb0d1c65af6d5a36858c9633c929b7e4cd/build/config/dcheck_always_on.gni
[modify] https://crrev.com/8cc767cb0d1c65af6d5a36858c9633c929b7e4cd/ios/web/web_state/ui/wk_web_view_configuration_provider.mm

Comment 3 by gab@chromium.org, May 16 2017

Blocking: 722827

Comment 4 by gab@chromium.org, May 16 2017

Blockedon: -722827

Comment 5 by gab@chromium.org, May 17 2017

Cc: esum@chromium.org thakis@chromium.org
+esum who added this very logic to prevent this very check a long time ago in r341808 (https://codereview.chromium.org/1263273003/).

Any idea why this would not compile under dcheck_always_on=true?

+thakis as well for compiler/toolchain tip?

Comment 6 by esum@chromium.org, May 17 2017

Cc: lcwu@chromium.org
+lcwu

Sorry, I took a look but am not sure. Maybe Le-Chun would know.

Comment 7 by gab@chromium.org, May 17 2017

Maybe it's because this method is used inside a DCHECK in the same .cc (which potentially inlines and that potentially breaks what r341808 is trying to achieve to avoid the compile warning about unchecked signed overflow?).

Comment 8 by peria@chromium.org, May 17 2017

Status: Fixed (was: Assigned)

Comment 9 by gab@chromium.org, May 17 2017

Status: Assigned (was: Fixed)
Summary: compile failure on chromium.chromiumos/ChromiumOS x86-generic with dcheck_always_on:true (was: compile failure on chromium.chromiumos/ChromiumOS x86-generic Compile)
Keep this opened as it's still an issue, it's just that the dcheck CL was temporarily reverted.
Labels: -Sheriff-Chromium
Removing from sheriff queue since not blocking tree

Comment 11 by gab@chromium.org, May 17 2017

Forcing NOINLINE fixes this :) https://codereview.chromium.org/2888783004/#
Project Member

Comment 12 by bugdroid1@chromium.org, May 18 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2a81129250714241687f3a8f51fb0915aa4f1a6f

commit 2a81129250714241687f3a8f51fb0915aa4f1a6f
Author: gab <gab@chromium.org>
Date: Thu May 18 04:29:34 2017

NOINLINE in VideoCaptureOracle::IsFrameInRecentHistory to address  issue 723090 

Inlining was the issue, tested @ https://codereview.chromium.org/2890783002/
with (PS3) and without (PS2) this change. Only compiles with.

BUG= 723090 

Review-Url: https://codereview.chromium.org/2888783004
Cr-Commit-Position: refs/heads/master@{#472663}

[modify] https://crrev.com/2a81129250714241687f3a8f51fb0915aa4f1a6f/media/capture/content/video_capture_oracle.cc

Comment 13 by gab@chromium.org, May 18 2017

Status: Fixed (was: Assigned)

Sign in to add a comment