Add Clang static analysis control to all assert functions in logging.h

Add Clang static analysis control to all assert functions in logging.h

This CL adds static analysis hints to all assertion functions in logging.h. On builds with static analysis enabled, ANALYSIS_ASSUME_TRUE generates code which prevents the analyzer from evaluating codepaths that are asserted to be impossible. 


R=brucedawson@chromium.org,danakj@chromium.org
BUG=327707
Review-Url: https://codereview.chromium.org/2729503004
Cr-Commit-Position: refs/heads/master@{#466129}
Committed: https://chromium.googlesource.com/chromium/src/+/fe2f09f858b77a7bd9437ef3b377e9af7f50f62d

[Kevin M, 2017-03-02 22:50:04]

Description was changed from

==========
Re-add AnalysisAssumeTrue() with boolean-only inputs.

The previous implementation of AnalysisAssumeTrue() preserved
the type of |arg| which was problematic because the myriad
variations of types was rife with ambiguity for the template type
deduction and overload systems: lvalue refs, rvalue refs,
pointers, literals, and unaligned literals (e.g. bitfields).

Given that ANALYSIS_ASSUME_TRUE is only invoked in the context
of a conditional, it is OK if we coerce the expression to a boolean
via logical NOT before passing it to AnalysisAssumeTrue(). Now that
the AnalysisAssumeTrue() needs only to support booleans, the
implementation becomes trivial.

R=brucedawson@chromium.org,danakj@chromium.org
BUG=327707
==========

to

==========
Re-add ANALYSIS_ASSUME_TRUE() with simplified bool inputs.

The previous implementation of AnalysisAssumeTrue() preserved
the type of |arg| which was problematic because the myriad
variations of types was rife with ambiguity for the template type
deduction and overload systems: lvalue refs, rvalue refs,
pointers, literals, and unaligned literals (e.g. bitfields).

Given that ANALYSIS_ASSUME_TRUE is only invoked in the context
of a conditional, it is OK if we coerce the expression to a boolean
via logical NOT before passing it to AnalysisAssumeTrue(). Now that
the AnalysisAssumeTrue() needs only to support booleans, the
implementation becomes trivial.

(This CL reverts the rollback CL #2720893003).

R=brucedawson@chromium.org,danakj@chromium.org
BUG=327707
==========

[Kevin M, 2017-03-03 00:17:26]

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

[commit-bot: I haz the power, 2017-03-03 00:17:53]

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

[Kevin M, 2017-03-03 02:00:34]

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

[commit-bot: I haz the power, 2017-03-03 02:01:09]

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

[commit-bot: I haz the power, 2017-03-03 02:12:45]

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

[commit-bot: I haz the power, 2017-03-03 02:12:45]

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

[danakj, 2017-03-03 18:41:36]

Bots look unhappy, should I wait for that to get sorted?

[brucedawson, 2017-03-03 19:11:50]

On 2017/03/03 18:41:36, danakj wrote:
> Bots look unhappy, should I wait for that to get sorted?

Making the function not constexpr means that DCHECK fails when called from a
constexpr function.

d:\src\chromium\src\base\time\time.h(685): error C2134:
'logging::AnalysisAssumeTrue': call does not result in a constant expression
d:\src\chromium\src\base\time\time.h(685): note: failure was caused by call of
undefined function or one not declared 'constexpr'
d:\src\chromium\src\base\logging.h(316): note: see declaration of
'logging::AnalysisAssumeTrue'

VC++ 2017 improves constexpr support - maybe it supports the previous solution?

[Kevin M, 2017-03-10 22:14:34]

I'm close to having my own Win box up and running with MSVC, so I'll ping you
when I have a solution that works on my end.

[Kevin M, 2017-03-10 22:14:49]

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

[commit-bot: I haz the power, 2017-03-10 22:15:16]

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

[commit-bot: I haz the power, 2017-03-11 00:01:00]

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

[commit-bot: I haz the power, 2017-03-11 00:01:00]

Dry run: This issue passed the CQ dry run.

[Kevin M, 2017-03-14 20:23:49]

Description was changed from

==========
Re-add ANALYSIS_ASSUME_TRUE() with simplified bool inputs.

The previous implementation of AnalysisAssumeTrue() preserved
the type of |arg| which was problematic because the myriad
variations of types was rife with ambiguity for the template type
deduction and overload systems: lvalue refs, rvalue refs,
pointers, literals, and unaligned literals (e.g. bitfields).

Given that ANALYSIS_ASSUME_TRUE is only invoked in the context
of a conditional, it is OK if we coerce the expression to a boolean
via logical NOT before passing it to AnalysisAssumeTrue(). Now that
the AnalysisAssumeTrue() needs only to support booleans, the
implementation becomes trivial.

(This CL reverts the rollback CL #2720893003).

R=brucedawson@chromium.org,danakj@chromium.org
BUG=327707
==========

to

==========
Add static analysis path control to asserts in logging.h

This CL adds static analysis hooks to all asserts in logging.h. For builds with
static analysis enabled, ANALYSIS_ASSUME_TRUE generates code which tells the
analyzer to stop evaluating the paths which are asserted to be impossible. For
example, DCHECK(ptr) declares that 'ptr' cannot be null, so the static analyzer
will only consider scenarios where 'ptr' is non-null.

Separate implementations of ANALYSIS_ASSUME_TRUE are provided for Clang and MSVC
PREfast cases which have subtly different semantics. For builds in which there
is no static analyzer present, ANALYSIS_ASSUME_TRUE() is a no-op.

This CL also merges the analysis/non-analysis variants of assertion functions,
which is made possible by the no-op implementation of ANALYSIS_ASSUME_TRUE. The
analysis-only implementations of CHECK/DCHECK/etc. are deleted.


R=brucedawson@chromium.org,danakj@chromium.org
BUG=327707
==========

[Kevin M, 2017-03-14 20:25:53]

Reviewers, PTAL. I have verified the changes with the PREfast and Clang
analyzers. Will use the trybots to verify the changes w/o analysis.

[Kevin M, 2017-03-14 20:26:25]

Description was changed from

==========
Add static analysis path control to asserts in logging.h

This CL adds static analysis hooks to all asserts in logging.h. For builds with
static analysis enabled, ANALYSIS_ASSUME_TRUE generates code which tells the
analyzer to stop evaluating the paths which are asserted to be impossible. For
example, DCHECK(ptr) declares that 'ptr' cannot be null, so the static analyzer
will only consider scenarios where 'ptr' is non-null.

Separate implementations of ANALYSIS_ASSUME_TRUE are provided for Clang and MSVC
PREfast cases which have subtly different semantics. For builds in which there
is no static analyzer present, ANALYSIS_ASSUME_TRUE() is a no-op.

This CL also merges the analysis/non-analysis variants of assertion functions,
which is made possible by the no-op implementation of ANALYSIS_ASSUME_TRUE. The
analysis-only implementations of CHECK/DCHECK/etc. are deleted.


R=brucedawson@chromium.org,danakj@chromium.org
BUG=327707
==========

to

==========
Add static analysis path control to asserts in logging.h

This CL adds static analysis hooks to all asserts in logging.h. For 
builds with static analysis enabled, ANALYSIS_ASSUME_TRUE generates
code which tells the analyzer to stop evaluating the paths which are
asserted to be impossible. For example, DCHECK(ptr) declares that 'ptr'
cannot be null, so the static analyzer will only consider scenarios
where 'ptr' is non-null.

Separate implementations of ANALYSIS_ASSUME_TRUE are provided for Clang
and MSVC PREfast cases which have subtly different semantics. For
builds in which there is no static analyzer present,
ANALYSIS_ASSUME_TRUE() is a no-op.

This CL also merges the analysis/non-analysis variants of assertion
functions, which is made possible by the no-op implementation of
ANALYSIS_ASSUME_TRUE. The analysis-only implementations of
CHECK/DCHECK/etc. are deleted.


R=brucedawson@chromium.org,danakj@chromium.org
BUG=327707
==========

[Kevin M, 2017-03-14 20:26:30]

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

[commit-bot: I haz the power, 2017-03-14 20:27:05]

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

[Kevin M, 2017-03-14 20:32:32]

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

[commit-bot: I haz the power, 2017-03-14 20:33:36]

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

[Kevin M, 2017-03-14 20:45:05]

Description was changed from

==========
Add static analysis path control to asserts in logging.h

This CL adds static analysis hooks to all asserts in logging.h. For 
builds with static analysis enabled, ANALYSIS_ASSUME_TRUE generates
code which tells the analyzer to stop evaluating the paths which are
asserted to be impossible. For example, DCHECK(ptr) declares that 'ptr'
cannot be null, so the static analyzer will only consider scenarios
where 'ptr' is non-null.

Separate implementations of ANALYSIS_ASSUME_TRUE are provided for Clang
and MSVC PREfast cases which have subtly different semantics. For
builds in which there is no static analyzer present,
ANALYSIS_ASSUME_TRUE() is a no-op.

This CL also merges the analysis/non-analysis variants of assertion
functions, which is made possible by the no-op implementation of
ANALYSIS_ASSUME_TRUE. The analysis-only implementations of
CHECK/DCHECK/etc. are deleted.


R=brucedawson@chromium.org,danakj@chromium.org
BUG=327707
==========

to

==========
Add static analysis path control to asserts in logging.h

This CL adds static analysis hooks to all asserts in logging.h. For 
builds with static analysis enabled, ANALYSIS_ASSUME_TRUE generates
code which tells the analyzer to stop evaluating the paths which are
asserted to be impossible. For example, DCHECK(ptr) declares that 'ptr'
cannot be null, so the static analyzer will only consider scenarios
where 'ptr' is non-null.

Separate implementations of ANALYSIS_ASSUME_TRUE are provided for Clang
and MSVC PREfast cases which have subtly different semantics. For
builds in which there is no static analyzer present,
ANALYSIS_ASSUME_TRUE() is a no-op which simply passes the input value
straight through.

This CL also merges the analysis/non-analysis variants of assertion
functions, which is made possible by the no-op implementation of
ANALYSIS_ASSUME_TRUE. The analysis-only implementations of
CHECK/DCHECK/etc. are deleted.


R=brucedawson@chromium.org,danakj@chromium.org
BUG=327707
==========

[commit-bot: I haz the power, 2017-03-14 21:59:44]

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

[commit-bot: I haz the power, 2017-03-14 21:59:45]

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

[brucedawson, 2017-03-15 00:23:51]

On 2017/03/14 20:25:53, Kevin M wrote:
> Reviewers, PTAL. I have verified the changes with the PREfast and Clang
> analyzers. Will use the trybots to verify the changes w/o analysis.

Are the bot failures spurious or indicative of actual problems with the CL? The
one I looked at was a CHECK or DCHECK failure.

[Kevin M, 2017-03-15 00:25:37]

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

[commit-bot: I haz the power, 2017-03-15 00:26:30]

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

[Kevin M, 2017-03-15 00:27:15]

Just fixed the issue which manifested on release builds with DCHECK_ALWAYS_ON.
Trying again.

[commit-bot: I haz the power, 2017-03-15 00:35:23]

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

[commit-bot: I haz the power, 2017-03-15 00:35:24]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Kevin M, 2017-03-15 00:40:10]

Nevermind, the game of compiler whack-a-mole continues...

[Kevin M, 2017-03-15 17:31:21]

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

[commit-bot: I haz the power, 2017-03-15 17:32:34]

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

[commit-bot: I haz the power, 2017-03-15 20:23:44]

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

[commit-bot: I haz the power, 2017-03-15 20:23:45]

Dry run: This issue passed the CQ dry run.

[Kevin M, 2017-03-15 23:46:31]

OK, PTAL. Looks like I've appeased all three compilers (clang, msvc, cc1plus)
with this latest patch.

[brucedawson, 2017-03-16 20:15:41]

A few comments and questions.

https://codereview.chromium.org/2729503004/diff/160002/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/2729503004/diff/160002/base/logging.h#newcode301
base/logging.h:301: // errors will be generated for the current path.
This doesn't fit my mental model for how /analyze uses these results. The
/analyze intrinsic is __analyzis_assume and, like the name says, it tells the
analyzer to assume that the argument is true. This doesn't necessarily have to
do with a particular execution path. It is often used to indicate that a
particular index, for instance, is constrained to a particular range in order to
avoid spurious out-of-range access warnings.

It's just a comment, and it's not really a *blocking* issue but I wanted to
mention it.

https://codereview.chromium.org/2729503004/diff/160002/base/logging.h#newcode317
base/logging.h:317: #define ANALYZER_ASSUME_TRUE(arg)
(__analysis_assume(!!(arg)), !!(arg))
The second !! is not needed.

https://codereview.chromium.org/2729503004/diff/160002/base/logging.h#newcode321
base/logging.h:321: #define ANALYZER_ASSUME_TRUE(arg) !!(arg)
The !! should not be needed, should it? It is needed for __analysis_assume due
to a quirk of its implementation. It is probably needed for AnalyzerAssumeTrue
because it expects an actual bool. But elsewhere maybe not?

https://codereview.chromium.org/2729503004/diff/160002/base/logging.h#newcode852
base/logging.h:852: // Note that the contract of DCHECK_EQ, etc is that
arguments are only evaluated
Is this contract still respected? I'm not sure whether it matters or not, but
the use of the comma operator in the /analyze version of ANALYZER_ASSUME_TRUE
suggests the possibility of double evaluation.

Note that the previous usage of __analysis_assume avoided this double evaluation
by passing false to LAZY_STREAM. I believe this was determined to be harmless
because the resulting binary is never run so effectively compiling out all
DCHECKs is irrelevant.

It may be that similar logic means that the current implementation is fine also,
but the alternative would be replacing:

#define ANALYZER_ASSUME_TRUE(arg) (__analysis_assume(!!(arg)), !!(arg))

with:

#define ANALYZER_ASSUME_TRUE(arg) (__analysis_assume(!!(arg)), false)

[Kevin M, 2017-03-16 23:08:14]

https://codereview.chromium.org/2729503004/diff/160002/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/2729503004/diff/160002/base/logging.h#newcode301
base/logging.h:301: // errors will be generated for the current path.
On 2017/03/16 20:15:40, brucedawson wrote:
> This doesn't fit my mental model for how /analyze uses these results. The
> /analyze intrinsic is __analyzis_assume and, like the name says, it tells the
> analyzer to assume that the argument is true. This doesn't necessarily have to
> do with a particular execution path. It is often used to indicate that a
> particular index, for instance, is constrained to a particular range in order
to
> avoid spurious out-of-range access warnings.
> 
> It's just a comment, and it's not really a *blocking* issue but I wanted to
> mention it.

Interesting. In the Clang case, noreturn is used for culling analysis paths, not
for creating new ones.

OK, I'll reword this to be more general. "This macro does things on stuff." ;)
Maybe not THAT general.

https://codereview.chromium.org/2729503004/diff/160002/base/logging.h#newcode317
base/logging.h:317: #define ANALYZER_ASSUME_TRUE(arg)
(__analysis_assume(!!(arg)), !!(arg))
On 2017/03/16 20:15:40, brucedawson wrote:
> The second !! is not needed.

It makes the expression value consistent with the Clang case, which works with
bools.

https://codereview.chromium.org/2729503004/diff/160002/base/logging.h#newcode321
base/logging.h:321: #define ANALYZER_ASSUME_TRUE(arg) !!(arg)
On 2017/03/16 20:15:40, brucedawson wrote:
> The !! should not be needed, should it? It is needed for __analysis_assume due
> to a quirk of its implementation. It is probably needed for AnalyzerAssumeTrue
> because it expects an actual bool. But elsewhere maybe not?

See above..

https://codereview.chromium.org/2729503004/diff/160002/base/logging.h#newcode852
base/logging.h:852: // Note that the contract of DCHECK_EQ, etc is that
arguments are only evaluated
On 2017/03/16 20:15:40, brucedawson wrote:
> Is this contract still respected? I'm not sure whether it matters or not, but
> the use of the comma operator in the /analyze version of ANALYZER_ASSUME_TRUE
> suggests the possibility of double evaluation.
> 
> Note that the previous usage of __analysis_assume avoided this double
evaluation
> by passing false to LAZY_STREAM. I believe this was determined to be harmless
> because the resulting binary is never run so effectively compiling out all
> DCHECKs is irrelevant.
> 
> It may be that similar logic means that the current implementation is fine
also,
> but the alternative would be replacing:
> 
> #define ANALYZER_ASSUME_TRUE(arg) (__analysis_assume(!!(arg)), !!(arg))
> 
> with:
> 
> #define ANALYZER_ASSUME_TRUE(arg) (__analysis_assume(!!(arg)), false)

Actually, the potential for double evaluation of an expression w/side effects
(e.g. DCHECK(foo++)) is concerning.

I adapted the MSVC #ifdef to use an AnalysisAssumeTrue() function a la Clang,
which just calls analysis_assume & passes the computed boolean result straight
through. That way the expression is evaluated only once.

[Kevin M, 2017-03-16 23:08:21]

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

[commit-bot: I haz the power, 2017-03-16 23:09:05]

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

[commit-bot: I haz the power, 2017-03-17 02:40:40]

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

[commit-bot: I haz the power, 2017-03-17 02:40:42]

Dry run: This issue passed the CQ dry run.

[brucedawson, 2017-03-17 21:42:42]

> I adapted the MSVC #ifdef to use an AnalysisAssumeTrue() function a la Clang,
> which just calls analysis_assume & passes the computed boolean result straight
> through. That way the expression is evaluated only once.

I think we've been here before - doesn't this lead us to the problem where
AnalysisAssumeTrue cannot be constexpr but must be constexpr?

I think using false to avoid double evaluation (perhaps with a comment
explaining why, because it is way too subtle) is necessary.

[Kevin M, 2017-03-18 00:10:35]

> I think we've been here before - doesn't this lead us to the problem where
AnalysisAssumeTrue cannot be constexpr but must be constexpr?

The problem I encountered before was due to the fact that I had written
variations on AnalysisAssumeTrue to accept all types, constnesses, &c. for the
argument. That complexity's gone with the cast-to-boolean approach. :)

MSVC objects to using CHECK functions inside the bodies of constexpr functions,
but it looks like you already #ifdef'd those cases out of existence for Windows
builds, e.g.
https://cs.chromium.org/chromium/src/base/time/time.h?rcl=8e63e7d33496a458953...
.

[brucedawson, 2017-03-20 23:29:23]

On 2017/03/18 00:10:35, Kevin M wrote:
> > I think we've been here before - doesn't this lead us to the problem where
> AnalysisAssumeTrue cannot be constexpr but must be constexpr?
> 
> The problem I encountered before was due to the fact that I had written
> variations on AnalysisAssumeTrue to accept all types, constnesses, &c. for the
> argument. That complexity's gone with the cast-to-boolean approach. :)

That was one of the problems, but I was referring to the failures we hit
previously when AnalyzerAssumeTrue had a bool arg. At that time it was calling
AnalyzerNoReturn on all platforms and VC++ did not like that. Now that you are
using __analysis_assume on VC++ it may be okay to mark the function as
constexpr, thus avoiding all limitations.

> MSVC objects to using CHECK functions inside the bodies of constexpr
functions,
> but it looks like you already #ifdef'd those cases out of existence for
Windows
> builds, e.g.
>
https://cs.chromium.org/chromium/src/base/time/time.h?rcl=8e63e7d33496a458953...

The internal compiler error which that code works around has been fixed in VS
2017 and we would like to delete it at some point. Requiring that DCHECK not be
used in constexpr functions will force us to retain that workaround and will
introduce future problems as we start marking far more functions as constexpr.
On the other hand, some quick testing shows that VS 2017 can handle
__analysis_assume in constexpr functions, so the constexpr restriction is
temporary anyway.

I was going to suggest this for Windows:

    #define ANALYZER_ASSUME_TRUE(arg) (__analysis_assume(!!(arg)), false)

It avoids double-evaluations of |arg|, and it avoids any restrictions on where
DCHECK can be used. It passes my crude testing (including compiling
..\..\media\capture\content\animated_content_sampler.cc^^ with VC++ 2017 with
the time.h DCHECK re-enabled).

But I'm now inclined to suggest this:

#if _MSC_VER >= 1910 // VC++ 2017 allows __analysis_assume inside constexpr
functions
inline constexpr bool AnalyzerAssumeTrue(bool arg) {
  __analysis_assume(arg);
  return arg;
}
#else
inline bool AnalyzerAssumeTrue(bool arg) {
  __analysis_assume(arg);
  return arg;
}
#endif

It's irrelevant for now, but it will save us from re-learning this if/when we
transition, and it will make removing the compiler-bug workarounds easier.

[Kevin M, 2017-03-23 01:00:39]

Done. Will verify it on MSVS 2k7 when the Microsoft account SMS verification
starts working again.

[brucedawson, 2017-03-23 01:07:34]

On 2017/03/23 01:00:39, Kevin M wrote:
> Done. Will verify it on MSVS 2k7 when the Microsoft account SMS verification
> starts working again.

You can also cherry-pick the vs_toolchain.py change from crrev.com/2762093003 so
that you can use VS 2017 from depot_tools. This is a preliminary package, but it
does work. Note that you may need to build/config/compiler/BUILD.gn change as
well, or else "treat_warnings_as_errors = false", since there is one outstanding
warning left.

You probably don't want the landmine...

[Kevin M, 2017-04-13 22:43:31]

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

[commit-bot: I haz the power, 2017-04-13 22:44:05]

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

[Kevin M, 2017-04-13 22:54:14]

Description was changed from

==========
Add static analysis path control to asserts in logging.h

This CL adds static analysis hooks to all asserts in logging.h. For 
builds with static analysis enabled, ANALYSIS_ASSUME_TRUE generates
code which tells the analyzer to stop evaluating the paths which are
asserted to be impossible. For example, DCHECK(ptr) declares that 'ptr'
cannot be null, so the static analyzer will only consider scenarios
where 'ptr' is non-null.

Separate implementations of ANALYSIS_ASSUME_TRUE are provided for Clang
and MSVC PREfast cases which have subtly different semantics. For
builds in which there is no static analyzer present,
ANALYSIS_ASSUME_TRUE() is a no-op which simply passes the input value
straight through.

This CL also merges the analysis/non-analysis variants of assertion
functions, which is made possible by the no-op implementation of
ANALYSIS_ASSUME_TRUE. The analysis-only implementations of
CHECK/DCHECK/etc. are deleted.


R=brucedawson@chromium.org,danakj@chromium.org
BUG=327707
==========

to

==========
Add static analysis path control to asserts in logging.h

This CL adds static analysis hooks to all asserts in logging.h. For 
builds with static analysis enabled, ANALYSIS_ASSUME_TRUE generates
code which tells the analyzer to stop evaluating the paths which are
asserted to be impossible. For example, DCHECK(ptr) declares that 'ptr'
cannot be null, so the static analyzer will only consider scenarios
where 'ptr' is non-null.

Separate implementations of ANALYSIS_ASSUME_TRUE are provided for Clang
and MSVC PREfast cases which have subtly different semantics. For
builds in which there is no static analyzer present,
ANALYSIS_ASSUME_TRUE() is a no-op which simply passes the input value
straight through.

This CL also merges the analysis/non-analysis variants of our assert
functions, which is made possible by the no-op implementation of
ANALYSIS_ASSUME_TRUE. The analysis-only implementations of
CHECK/DCHECK/etc. are deleted.


R=brucedawson@chromium.org,danakj@chromium.org
BUG=327707
==========

[commit-bot: I haz the power, 2017-04-13 22:58:24]

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

[commit-bot: I haz the power, 2017-04-13 22:58:25]

Dry run: Try jobs failed on following builders:
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)

[Kevin M, 2017-04-13 23:55:56]

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

[commit-bot: I haz the power, 2017-04-13 23:56:16]

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

[commit-bot: I haz the power, 2017-04-14 00:27:24]

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

[commit-bot: I haz the power, 2017-04-14 00:27:25]

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

[Kevin M, 2017-04-18 18:27:12]

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

[commit-bot: I haz the power, 2017-04-18 18:27:32]

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

[commit-bot: I haz the power, 2017-04-18 20:25:57]

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

[commit-bot: I haz the power, 2017-04-18 20:26:00]

Dry run: This issue passed the CQ dry run.

[Kevin M, 2017-04-19 22:13:16]

It appears that merging MSVC and Clang's approaches into a single macro in a way
that doesn't produce more error noise is impossible.

I've rewritten the CL a number of times to make our various compilers happy, but
the latest obstacle I've run into is that MSVC complains when a comma operator
w/o side effects is used inside a test expression context (error C6319).
Unfortunately this is the only bit of syntax that I know of which will let me
invoke a void function inside a conditional expr.

I'm going to rewrite this CL to only add Clang analysis hooks, keeping the
existing PREfast logging implementations as they are today w/no regressions.

[Kevin M, 2017-04-19 23:41:08]

Description was changed from

==========
Add static analysis path control to asserts in logging.h

This CL adds static analysis hooks to all asserts in logging.h. For 
builds with static analysis enabled, ANALYSIS_ASSUME_TRUE generates
code which tells the analyzer to stop evaluating the paths which are
asserted to be impossible. For example, DCHECK(ptr) declares that 'ptr'
cannot be null, so the static analyzer will only consider scenarios
where 'ptr' is non-null.

Separate implementations of ANALYSIS_ASSUME_TRUE are provided for Clang
and MSVC PREfast cases which have subtly different semantics. For
builds in which there is no static analyzer present,
ANALYSIS_ASSUME_TRUE() is a no-op which simply passes the input value
straight through.

This CL also merges the analysis/non-analysis variants of our assert
functions, which is made possible by the no-op implementation of
ANALYSIS_ASSUME_TRUE. The analysis-only implementations of
CHECK/DCHECK/etc. are deleted.


R=brucedawson@chromium.org,danakj@chromium.org
BUG=327707
==========

to

==========
Add Clang static analysis control to all assert functions in logging.h

This CL adds static analysis hints to all assertion functions in logging.h. On
builds with static analysis enabled, ANALYSIS_ASSUME_TRUE generates code which
prevents the analyzer from evaluating codepaths that are asserted to be
impossible. 


R=brucedawson@chromium.org,danakj@chromium.org
BUG=327707
==========

[Kevin M, 2017-04-19 23:43:23]

danakj, brucedawson: PTAL

The PREfast logging function variants are back. I verified that the analysis
build output is consistent with what's in ToT.

I updated the CL description to reflect the fact that this is a Clang-only
improvement.

[Kevin M, 2017-04-20 00:23:04]

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

[commit-bot: I haz the power, 2017-04-20 00:23:53]

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

[commit-bot: I haz the power, 2017-04-20 02:25:45]

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

[commit-bot: I haz the power, 2017-04-20 02:25:45]

Dry run: This issue passed the CQ dry run.

[danakj, 2017-04-20 19:25:31]

I'll give it a stamp when bruce is happy

[brucedawson, 2017-04-20 20:51:09]

lgtm

[danakj, 2017-04-20 20:51:53]

LGTM

[Kevin M, 2017-04-20 20:55:30]

The CQ bit was checked by kmarshall@chromium.org

[commit-bot: I haz the power, 2017-04-20 20:56:13]

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-04-20 21:05:31]

