Issue 648210 has been merged into this issue.

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

commit e23eed07b41b99be905bffbbcadb0d761c40eb7d
Author: kmarshall <kmarshall@chromium.org>
Date: Sat Feb 11 02:13:23 2017

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}

[modify] https://crrev.com/e23eed07b41b99be905bffbbcadb0d761c40eb7d/base/logging.h

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

commit 549cad5d1141989cc3cac9ae7161984b40ae5607
Author: kmarshall <kmarshall@chromium.org>
Date: Thu Feb 23 02:30:37 2017

Use universal references in templated analysis passthrough functions.

The ANALYSIS_ASSUME_TRUE macros must support lvalues and rvalues without
copying. Currently we are taking arguments by-value; this does not work
for things like null checks on unique_ptr (DCHECK(my_unique_ptr)).
Taking a non-const reference doesn't work either, as one cannot take
a reference to an rvalue parameter. Multiple definitions for by-val
and by-ref don't work, because the choice between TVal and TVal&
is too ambiguous for the compiler.

Universal references work nicely in this case because they allow lvalue and rvalue parameters to be taken without making copies or the need for adding overloads.

Also added ANALYSIS_ASSUME_TRUE to DCHECK_OP implementation bodies.
This prevents the static analyzer from proceeding if a comparison-
based DCHECK fails, which is a static assertion that the asserted
condition will always hold.

R=danakj@chromium.org,wez@chromium.org
BUG=687243

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

[modify] https://crrev.com/549cad5d1141989cc3cac9ae7161984b40ae5607/base/logging.h

This somehow managed to break /analyze on VC++:

d:\src\chromium\src\courgette\adjustment_method_2.cc(1114): error C2664: 'TVal logging::AnalysisAssumeTrue<uint32_t&>(TVal)': cannot convert argument 1 from 'uint32_t' to 'unsigned int &'
        with
        [
            TVal=uint32_t &
        ]

Oh joy. I can probably find a workaround, and it's not critical anyway (we'll drop /analyze at some point) but I'm going to investigate.

The bug happens with VC++ 2017 as well, for reasons I don't understand. For now I want to fix this bug by skipping the call to AnalysisAssumeTrue. I've got a cl created here:

https://codereview.chromium.org/2720453003

From https://codereview.chromium.org/2720453003/:

On 2017/02/27 19:59:05, brucedawson wrote:
> Also, I found out what types cause the issue - bit fields. A universal reference
> to a bit field does seem a bit odd - does clang really handle this? Details
> here:
> 
> https://bugs.chromium.org/p/chromium/issues/detail?id=427616#c97

Uh - does this code actually build for clang? 'cause I can't get it to compile for clang. I just tested on Linux using this minimal repro of the 'bug':

#include <utility>

template <typename TVal>
inline constexpr TVal&& TestUniversalReference(TVal&& arg) {
  return std::forward<TVal>(arg);
}

struct TestStruct {
  unsigned is_model_ : 1;
};

void TestFunction(TestStruct* p) {
  TestUniversalReference(p->is_model_);
}

and this compile command line:

clang-3.5 -std=c++14 test.cpp

The error message is much better than VC++'s, but it is an error message regardless:

test.cpp:13:26: error: non-const reference cannot bind to bit-field 'is_model_'
  TestUniversalReference(p->is_model_);
                         ^~~~~~~~~~~~
test.cpp:9:12: note: bit-field is declared here
  unsigned is_model_ : 1;
           ^

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

commit 08c892f77027cc70fd4a26bb218e88fe1e30161e
Author: kmarshall <kmarshall@chromium.org>
Date: Tue Feb 28 03:46:18 2017

Roll back ANALYSIS_ASSUME_TRUE() until bitfield support is developed

The definition of AnalysisAssumeTrue() does not support bitfield
parameter types. This CL unblocks the Windows team from running
PREfast-analyze by rolling back the functionality. It will be relanded
when bitfields are supported.

BUG=687243
CC=brucedawson@chromium.org.
TBR=danakj@chromium.org

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

