Add dvlog_always_on to enable DVLOG without DEBUG and DCHECK option

Add dvlog_always_on to enable DVLOG without DEBUG and DCHECK option

dvlog_always_on make DLOG() and DVLOG() enable without DEBUG and DCHECK option.
This is useful because dvlog_always_on does not enable DCHECK(),
and therefore, the program is not failed due to DCHECK().

BUG=None
Test=Seeing DVLOGs and DLOGs are shown.

[hiroh, 2017-06-26 05:23:39]

hiroh@chromium.org changed reviewers:
+ brettw@chromium.org

[hiroh, 2017-06-26 05:23:39]

This patch have to be merged after a patch for WebKit (crrev.com/2954873002) is
merged.

[hiroh, 2017-06-26 06:22:54]

Please first approve the idea of dvlog_always_on . 
That is the first step to add dvlog_always_on in Chromium (See the workflow:
crrev.com/2954873002/#msg9)

[brettw, 2017-06-26 18:21:20]

I can understand that you and one or two other people might use this right now,
but in 2 years, who will use this?

DCHECK_ALWAYS_ON was designed to solve a specific problem where we wanted to run
with DCHECKs on the bots, but debug mode is too slow. I don't see a similar use
case here.

Please feel free file P1 bugs on whoever wrote DCHECKs firing in normal usage,
and to write patches that disable the check if the bugs are ignored.

[Pawel Osciak, 2017-06-27 01:07:10]

On 2017/06/26 18:21:20, brettw wrote:
> I can understand that you and one or two other people might use this right
now,
> but in 2 years, who will use this?
> 
> DCHECK_ALWAYS_ON was designed to solve a specific problem where we wanted to
run
> with DCHECKs on the bots, but debug mode is too slow. I don't see a similar
use
> case here.
> 
> Please feel free file P1 bugs on whoever wrote DCHECKs firing in normal usage,
> and to write patches that disable the check if the bugs are ignored.

DVLOG_ALWAYS_ON would be used by us to enable debug logs for builds not only in
local testing by developers, but also for automated tests in labs, qualification
and various CQs, PFQs, etc.

Right now we are not able to use DCHECK_ALWAYS_ON because of the failing
DCHECKs.

I definitely agree that we would prefer to use DCHECK_ALWAYS_ON instead, every
time, and not have DVLOG_ALWAYS_ON at all. However, unfortunately, the reality
is that the breakages are relatively frequent and we had various levels of luck
with resolving these issues. In ideal world, this should always take priority,
but it's not only a time/priority issue, but also they usually happen on CrOS
devices, so it's difficult for other developers to obtain a specific CrOS device
to test/fix on. These bugs may also be pretty obscure and not straightforward to
fix and require time for that (one example could be crbug.com/454513).

As for just removing offending DCHECKs, personally I would prefer not doing this
and I would consider doing this only as a last resort. Moreover, given that this
is not tested in Chromium CQ, it's not realistic for us to contribute time to
identify and even only remove (not even fix) every failing DCHECK manually, each
time.

We would appreciate any ideas and suggestions what we could do here, and I do
agree we should try to improve the DCHECK situation and will continue to do
whatever we can from our side as well, however I feel we could still have some
benefit by being able to enable just DVLOGs, instead of having neither, which is
the current situation.

[brettw, 2017-06-27 01:48:03]

brettw@chromium.org changed reviewers:
+ dpranke@chromium.org

[brettw, 2017-06-27 01:48:04]

+Dirk who was asking me about this. Can you be more specific about where you
would compile with this on?

[Pawel Osciak, 2017-06-27 07:47:27]

On 2017/06/27 01:48:04, brettw wrote:
> +Dirk who was asking me about this. Can you be more specific about where you
> would compile with this on?

Right now this is used in Chromium CQ only (to my knowledge); my hope was that
we could perhaps somehow explore the possibility of having builds with DCHECKs
enabled for PFQ/automated testing on CrOS devices, if at all possible. This is
just an idea only though right now, no particular specifics how/where this could
be done.

[danakj, 2017-06-27 16:17:36]

On Mon, Jun 26, 2017 at 9:07 PM, <posciak@chromium.org> wrote:

> On 2017/06/26 18:21:20, brettw wrote:
> > I can understand that you and one or two other people might use this
> right
> now,
> > but in 2 years, who will use this?
> >
> > DCHECK_ALWAYS_ON was designed to solve a specific problem where we
> wanted to
> run
> > with DCHECKs on the bots, but debug mode is too slow. I don't see a
> similar
> use
> > case here.
> >
> > Please feel free file P1 bugs on whoever wrote DCHECKs firing in normal
> usage,
> > and to write patches that disable the check if the bugs are ignored.
>
> DVLOG_ALWAYS_ON would be used by us to enable debug logs for builds not
> only in
> local testing by developers, but also for automated tests in labs,
> qualification
> and various CQs, PFQs, etc.
>
> Right now we are not able to use DCHECK_ALWAYS_ON because of the failing
> DCHECKs.
>
> I definitely agree that we would prefer to use DCHECK_ALWAYS_ON instead,
> every
> time, and not have DVLOG_ALWAYS_ON at all. However, unfortunately, the
> reality
> is that the breakages are relatively frequent and we had various levels of
> luck
> with resolving these issues. In ideal world, this should always take
> priority,
> but it's not only a time/priority issue, but also they usually happen on
> CrOS
> devices, so it's difficult for other developers to obtain a specific CrOS
> device
> to test/fix on. These bugs may also be pretty obscure and not
> straightforward to
> fix and require time for that (one example could be crbug.com/454513).
>
> As for just removing offending DCHECKs, personally I would prefer not
> doing this
> and I would consider doing this only as a last resort.
>

It seems this argument is that disabling all DCHECKs is better than
disabling a DCHECK that is firing but not being fixed, and I don't agree. I
keep seeing investments in ways to not see DCHECKs and it's not encouraging
at all.

Moreover, given that this
> is not tested in Chromium CQ,
>

The CQ builds release + DCHECK_ALWAYS_ON, it sounds like you're claiming
the opposite?

An example build
<https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.chromium.linux...>
shows
"dcheck_always_on = true".


> it's not realistic for us to contribute time to
> identify and even only remove (not even fix) every failing DCHECK
> manually, each
> time.
>

> We would appreciate any ideas and suggestions what we could do here, and I
> do
> agree we should try to improve the DCHECK situation and will continue to do
> whatever we can from our side as well, however I feel we could still have
> some
> benefit by being able to enable just DVLOGs, instead of having neither,
> which is
> the current situation.
>
> https://codereview.chromium.org/2954883002/
>

-- 
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.

[Dirk Pranke, 2017-06-30 00:37:22]

I have no desire to run builds in the Chromium CQ that have DVLOG enabled but
not DCHECKs. I agree with what the others have said, that if we're seeing
DCHECKs firing too often but they're not being fixed, that's a problem that we
also need to fix, and I'm not very interested in working around it.

I can understand that you might want to run with DCHECK/DVLOG in the PFQ, on
real devices, etc., and that it is currently hard for us to address any DCHECKs
that are showing up only in that situation. However, I'd also like us to be
running more tests on real devices in the Chromium CQ, and so I'd like to
address that set of problems that way.

If there's stuff that you really want to be logged but you don't want to see
dchecks in the same configuration, maybe they should be VLOGged instead?

[Pawel Osciak, 2017-06-30 04:12:33]

On 2017/06/27 16:17:36, danakj wrote:
> On Mon, Jun 26, 2017 at 9:07 PM, <mailto:posciak@chromium.org> wrote:
> 
> > On 2017/06/26 18:21:20, brettw wrote:
> > > I can understand that you and one or two other people might use this
> > right
> > now,
> > > but in 2 years, who will use this?
> > >
> > > DCHECK_ALWAYS_ON was designed to solve a specific problem where we
> > wanted to
> > run
> > > with DCHECKs on the bots, but debug mode is too slow. I don't see a
> > similar
> > use
> > > case here.
> > >
> > > Please feel free file P1 bugs on whoever wrote DCHECKs firing in normal
> > usage,
> > > and to write patches that disable the check if the bugs are ignored.
> >
> > DVLOG_ALWAYS_ON would be used by us to enable debug logs for builds not
> > only in
> > local testing by developers, but also for automated tests in labs,
> > qualification
> > and various CQs, PFQs, etc.
> >
> > Right now we are not able to use DCHECK_ALWAYS_ON because of the failing
> > DCHECKs.
> >
> > I definitely agree that we would prefer to use DCHECK_ALWAYS_ON instead,
> > every
> > time, and not have DVLOG_ALWAYS_ON at all. However, unfortunately, the
> > reality
> > is that the breakages are relatively frequent and we had various levels of
> > luck
> > with resolving these issues. In ideal world, this should always take
> > priority,
> > but it's not only a time/priority issue, but also they usually happen on
> > CrOS
> > devices, so it's difficult for other developers to obtain a specific CrOS
> > device
> > to test/fix on. These bugs may also be pretty obscure and not
> > straightforward to
> > fix and require time for that (one example could be crbug.com/454513).
> >
> > As for just removing offending DCHECKs, personally I would prefer not
> > doing this
> > and I would consider doing this only as a last resort.
> >
> 
> It seems this argument is that disabling all DCHECKs is better than
> disabling a DCHECK that is firing but not being fixed, and I don't agree. I
> keep seeing investments in ways to not see DCHECKs and it's not encouraging
> at all.

I definitely never intended to propose disabling all DCHECKs vs disabling one,
apologies if this was caused by any confusing wording on my side. What I meant
was that preferably not even a single (correct) DCHECK should ever be removed,
but the underlying issue should be fixed instead.

Perhaps this is due to a misunderstanding that this CL is proposing using
DVLOG_ALWAYS_ON instead of DCHECK_ALWAYS_ON, however it's actually not the case.
For CrOS testing (not in Chromium CQ, please see below) there is no
DCHECK_ALWAYS_ON enabled. So we are actually trying to add some partial
usefulness of DCHECK_ALWAYS_ON (even if only DVLOGs), because we can't easily
enable DCHECKs for the reasons I mentioned previously.

> 
> Moreover, given that this
> > is not tested in Chromium CQ,
> >
> 
> The CQ builds release + DCHECK_ALWAYS_ON, it sounds like you're claiming
> the opposite?
> 
> An example build
>
<https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.chromium.linux...>
> shows
> "dcheck_always_on = true".
> 

Sorry for the confusing wording. When I said "this is not tested", I referred to
testing on Chrome OS/Chrome OS devices only, which are not included in Chromium
CQ and have to be tested in CrOS lab/PFQ. So any DCHECKs triggered on CrOS only
would not be hit in Chromium CQ.

[Pawel Osciak, 2017-06-30 04:35:14]

On 2017/06/30 00:37:22, Dirk Pranke wrote:
> I have no desire to run builds in the Chromium CQ that have DVLOG enabled but
> not DCHECKs. I agree with what the others have said, that if we're seeing
> DCHECKs firing too often but they're not being fixed, that's a problem that we
> also need to fix, and I'm not very interested in working around it.
> 
> I can understand that you might want to run with DCHECK/DVLOG in the PFQ, on
> real devices, etc., and that it is currently hard for us to address any
DCHECKs
> that are showing up only in that situation. However, I'd also like us to be
> running more tests on real devices in the Chromium CQ, and so I'd like to
> address that set of problems that way.
> 

We are definitely not proposing disabling DCHECKs in Chromium CQ. We are only
proposing adding a feature that would allow us to have at least DVLOGs in PFQ,
where we do not/cannot easily at this moment have DCHECK_ALWAYS_ON.

Of course having real Chrome OS devices in Chromium CQ instead would be the
ideal and preferred solution, however whether and to what extent that would be
possible is perhaps a separate topic.

> If there's stuff that you really want to be logged but you don't want to see
> dchecks in the same configuration, maybe they should be VLOGged instead?

No, not really, ideally we'd always want to have DCHECKs.
The whole issue is not for DCHECKs in our code, this is for DCHECKs in code
added by other developers that fail only on CrOS and not in Chromium CQ.

[Pawel Osciak, 2017-06-30 04:36:01]

Description was changed from

==========
Add dvlog_always_on to enable DVLOG without DEBUG and DCHECK option

dvlog_always_on make DLOG() and DVLOG() enable without DEBUG and DCHECK option.
This is useful because dvlog_always_on does not enable DCHECK(),
and therefore, the program is not failed due to DCHECK().

BUG=None
Test=Seeing DVLOGs and DLOGs are shown.
==========

to

==========
Add dvlog_always_on to enable DVLOG without DEBUG and DCHECK option

dvlog_always_on make DLOG() and DVLOG() enable without DEBUG and DCHECK option.
This is useful because dvlog_always_on does not enable DCHECK(),
and therefore, the program is not failed due to DCHECK().

BUG=None
Test=Seeing DVLOGs and DLOGs are shown.
==========

[danakj, 2017-07-04 16:30:32]

On Fri, Jun 30, 2017 at 12:35 AM, <posciak@chromium.org> wrote:

> On 2017/06/30 00:37:22, Dirk Pranke wrote:
> > I have no desire to run builds in the Chromium CQ that have DVLOG
> enabled but
> > not DCHECKs. I agree with what the others have said, that if we're seeing
> > DCHECKs firing too often but they're not being fixed, that's a problem
> that we
> > also need to fix, and I'm not very interested in working around it.
> >
> > I can understand that you might want to run with DCHECK/DVLOG in the
> PFQ, on
> > real devices, etc., and that it is currently hard for us to address any
> DCHECKs
> > that are showing up only in that situation. However, I'd also like us to
> be
> > running more tests on real devices in the Chromium CQ, and so I'd like to
> > address that set of problems that way.
> >
>
> We are definitely not proposing disabling DCHECKs in Chromium CQ. We are
> only
> proposing adding a feature that would allow us to have at least DVLOGs in
> PFQ,
> where we do not/cannot easily at this moment have DCHECK_ALWAYS_ON.
>
> Of course having real Chrome OS devices in Chromium CQ instead would be the
> ideal and preferred solution, however whether and to what extent that
> would be
> possible is perhaps a separate topic.
>
> > If there's stuff that you really want to be logged but you don't want to
> see
> > dchecks in the same configuration, maybe they should be VLOGged instead?
>
> No, not really, ideally we'd always want to have DCHECKs.
> The whole issue is not for DCHECKs in our code, this is for DCHECKs in code
> added by other developers that fail only on CrOS and not in Chromium CQ.
>

This reminds me of what we do for NOTREACHED() today:

#if !DCHECK_IS_ON
<https://cs.chromium.org/chromium/src/base/logging.h?l=769&ct=xref_jump_to_def...
&& defined(OS_CHROMEOS)// Implement logging of NOTREACHED() as a
dedicated function to get function// call overhead down to a
minimum.void LogErrorNotReached(const char* file, int line);#define
NOTREACHED()                                       \  true ?
::logging::LogErrorNotReached(__FILE__, __LINE__) \       :
EAT_STREAM_PARAMETERS#else#define NOTREACHED
<https://cs.chromium.org/chromium/src/base/logging.h?l=942&gs=cpp%253Amacro-NO...
DCHECK(false)#endif
As ChromeOS builds with DCHECKs off for the problems you describe
above I guess. So if I understand you're proposing to take chromeOS
builds from a dcheck_always_on=false to instead {
dcheck_always_on=false + dlog_always_on=true }. But this would only
log DLOGs and DVLOGs. Why do you exclude logging DCHECKs as well?
Maybe what you're looking for is to extend this NOTREACHED() behaviour
to DCHECKs and D{V}LOGs, something like {dcheck_always_on=true +
dcheck_as_logs=true}. Then you'd get DLOG for free, but need to make
DCHECK not crash.
wez has been working on a Dump-on-DCHECK
<https://codereview.chromium.org/1884023002/> behaviour to ship to
canary, such that we'd enable DCHECKs there but not crash users, just
send crash reports. This seems to have a lot of similarity to me,
except it's Log-on-DCHECK instead.
+wez do you see similarities here, what do you think?
- Dana

-- 
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.

[Wez, 2017-07-05 18:36:42]

We decided against the Dump-on-DCHECK approach, but we're instead working
on a DCHECKs-in-ASAN alternative that has the LOG_DCHECK severity
dynamically switchable between FATAL and INFO (and, technically, anything
else...) at run-time.  The changes to base/logging are fairly small, and we
could generalize them to allow a separate DCHECK "mode" to be specified,
defaulting to FATAL, e.g:

dcheck_always_on = true
dcheck_mode = "info"

to enable DCHECKs but only have them generate INFO log output

dcheck_always_on = true
dcheck_mode = dynamic

to enable DCHECKs and get the run-time switchability we need for the ASAN
build, etc.

If that sounds useful then I can break out a separate CL for dcheck_mode to
allow build-time selection of FATAL vs INFO severity level, this week.

WDYT?

On 4 July 2017 at 09:24, <danakj@chromium.org> wrote:

> On Fri, Jun 30, 2017 at 12:35 AM, <posciak@chromium.org> wrote:
>
>> On 2017/06/30 00:37:22, Dirk Pranke wrote:
>> > I have no desire to run builds in the Chromium CQ that have DVLOG
>> enabled but
>> > not DCHECKs. I agree with what the others have said, that if we're
>> seeing
>> > DCHECKs firing too often but they're not being fixed, that's a problem
>> that we
>> > also need to fix, and I'm not very interested in working around it.
>> >
>> > I can understand that you might want to run with DCHECK/DVLOG in the
>> PFQ, on
>> > real devices, etc., and that it is currently hard for us to address any
>> DCHECKs
>> > that are showing up only in that situation. However, I'd also like us
>> to be
>> > running more tests on real devices in the Chromium CQ, and so I'd like
>> to
>> > address that set of problems that way.
>> >
>>
>> We are definitely not proposing disabling DCHECKs in Chromium CQ. We are
>> only
>> proposing adding a feature that would allow us to have at least DVLOGs in
>> PFQ,
>> where we do not/cannot easily at this moment have DCHECK_ALWAYS_ON.
>>
>> Of course having real Chrome OS devices in Chromium CQ instead would be
>> the
>> ideal and preferred solution, however whether and to what extent that
>> would be
>> possible is perhaps a separate topic.
>>
>> > If there's stuff that you really want to be logged but you don't want
>> to see
>> > dchecks in the same configuration, maybe they should be VLOGged instead?
>>
>> No, not really, ideally we'd always want to have DCHECKs.
>> The whole issue is not for DCHECKs in our code, this is for DCHECKs in
>> code
>> added by other developers that fail only on CrOS and not in Chromium CQ.
>>
>
> This reminds me of what we do for NOTREACHED() today:
>
> #if !DCHECK_IS_ON
<https://cs.chromium.org/chromium/src/base/logging.h?l=769&ct=xref_jump_to_def...
&& defined(OS_CHROMEOS)// Implement logging of NOTREACHED() as a dedicated
function to get function// call overhead down to a minimum.void
LogErrorNotReached(const char* file, int line);#define NOTREACHED()             
                         \  true ? ::logging::LogErrorNotReached(__FILE__,
__LINE__) \       : EAT_STREAM_PARAMETERS#else#define NOTREACHED
<https://cs.chromium.org/chromium/src/base/logging.h?l=942&gs=cpp%253Amacro-NO...
DCHECK(false)#endif
> As ChromeOS builds with DCHECKs off for the problems you describe above I
guess. So if I understand you're proposing to take chromeOS builds from a
dcheck_always_on=false to instead { dcheck_always_on=false + dlog_always_on=true
}. But this would only log DLOGs and DVLOGs. Why do you exclude logging DCHECKs
as well? Maybe what you're looking for is to extend this NOTREACHED() behaviour
to DCHECKs and D{V}LOGs, something like {dcheck_always_on=true +
dcheck_as_logs=true}. Then you'd get DLOG for free, but need to make DCHECK not
crash.
> wez has been working on a Dump-on-DCHECK
<https://codereview.chromium.org/1884023002/> behaviour to ship to canary, such
that we'd enable DCHECKs there but not crash users, just send crash reports.
This seems to have a lot of similarity to me, except it's Log-on-DCHECK
instead.
> +wez do you see similarities here, what do you think?
> - Dana
>
>

-- 
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.

[danakj, 2017-07-05 18:50:10]

On Wed, Jul 5, 2017 at 2:36 PM, Wez <wez@chromium.org> wrote:

> We decided against the Dump-on-DCHECK approach, but we're instead working
> on a DCHECKs-in-ASAN alternative that has the LOG_DCHECK severity
> dynamically switchable between FATAL and INFO (and, technically, anything
> else...) at run-time.  The changes to base/logging are fairly small, and we
> could generalize them to allow a separate DCHECK "mode" to be specified,
> defaulting to FATAL, e.g:
>
> dcheck_always_on = true
> dcheck_mode = "info"
>
> to enable DCHECKs but only have them generate INFO log output
>
> dcheck_always_on = true
> dcheck_mode = dynamic
>
> to enable DCHECKs and get the run-time switchability we need for the ASAN
> build, etc.
>
> If that sounds useful then I can break out a separate CL for dcheck_mode
> to allow build-time selection of FATAL vs INFO severity level, this week.
>
> WDYT?
>

That seems pretty useful to me! I'd like if we made dcheck_mode only work
when building for chromeos or asan though, to keep on gatekeeping disabling
dchecks.

What do others here think?


>
> On 4 July 2017 at 09:24, <danakj@chromium.org> wrote:
>
>> On Fri, Jun 30, 2017 at 12:35 AM, <posciak@chromium.org> wrote:
>>
>>> On 2017/06/30 00:37:22, Dirk Pranke wrote:
>>> > I have no desire to run builds in the Chromium CQ that have DVLOG
>>> enabled but
>>> > not DCHECKs. I agree with what the others have said, that if we're
>>> seeing
>>> > DCHECKs firing too often but they're not being fixed, that's a problem
>>> that we
>>> > also need to fix, and I'm not very interested in working around it.
>>> >
>>> > I can understand that you might want to run with DCHECK/DVLOG in the
>>> PFQ, on
>>> > real devices, etc., and that it is currently hard for us to address any
>>> DCHECKs
>>> > that are showing up only in that situation. However, I'd also like us
>>> to be
>>> > running more tests on real devices in the Chromium CQ, and so I'd like
>>> to
>>> > address that set of problems that way.
>>> >
>>>
>>> We are definitely not proposing disabling DCHECKs in Chromium CQ. We are
>>> only
>>> proposing adding a feature that would allow us to have at least DVLOGs
>>> in PFQ,
>>> where we do not/cannot easily at this moment have DCHECK_ALWAYS_ON.
>>>
>>> Of course having real Chrome OS devices in Chromium CQ instead would be
>>> the
>>> ideal and preferred solution, however whether and to what extent that
>>> would be
>>> possible is perhaps a separate topic.
>>>
>>> > If there's stuff that you really want to be logged but you don't want
>>> to see
>>> > dchecks in the same configuration, maybe they should be VLOGged
>>> instead?
>>>
>>> No, not really, ideally we'd always want to have DCHECKs.
>>> The whole issue is not for DCHECKs in our code, this is for DCHECKs in
>>> code
>>> added by other developers that fail only on CrOS and not in Chromium CQ.
>>>
>>
>> This reminds me of what we do for NOTREACHED() today:
>>
>> #if !DCHECK_IS_ON
<https://cs.chromium.org/chromium/src/base/logging.h?l=769&ct=xref_jump_to_def...
&& defined(OS_CHROMEOS)// Implement logging of NOTREACHED() as a dedicated
function to get function// call overhead down to a minimum.void
LogErrorNotReached(const char* file, int line);#define NOTREACHED()             
                         \  true ? ::logging::LogErrorNotReached(__FILE__,
__LINE__) \       : EAT_STREAM_PARAMETERS#else#define NOTREACHED
<https://cs.chromium.org/chromium/src/base/logging.h?l=942&gs=cpp%253Amacro-NO...
DCHECK(false)#endif
>> As ChromeOS builds with DCHECKs off for the problems you describe above I
guess. So if I understand you're proposing to take chromeOS builds from a
dcheck_always_on=false to instead { dcheck_always_on=false + dlog_always_on=true
}. But this would only log DLOGs and DVLOGs. Why do you exclude logging DCHECKs
as well? Maybe what you're looking for is to extend this NOTREACHED() behaviour
to DCHECKs and D{V}LOGs, something like {dcheck_always_on=true +
dcheck_as_logs=true}. Then you'd get DLOG for free, but need to make DCHECK not
crash.
>> wez has been working on a Dump-on-DCHECK
<https://codereview.chromium.org/1884023002/> behaviour to ship to canary, such
that we'd enable DCHECKs there but not crash users, just send crash reports.
This seems to have a lot of similarity to me, except it's Log-on-DCHECK
instead.
>> +wez do you see similarities here, what do you think?
>> - Dana
>>
>>
>

-- 
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.

[Wez, 2017-07-05 18:53:53]

OK, I'll break out a fresh CL with dcheck_mode; default will be
LOG_DCHECK=LOG_FATAL, so we can gate existence of dcheck_mode on whatever
we want, pretty easily.

On 5 July 2017 at 11:42, <danakj@chromium.org> wrote:

> On Wed, Jul 5, 2017 at 2:36 PM, Wez <wez@chromium.org> wrote:
>
>> We decided against the Dump-on-DCHECK approach, but we're instead working
>> on a DCHECKs-in-ASAN alternative that has the LOG_DCHECK severity
>> dynamically switchable between FATAL and INFO (and, technically, anything
>> else...) at run-time.  The changes to base/logging are fairly small, and we
>> could generalize them to allow a separate DCHECK "mode" to be specified,
>> defaulting to FATAL, e.g:
>>
>> dcheck_always_on = true
>> dcheck_mode = "info"
>>
>> to enable DCHECKs but only have them generate INFO log output
>>
>> dcheck_always_on = true
>> dcheck_mode = dynamic
>>
>> to enable DCHECKs and get the run-time switchability we need for the ASAN
>> build, etc.
>>
>> If that sounds useful then I can break out a separate CL for dcheck_mode
>> to allow build-time selection of FATAL vs INFO severity level, this week.
>>
>> WDYT?
>>
>
> That seems pretty useful to me! I'd like if we made dcheck_mode only work
> when building for chromeos or asan though, to keep on gatekeeping disabling
> dchecks.
>
> What do others here think?
>
>
>>
>> On 4 July 2017 at 09:24, <danakj@chromium.org> wrote:
>>
>>> On Fri, Jun 30, 2017 at 12:35 AM, <posciak@chromium.org> wrote:
>>>
>>>> On 2017/06/30 00:37:22, Dirk Pranke wrote:
>>>> > I have no desire to run builds in the Chromium CQ that have DVLOG
>>>> enabled but
>>>> > not DCHECKs. I agree with what the others have said, that if we're
>>>> seeing
>>>> > DCHECKs firing too often but they're not being fixed, that's a
>>>> problem that we
>>>> > also need to fix, and I'm not very interested in working around it.
>>>> >
>>>> > I can understand that you might want to run with DCHECK/DVLOG in the
>>>> PFQ, on
>>>> > real devices, etc., and that it is currently hard for us to address
>>>> any
>>>> DCHECKs
>>>> > that are showing up only in that situation. However, I'd also like us
>>>> to be
>>>> > running more tests on real devices in the Chromium CQ, and so I'd
>>>> like to
>>>> > address that set of problems that way.
>>>> >
>>>>
>>>> We are definitely not proposing disabling DCHECKs in Chromium CQ. We
>>>> are only
>>>> proposing adding a feature that would allow us to have at least DVLOGs
>>>> in PFQ,
>>>> where we do not/cannot easily at this moment have DCHECK_ALWAYS_ON.
>>>>
>>>> Of course having real Chrome OS devices in Chromium CQ instead would be
>>>> the
>>>> ideal and preferred solution, however whether and to what extent that
>>>> would be
>>>> possible is perhaps a separate topic.
>>>>
>>>> > If there's stuff that you really want to be logged but you don't want
>>>> to see
>>>> > dchecks in the same configuration, maybe they should be VLOGged
>>>> instead?
>>>>
>>>> No, not really, ideally we'd always want to have DCHECKs.
>>>> The whole issue is not for DCHECKs in our code, this is for DCHECKs in
>>>> code
>>>> added by other developers that fail only on CrOS and not in Chromium CQ.
>>>>
>>>
>>> This reminds me of what we do for NOTREACHED() today:
>>>
>>> #if !DCHECK_IS_ON
<https://cs.chromium.org/chromium/src/base/logging.h?l=769&ct=xref_jump_to_def...
&& defined(OS_CHROMEOS)// Implement logging of NOTREACHED() as a dedicated
function to get function// call overhead down to a minimum.void
LogErrorNotReached(const char* file, int line);#define NOTREACHED()             
                         \  true ? ::logging::LogErrorNotReached(__FILE__,
__LINE__) \       : EAT_STREAM_PARAMETERS#else#define NOTREACHED
<https://cs.chromium.org/chromium/src/base/logging.h?l=942&gs=cpp%253Amacro-NO...
DCHECK(false)#endif
>>> As ChromeOS builds with DCHECKs off for the problems you describe above I
guess. So if I understand you're proposing to take chromeOS builds from a
dcheck_always_on=false to instead { dcheck_always_on=false + dlog_always_on=true
}. But this would only log DLOGs and DVLOGs. Why do you exclude logging DCHECKs
as well? Maybe what you're looking for is to extend this NOTREACHED() behaviour
to DCHECKs and D{V}LOGs, something like {dcheck_always_on=true +
dcheck_as_logs=true}. Then you'd get DLOG for free, but need to make DCHECK not
crash.
>>> wez has been working on a Dump-on-DCHECK
<https://codereview.chromium.org/1884023002/> behaviour to ship to canary, such
that we'd enable DCHECKs there but not crash users, just send crash reports.
This seems to have a lot of similarity to me, except it's Log-on-DCHECK
instead.
>>> +wez do you see similarities here, what do you think?
>>> - Dana
>>>
>>>
>>
>

-- 
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.

[Wez, 2017-07-05 21:04:09]

Ah, one down-side to this: death-tests will tend to bit-rot if we don't
have a build somewhere on the CQ/waterfall that uses a non-FATAL DCHECK
mode.

On 5 July 2017 at 11:53, Wez <wez@chromium.org> wrote:

> OK, I'll break out a fresh CL with dcheck_mode; default will be
> LOG_DCHECK=LOG_FATAL, so we can gate existence of dcheck_mode on whatever
> we want, pretty easily.
>
> On 5 July 2017 at 11:42, <danakj@chromium.org> wrote:
>
>> On Wed, Jul 5, 2017 at 2:36 PM, Wez <wez@chromium.org> wrote:
>>
>>> We decided against the Dump-on-DCHECK approach, but we're instead
>>> working on a DCHECKs-in-ASAN alternative that has the LOG_DCHECK severity
>>> dynamically switchable between FATAL and INFO (and, technically, anything
>>> else...) at run-time.  The changes to base/logging are fairly small, and we
>>> could generalize them to allow a separate DCHECK "mode" to be specified,
>>> defaulting to FATAL, e.g:
>>>
>>> dcheck_always_on = true
>>> dcheck_mode = "info"
>>>
>>> to enable DCHECKs but only have them generate INFO log output
>>>
>>> dcheck_always_on = true
>>> dcheck_mode = dynamic
>>>
>>> to enable DCHECKs and get the run-time switchability we need for the
>>> ASAN build, etc.
>>>
>>> If that sounds useful then I can break out a separate CL for dcheck_mode
>>> to allow build-time selection of FATAL vs INFO severity level, this week.
>>>
>>> WDYT?
>>>
>>
>> That seems pretty useful to me! I'd like if we made dcheck_mode only work
>> when building for chromeos or asan though, to keep on gatekeeping disabling
>> dchecks.
>>
>> What do others here think?
>>
>>
>>>
>>> On 4 July 2017 at 09:24, <danakj@chromium.org> wrote:
>>>
>>>> On Fri, Jun 30, 2017 at 12:35 AM, <posciak@chromium.org> wrote:
>>>>
>>>>> On 2017/06/30 00:37:22, Dirk Pranke wrote:
>>>>> > I have no desire to run builds in the Chromium CQ that have DVLOG
>>>>> enabled but
>>>>> > not DCHECKs. I agree with what the others have said, that if we're
>>>>> seeing
>>>>> > DCHECKs firing too often but they're not being fixed, that's a
>>>>> problem that we
>>>>> > also need to fix, and I'm not very interested in working around it.
>>>>> >
>>>>> > I can understand that you might want to run with DCHECK/DVLOG in the
>>>>> PFQ, on
>>>>> > real devices, etc., and that it is currently hard for us to address
>>>>> any
>>>>> DCHECKs
>>>>> > that are showing up only in that situation. However, I'd also like
>>>>> us to be
>>>>> > running more tests on real devices in the Chromium CQ, and so I'd
>>>>> like to
>>>>> > address that set of problems that way.
>>>>> >
>>>>>
>>>>> We are definitely not proposing disabling DCHECKs in Chromium CQ. We
>>>>> are only
>>>>> proposing adding a feature that would allow us to have at least DVLOGs
>>>>> in PFQ,
>>>>> where we do not/cannot easily at this moment have DCHECK_ALWAYS_ON.
>>>>>
>>>>> Of course having real Chrome OS devices in Chromium CQ instead would
>>>>> be the
>>>>> ideal and preferred solution, however whether and to what extent that
>>>>> would be
>>>>> possible is perhaps a separate topic.
>>>>>
>>>>> > If there's stuff that you really want to be logged but you don't
>>>>> want to see
>>>>> > dchecks in the same configuration, maybe they should be VLOGged
>>>>> instead?
>>>>>
>>>>> No, not really, ideally we'd always want to have DCHECKs.
>>>>> The whole issue is not for DCHECKs in our code, this is for DCHECKs in
>>>>> code
>>>>> added by other developers that fail only on CrOS and not in Chromium
>>>>> CQ.
>>>>>
>>>>
>>>> This reminds me of what we do for NOTREACHED() today:
>>>>
>>>> #if !DCHECK_IS_ON
<https://cs.chromium.org/chromium/src/base/logging.h?l=769&ct=xref_jump_to_def...
&& defined(OS_CHROMEOS)// Implement logging of NOTREACHED() as a dedicated
function to get function// call overhead down to a minimum.void
LogErrorNotReached(const char* file, int line);#define NOTREACHED()             
                         \  true ? ::logging::LogErrorNotReached(__FILE__,
__LINE__) \       : EAT_STREAM_PARAMETERS#else#define NOTREACHED
<https://cs.chromium.org/chromium/src/base/logging.h?l=942&gs=cpp%253Amacro-NO...
DCHECK(false)#endif
>>>> As ChromeOS builds with DCHECKs off for the problems you describe above I
guess. So if I understand you're proposing to take chromeOS builds from a
dcheck_always_on=false to instead { dcheck_always_on=false + dlog_always_on=true
}. But this would only log DLOGs and DVLOGs. Why do you exclude logging DCHECKs
as well? Maybe what you're looking for is to extend this NOTREACHED() behaviour
to DCHECKs and D{V}LOGs, something like {dcheck_always_on=true +
dcheck_as_logs=true}. Then you'd get DLOG for free, but need to make DCHECK not
crash.
>>>> wez has been working on a Dump-on-DCHECK
<https://codereview.chromium.org/1884023002/> behaviour to ship to canary, such
that we'd enable DCHECKs there but not crash users, just send crash reports.
This seems to have a lot of similarity to me, except it's Log-on-DCHECK
instead.
>>>> +wez do you see similarities here, what do you think?
>>>> - Dana
>>>>
>>>>
>>>
>>
>

-- 
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.

[danakj, 2017-07-05 21:07:18]

On Wed, Jul 5, 2017 at 5:04 PM, Wez <wez@chromium.org> wrote:

> Ah, one down-side to this: death-tests will tend to bit-rot if we don't
> have a build somewhere on the CQ/waterfall that uses a non-FATAL DCHECK
> mode.
>

Can you elaborate? Death tests should DCHECK crash in all configs except
asan/chromeos builds that turn off crashing (and I guess disable the tests
there). These tests must not be running right now in those configs either
since DCHECKs are just disabled?


>
> On 5 July 2017 at 11:53, Wez <wez@chromium.org> wrote:
>
>> OK, I'll break out a fresh CL with dcheck_mode; default will be
>> LOG_DCHECK=LOG_FATAL, so we can gate existence of dcheck_mode on whatever
>> we want, pretty easily.
>>
>> On 5 July 2017 at 11:42, <danakj@chromium.org> wrote:
>>
>>> On Wed, Jul 5, 2017 at 2:36 PM, Wez <wez@chromium.org> wrote:
>>>
>>>> We decided against the Dump-on-DCHECK approach, but we're instead
>>>> working on a DCHECKs-in-ASAN alternative that has the LOG_DCHECK severity
>>>> dynamically switchable between FATAL and INFO (and, technically, anything
>>>> else...) at run-time.  The changes to base/logging are fairly small, and
we
>>>> could generalize them to allow a separate DCHECK "mode" to be specified,
>>>> defaulting to FATAL, e.g:
>>>>
>>>> dcheck_always_on = true
>>>> dcheck_mode = "info"
>>>>
>>>> to enable DCHECKs but only have them generate INFO log output
>>>>
>>>> dcheck_always_on = true
>>>> dcheck_mode = dynamic
>>>>
>>>> to enable DCHECKs and get the run-time switchability we need for the
>>>> ASAN build, etc.
>>>>
>>>> If that sounds useful then I can break out a separate CL for
>>>> dcheck_mode to allow build-time selection of FATAL vs INFO severity level,
>>>> this week.
>>>>
>>>> WDYT?
>>>>
>>>
>>> That seems pretty useful to me! I'd like if we made dcheck_mode only
>>> work when building for chromeos or asan though, to keep on gatekeeping
>>> disabling dchecks.
>>>
>>> What do others here think?
>>>
>>>
>>>>
>>>> On 4 July 2017 at 09:24, <danakj@chromium.org> wrote:
>>>>
>>>>> On Fri, Jun 30, 2017 at 12:35 AM, <posciak@chromium.org> wrote:
>>>>>
>>>>>> On 2017/06/30 00:37:22, Dirk Pranke wrote:
>>>>>> > I have no desire to run builds in the Chromium CQ that have DVLOG
>>>>>> enabled but
>>>>>> > not DCHECKs. I agree with what the others have said, that if we're
>>>>>> seeing
>>>>>> > DCHECKs firing too often but they're not being fixed, that's a
>>>>>> problem that we
>>>>>> > also need to fix, and I'm not very interested in working around it.
>>>>>> >
>>>>>> > I can understand that you might want to run with DCHECK/DVLOG in
>>>>>> the PFQ, on
>>>>>> > real devices, etc., and that it is currently hard for us to address
>>>>>> any
>>>>>> DCHECKs
>>>>>> > that are showing up only in that situation. However, I'd also like
>>>>>> us to be
>>>>>> > running more tests on real devices in the Chromium CQ, and so I'd
>>>>>> like to
>>>>>> > address that set of problems that way.
>>>>>> >
>>>>>>
>>>>>> We are definitely not proposing disabling DCHECKs in Chromium CQ. We
>>>>>> are only
>>>>>> proposing adding a feature that would allow us to have at least
>>>>>> DVLOGs in PFQ,
>>>>>> where we do not/cannot easily at this moment have DCHECK_ALWAYS_ON.
>>>>>>
>>>>>> Of course having real Chrome OS devices in Chromium CQ instead would
>>>>>> be the
>>>>>> ideal and preferred solution, however whether and to what extent that
>>>>>> would be
>>>>>> possible is perhaps a separate topic.
>>>>>>
>>>>>> > If there's stuff that you really want to be logged but you don't
>>>>>> want to see
>>>>>> > dchecks in the same configuration, maybe they should be VLOGged
>>>>>> instead?
>>>>>>
>>>>>> No, not really, ideally we'd always want to have DCHECKs.
>>>>>> The whole issue is not for DCHECKs in our code, this is for DCHECKs
>>>>>> in code
>>>>>> added by other developers that fail only on CrOS and not in Chromium
>>>>>> CQ.
>>>>>>
>>>>>
>>>>> This reminds me of what we do for NOTREACHED() today:
>>>>>
>>>>> #if !DCHECK_IS_ON
<https://cs.chromium.org/chromium/src/base/logging.h?l=769&ct=xref_jump_to_def...
&& defined(OS_CHROMEOS)// Implement logging of NOTREACHED() as a dedicated
function to get function// call overhead down to a minimum.void
LogErrorNotReached(const char* file, int line);#define NOTREACHED()             
                         \  true ? ::logging::LogErrorNotReached(__FILE__,
__LINE__) \       : EAT_STREAM_PARAMETERS#else#define NOTREACHED
<https://cs.chromium.org/chromium/src/base/logging.h?l=942&gs=cpp%253Amacro-NO...
DCHECK(false)#endif
>>>>> As ChromeOS builds with DCHECKs off for the problems you describe above I
guess. So if I understand you're proposing to take chromeOS builds from a
dcheck_always_on=false to instead { dcheck_always_on=false + dlog_always_on=true
}. But this would only log DLOGs and DVLOGs. Why do you exclude logging DCHECKs
as well? Maybe what you're looking for is to extend this NOTREACHED() behaviour
to DCHECKs and D{V}LOGs, something like {dcheck_always_on=true +
dcheck_as_logs=true}. Then you'd get DLOG for free, but need to make DCHECK not
crash.
>>>>> wez has been working on a Dump-on-DCHECK
<https://codereview.chromium.org/1884023002/> behaviour to ship to canary, such
that we'd enable DCHECKs there but not crash users, just send crash reports.
This seems to have a lot of similarity to me, except it's Log-on-DCHECK
instead.
>>>>> +wez do you see similarities here, what do you think?
>>>>> - Dana
>>>>>
>>>>>
>>>>
>>>
>>
>

-- 
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.

[Wez, 2017-07-05 21:15:04]

Right; so the risk is that people add more death-tests, or alter existing
death-tests.

We have a handy EXPECT_DCHECK_DEATH macro that is essentially a no-op in
!DCHECK_IS_ON() builds, which a lot of tests use.  We can just tweak that
to know about the DCHECK-mode. However, some tests explicitly check
DCHECK_IS_ON() and then make the (reasonable) assumption that DCHECK ==
LOG(FATAL) if DCHECK_IS_ON() - those will need explicitly fixing.  Fixing
them isn't always as simple as replacing EXPECT_DEATH with
EXPECT_DCHECK_DEATH because some of the tests are verifying that DCHECKs
fire that e.g. protect against mis-use of STL - so the test may genuinely
crash if the DCHECK doesn't fire. :P

On 5 July 2017 at 14:06, <danakj@chromium.org> wrote:

> On Wed, Jul 5, 2017 at 5:04 PM, Wez <wez@chromium.org> wrote:
>
>> Ah, one down-side to this: death-tests will tend to bit-rot if we don't
>> have a build somewhere on the CQ/waterfall that uses a non-FATAL DCHECK
>> mode.
>>
>
> Can you elaborate? Death tests should DCHECK crash in all configs except
> asan/chromeos builds that turn off crashing (and I guess disable the tests
> there). These tests must not be running right now in those configs either
> since DCHECKs are just disabled?
>
>
>>
>> On 5 July 2017 at 11:53, Wez <wez@chromium.org> wrote:
>>
>>> OK, I'll break out a fresh CL with dcheck_mode; default will be
>>> LOG_DCHECK=LOG_FATAL, so we can gate existence of dcheck_mode on whatever
>>> we want, pretty easily.
>>>
>>> On 5 July 2017 at 11:42, <danakj@chromium.org> wrote:
>>>
>>>> On Wed, Jul 5, 2017 at 2:36 PM, Wez <wez@chromium.org> wrote:
>>>>
>>>>> We decided against the Dump-on-DCHECK approach, but we're instead
>>>>> working on a DCHECKs-in-ASAN alternative that has the LOG_DCHECK severity
>>>>> dynamically switchable between FATAL and INFO (and, technically, anything
>>>>> else...) at run-time.  The changes to base/logging are fairly small, and
we
>>>>> could generalize them to allow a separate DCHECK "mode" to be specified,
>>>>> defaulting to FATAL, e.g:
>>>>>
>>>>> dcheck_always_on = true
>>>>> dcheck_mode = "info"
>>>>>
>>>>> to enable DCHECKs but only have them generate INFO log output
>>>>>
>>>>> dcheck_always_on = true
>>>>> dcheck_mode = dynamic
>>>>>
>>>>> to enable DCHECKs and get the run-time switchability we need for the
>>>>> ASAN build, etc.
>>>>>
>>>>> If that sounds useful then I can break out a separate CL for
>>>>> dcheck_mode to allow build-time selection of FATAL vs INFO severity
level,
>>>>> this week.
>>>>>
>>>>> WDYT?
>>>>>
>>>>
>>>> That seems pretty useful to me! I'd like if we made dcheck_mode only
>>>> work when building for chromeos or asan though, to keep on gatekeeping
>>>> disabling dchecks.
>>>>
>>>> What do others here think?
>>>>
>>>>
>>>>>
>>>>> On 4 July 2017 at 09:24, <danakj@chromium.org> wrote:
>>>>>
>>>>>> On Fri, Jun 30, 2017 at 12:35 AM, <posciak@chromium.org> wrote:
>>>>>>
>>>>>>> On 2017/06/30 00:37:22, Dirk Pranke wrote:
>>>>>>> > I have no desire to run builds in the Chromium CQ that have DVLOG
>>>>>>> enabled but
>>>>>>> > not DCHECKs. I agree with what the others have said, that if we're
>>>>>>> seeing
>>>>>>> > DCHECKs firing too often but they're not being fixed, that's a
>>>>>>> problem that we
>>>>>>> > also need to fix, and I'm not very interested in working around it.
>>>>>>> >
>>>>>>> > I can understand that you might want to run with DCHECK/DVLOG in
>>>>>>> the PFQ, on
>>>>>>> > real devices, etc., and that it is currently hard for us to
>>>>>>> address any
>>>>>>> DCHECKs
>>>>>>> > that are showing up only in that situation. However, I'd also like
>>>>>>> us to be
>>>>>>> > running more tests on real devices in the Chromium CQ, and so I'd
>>>>>>> like to
>>>>>>> > address that set of problems that way.
>>>>>>> >
>>>>>>>
>>>>>>> We are definitely not proposing disabling DCHECKs in Chromium CQ. We
>>>>>>> are only
>>>>>>> proposing adding a feature that would allow us to have at least
>>>>>>> DVLOGs in PFQ,
>>>>>>> where we do not/cannot easily at this moment have DCHECK_ALWAYS_ON.
>>>>>>>
>>>>>>> Of course having real Chrome OS devices in Chromium CQ instead would
>>>>>>> be the
>>>>>>> ideal and preferred solution, however whether and to what extent
>>>>>>> that would be
>>>>>>> possible is perhaps a separate topic.
>>>>>>>
>>>>>>> > If there's stuff that you really want to be logged but you don't
>>>>>>> want to see
>>>>>>> > dchecks in the same configuration, maybe they should be VLOGged
>>>>>>> instead?
>>>>>>>
>>>>>>> No, not really, ideally we'd always want to have DCHECKs.
>>>>>>> The whole issue is not for DCHECKs in our code, this is for DCHECKs
>>>>>>> in code
>>>>>>> added by other developers that fail only on CrOS and not in Chromium
>>>>>>> CQ.
>>>>>>>
>>>>>>
>>>>>> This reminds me of what we do for NOTREACHED() today:
>>>>>>
>>>>>> #if !DCHECK_IS_ON
<https://cs.chromium.org/chromium/src/base/logging.h?l=769&ct=xref_jump_to_def...
&& defined(OS_CHROMEOS)// Implement logging of NOTREACHED() as a dedicated
function to get function// call overhead down to a minimum.void
LogErrorNotReached(const char* file, int line);#define NOTREACHED()             
                         \  true ? ::logging::LogErrorNotReached(__FILE__,
__LINE__) \       : EAT_STREAM_PARAMETERS#else#define NOTREACHED
<https://cs.chromium.org/chromium/src/base/logging.h?l=942&gs=cpp%253Amacro-NO...
DCHECK(false)#endif
>>>>>> As ChromeOS builds with DCHECKs off for the problems you describe above I
guess. So if I understand you're proposing to take chromeOS builds from a
dcheck_always_on=false to instead { dcheck_always_on=false + dlog_always_on=true
}. But this would only log DLOGs and DVLOGs. Why do you exclude logging DCHECKs
as well? Maybe what you're looking for is to extend this NOTREACHED() behaviour
to DCHECKs and D{V}LOGs, something like {dcheck_always_on=true +
dcheck_as_logs=true}. Then you'd get DLOG for free, but need to make DCHECK not
crash.
>>>>>> wez has been working on a Dump-on-DCHECK
<https://codereview.chromium.org/1884023002/> behaviour to ship to canary, such
that we'd enable DCHECKs there but not crash users, just send crash reports.
This seems to have a lot of similarity to me, except it's Log-on-DCHECK
instead.
>>>>>> +wez do you see similarities here, what do you think?
>>>>>> - Dana
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>

-- 
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.

[danakj, 2017-07-05 21:25:36]

On Wed, Jul 5, 2017 at 5:15 PM, Wez <wez@chromium.org> wrote:

> Right; so the risk is that people add more death-tests, or alter existing
> death-tests.
>
> We have a handy EXPECT_DCHECK_DEATH macro that is essentially a no-op in
> !DCHECK_IS_ON() builds, which a lot of tests use.  We can just tweak that
> to know about the DCHECK-mode. However, some tests explicitly check
> DCHECK_IS_ON() and then make the (reasonable) assumption that DCHECK ==
> LOG(FATAL) if DCHECK_IS_ON() - those will need explicitly fixing.  Fixing
> them isn't always as simple as replacing EXPECT_DEATH with
> EXPECT_DCHECK_DEATH because some of the tests are verifying that DCHECKs
> fire that e.g. protect against mis-use of STL - so the test may genuinely
> crash if the DCHECK doesn't fire. :P
>

Ok, so it would be on whomever wanted to use this DCHECKS-are-logs flag to
fix/conditionally-disable any tests that crash in order to use it, that
seems fair. You're worried about patches adding tests that would break on
the chromeos bots, but that is not unique to death tests, and we revert or
fix them when that happens I guess. Maybe some fun comments on
EXPECT_DCHECK_DEATH to explain this as well.

I'm not sure which tests we'd be running with the flag either, would it be
unit tests, or platform tests that are using all of chrome? I'm not sure
what PFQ is.


>
> On 5 July 2017 at 14:06, <danakj@chromium.org> wrote:
>
>> On Wed, Jul 5, 2017 at 5:04 PM, Wez <wez@chromium.org> wrote:
>>
>>> Ah, one down-side to this: death-tests will tend to bit-rot if we don't
>>> have a build somewhere on the CQ/waterfall that uses a non-FATAL DCHECK
>>> mode.
>>>
>>
>> Can you elaborate? Death tests should DCHECK crash in all configs except
>> asan/chromeos builds that turn off crashing (and I guess disable the tests
>> there). These tests must not be running right now in those configs either
>> since DCHECKs are just disabled?
>>
>>
>>>
>>> On 5 July 2017 at 11:53, Wez <wez@chromium.org> wrote:
>>>
>>>> OK, I'll break out a fresh CL with dcheck_mode; default will be
>>>> LOG_DCHECK=LOG_FATAL, so we can gate existence of dcheck_mode on whatever
>>>> we want, pretty easily.
>>>>
>>>> On 5 July 2017 at 11:42, <danakj@chromium.org> wrote:
>>>>
>>>>> On Wed, Jul 5, 2017 at 2:36 PM, Wez <wez@chromium.org> wrote:
>>>>>
>>>>>> We decided against the Dump-on-DCHECK approach, but we're instead
>>>>>> working on a DCHECKs-in-ASAN alternative that has the LOG_DCHECK
severity
>>>>>> dynamically switchable between FATAL and INFO (and, technically,
anything
>>>>>> else...) at run-time.  The changes to base/logging are fairly small, and
we
>>>>>> could generalize them to allow a separate DCHECK "mode" to be specified,
>>>>>> defaulting to FATAL, e.g:
>>>>>>
>>>>>> dcheck_always_on = true
>>>>>> dcheck_mode = "info"
>>>>>>
>>>>>> to enable DCHECKs but only have them generate INFO log output
>>>>>>
>>>>>> dcheck_always_on = true
>>>>>> dcheck_mode = dynamic
>>>>>>
>>>>>> to enable DCHECKs and get the run-time switchability we need for the
>>>>>> ASAN build, etc.
>>>>>>
>>>>>> If that sounds useful then I can break out a separate CL for
>>>>>> dcheck_mode to allow build-time selection of FATAL vs INFO severity
level,
>>>>>> this week.
>>>>>>
>>>>>> WDYT?
>>>>>>
>>>>>
>>>>> That seems pretty useful to me! I'd like if we made dcheck_mode only
>>>>> work when building for chromeos or asan though, to keep on gatekeeping
>>>>> disabling dchecks.
>>>>>
>>>>> What do others here think?
>>>>>
>>>>>
>>>>>>
>>>>>> On 4 July 2017 at 09:24, <danakj@chromium.org> wrote:
>>>>>>
>>>>>>> On Fri, Jun 30, 2017 at 12:35 AM, <posciak@chromium.org> wrote:
>>>>>>>
>>>>>>>> On 2017/06/30 00:37:22, Dirk Pranke wrote:
>>>>>>>> > I have no desire to run builds in the Chromium CQ that have DVLOG
>>>>>>>> enabled but
>>>>>>>> > not DCHECKs. I agree with what the others have said, that if
>>>>>>>> we're seeing
>>>>>>>> > DCHECKs firing too often but they're not being fixed, that's a
>>>>>>>> problem that we
>>>>>>>> > also need to fix, and I'm not very interested in working around
>>>>>>>> it.
>>>>>>>> >
>>>>>>>> > I can understand that you might want to run with DCHECK/DVLOG in
>>>>>>>> the PFQ, on
>>>>>>>> > real devices, etc., and that it is currently hard for us to
>>>>>>>> address any
>>>>>>>> DCHECKs
>>>>>>>> > that are showing up only in that situation. However, I'd also
>>>>>>>> like us to be
>>>>>>>> > running more tests on real devices in the Chromium CQ, and so I'd
>>>>>>>> like to
>>>>>>>> > address that set of problems that way.
>>>>>>>> >
>>>>>>>>
>>>>>>>> We are definitely not proposing disabling DCHECKs in Chromium CQ.
>>>>>>>> We are only
>>>>>>>> proposing adding a feature that would allow us to have at least
>>>>>>>> DVLOGs in PFQ,
>>>>>>>> where we do not/cannot easily at this moment have DCHECK_ALWAYS_ON.
>>>>>>>>
>>>>>>>> Of course having real Chrome OS devices in Chromium CQ instead
>>>>>>>> would be the
>>>>>>>> ideal and preferred solution, however whether and to what extent
>>>>>>>> that would be
>>>>>>>> possible is perhaps a separate topic.
>>>>>>>>
>>>>>>>> > If there's stuff that you really want to be logged but you don't
>>>>>>>> want to see
>>>>>>>> > dchecks in the same configuration, maybe they should be VLOGged
>>>>>>>> instead?
>>>>>>>>
>>>>>>>> No, not really, ideally we'd always want to have DCHECKs.
>>>>>>>> The whole issue is not for DCHECKs in our code, this is for DCHECKs
>>>>>>>> in code
>>>>>>>> added by other developers that fail only on CrOS and not in
>>>>>>>> Chromium CQ.
>>>>>>>>
>>>>>>>
>>>>>>> This reminds me of what we do for NOTREACHED() today:
>>>>>>>
>>>>>>> #if !DCHECK_IS_ON
<https://cs.chromium.org/chromium/src/base/logging.h?l=769&ct=xref_jump_to_def...
&& defined(OS_CHROMEOS)// Implement logging of NOTREACHED() as a dedicated
function to get function// call overhead down to a minimum.void
LogErrorNotReached(const char* file, int line);#define NOTREACHED()             
                         \  true ? ::logging::LogErrorNotReached(__FILE__,
__LINE__) \       : EAT_STREAM_PARAMETERS#else#define NOTREACHED
<https://cs.chromium.org/chromium/src/base/logging.h?l=942&gs=cpp%253Amacro-NO...
DCHECK(false)#endif
>>>>>>> As ChromeOS builds with DCHECKs off for the problems you describe above
I guess. So if I understand you're proposing to take chromeOS builds from a
dcheck_always_on=false to instead { dcheck_always_on=false + dlog_always_on=true
}. But this would only log DLOGs and DVLOGs. Why do you exclude logging DCHECKs
as well? Maybe what you're looking for is to extend this NOTREACHED() behaviour
to DCHECKs and D{V}LOGs, something like {dcheck_always_on=true +
dcheck_as_logs=true}. Then you'd get DLOG for free, but need to make DCHECK not
crash.
>>>>>>> wez has been working on a Dump-on-DCHECK
<https://codereview.chromium.org/1884023002/> behaviour to ship to canary, such
that we'd enable DCHECKs there but not crash users, just send crash reports.
This seems to have a lot of similarity to me, except it's Log-on-DCHECK
instead.
>>>>>>> +wez do you see similarities here, what do you think?
>>>>>>> - Dana
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>

-- 
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.

[Wez, 2017-07-06 01:39:25]

+cc:dcheng, who I mentioned this to, and who expressed a preference for treating
DVLOG separately from DCHECK, over having DCHECK have a check-but-don't-fail
mode - I'd suggest that we take that argument off-CL though.

[Dirk Pranke, 2017-07-06 20:45:02]

I think I can probably live with whatever we come up with here. Ultimately my
goal is to
get DCHECKs usable everywhere so that CrOS doesn't have to disable them, but
that
may not be practical at the moment.

The one nit I have is that it seems like even if the DCHECKs aren't fatal,
shouldn't they at
least be LOG_WARNING or LOG_ERROR and not LOG_INFO?

[dcheng1, 2017-07-12 09:25:41]

On 2017/07/06 01:39:25, Wez wrote:
> +cc:dcheng, who I mentioned this to, and who expressed a preference for
treating
> DVLOG separately from DCHECK, over having DCHECK have a check-but-don't-fail
> mode - I'd suggest that we take that argument off-CL though.

To summarize the opinion I previously expressed, it seems a bit confusing to me
that DCHECK_IS_ON() also controls DLOG behavior. To me, I think they should just
be gated on NDEBUG rather than DCHECK_IS_ON():
https://cs.chromium.org/chromium/src/base/logging.h?rcl=377951a272696effa274c...

[danakj, 2017-07-12 15:55:54]

On Wed, Jul 12, 2017 at 5:25 AM, <dcheng@google.com> wrote:

> On 2017/07/06 01:39:25, Wez wrote:
> > +cc:dcheng, who I mentioned this to, and who expressed a preference for
> treating
> > DVLOG separately from DCHECK, over having DCHECK have a
> check-but-don't-fail
> > mode - I'd suggest that we take that argument off-CL though.
>
> To summarize the opinion I previously expressed, it seems a bit confusing
> to me
> that DCHECK_IS_ON() also controls DLOG behavior. To me, I think they
> should just
> be gated on NDEBUG rather than DCHECK_IS_ON():
> https://cs.chromium.org/chromium/src/base/logging.h?rcl=
> 377951a272696effa274c2786a00d18e45c497b6&l=774
>

My thought is: DLOGs are used for debugging. DCHECK_IS_ON is for debug
builds and bots. When something fails on bots, you'd want the debug output.
DCHECK_IS_ON is basically a cheaper building debug build, so they make
sense to be on there to me.


>
>
> https://codereview.chromium.org/2954883002/
>

-- 
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.

[brettw, 2017-07-12 19:29:20]

Also keep in mind that DCHECK is really DLOG_IF(FATAL...). Although in Chrome we
don't usually use it this way, I think internal to Google there's more use of
the log levels.

[Wez, 2017-07-12 20:47:59]

On 2017/07/12 19:29:20, brettw wrote:
> Also keep in mind that DCHECK is really DLOG_IF(FATAL...). Although in Chrome
we
> don't usually use it this way, I think internal to Google there's more use of
> the log levels.

It's actually not quite DLOG_IF(FATAL, ...) - DCHECK(condition) guarantees to
"reference" |condition|, so you can write:

bool success = DoStuff();
DCHECK(success)

whereas DLOG_IF() does not - the example above would fail to build for Release,
with an unused variable warning->error.

You're right, though, that at present the LogSeverity of DCHECK is FATAL in
DCHECK_IS_ON() builds.  If we go with the debug_mode build option to allow
DCHECK to be non-FATAL then that won't affect DLOG*(FATAL, ...) since those
lines are explicitly choosing FATAL.  We would necessarily have a new LOG_DCHECK
LogSeverity, though, so it would be possible to write DLOG*(DCHECK, ...) to get
same-as-DCHECK LogSeverity.

My preference is still to implement the debug_mode flag - since it seems
controversial I'll write up a quick Chromium.org doc on it for base/OWNERS to
argue over.