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

Issue 812058 link

Starred by 5 users

Issue metadata

Status: Fixed
Owner:
Closed: Apr 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 1
Type: Bug



Sign in to add a comment

Migrate Albatross (DCHECK-enabled builds w/ run-time switchable behaviour) to not depend on SyzyASAN

Project Member Reported by w...@chromium.org, Feb 14 2018

Issue description

"Albatross" builds (ones with DCHECKs built-in, but configured to ERROR level by default, and run-time switchable to FATAL level) are currently implicitly tied to the is_syzyasan GN configuration.  We would like Albatross to be able to lumber along with or without SyzyASAN in future, so:

- Add a |dcheck_is_runtime_switchable| (or something shorter) alongside |dcheck_always_on|.
- If possible, have |dcheck_always_on| implicitly set if |dcheck_is_runtime_switchable| is set.
- If possible, have |dcheck_is_runtime_switchable| set if |is_syzyasan| is set.
- Add a buildflag_header() (or add to the existing "debug" header) for the few call-sites that need to know when runtime-switchable behaviour is enabled.
- Switch over the call-sites to use the buildflag header.
 

Comment 1 by w...@chromium.org, Feb 14 2018

Owner: w...@chromium.org
Status: Assigned (was: Untriaged)

Comment 2 by w...@chromium.org, Mar 13 2018

Labels: -Pri-2 Pri-1

Comment 3 by siggi@chromium.org, Mar 14 2018

Blocking: 821764

Comment 4 by w...@chromium.org, Mar 19 2018

Status: Started (was: Assigned)
Project Member

Comment 5 by bugdroid1@chromium.org, Mar 22 2018

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

commit 02baf96300ee981ae30653d7e7ac5da94a252c3a
Author: Wez <wez@chromium.org>
Date: Thu Mar 22 02:03:33 2018

Add missing BUILD.gn dependencies and enable GN checks.

While adding the |dcheck_is_configurable| build argument, the new
dependency of base/logging.h on a buildflags_header() caused missing GN
dependencies to break the build.

Add the (known) missing dependencies, and enable GN checks for those
targets.

