Add new macro "ANALYZER_ASSUME_TRUE" and integrate with all assertion types.

Add new macro "ANALYZER_ASSUME_TRUE" and integrate with all assertion types.

ANALYZER_ASSUME_TRUE(...) adds compiler-specific annotations that stop static analysis if an asserted condition for an analyzed codepath is untrue.

Now that this macro encapsulates most of the complexity of handling
analysis cases, we can trim some redundancy from logging.h.

Add ANALYZER_ASSUME_TRUE() to all CHECK asserts and variants of DCHECK_OP (DCHECK_EQ, DCHECK_NE, DCHECK_LT...)


BUG=687243
R=bdawson@chromium.org,thakis@chromium.org,wez@chromium.org

Review-Url: https://codereview.chromium.org/2669213005
Cr-Commit-Position: refs/heads/master@{#449832}
Committed: https://chromium.googlesource.com/chromium/src/+/e23eed07b41b99be905bffbbcadb0d761c40eb7d

[Kevin M, 2017-02-02 20:13:55]

kmarshall@chromium.org changed reviewers:
+ brucedawson@chromium.org
- bdawson@chromium.org

[Kevin M, 2017-02-02 21:24:42]

The CQ bit was checked by kmarshall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-02 21:25:10]

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

[brucedawson, 2017-02-02 21:37:57]

Comments inline.

Also, as discussed at lunch, I'm not to bothered about whether or not this
breaks /analyze as it hasn't been adding much value lately. But, we might as
well try to keep it running for a bit longer. The builder results can be seen
here:

http://build.chromium.org/p/chromium.fyi/builders/Chromium%20Windows%20Analyze/

https://codereview.chromium.org/2669213005/diff/20001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/2669213005/diff/20001/base/logging.h#newcode311
base/logging.h:311: #define ANALYSIS_MARK_ASSERTION(condition) !!(condition)
Why the double ! here? They are needed in the /analyze case because of a VC++
/analyze bug (which may be fixed now) but they are probably not needed in the
normal case, especially since this actually leads to a triple ! after macro
expansion in some places. The double !! should not be needed outside of
/analyze.

https://codereview.chromium.org/2669213005/diff/20001/base/logging.h#newcode538
base/logging.h:538: ANALYSIS_MARK_ASSERTION(condition))
I think you've gone from !(condition) to !!(condition), thus reversing the sense
of the check.

https://codereview.chromium.org/2669213005/diff/20001/base/logging.h#newcode541
base/logging.h:541: LAZY_STREAM(PLOG_STREAM(FATAL),
ANALYSIS_MARK_ASSERTION(condition)) \
I think you've gone from !(condition) to !!(condition), thus reversing the sense
of the check.

[Kevin M, 2017-02-02 21:46:44]

https://codereview.chromium.org/2669213005/diff/20001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/2669213005/diff/20001/base/logging.h#newcode311
base/logging.h:311: #define ANALYSIS_MARK_ASSERTION(condition) !!(condition)
On 2017/02/02 21:37:57, brucedawson (slow) wrote:
> Why the double ! here? They are needed in the /analyze case because of a VC++
> /analyze bug (which may be fixed now) but they are probably not needed in the
> normal case, especially since this actually leads to a triple ! after macro
> expansion in some places. The double !! should not be needed outside of
> /analyze.

Wasn't sure if casting to a bool in this way yielded more efficient code.
Anyways, done here and on line 302.

https://codereview.chromium.org/2669213005/diff/20001/base/logging.h#newcode538
base/logging.h:538: ANALYSIS_MARK_ASSERTION(condition))
On 2017/02/02 21:37:57, brucedawson (slow) wrote:
> I think you've gone from !(condition) to !!(condition), thus reversing the
sense
> of the check.

Done.

https://codereview.chromium.org/2669213005/diff/20001/base/logging.h#newcode541
base/logging.h:541: LAZY_STREAM(PLOG_STREAM(FATAL),
ANALYSIS_MARK_ASSERTION(condition)) \
On 2017/02/02 21:37:57, brucedawson (slow) wrote:
> I think you've gone from !(condition) to !!(condition), thus reversing the
sense
> of the check.

Done.

[brucedawson, 2017-02-02 21:51:29]

lgtm - with the assumption that you triple check that the sense of the
conditions is all correct now. I assume that the previous try jobs will fail
with lots of CHECK failures due to the sense of those being reversed in the
previous patch.

[Nico, 2017-02-02 22:07:01]

lgtm

[Wez, 2017-02-03 20:51:52]

