Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2667853004:
Remove dependency on scan-build wrapper script for Clang analysis builds. (Closed)
DescriptionRemove dependency on scan-build wrapper script for analysis builds.
This CL replaces the Clang 'c++-analyzer' Perl script with logic added to the Python wrapper script. This gives us the hooks we need to run analysis builds with "gomacc" (necessary for distributed builds) and gives us a convenient place to specify analyzer config flags which aren't exposed by scan-build.
Adds a warning suppression rule which prevents the analyzer from raising false positives from stdlib code (code w/namespace "std").
Other changes:
* Modify Goma portions of the GCC toolchain GNI to build paths to
the Clang static analyzer.
* Create an exception for ASM tool invocations, which don't play
well with the analysis command line flags.
* Removed 'scan-build' DEP.
BUG=687245
R=thakis@chromium.org,wez@chromium.org
Review-Url: https://codereview.chromium.org/2667853004
Cr-Commit-Position: refs/heads/master@{#456136}
Committed: https://chromium.googlesource.com/chromium/src/+/6694c0e5e6727606e83542d3d471f3967fdb0128
Patch Set 1 #Patch Set 2 : rebase #Patch Set 3 : Redo buggy rebase #Patch Set 4 : Cleanup #Patch Set 5 : Removed goma TODO from docs #
Total comments: 22
Patch Set 6 : wez feedback #
Total comments: 8
Patch Set 7 : wez feedback #Patch Set 8 : fixed bool type coercion bug in GNI #
Total comments: 4
Patch Set 9 : Added stdlib error suppression flag. #
Total comments: 7
Patch Set 10 : thakis feedback #Patch Set 11 : r3b453 #Patch Set 12 : WIP clang on windows #
Messages
Total messages: 39 (18 generated)
Friendly ping. I have another CL out for review shortly that implements file whitelisting, which depends on this CL.
The CQ bit was checked by kmarshall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by kmarshall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Note that the "Goma requires multiple parameters to work" bit of the CL description was not very clear, IMO. https://codereview.chromium.org/2667853004/diff/80001/build/toolchain/clang_s... File build/toolchain/clang_static_analyzer_wrapper.py (right): https://codereview.chromium.org/2667853004/diff/80001/build/toolchain/clang_s... build/toolchain/clang_static_analyzer_wrapper.py:6: """Adds a analysis build step to invocations of the Clang C/C++ compiler. nit: "an" https://codereview.chromium.org/2667853004/diff/80001/build/toolchain/clang_s... build/toolchain/clang_static_analyzer_wrapper.py:20: "--analyze", nit: Not a biggie, but the other .py files in this directory use ' rather than " https://codereview.chromium.org/2667853004/diff/80001/build/toolchain/clang_s... build/toolchain/clang_static_analyzer_wrapper.py:45: assert len(args) > 0 nit: Just |assert args| here? https://codereview.chromium.org/2667853004/diff/80001/build/toolchain/clang_s... build/toolchain/clang_static_analyzer_wrapper.py:52: list(itertools.chain(*zip(["-Xanalyzer"] * len(analyzer_option_flags), Can we line-wrap after "list(" to avoid the need for \ line continuation? https://codereview.chromium.org/2667853004/diff/80001/build/toolchain/clang_s... build/toolchain/clang_static_analyzer_wrapper.py:53: analyzer_option_flags))) I think you can write this more simply, e.g: reduce(lambda x, y: x + ["-Xanalyzer", y], analyzer_option_flags, []) or list(itertools.chain(zip(itertools.repeat("-Xanalyzer"), analyzer_options_flags))) https://codereview.chromium.org/2667853004/diff/80001/build/toolchain/clang_s... build/toolchain/clang_static_analyzer_wrapper.py:57: interleaved_analyzer_flags)) What happens to any stdout output here? https://codereview.chromium.org/2667853004/diff/80001/build/toolchain/clang_s... build/toolchain/clang_static_analyzer_wrapper.py:59: # Post-process the analysis output and remove any errors for excluded files. This should be a TODO? https://codereview.chromium.org/2667853004/diff/80001/build/toolchain/gcc_too... File build/toolchain/gcc_toolchain.gni (right): https://codereview.chromium.org/2667853004/diff/80001/build/toolchain/gcc_too... build/toolchain/gcc_toolchain.gni:134: if (toolchain_uses_goma) { Should we be asserting that toolchain_args.cc_wrapper isn't defined, here? https://codereview.chromium.org/2667853004/diff/80001/build/toolchain/gcc_too... build/toolchain/gcc_toolchain.gni:157: invoker.is_clang_analysis_supported)) { Could you move this check ahead of the toolchain_uses_goma block, and just modify the compiler_prefix and asm in the GOMA case? Basically it looks like compiler_prefix can start off empty, and the two options each add their parameters to it. https://codereview.chromium.org/2667853004/diff/80001/build/toolchain/nacl/BU... File build/toolchain/nacl/BUILD.gn (right): https://codereview.chromium.org/2667853004/diff/80001/build/toolchain/nacl/BU... build/toolchain/nacl/BUILD.gn:111: if (use_clang_static_analyzer) { nit: Why is this conditional on the analyzer being enabled? Surely it's not supported no matter what?
https://codereview.chromium.org/2667853004/diff/80001/build/toolchain/clang_s... File build/toolchain/clang_static_analyzer_wrapper.py (right): https://codereview.chromium.org/2667853004/diff/80001/build/toolchain/clang_s... build/toolchain/clang_static_analyzer_wrapper.py:6: """Adds a analysis build step to invocations of the Clang C/C++ compiler. On 2017/02/03 08:05:00, Wez wrote: > nit: "an" Done. https://codereview.chromium.org/2667853004/diff/80001/build/toolchain/clang_s... build/toolchain/clang_static_analyzer_wrapper.py:45: assert len(args) > 0 On 2017/02/03 08:05:00, Wez wrote: > nit: Just |assert args| here? Done. https://codereview.chromium.org/2667853004/diff/80001/build/toolchain/clang_s... build/toolchain/clang_static_analyzer_wrapper.py:52: list(itertools.chain(*zip(["-Xanalyzer"] * len(analyzer_option_flags), On 2017/02/03 08:05:00, Wez wrote: > Can we line-wrap after "list(" to avoid the need for \ line continuation? Done. https://codereview.chromium.org/2667853004/diff/80001/build/toolchain/clang_s... build/toolchain/clang_static_analyzer_wrapper.py:53: analyzer_option_flags))) On 2017/02/03 08:05:00, Wez wrote: > I think you can write this more simply, e.g: > > reduce(lambda x, y: x + ["-Xanalyzer", y], analyzer_option_flags, []) > > or > > list(itertools.chain(zip(itertools.repeat("-Xanalyzer"), > analyzer_options_flags))) Done. https://codereview.chromium.org/2667853004/diff/80001/build/toolchain/clang_s... build/toolchain/clang_static_analyzer_wrapper.py:57: interleaved_analyzer_flags)) On 2017/02/03 08:05:00, Wez wrote: > What happens to any stdout output here? Goes right to stdout. The stderr capturing feature comes in handy in the followup CL in which I filter the analysis output. https://codereview.chromium.org/2667853004/diff/80001/build/toolchain/clang_s... build/toolchain/clang_static_analyzer_wrapper.py:59: # Post-process the analysis output and remove any errors for excluded files. On 2017/02/03 08:05:00, Wez wrote: > This should be a TODO? Done. https://codereview.chromium.org/2667853004/diff/80001/build/toolchain/gcc_too... File build/toolchain/gcc_toolchain.gni (right): https://codereview.chromium.org/2667853004/diff/80001/build/toolchain/gcc_too... build/toolchain/gcc_toolchain.gni:134: if (toolchain_uses_goma) { Done - did a little restructuring here while I'm at it, ptal https://codereview.chromium.org/2667853004/diff/80001/build/toolchain/gcc_too... build/toolchain/gcc_toolchain.gni:157: invoker.is_clang_analysis_supported)) { On 2017/02/03 08:05:00, Wez wrote: > Could you move this check ahead of the toolchain_uses_goma block, and just > modify the compiler_prefix and asm in the GOMA case? > > Basically it looks like compiler_prefix can start off empty, and the two options > each add their parameters to it. Done. https://codereview.chromium.org/2667853004/diff/80001/build/toolchain/nacl/BU... File build/toolchain/nacl/BUILD.gn (right): https://codereview.chromium.org/2667853004/diff/80001/build/toolchain/nacl/BU... build/toolchain/nacl/BUILD.gn:111: if (use_clang_static_analyzer) { On 2017/02/03 08:05:01, Wez wrote: > nit: Why is this conditional on the analyzer being enabled? Surely it's not > supported no matter what? It's a dead store if analysis isn't enabled, which GN treats as errors.
The CQ bit was checked by kmarshall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2667853004/diff/80001/build/toolchain/clang_s... File build/toolchain/clang_static_analyzer_wrapper.py (right): https://codereview.chromium.org/2667853004/diff/80001/build/toolchain/clang_s... build/toolchain/clang_static_analyzer_wrapper.py:57: interleaved_analyzer_flags)) On 2017/02/03 18:46:29, Kevin M wrote: > On 2017/02/03 08:05:00, Wez wrote: > > What happens to any stdout output here? > > Goes right to stdout. The stderr capturing feature comes in handy in the > followup CL in which I filter the analysis output. Acknowledged. https://codereview.chromium.org/2667853004/diff/80001/build/toolchain/gcc_too... File build/toolchain/gcc_toolchain.gni (right): https://codereview.chromium.org/2667853004/diff/80001/build/toolchain/gcc_too... build/toolchain/gcc_toolchain.gni:134: if (toolchain_uses_goma) { On 2017/02/03 18:46:29, Kevin M wrote: > Done - did a little restructuring here while I'm at it, ptal This looks better, but isn't it the case that compiler_prefix always ends up with: analyzer+goma goma analyzer toolchain-specified wrapper | default wrapper So it feels like this is almost a pair of orthogonal configuration options, with a fall-back-to-default case? At least lets take the is-analysis-actually-enabled check and write the result to a local variable to make things more readable. :) https://codereview.chromium.org/2667853004/diff/80001/build/toolchain/nacl/BU... File build/toolchain/nacl/BUILD.gn (right): https://codereview.chromium.org/2667853004/diff/80001/build/toolchain/nacl/BU... build/toolchain/nacl/BUILD.gn:111: if (use_clang_static_analyzer) { On 2017/02/03 18:46:29, Kevin M wrote: > On 2017/02/03 08:05:01, Wez wrote: > > nit: Why is this conditional on the analyzer being enabled? Surely it's not > > supported no matter what? > > It's a dead store if analysis isn't enabled, which GN treats as errors. Aha - thanks for clarifying. :) https://codereview.chromium.org/2667853004/diff/100001/build/toolchain/clang_... File build/toolchain/clang_static_analyzer_wrapper.py (right): https://codereview.chromium.org/2667853004/diff/100001/build/toolchain/clang_... build/toolchain/clang_static_analyzer_wrapper.py:54: # excluded files. nit: Add a bug # :D https://codereview.chromium.org/2667853004/diff/100001/build/toolchain/gcc_to... File build/toolchain/gcc_toolchain.gni (right): https://codereview.chromium.org/2667853004/diff/100001/build/toolchain/gcc_to... build/toolchain/gcc_toolchain.gni:136: assert(!(toolchain_cc_wrapper != "" && toolchain_uses_goma != ""), nit: Do you need the !='s if you're doing &&? (Not sure what GN's type coercion rules are ;) https://codereview.chromium.org/2667853004/diff/100001/build/toolchain/wrappe... File build/toolchain/wrapper_utils.py (right): https://codereview.chromium.org/2667853004/diff/100001/build/toolchain/wrappe... build/toolchain/wrapper_utils.py:95: nit: Spurious extra line!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2667853004/diff/100001/build/toolchain/clang_... File build/toolchain/clang_static_analyzer_wrapper.py (right): https://codereview.chromium.org/2667853004/diff/100001/build/toolchain/clang_... build/toolchain/clang_static_analyzer_wrapper.py:54: # excluded files. On 2017/02/03 19:27:54, Wez wrote: > nit: Add a bug # :D Done. https://codereview.chromium.org/2667853004/diff/100001/build/toolchain/gcc_to... File build/toolchain/gcc_toolchain.gni (right): https://codereview.chromium.org/2667853004/diff/100001/build/toolchain/gcc_to... build/toolchain/gcc_toolchain.gni:136: assert(!(toolchain_cc_wrapper != "" && toolchain_uses_goma != ""), On 2017/02/03 19:27:55, Wez wrote: > nit: Do you need the !='s if you're doing &&? (Not sure what GN's type coercion > rules are ;) Done. https://codereview.chromium.org/2667853004/diff/100001/build/toolchain/wrappe... File build/toolchain/wrapper_utils.py (right): https://codereview.chromium.org/2667853004/diff/100001/build/toolchain/wrappe... build/toolchain/wrapper_utils.py:95: On 2017/02/03 19:27:55, Wez wrote: > nit: Spurious extra line! Done.
https://codereview.chromium.org/2667853004/diff/100001/build/toolchain/gcc_to... File build/toolchain/gcc_toolchain.gni (right): https://codereview.chromium.org/2667853004/diff/100001/build/toolchain/gcc_to... build/toolchain/gcc_toolchain.gni:136: assert(!(toolchain_cc_wrapper != "" && toolchain_uses_goma != ""), On 2017/02/06 17:39:56, Kevin M wrote: > On 2017/02/03 19:27:55, Wez wrote: > > nit: Do you need the !='s if you're doing &&? (Not sure what GN's type > coercion > > rules are ;) > > Done. Undone. Yes, you do need !=; GN doesn't take truthy types as operands
lgtm https://codereview.chromium.org/2667853004/diff/100001/build/toolchain/gcc_to... File build/toolchain/gcc_toolchain.gni (right): https://codereview.chromium.org/2667853004/diff/100001/build/toolchain/gcc_to... build/toolchain/gcc_toolchain.gni:136: assert(!(toolchain_cc_wrapper != "" && toolchain_uses_goma != ""), On 2017/02/06 18:44:18, Kevin M wrote: > On 2017/02/06 17:39:56, Kevin M wrote: > > On 2017/02/03 19:27:55, Wez wrote: > > > nit: Do you need the !='s if you're doing &&? (Not sure what GN's type > > coercion > > > rules are ;) > > > > Done. > > Undone. Yes, you do need !=; GN doesn't take truthy types as operands Acknowledged.
I think upstream also has a c++-analyzer python version. Maybe that works better (and if not, maybe we could try working with upstream to make it work better?)? I'm a bit queasy about not going through one of the upstream scripts, so I'd like to make sure there's a great reason to not do that before committing to this https://codereview.chromium.org/2667853004/diff/140001/build/toolchain/clang_... File build/toolchain/clang_static_analyzer_wrapper.py (right): https://codereview.chromium.org/2667853004/diff/140001/build/toolchain/clang_... build/toolchain/clang_static_analyzer_wrapper.py:34: '-analyzer-checker=cplusplus', Why this list and not a different one? Before, upstream got to pick what they thought was a good default set. https://codereview.chromium.org/2667853004/diff/140001/build/toolchain/clang_... build/toolchain/clang_static_analyzer_wrapper.py:60: # If it fails, we should recover gracefully and continue with the build. Huh? Shouldn't we report the crash upstream instead?
Hey Nico, I'm eager to land Goma support for the analyzer as it really lowers the friction to using the analyzer. My teammates told me that they won't really bother doing local builds until Goma is supported since the local builds are too slow. Upstreaming to the LLVM project seems like a worthy effort - and is an interesting task that I would be willing to do - but IMO it could be done as a parallel effort done at the same time the Chrome-specific support is landed. At the same time are puzzling out a solution that's generally useful and acceptable to the LLVM maintainers, Chrome developers could already be making performance and stability gains from bugs discovered via the analyzer. We can then migrate the interim solution to the LLVM official solution once that's landed. Please let me know if you have other concerns and we could discuss them over VC. :) Thanks! https://codereview.chromium.org/2667853004/diff/140001/build/toolchain/clang_... File build/toolchain/clang_static_analyzer_wrapper.py (right): https://codereview.chromium.org/2667853004/diff/140001/build/toolchain/clang_... build/toolchain/clang_static_analyzer_wrapper.py:34: '-analyzer-checker=cplusplus', On 2017/02/06 22:19:12, Nico wrote: > Why this list and not a different one? > > > Before, upstream got to pick what they thought was a good default set. This is the default list that's set by scan-build and scan-build-py, minus a few security checks for certain unsafe low level functions that aren't applicable to Chromium's codebase. Maintaining our own list makes sense if we want to add support for new checker plugins in the future. https://codereview.chromium.org/2667853004/diff/140001/build/toolchain/clang_... build/toolchain/clang_static_analyzer_wrapper.py:60: # If it fails, we should recover gracefully and continue with the build. On 2017/02/06 22:19:12, Nico wrote: > Huh? Shouldn't we report the crash upstream instead? Upstream yes, but as the crashes aren't actionable for users - they originate from the same Clang AST traversal crash - it would be better to track this as an LLVM bug and hide the noise from Chromium devs.
On 2017/02/08 19:41:34, Kevin M wrote: > Hey Nico, > > I'm eager to land Goma support for the analyzer as it really lowers the > friction to using the analyzer. My teammates told me that they won't really > bother doing local builds until Goma is supported since the local builds are too > slow. > > Upstreaming to the LLVM project seems like a worthy effort - and is an > interesting task that I would be willing to do - but IMO it could be done as a > parallel effort done at the same time the Chrome-specific support is landed. At > the same time are puzzling out a solution that's generally useful and acceptable > to the LLVM maintainers, Chrome developers could already be making performance > and stability gains from bugs discovered via the analyzer. We can then migrate > the interim solution to the LLVM official solution once that's landed. I feel there's about 0 reason to rush anything in. We should at least start a conversation with upstream. > > Please let me know if you have other concerns and we could discuss them over VC. > :) Thanks! > > https://codereview.chromium.org/2667853004/diff/140001/build/toolchain/clang_... > File build/toolchain/clang_static_analyzer_wrapper.py (right): > > https://codereview.chromium.org/2667853004/diff/140001/build/toolchain/clang_... > build/toolchain/clang_static_analyzer_wrapper.py:34: > '-analyzer-checker=cplusplus', > On 2017/02/06 22:19:12, Nico wrote: > > Why this list and not a different one? > > > > > > Before, upstream got to pick what they thought was a good default set. > > This is the default list that's set by scan-build and scan-build-py, minus a few > security checks for certain unsafe low level functions that aren't applicable to > Chromium's codebase. > > Maintaining our own list makes sense if we want to add support for new checker > plugins in the future. > > https://codereview.chromium.org/2667853004/diff/140001/build/toolchain/clang_... > build/toolchain/clang_static_analyzer_wrapper.py:60: # If it fails, we should > recover gracefully and continue with the build. > On 2017/02/06 22:19:12, Nico wrote: > > Huh? Shouldn't we report the crash upstream instead? > > Upstream yes, but as the crashes aren't actionable for users - they originate > from the same Clang AST traversal crash - it would be better to track this as an > LLVM bug and hide the noise from Chromium devs. We can just fix it upstream.
I think that it is reasonable to push for an iterative approach in this case, so we can sooner evaluate the errors in the codebase and look for areas for improvement in the analyzer. The solution is straightforward to implement, easy to revert, and the behavior is opt-in only; it doesn't affect existing builds. I'm not sure where to start WRT LLVM upstreaming, as I've never had to need to interact with the LLVM project before. If you have resources or points of contact I am happy to pursue that path. Perhaps it'll be a quicker process than I am anticipating. If it looks like the effort will be a significant time investment, however, I think it's prudent to land an interim solution like the one in this CL.
Friendly ping!
On the LLVM error front, I have removed the code that suppresses the error output. I have requested an LLVM bug account to report it, and am working with a local checkout of LLVM to try and pin down the issue. WDYT, does that work suffice for the purpose of unblocking this Goma CL?
Description was changed from ========== Enable Goma support for clang static analysis builds. This CL replaces the Clang 'c++-analyzer' Perl script with logic added to the Python wrapper script. Clang's Perl script was too inflexible in how it took compiler commands to support Goma. Goma requires multiple arguments to work, whereas the Clang script required a path or an executable in $PATH. Also covered: * Modify Goma portions of the GCC toolchain GNI to build paths to the Clang static analyzer. * Create an exception for ASM tool invocations, which don't play well with the analysis command line flags. * Removed 'scan-build' DEP. BUG=687245 R=thakis@chromium.org,wez@chromium.org ========== to ========== Remove dependency on scan-build wrapper script for analysis builds. This CL replaces the Clang 'c++-analyzer' Perl script with logic added to the Python wrapper script. This gives us the hooks we need to run analysis builds with "gomacc" (necessary for distributed builds) and gives us a convenient place to specify analyzer config flags which aren't exposed by scan-build. Adds a warning suppression rule which prevents the analyzer from raising false positives from stdlib code (code w/namespace "std"). Other changes: * Modify Goma portions of the GCC toolchain GNI to build paths to the Clang static analyzer. * Create an exception for ASM tool invocations, which don't play well with the analysis command line flags. * Removed 'scan-build' DEP. BUG=687245 R=thakis@chromium.org,wez@chromium.org ==========
PTAL. I updated the CL title and description to reflect the current state of the changes.
Cool, this basically lgtm. I feel the upstream discussion was very fruitful, so thank you for starting it :-) (Please reply on that review with a list of bugs filed for false positives to keep that conversation going) https://codereview.chromium.org/2667853004/diff/160001/build/toolchain/clang_... File build/toolchain/clang_static_analyzer_wrapper.py (right): https://codereview.chromium.org/2667853004/diff/160001/build/toolchain/clang_... build/toolchain/clang_static_analyzer_wrapper.py:44: assert(args) nit: assert args (no parens needed) https://codereview.chromium.org/2667853004/diff/160001/build/toolchain/clang_... build/toolchain/clang_static_analyzer_wrapper.py:51: lambda x, y: x + ['-Xanalyzer', y], analyzer_option_flags, []) sum(zip([1, 2, 3], [4, 5, 6]), ()) => maybe simpler to spell this sum(zip(['-Xanalyzer'] * len(analyzer_option_flags), analyzer_option_flags), ()) ? https://codereview.chromium.org/2667853004/diff/160001/build/toolchain/clang_... build/toolchain/clang_static_analyzer_wrapper.py:66: 'WARNING: Clang analyzer exited with return code %d.\n' % (returncode)) I still think we shouldn't have this in here. Please add a "TODO: fix once all crashes are fixed, crbug.com/XXX" and mention all known crashes there (and file llvm bugs for them and mention them in the crbug) https://codereview.chromium.org/2667853004/diff/160001/build/toolchain/gcc_to... File build/toolchain/gcc_toolchain.gni (right): https://codereview.chromium.org/2667853004/diff/160001/build/toolchain/gcc_to... build/toolchain/gcc_toolchain.gni:17: # GN is really picky about dead stores of variables except at the global scope. I think the usual workaround is to assert() on the dead variable to suppress that warning. Example: https://cs.chromium.org/chromium/src/BUILD.gn?type=cs&q=assert+file:%5C.gn+pa...
https://codereview.chromium.org/2667853004/diff/160001/build/toolchain/clang_... File build/toolchain/clang_static_analyzer_wrapper.py (right): https://codereview.chromium.org/2667853004/diff/160001/build/toolchain/clang_... build/toolchain/clang_static_analyzer_wrapper.py:44: assert(args) On 2017/03/10 01:55:02, Nico wrote: > nit: > assert args > > (no parens needed) Done. https://codereview.chromium.org/2667853004/diff/160001/build/toolchain/clang_... build/toolchain/clang_static_analyzer_wrapper.py:51: lambda x, y: x + ['-Xanalyzer', y], analyzer_option_flags, []) On 2017/03/10 01:55:02, Nico wrote: > sum(zip([1, 2, 3], [4, 5, 6]), ()) > > => maybe simpler to spell this > > sum(zip(['-Xanalyzer'] * len(analyzer_option_flags), analyzer_option_flags), ()) > > ? Done. https://codereview.chromium.org/2667853004/diff/160001/build/toolchain/clang_... build/toolchain/clang_static_analyzer_wrapper.py:66: 'WARNING: Clang analyzer exited with return code %d.\n' % (returncode)) On 2017/03/10 01:55:02, Nico wrote: > I still think we shouldn't have this in here. Please add a "TODO: fix once all > crashes are fixed, crbug.com/XXX" and mention all known crashes there (and file > llvm bugs for them and mention them in the crbug) There is the potential for new bugs and regressions to occur in the future. It makes sense to wave our arms and call attention to the crbug in the logging output so that Chromium developers know to take the appropriate action with the crbug. Not many people will be reading this script for comments. :)
The CQ bit was checked by kmarshall@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wez@chromium.org, thakis@chromium.org Link to the patchset: https://codereview.chromium.org/2667853004/#ps180001 (title: "thakis feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by kmarshall@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org, wez@chromium.org Link to the patchset: https://codereview.chromium.org/2667853004/#ps200001 (title: "r3b453")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1489169976581280,
"parent_rev": "c58e8f82526098f4ef3dca5f957a46c439540a59", "commit_rev":
"6694c0e5e6727606e83542d3d471f3967fdb0128"}
Message was sent while issue was closed.
Description was changed from ========== Remove dependency on scan-build wrapper script for analysis builds. This CL replaces the Clang 'c++-analyzer' Perl script with logic added to the Python wrapper script. This gives us the hooks we need to run analysis builds with "gomacc" (necessary for distributed builds) and gives us a convenient place to specify analyzer config flags which aren't exposed by scan-build. Adds a warning suppression rule which prevents the analyzer from raising false positives from stdlib code (code w/namespace "std"). Other changes: * Modify Goma portions of the GCC toolchain GNI to build paths to the Clang static analyzer. * Create an exception for ASM tool invocations, which don't play well with the analysis command line flags. * Removed 'scan-build' DEP. BUG=687245 R=thakis@chromium.org,wez@chromium.org ========== to ========== Remove dependency on scan-build wrapper script for analysis builds. This CL replaces the Clang 'c++-analyzer' Perl script with logic added to the Python wrapper script. This gives us the hooks we need to run analysis builds with "gomacc" (necessary for distributed builds) and gives us a convenient place to specify analyzer config flags which aren't exposed by scan-build. Adds a warning suppression rule which prevents the analyzer from raising false positives from stdlib code (code w/namespace "std"). Other changes: * Modify Goma portions of the GCC toolchain GNI to build paths to the Clang static analyzer. * Create an exception for ASM tool invocations, which don't play well with the analysis command line flags. * Removed 'scan-build' DEP. BUG=687245 R=thakis@chromium.org,wez@chromium.org Review-Url: https://codereview.chromium.org/2667853004 Cr-Commit-Position: refs/heads/master@{#456136} Committed: https://chromium.googlesource.com/chromium/src/+/6694c0e5e6727606e83542d3d471... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as https://chromium.googlesource.com/chromium/src/+/6694c0e5e6727606e83542d3d471... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