[modify] https://crrev.com/08c892f77027cc70fd4a26bb218e88fe1e30161e/base/logging.h

I have a pending fix on the LLVM project which eliminates errors generated by system headers (-isystem, -isysroot): https://reviews.llvm.org/D30593

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

commit 5847e4bdf3b65ccc136bc42ef00d573f2adb50fb
Author: kmarshall <kmarshall@chromium.org>
Date: Thu Mar 16 21:39:51 2017

Add static analysis support to Win Clang builds

This CL enables static analysis support for Clang builds on Windows.
Developers can opt-in to receiving analysis text by setting
"use_clang_static_analyzer = true" in their GN build
args. The solution works with Goma.

* Added flags to Clang portions of build/toolchain/win/BUILD.gn.
* Added options to the Analysis wrapper script to handle clang-cl.exe's
flag style.

BUG=687243

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

[modify] https://crrev.com/5847e4bdf3b65ccc136bc42ef00d573f2adb50fb/build/toolchain/clang_static_analyzer_wrapper.py
[modify] https://crrev.com/5847e4bdf3b65ccc136bc42ef00d573f2adb50fb/build/toolchain/gcc_toolchain.gni
[modify] https://crrev.com/5847e4bdf3b65ccc136bc42ef00d573f2adb50fb/build/toolchain/win/BUILD.gn

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

commit d10bb7f7a2f052d8240079b43572c9c5124a20c3
Author: kmarshall <kmarshall@chromium.org>
Date: Wed Apr 26 02:55:41 2017

Simplify ALLOW_UNUSED_LOCAL to use (void)x directly instead of conditionally.

Add ALLOW_UNUSED_LOCAL implementation for path-sensitive analyzers.
The current definition of ALLOW_UNUSED_LOCAL uses "(void)x" to suppress
warnings that "x" was never used, but places the expression in a
never-executed codepath of a ternary expression, forcing this
statement to be a no-op.

Static analyzers which are codepath sensitive, like Clang's scan-build,
will only trace along the no-op codepath and therefore will never
evaluate the voidification clause. The result is a lot of warning noise
like this:

"warning: Value stored to 'x' during its initialization is never read"

This CL removes the ternary expression from ALLOW_UNUSED_LOCAL so that the voidification statement is evaluated by path sensitive checkers. The build size was not affected by this change, therefore it's reasonable to assume that this won't have an effect on runtime behavior.

R=pkasting@chromium.org
CC=wez@chromium.org
BUG=687243

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

[modify] https://crrev.com/d10bb7f7a2f052d8240079b43572c9c5124a20c3/base/compiler_specific.h

Are we still interested in this idea? There's still a "Linux Clang Analyzer" bot on chromium.fyi, but I'm not sure if anyone is looking at it?

IMO yes we should use it (IIRC when we first enabled it it found some
genuine bugs, and was less noisy that alternatives), but we'll need someone
to nudge things along, using the FYI bot output to help maintainers take
action and whittle down the issues.