https://codereview.chromium.org/2669213005/diff/40001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/2669213005/diff/40001/base/logging.h#newcode292
base/logging.h:292: // ANALYSIS_MARK_ASSERTION(...) adds compiler-specific
annotations to an
nit: Can we give this a more specific name; it's not immediately clear to me
what it means to "mark assertion".

Perhaps ANALYSIS_ASSUME_TRUE()?

femto-nit: I'd also suggest calling it ANALYZE[R]_<foo> rather than
ANALYSIS_<foo>, so that a trivial code-search for "analyz*" will match these as
well as the analyzer build flags in GN, etc.

https://codereview.chromium.org/2669213005/diff/40001/base/logging.h#newcode519
base/logging.h:519: #if defined(_PREFAST_) && defined(OS_WIN)
Isn't the point of the ANALYSIS_... macro to allow us to have a single
definition of CHECK etc, which looks like the non-PREFAST case below?

https://codereview.chromium.org/2669213005/diff/40001/base/logging.h#newcode527
base/logging.h:527: , LAZY_STREAM(LOG_STREAM(FATAL), false) << "Check failed: "
#condition ". "
Not this CL: I'm confused; does CHECK ever actually fire in these PreFast
builds, given that we pass |false| to LAZY_STREAM()?

[Kevin M, 2017-02-03 22:36:27]

Description was changed from

==========
Unify Prefast and Clang assertion annotations with new macro.

ANALYSIS_MARK_ASSERTION(...) adds compiler-specific annotations to an
asserted condition which stop static analysis if the asserted condition is
untrue. The macro definition varies depending on whether Prefast is
enabled, Clang analysis is enabled, or if no analysis tool is active.

Now that this macro encapsulates most of the complexity of handling
analysis cases, we can trim some redundancy from logging.h.


BUG=687243
R=bdawson@chromium.org,thakis@chromium.org,wez@chromium.org
==========

to

==========
Unify Prefast and Clang assertion annotations with new macro.

ANALYZER_ASSUME_TRUE(...) adds compiler-specific annotations to an
asserted condition which stop static analysis if the asserted condition is
untrue.

Now that this macro encapsulates most of the complexity of handling
analysis cases, we can trim some redundancy from logging.h.

Add ANALYZER_ASSUME_TRUE() to all CHECK asserts and variants of DCHECK_OP
(DCHECK_EQ, DCHECK_NE, DCHECK_LT...)


BUG=687243
R=bdawson@chromium.org,thakis@chromium.org,wez@chromium.org
==========

[Kevin M, 2017-02-03 22:37:23]

PTAL. I hooked ANALYZER_ASSUME_TRUE into all the DCHECK_OP codepaths and wow -
it cut down on the error noise significantly!

https://codereview.chromium.org/2669213005/diff/40001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/2669213005/diff/40001/base/logging.h#newcode527
base/logging.h:527: , LAZY_STREAM(LOG_STREAM(FATAL), false) << "Check failed: "
#condition ". "
On 2017/02/03 20:51:52, Wez wrote:
> Not this CL: I'm confused; does CHECK ever actually fire in these PreFast
> builds, given that we pass |false| to LAZY_STREAM()?

It doesn't, but the analysis output should be correct.

[Kevin M, 2017-02-06 18:28:40]

Description was changed from

==========
Unify Prefast and Clang assertion annotations with new macro.

ANALYZER_ASSUME_TRUE(...) adds compiler-specific annotations to an
asserted condition which stop static analysis if the asserted condition is
untrue.

Now that this macro encapsulates most of the complexity of handling
analysis cases, we can trim some redundancy from logging.h.

Add ANALYZER_ASSUME_TRUE() to all CHECK asserts and variants of DCHECK_OP
(DCHECK_EQ, DCHECK_NE, DCHECK_LT...)


BUG=687243
R=bdawson@chromium.org,thakis@chromium.org,wez@chromium.org
==========

to

==========
Add new macro "ANALYZER_ASSUME_TRUE" and integrate with all assertion types.

ANALYZER_ASSUME_TRUE(...) adds compiler-specific annotations that stop static
analysis if an asserted condition for an analyzed codepath is untrue.

Now that this macro encapsulates most of the complexity of handling
analysis cases, we can trim some redundancy from logging.h.

Add ANALYZER_ASSUME_TRUE() to all CHECK asserts and variants of DCHECK_OP
(DCHECK_EQ, DCHECK_NE, DCHECK_LT...)


BUG=687243
R=bdawson@chromium.org,thakis@chromium.org,wez@chromium.org
==========

[Wez, 2017-02-06 21:04:39]