Bug:  812058 
Change-Id: I5aed4d5a62619e026cdb86e5d6a5c33858875bd4
Reviewed-on: https://chromium-review.googlesource.com/970165
Commit-Queue: Wez <wez@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: John Budorick <jbudorick@chromium.org>
Cr-Commit-Position: refs/heads/master@{#544946}
[modify] https://crrev.com/02baf96300ee981ae30653d7e7ac5da94a252c3a/.gn
[modify] https://crrev.com/02baf96300ee981ae30653d7e7ac5da94a252c3a/extensions/browser/install/BUILD.gn
[modify] https://crrev.com/02baf96300ee981ae30653d7e7ac5da94a252c3a/extensions/common/BUILD.gn
[modify] https://crrev.com/02baf96300ee981ae30653d7e7ac5da94a252c3a/ppapi/tests/extensions/BUILD.gn

Project Member

Comment 6 by bugdroid1@chromium.org, Mar 22 2018

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

commit 51afb793d95a621b85aa812cb286e2385fc6de8f
Author: Wez <wez@chromium.org>
Date: Thu Mar 22 16:10:07 2018

Add missing BUILD.gn dependencies and enable GN checks.

Adds some missing dependencies in //third_party/webrtc_overrides, and
enables GN checks on that directory.

Bug:  812058 
Change-Id: I23fc0f45887a2c9ab50c6d217c9fb2c327ec4e7e
Reviewed-on: https://chromium-review.googlesource.com/974755
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Commit-Queue: Wez <wez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#545101}
[modify] https://crrev.com/51afb793d95a621b85aa812cb286e2385fc6de8f/.gn
[modify] https://crrev.com/51afb793d95a621b85aa812cb286e2385fc6de8f/third_party/webrtc_overrides/BUILD.gn

Project Member

Comment 7 by bugdroid1@chromium.org, Mar 22 2018

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

commit ff2a63a769f318927e1bd33b9a73b65262a8c05f
Author: Wez <wez@chromium.org>
Date: Thu Mar 22 19:25:28 2018

Add bug reference on //third_party/webrtc/* entry in .gn.

TBR: jochen
Bug:  812058 
Change-Id: I36f9060adf3c8255cee85d688aab0d26c75819c2
Reviewed-on: https://chromium-review.googlesource.com/976046
Reviewed-by: Wez <wez@chromium.org>
Commit-Queue: Wez <wez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#545194}
[modify] https://crrev.com/ff2a63a769f318927e1bd33b9a73b65262a8c05f/.gn

Project Member

Comment 8 by bugdroid1@chromium.org, Mar 23 2018

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

commit a6ca5b9b7a337cdeb8a601932f0300a207c3fe93
Author: Wez <wez@chromium.org>
Date: Fri Mar 23 19:03:07 2018

Add a |dcheck_is_configurable| build argument, independent of SyzyASAN.

Previously we gated run-time-configurable DCHECKs on the |is_syzyasan|
argument, to have them built-in to Chrome Canary SyzyASAN binaries.

We implement |dcheck_is_configurable| as a macro define rather than via
a buildflag, because Chromium currently has too many components with
undeclared dependencies on //base for introducing a new dependency of
//base/logging.h on a generated buildflags_header() to be safe.

Bug:  812058 
Change-Id: Iac48c88e0c5964cc8bceac6c444e44f84133120e
Reviewed-on: https://chromium-review.googlesource.com/974904
Reviewed-by: Nick Carter <nick@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Wez <wez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#545542}
[modify] https://crrev.com/a6ca5b9b7a337cdeb8a601932f0300a207c3fe93/base/feature_list.cc
[modify] https://crrev.com/a6ca5b9b7a337cdeb8a601932f0300a207c3fe93/base/feature_list.h
[modify] https://crrev.com/a6ca5b9b7a337cdeb8a601932f0300a207c3fe93/base/logging.cc
[modify] https://crrev.com/a6ca5b9b7a337cdeb8a601932f0300a207c3fe93/base/logging.h
[modify] https://crrev.com/a6ca5b9b7a337cdeb8a601932f0300a207c3fe93/base/logging_unittest.cc
[modify] https://crrev.com/a6ca5b9b7a337cdeb8a601932f0300a207c3fe93/build/config/BUILD.gn
[modify] https://crrev.com/a6ca5b9b7a337cdeb8a601932f0300a207c3fe93/build/config/dcheck_always_on.gni
[modify] https://crrev.com/a6ca5b9b7a337cdeb8a601932f0300a207c3fe93/chrome/browser/about_flags.cc
[modify] https://crrev.com/a6ca5b9b7a337cdeb8a601932f0300a207c3fe93/chrome/browser/flag_descriptions.cc
[modify] https://crrev.com/a6ca5b9b7a337cdeb8a601932f0300a207c3fe93/chrome/browser/flag_descriptions.h
[modify] https://crrev.com/a6ca5b9b7a337cdeb8a601932f0300a207c3fe93/content/renderer/render_process_impl.cc

Comment 9 by siggi@chromium.org, Mar 26 2018

I'm cleaning out the Syzygy/SyzyASAN configuration in this CL: https://chromium-review.googlesource.com/c/chromium/src/+/980912, I've tripped over some Albatross-relevant detail. Notably that you probably want to add something to the channel info for Albatross builds (https://cs.chromium.org/chromium/src/chrome/common/channel_info_win.cc?q=channel_info_win.cc&dr&l=17) to make it easy to see in chrome://settings/help whether slowness is due to DCHECKs.
There's also a debug URL for DCHECK that should stay under DCHECK_IS_ON()...

Comment 10 by w...@chromium.org, Mar 26 2018

Re #9: Thanks siggi@.  Would you like to switch (rather than remove) the relevant bits in your SyzyASAN-removal CL, or shall I do a run through and construct a separate CL that does that?

Comment 11 by w...@chromium.org, Apr 10 2018

Brought back up 'win-asan' builder with |dcheck_is_configurable=true|, will verify that that produces builds as expected, and re-instate the push to 5% of the Canary population.

Note to self: Rename the builder to something more appropriate.
Project Member

Comment 12 by bugdroid1@chromium.org, Apr 11 2018

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

commit ed05d62660b3fd83ed7724fbe5609b529f9e31d1
Author: Wez <wez@chromium.org>
Date: Wed Apr 11 20:06:25 2018

Fix string literal in GetChannelName() for official builds.

Bug:  812058 ,  821764 ,  831697 
Change-Id: I86759e3ed14cdda2e397f67a23481dd1cf5c7844
Reviewed-on: https://chromium-review.googlesource.com/1007882
Commit-Queue: Wez <wez@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Reviewed-by: Sigurður Ásgeirsson <siggi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#549946}
[modify] https://crrev.com/ed05d62660b3fd83ed7724fbe5609b529f9e31d1/chrome/common/channel_info_win.cc

Project Member

Comment 13 by bugdroid1@chromium.org, Apr 12 2018

Labels: merge-merged-3395
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/330c3615157cd58b987a27440fd2abaa1573d3a2

commit 330c3615157cd58b987a27440fd2abaa1573d3a2
Author: Wez <wez@chromium.org>
Date: Thu Apr 12 04:58:36 2018

Fix string literal in GetChannelName() for official builds.

Bug:  812058 ,  821764 ,  831697 
Change-Id: I86759e3ed14cdda2e397f67a23481dd1cf5c7844
Reviewed-on: https://chromium-review.googlesource.com/1007882
Commit-Queue: Wez <wez@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Reviewed-by: Sigurður Ásgeirsson <siggi@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#549946}(cherry picked from commit ed05d62660b3fd83ed7724fbe5609b529f9e31d1)
Reviewed-on: https://chromium-review.googlesource.com/1009126
Reviewed-by: Wez <wez@chromium.org>
Cr-Commit-Position: refs/branch-heads/3395@{#2}
Cr-Branched-From: 19ac07e50344cdf4089e5d1b696958867736fc03-refs/heads/master@{#549859}
[modify] https://crrev.com/330c3615157cd58b987a27440fd2abaa1573d3a2/chrome/common/channel_info_win.cc

Comment 14 by w...@chromium.org, Apr 12 2018

Blocking: -821764

Comment 15 by w...@chromium.org, Apr 12 2018

Blockedon: 832156

Comment 16 by w...@chromium.org, Apr 16 2018

Blockedon: -832156
Status: Fixed (was: Started)
Will track remaining bot work on issue 832156, and cleanup test failures in  issue 832728 .
Project Member

Comment 17 by bugdroid1@chromium.org, Apr 17 2018

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

commit 54dc71b1e039f594b89646a6942f8c9f5446c8ce
Author: Wez <wez@chromium.org>
Date: Tue Apr 17 00:42:34 2018

Move kDcheckIsFatal* flag definitions to be provided cross-platform.

Although these will only be used in official builds on Windows,
providing the flag definitions cross-platform allows developers to
build for other platforms with |dcheck_is_configurable=true| if they
want.

Bug:  812058 
Change-Id: I9a53681487e432129001620548e0a62052abab50
Reviewed-on: https://chromium-review.googlesource.com/1012267
Reviewed-by: Sigurður Ásgeirsson <siggi@chromium.org>
Commit-Queue: Wez <wez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551191}
[modify] https://crrev.com/54dc71b1e039f594b89646a6942f8c9f5446c8ce/chrome/browser/flag_descriptions.cc
[modify] https://crrev.com/54dc71b1e039f594b89646a6942f8c9f5446c8ce/chrome/browser/flag_descriptions.h

Sign in to add a comment