CQ is committing da patch.

Bot data: {"patchset_id": 330001, "attempt_start_ts": 1492721730683230,
"parent_rev": "2269e30d4ea0874704bfbd4021c087ba960e7ea5", "commit_rev":
"fe2f09f858b77a7bd9437ef3b377e9af7f50f62d"}

[commit-bot: I haz the power, 2017-04-20 21:05:43]

Description was changed from

==========
Add Clang static analysis control to all assert functions in logging.h

This CL adds static analysis hints to all assertion functions in logging.h. On
builds with static analysis enabled, ANALYSIS_ASSUME_TRUE generates code which
prevents the analyzer from evaluating codepaths that are asserted to be
impossible. 


R=brucedawson@chromium.org,danakj@chromium.org
BUG=327707
==========

to

==========
Add Clang static analysis control to all assert functions in logging.h

This CL adds static analysis hints to all assertion functions in logging.h. On
builds with static analysis enabled, ANALYSIS_ASSUME_TRUE generates code which
prevents the analyzer from evaluating codepaths that are asserted to be
impossible. 


R=brucedawson@chromium.org,danakj@chromium.org
BUG=327707

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

[commit-bot: I haz the power, 2017-04-20 21:05:44]

Committed patchset #18 (id:330001) as
https://chromium.googlesource.com/chromium/src/+/fe2f09f858b77a7bd9437ef3b377...