https://codereview.chromium.org/2669213005/diff/40001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/2669213005/diff/40001/base/logging.h#newcode527
base/logging.h:527: , LAZY_STREAM(LOG_STREAM(FATAL), false) << "Check failed: "
#condition ". "
On 2017/02/03 22:37:23, Kevin M wrote:
> On 2017/02/03 20:51:52, Wez wrote:
> > Not this CL: I'm confused; does CHECK ever actually fire in these PreFast
> > builds, given that we pass |false| to LAZY_STREAM()?
> 
> It doesn't, but the analysis output should be correct.

Feels cleaner to replace false with condition here, then, so that a PREFAST
build will still crash as intended.

But, as noted above, surely the point of ANALYZER_ASSUME_TRUE is to let us use
the non-PREFAST definition below, all the time, and not have this special-case
definition of CHECK et al at all?

https://codereview.chromium.org/2669213005/diff/60001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/2669213005/diff/60001/base/logging.h#newcode745
base/logging.h:745: // See comments on the previous use of __analysis_assume.
See comment above; do we need this if we have a "standardized"
ANALYZER_ASSUME_TRUE form..?

[Kevin M, 2017-02-09 23:36:25]

PTAL

https://codereview.chromium.org/2669213005/diff/60001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/2669213005/diff/60001/base/logging.h#newcode745
base/logging.h:745: // See comments on the previous use of __analysis_assume.
On 2017/02/06 21:04:39, Wez wrote:
> See comment above; do we need this if we have a "standardized"
> ANALYZER_ASSUME_TRUE form..?

Done.

[Wez, 2017-02-10 00:40:45]

LGTM :)

https://codereview.chromium.org/2669213005/diff/60001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/2669213005/diff/60001/base/logging.h#newcode745
base/logging.h:745: // See comments on the previous use of __analysis_assume.
On 2017/02/09 23:36:25, Kevin M wrote:
> On 2017/02/06 21:04:39, Wez wrote:
> > See comment above; do we need this if we have a "standardized"
> > ANALYZER_ASSUME_TRUE form..?
> 
> Done.

Woohoo, that looks great. Hopefully it doesn't trigger analyzer bugs :P

[Kevin M, 2017-02-10 00:58:01]

kmarshall@chromium.org changed reviewers:
+ stanisc@chromium.org

[Kevin M, 2017-02-10 00:58:02]

+R stanisc@ , who is kind enough to try building this on his Windows machine.

Stanislav, it looks like you just need to set "use_vs_code_analysis=true" in
your GN args to turn it on. Please reply in the CL comments whether this
succeeded or failed. Thanks!

[Kevin M, 2017-02-10 01:14:50]

PREfast appeared to be functioning normally on Stanislav's machine.
Submitting...

[Kevin M, 2017-02-10 01:14:54]

The CQ bit was checked by kmarshall@chromium.org

[Kevin M, 2017-02-10 01:14:54]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org,
brucedawson@chromium.org
Link to the patchset: https://codereview.chromium.org/2669213005/#ps80001
(title: "Removed redundant prefast CHECK/DCHECK definitions")

[commit-bot: I haz the power, 2017-02-10 01:15:24]

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

[commit-bot: I haz the power, 2017-02-10 01:15:25]

This CL has an open dependency (Issue 2667853004 Patch 140001). Please resolve
the dependency and try again.
If you are sure that there is no real dependency, please use one of the options
listed in https://goo.gl/9Es4OR to land the CL.

[Kevin M, 2017-02-10 01:19:54]

The CQ bit was checked by kmarshall@chromium.org

[Kevin M, 2017-02-10 01:19:55]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org,
wez@chromium.org, brucedawson@chromium.org
Link to the patchset: https://codereview.chromium.org/2669213005/#ps100001
(title: "rebase")

[commit-bot: I haz the power, 2017-02-10 01:20:37]

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, 2017-02-10 07:21:51]

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

[commit-bot: I haz the power, 2017-02-10 07:21:52]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Kevin M, 2017-02-10 17:45:00]

The CQ bit was checked by kmarshall@chromium.org

[commit-bot: I haz the power, 2017-02-10 17:45:37]

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

[Nico, 2017-02-10 18:08:47]

https://codereview.chromium.org/2669213005/diff/100001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/2669213005/diff/100001/base/logging.h#newcode306
base/logging.h:306: return std::forward<TVal>(arg);
this looks a lot more complicated than the last time i looked at it. it's nice
that this removes net 20 lines, but can we get away without the rvalue ref and
the move()? seems like we didn't need that before

[Kevin M, 2017-02-10 18:20:09]

https://codereview.chromium.org/2669213005/diff/100001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/2669213005/diff/100001/base/logging.h#newcode306
base/logging.h:306: return std::forward<TVal>(arg);
On 2017/02/10 18:08:47, Nico wrote:
> this looks a lot more complicated than the last time i looked at it. it's nice
> that this removes net 20 lines, but can we get away without the rvalue ref and
> the move()? seems like we didn't need that before