I am now actively working on getting the clang-tidy static analyzer running on Chrome in ChromeOS (and play nicely with the compiler), and getting scripts to parse the results.  Is anybody else working on this, or should I take over this issue?  (If I don't get a response in a day or so, I will take this over).

We've created infrastructure to run analyses like this and add annotations to code reviews (goto.google.com/tricium) but haven't hooked up clang-tidy. Please talk to the tricium folks so you don't duplicate work.

Re #17: FWIW you can find docs on the current analyzer at https://chromium.googlesource.com/chromium/src/%2B/lkcr/docs/clang_static_analyzer.md and the FYI bot at https://ci.chromium.org/buildbot/chromium.fyi/Linux%20Clang%20Analyzer/

Depending on the direction & progress on tricium, we should either migrate that bot to use clang-tidy, or turn it down.

I'm not sure if tricium is appropriate for my work -- I'm looking at doing this work strictly for Chrome in ChromeOS, using our ChromeOS infrastructure.  If you want to do this outside of ChromeOS, maybe someone else should own this bug, and I'll create a new one for my work in ChromeOS?

The bots' existence is a workaround for the fact that the analyzer is an all-or-nothing setting; either you have to build and analyze everything, or you build nothing at all.

Having regular runs on the bot lets you ctrl-F through the build logs to spot check the files you're interested in. Clang-tidy lets you analyze individual .cc files, doesn't it? If so, then that makes having precomputed analysis logs a little less compelling, because developers can easily analyze just their code on their workstations.

Yes, clang-tidy allows you to analyze individual .cc files.  But although developers *can* run clang-tidy (if they can figure out how to build and invoke it), that doesn't mean very many of them do.

In ChromeOS we would like to ideally set up a builder that runs clang-tidy on an entire build of Chrome regularly, generating a single large log file containing all the warnings, which we would run through some parsing scripts to parse and sort the warnings.  We might even do something like the Android team does, and set up a dashboard where you can examine each week's warnings and see how they have changed from week to week...

Again, these plans may be different from what you had in mind when you set up this issue, in which case I should start a new one?

I think those goals sounds pretty good, especially if you have the time and resources to carry them across the finish line. Are you planning on adding presubmits, too?

The effort probably deserves a bug of its own, with this bug closed out and marked as Fixed or Duplicate.

Re comment 22: I think in our (chrome's) experience, static analyzers are too noisy for this to work. We had the setup you describe several times with several static analyzers, and due to false positives, people eventually stopped looking. Google-internally, tricorder finally made static analyzers work by making them add comments at code review time (public paper: https://research.google.com/pubs/pub43322.html). Tricium (more details at go/tricium-paper) aims to bring this known-better-working approach to chrome.

If you want, you're welcome to set up clang-tidy the way you suggest of course, but past experience has shown that this approach doesn't work super well. People will look at the reports for a month or two and then stop. Hence, my suggestion to work with the tricium folks (maruel/qyearsley).

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/ab55d8b8d2023aa083fd5d06221869d2807125a9

commit ab55d8b8d2023aa083fd5d06221869d2807125a9
Author: Caroline Tice <cmtice@google.com>
Date: Sat Mar 10 01:39:02 2018

Add 'clang_tidy' USE flag to chrome ebulid.

This CL adds a 'clang_tidy' USE flag to the
chromeos-chrome ebuild, and builds chrome with clang_tidy when
the use flag is invoked.

BUG=chromium:687243
TEST=emerge sys-devel/llvm (files are installed properly)
     emerge chrome with USE=clang_tidy => build log has tidy
warnings
     emerge chrome without USE=clang_tidy => normal emerge

Change-Id: Ia6d4fd08c307fdb8b84e1238d21d6075ca0989c4
Reviewed-on: https://chromium-review.googlesource.com/956665
Commit-Ready: Caroline Tice <cmtice@chromium.org>
Tested-by: Caroline Tice <cmtice@chromium.org>
Reviewed-by: Yunlian Jiang <yunlian@chromium.org>

[modify] https://crrev.com/ab55d8b8d2023aa083fd5d06221869d2807125a9/chromeos-base/chromeos-chrome/metadata.xml
[modify] https://crrev.com/ab55d8b8d2023aa083fd5d06221869d2807125a9/chromeos-base/chromeos-chrome/chromeos-chrome-9999.ebuild

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/49a774f5c80fff16ff464ba0bffd433cd5fd10a5

commit 49a774f5c80fff16ff464ba0bffd433cd5fd10a5
Author: Caroline Tice <cmtice@google.com>
Date: Sat Mar 10 06:59:18 2018

Add ability to parse clang-tidy warning logs.

This CL adds the scripts necessary to parse clang-tidy warnings
from build logs.

BUG=chromium:687243
TEST=emerge sys-devel/llvm (files are installed properly)
     emerge chrome with USE=clang_tidy => build log has tidy
warnings
     emerge chrome without USE=clang_tidy => normal emerge

Change-Id: I9581531e26fe68027f6ff812e56ff109546eb2a2
Reviewed-on: https://chromium-review.googlesource.com/924752
Commit-Ready: Caroline Tice <cmtice@chromium.org>
Tested-by: Caroline Tice <cmtice@chromium.org>
Reviewed-by: Manoj Gupta <manojgupta@chromium.org>

[rename] https://crrev.com/49a774f5c80fff16ff464ba0bffd433cd5fd10a5/sys-devel/llvm/llvm-6.0_pre321490_p20180131-r8.ebuild
[add] https://crrev.com/49a774f5c80fff16ff464ba0bffd433cd5fd10a5/sys-devel/llvm/files/clang-tidy-parse-build-log.py
[add] https://crrev.com/49a774f5c80fff16ff464ba0bffd433cd5fd10a5/sys-devel/llvm/files/clang_tidy_execute.py
[add] https://crrev.com/49a774f5c80fff16ff464ba0bffd433cd5fd10a5/sys-devel/llvm/files/clang-tidy-warn.py

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/chromite/+/907239157e6a16f20a6587c9457af9b0fa144341

commit 907239157e6a16f20a6587c9457af9b0fa144341
Author: Caroline Tice <cmtice@google.com>
Date: Fri Mar 30 23:53:01 2018

Add new builder and builder stages for clang-tidy warnings.

This adds a new waterfall builder, with the appropriate new stages,
for generating clang-tidy warnings for chrome in the build logs,
parsing the build logs to generate warnings.{html,csv} files,
generating a tarball of the warnings files and uploading them to
gs.

BUG=chromium:687243
TEST=In the process of testing.

Change-Id: Id9c161aaaef999125fb454fc2537ea16ceb9ba4b
Reviewed-on: https://chromium-review.googlesource.com/985139
Commit-Ready: Caroline Tice <cmtice@chromium.org>
Tested-by: Caroline Tice <cmtice@chromium.org>
Reviewed-by: Caroline Tice <cmtice@chromium.org>

[add] https://crrev.com/907239157e6a16f20a6587c9457af9b0fa144341/scripts/cros_generate_tidy_warnings.py
[modify] https://crrev.com/907239157e6a16f20a6587c9457af9b0fa144341/cbuildbot/chromeos_config.py
[modify] https://crrev.com/907239157e6a16f20a6587c9457af9b0fa144341/cbuildbot/config_dump.json
[modify] https://crrev.com/907239157e6a16f20a6587c9457af9b0fa144341/cbuildbot/waterfall_layout_dump.txt
[modify] https://crrev.com/907239157e6a16f20a6587c9457af9b0fa144341/cbuildbot/stages/artifact_stages.py
[add] https://crrev.com/907239157e6a16f20a6587c9457af9b0fa144341/cbuildbot/builders/clang_tidy_builders.py

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

commit 030f1ce40fe87b00a7b1d16817579a17c3e2ace8
Author: chromite-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com <chromite-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Date: Sat Mar 31 01:34:21 2018

Roll src/third_party/chromite/ 36a9c890b..907239157 (1 commit)

https://chromium.googlesource.com/chromiumos/chromite.git/+log/36a9c890b979..907239157e6a

$ git log 36a9c890b..907239157 --date=short --no-merges --format='%ad %ae %s'
2018-03-28 cmtice Add new builder and builder stages for clang-tidy warnings.

Created with:
  roll-dep src/third_party/chromite
BUG=chromium:687243


The AutoRoll server is located here: https://chromite-chromium-roll.skia.org

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC'd on the roll, and stop the roller if necessary.


TBR=chrome-os-gardeners@chromium.org

Change-Id: I51cccba654c5dd6ae667a49b2e6794017c5d9c24
Reviewed-on: https://chromium-review.googlesource.com/989113
Reviewed-by: Chromite Chromium Autoroll <chromite-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Commit-Queue: Chromite Chromium Autoroll <chromite-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#547351}
[modify] https://crrev.com/030f1ce40fe87b00a7b1d16817579a17c3e2ace8/DEPS