Using rvalue/forward() means this function can be applied more broadly  across
both move-only and non-move-only types. Also, I believe that perfect forwarding
is a stronger signal to the optimizer? (Please correct me if I'm wrong.)

[Kevin M, 2017-02-10 18:25:47]

The CQ bit was unchecked by kmarshall@chromium.org

[Nico, 2017-02-10 18:35:06]

the forward() calls only happen in analyze builds, so performance shouldn't
matter there, right?

I tried to find out where the forwarding got added but couldn't figure it out.
Can you give an example of a DCHECK (or whatever) that wasn't writable before
but is with it?

(the motivation here is that logging.h is a way to complicated header already --
so making it smaller is cool, but it'd be even cooler if we could do that
without using yet more tricks from c++'s bag of stuff)

[Kevin M, 2017-02-10 18:45:58]

I'll try to reproduce the issue that caused me to use rvalues. If I can't, I'll
switch it to use non-rvalue types. BTW, what are the issues in using C++11
language features in logging.h?

The current PREfast DCHECK definitions are really hacky and bypass lazy stream
instantiation. It makes the builds suitable for analysis but not for execution
as it bypasses runtime checks entirely. The primary motivations of this CL are
to fix that, and normalize the way in which we annotate our assertion handlers.

[Nico, 2017-02-10 18:46:47]

On Fri, Feb 10, 2017 at 1:45 PM, <kmarshall@chromium.org> wrote:

> I'll try to reproduce the issue that caused me to use rvalues. If I can't,
> I'll
> switch it to use non-rvalue types. BTW, what are the issues in using C++11
> language features in logging.h?
>

Just that things get more complicated for humans the more features get used
in one place.


>
> The current PREfast DCHECK definitions are really hacky and bypass lazy
> stream
> instantiation. It makes the builds suitable for analysis but not for
> execution
> as it bypasses runtime checks entirely. The primary motivations of this CL
> are
> to fix that, and normalize the way in which we annotate our assertion
> handlers.
>
> https://codereview.chromium.org/2669213005/
>

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

[Kevin M, 2017-02-10 19:14:54]

Still built w/o rvalue references; removed them

[Kevin M, 2017-02-10 22:43:33]

The CQ bit was checked by kmarshall@chromium.org

[Kevin M, 2017-02-10 22:43:33]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org,
wez@chromium.org, brucedawson@chromium.org
Link to the patchset: https://codereview.chromium.org/2669213005/#ps120001
(title: "RIP RValue references")

[commit-bot: I haz the power, 2017-02-10 22:44:01]

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, 2017-02-11 02:13:28]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1486766613251760,
"parent_rev": "f062c42d082ebf9c2f2e39ac6683f7755d232ac2", "commit_rev":
"e23eed07b41b99be905bffbbcadb0d761c40eb7d"}

[commit-bot: I haz the power, 2017-02-11 02:14:06]

Description was changed from

==========
Add new macro "ANALYZER_ASSUME_TRUE" and integrate with all assertion types.

ANALYZER_ASSUME_TRUE(...) adds compiler-specific annotations that stop static
analysis if an asserted condition for an analyzed codepath is untrue.

Now that this macro encapsulates most of the complexity of handling
analysis cases, we can trim some redundancy from logging.h.

Add ANALYZER_ASSUME_TRUE() to all CHECK asserts and variants of DCHECK_OP
(DCHECK_EQ, DCHECK_NE, DCHECK_LT...)


BUG=687243
R=bdawson@chromium.org,thakis@chromium.org,wez@chromium.org
==========

to

==========
Add new macro "ANALYZER_ASSUME_TRUE" and integrate with all assertion types.

ANALYZER_ASSUME_TRUE(...) adds compiler-specific annotations that stop static
analysis if an asserted condition for an analyzed codepath is untrue.

Now that this macro encapsulates most of the complexity of handling
analysis cases, we can trim some redundancy from logging.h.

Add ANALYZER_ASSUME_TRUE() to all CHECK asserts and variants of DCHECK_OP
(DCHECK_EQ, DCHECK_NE, DCHECK_LT...)


BUG=687243
R=bdawson@chromium.org,thakis@chromium.org,wez@chromium.org

Review-Url: https://codereview.chromium.org/2669213005
Cr-Commit-Position: refs/heads/master@{#449832}
Committed:
https://chromium.googlesource.com/chromium/src/+/e23eed07b41b99be905bffbbcadb...
==========

[commit-bot: I haz the power, 2017-02-11 02:14:07]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/e23eed07b41b99be905bffbbcadb...