Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2617283002:
Add Clang static analyzer to Clang toolchain defs in GN (Closed)
DescriptionAdd Clang static analyzer support to Clang toolchain defs in GN.
If "use_clang_static_analyzer" is enabled in args.gn, Clang builds will redirect compilation to use Clang's ccc-analyzer and cxx-analyzer for static analysis.
Review-Url: https://codereview.chromium.org/2617283002
Cr-Commit-Position: refs/heads/master@{#445854}
Committed: https://chromium.googlesource.com/chromium/src/+/520f95147c821e13003362194a5de003a9f5c8f9
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : gni comment #
Total comments: 10
Patch Set 4 : wez feedback #
Total comments: 1
Patch Set 5 : wez feedback #Patch Set 6 : Modified Mac toolchain gni #Patch Set 7 : rebase #Patch Set 8 : . #
Total comments: 6
Patch Set 9 : sylvain feedback #
Total comments: 23
Patch Set 10 : wez/thakis feedback #
Total comments: 2
Patch Set 11 : wez feedback #
Total comments: 4
Patch Set 12 : thakis feedback #
Total comments: 1
Patch Set 13 : rebase #
Messages
Total messages: 41 (19 generated)
Description was changed from ========== (WIP) Add Clang static analyzer to Clang toolchain defs in GN ========== to ========== Add Clang static analyzer support to Clang toolchain defs in GN. If "use_clang_static_analyzer" is enabled in args.gn, Clang builds will redirect compilation to use Clang's ccc-analyzer and cxx-analyzer for static analysis. ==========
kmarshall@chromium.org changed reviewers: + sdefresne@chromium.org, thakis@chromium.org
wez@chromium.org changed reviewers: + wez@chromium.org
https://codereview.chromium.org/2617283002/diff/40001/build/toolchain/clang_s... File build/toolchain/clang_static_analyzer_wrapper.py (right): https://codereview.chromium.org/2617283002/diff/40001/build/toolchain/clang_s... build/toolchain/clang_static_analyzer_wrapper.py:37: if args.clang_cxx_path != None: Should we assert that clang_cc_path is None if so? I'd suggest assigning to e.g. clang_command here, so that you'd have: is_cxx = (args.clang_cxx_path != None) clang_command = args.clang_cxx_path or args.clang_cc_path Then most of the logic below can be expressed in terms of clang_command, with just a couple of conditionals on is_cxx. https://codereview.chromium.org/2617283002/diff/40001/build/toolchain/clang_s... build/toolchain/clang_static_analyzer_wrapper.py:52: clang_path = args.clang_cc_path typo cc -> cxx https://codereview.chromium.org/2617283002/diff/40001/build/toolchain/clang_s... build/toolchain/clang_static_analyzer_wrapper.py:66: command = [args.clang_cxx_path] + compile_args You don't seem to use |command|? https://codereview.chromium.org/2617283002/diff/40001/build/toolchain/clang_s... build/toolchain/clang_static_analyzer_wrapper.py:67: return run_command(args.clang_cxx_path, compile_args) Why not have a single run_command that uses clang_path? That said, it looks like you're simultaneously assuming that clang_cc/cxx_path are directory paths for Clang, and that they're the full paths including the actually command - which is it? Should you be using dirname() when you sassign to env['CLANG'], for instance? https://codereview.chromium.org/2617283002/diff/40001/build/toolchain/gcc_too... File build/toolchain/gcc_toolchain.gni (right): https://codereview.chromium.org/2617283002/diff/40001/build/toolchain/gcc_too... build/toolchain/gcc_toolchain.gni:502: root_build_dir) As noted in the script itself, if we make it a script with two generic parameters e.g. --compiler and --analyzer with a flag to indicate explicitly whether it's CC or CXX, might be cleaner.
https://codereview.chromium.org/2617283002/diff/60001/build/toolchain/gcc_too... File build/toolchain/gcc_toolchain.gni (right): https://codereview.chromium.org/2617283002/diff/60001/build/toolchain/gcc_too... build/toolchain/gcc_toolchain.gni:490: if (use_clang_static_analyzer) { You'll also need to add this to build/toolchain/mac/BUILD.gn as it does not use gcc_toolchain template.
https://codereview.chromium.org/2617283002/diff/40001/build/toolchain/clang_s... File build/toolchain/clang_static_analyzer_wrapper.py (right): https://codereview.chromium.org/2617283002/diff/40001/build/toolchain/clang_s... build/toolchain/clang_static_analyzer_wrapper.py:37: if args.clang_cxx_path != None: On 2017/01/10 02:41:48, Wez wrote: > Should we assert that clang_cc_path is None if so? Did something different - moved the one-of validation to the top to clean up the logic a bit. > > I'd suggest assigning to e.g. clang_command here, so that you'd have: > > is_cxx = (args.clang_cxx_path != None) > clang_command = args.clang_cxx_path or args.clang_cc_path > > Then most of the logic below can be expressed in terms of clang_command, with > just a couple of conditionals on is_cxx. https://codereview.chromium.org/2617283002/diff/40001/build/toolchain/clang_s... build/toolchain/clang_static_analyzer_wrapper.py:52: clang_path = args.clang_cc_path On 2017/01/10 02:41:48, Wez wrote: > typo cc -> cxx Done. https://codereview.chromium.org/2617283002/diff/40001/build/toolchain/clang_s... build/toolchain/clang_static_analyzer_wrapper.py:66: command = [args.clang_cxx_path] + compile_args On 2017/01/10 02:41:48, Wez wrote: > You don't seem to use |command|? Done. https://codereview.chromium.org/2617283002/diff/40001/build/toolchain/clang_s... build/toolchain/clang_static_analyzer_wrapper.py:67: return run_command(args.clang_cxx_path, compile_args) On 2017/01/10 02:41:48, Wez wrote: > Why not have a single run_command that uses clang_path? Need a fallback for the time being - there were a couple files that failed analysis as a workaround for this minor bug in ccc-analyzer (observed when building third_party/libxml). We could potentially send a fix pull request but there's no need to block on it right now: Use of uninitialized value $HtmlDir in concatenation (.) or string at ../../third_party/llvm/tools/clang/tools/scan-build/libexec/ccc-analyzer line 150, <$fh> line 7. mkdir /failures: Permission denied at ../../third_party/llvm/tools/clang/tools/scan-build/libexec/ccc-analyzer line 151. WARNING: static analysis failed with return code 13.Traceback (most recent call last): > That said, it looks like you're simultaneously assuming that clang_cc/cxx_path > are directory paths for Clang, and that they're the full paths including the > actually command - which is it? I don't understand - they are treated as fully qualified paths to the clang executable. The list concatenation that I'm performing prior to running a command is just following Python convention for joining command line arguments. Instead of using spaces in a string as a delimiter, it breaks the arguments up as discrete list items.
The CQ bit was checked by kmarshall@chromium.org to run a CQ dry run
Sylvain, can you check if this works for you on your Mac?
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: 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-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) 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_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
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: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
With the suggested changes, I'm able to build (with use_goma = false) and I see clang static analyzer printed on the standard error. Are they also saved to disk as html so that I can use scan-view? If so where are they? https://codereview.chromium.org/2617283002/diff/140001/build/toolchain/mac/BU... File build/toolchain/mac/BUILD.gn (left): https://codereview.chromium.org/2617283002/diff/140001/build/toolchain/mac/BU... build/toolchain/mac/BUILD.gn:173: command = "$env_wrapper $cc -MMD -MF $depfile {{defines}} {{include_dirs}} {{cflags}} {{cflags_c}} -c {{source}} -o {{output}}" You need to restore "$env_wrapper" here otherwise the hermetic build will be broken. https://codereview.chromium.org/2617283002/diff/140001/build/toolchain/mac/BU... File build/toolchain/mac/BUILD.gn (right): https://codereview.chromium.org/2617283002/diff/140001/build/toolchain/mac/BU... build/toolchain/mac/BUILD.gn:23: You need to add import of //build/toolchain/clang_static_analyzer.gni here. https://codereview.chromium.org/2617283002/diff/140001/build/toolchain/mac/BU... build/toolchain/mac/BUILD.gn:126: if (use_clang_static_analyzer) { I tried to run it with goma enabled and it got stuck. Maybe you want to assert that toolchain_uses_goma == false here.
I haven't yet been able to replicate the report summarization and ephemeral output directory behavior in GN/py that we get "for free" with scan-build. I figured that I should prioritize stdout output first then tackle the html problem later.
On 2017/01/11 18:23:54, Kevin M wrote: > I haven't yet been able to replicate the report summarization and ephemeral > output directory behavior in GN/py that we get "for free" with scan-build. I > figured that I should prioritize stdout output first then tackle the html > problem later. Sound good to me. LG with suggested modifications (import and restoring env_wrapper).
https://codereview.chromium.org/2617283002/diff/140001/build/toolchain/mac/BU... File build/toolchain/mac/BUILD.gn (left): https://codereview.chromium.org/2617283002/diff/140001/build/toolchain/mac/BU... build/toolchain/mac/BUILD.gn:173: command = "$env_wrapper $cc -MMD -MF $depfile {{defines}} {{include_dirs}} {{cflags}} {{cflags_c}} -c {{source}} -o {{output}}" On 2017/01/11 16:49:11, sdefresne wrote: > You need to restore "$env_wrapper" here otherwise the hermetic build will be > broken. Done. https://codereview.chromium.org/2617283002/diff/140001/build/toolchain/mac/BU... File build/toolchain/mac/BUILD.gn (right): https://codereview.chromium.org/2617283002/diff/140001/build/toolchain/mac/BU... build/toolchain/mac/BUILD.gn:23: On 2017/01/11 16:49:11, sdefresne wrote: > You need to add import of //build/toolchain/clang_static_analyzer.gni here. Done. https://codereview.chromium.org/2617283002/diff/140001/build/toolchain/mac/BU... build/toolchain/mac/BUILD.gn:126: if (use_clang_static_analyzer) { On 2017/01/11 16:49:11, sdefresne wrote: > I tried to run it with goma enabled and it got stuck. Maybe you want to assert > that toolchain_uses_goma == false here. Done.
This looks nice. https://codereview.chromium.org/2617283002/diff/160001/base/logging.h File base/logging.h (right): https://codereview.chromium.org/2617283002/diff/160001/base/logging.h#newcode732 base/logging.h:732: // otherwise false positive errors may be generated by null pointer checks. this is getting reviewed separately, right? https://codereview.chromium.org/2617283002/diff/160001/build/toolchain/clang_... File build/toolchain/clang_static_analyzer.gni (right): https://codereview.chromium.org/2617283002/diff/160001/build/toolchain/clang_... build/toolchain/clang_static_analyzer.gni:5: # Defines the configuration of Clang static analysis tools. Can you a) update https://chromium.googlesource.com/chromium/src/+/master/docs/clang_static_ana... with the current state of the world (make sure to add a disclaimer that it's in early bring-up up as of early 2017) b) add a link to that in a comment here? ("# See docs/clang_static_analyzer.md", or the link I pasted) https://codereview.chromium.org/2617283002/diff/160001/build/toolchain/clang_... File build/toolchain/clang_static_analyzer_wrapper.py (right): https://codereview.chromium.org/2617283002/diff/160001/build/toolchain/clang_... build/toolchain/clang_static_analyzer_wrapper.py:22: parser.add_argument('--clang-cc-path', can't the script hardcode that to script_dir/../../third_party/llvm-build/Release+Asserts/bin/clang? But after reading the rest of the CL, I actually think what you have is better, so please ignore. https://codereview.chromium.org/2617283002/diff/160001/build/toolchain/gcc_to... File build/toolchain/gcc_toolchain.gni (right): https://codereview.chromium.org/2617283002/diff/160001/build/toolchain/gcc_to... build/toolchain/gcc_toolchain.gni:499: "//third_party/llvm/tools/clang/tools/scan-build/libexec/ccc-analyzer", We shouldn't check in stuff referring to third_party/llvm. That will make people think they should have that, and almost everyone shouldn't have that. Can we DEPS in just the scan-build scripts somewhere and refer to that?
https://codereview.chromium.org/2617283002/diff/40001/build/toolchain/clang_s... File build/toolchain/clang_static_analyzer_wrapper.py (right): https://codereview.chromium.org/2617283002/diff/40001/build/toolchain/clang_s... build/toolchain/clang_static_analyzer_wrapper.py:67: return run_command(args.clang_cxx_path, compile_args) On 2017/01/10 19:12:27, Kevin M wrote: > On 2017/01/10 02:41:48, Wez wrote: > > Why not have a single run_command that uses clang_path? > > Need a fallback for the time being - there were a couple files that failed > analysis as a workaround for this minor bug in ccc-analyzer (observed when > building third_party/libxml). We could potentially send a fix pull request but > there's no need to block on it right now: > > Use of uninitialized value $HtmlDir in concatenation (.) or string at > ../../third_party/llvm/tools/clang/tools/scan-build/libexec/ccc-analyzer line > 150, <$fh> line 7. > mkdir /failures: Permission denied at > ../../third_party/llvm/tools/clang/tools/scan-build/libexec/ccc-analyzer line > 151. > WARNING: static analysis failed with return code 13.Traceback (most recent call > last): > > > That said, it looks like you're simultaneously assuming that clang_cc/cxx_path > > are directory paths for Clang, and that they're the full paths including the > > actually command - which is it? > > I don't understand - they are treated as fully qualified paths to the clang > executable. The list concatenation that I'm performing prior to running a > command is just following Python convention for joining command line arguments. > Instead of using spaces in a string as a delimiter, it breaks the arguments up > as discrete list items. My comment was based on the fact that you were setting an env variable CLANG to either clang_cxx_path or clang_cc, which given that the env variable was not language specific implied it expected a directory path, not a filename. In part I think the confusion is in the _path suffix, which is ambiguous in meaning but which in the context of running shell commands I would tend to think of as the directory in which the command resides, not the full path to the command. https://codereview.chromium.org/2617283002/diff/160001/base/logging.h File base/logging.h (right): https://codereview.chromium.org/2617283002/diff/160001/base/logging.h#newcode732 base/logging.h:732: // otherwise false positive errors may be generated by null pointer checks. On 2017/01/11 20:20:05, Nico wrote: > this is getting reviewed separately, right? Rather than have duplicate D[P]CHECK definitions, could we instead define e.g. a LOGGING_NORETURN(command) macro that depends on __clang_analyzer__, to keep the analyzer changes more localized? i.e.: #if __clang_analyzer__ #define LOGGING_NORETURN(command) (AnalyzerNoReturn(), command) #else #define LOGGING_NORETURN(command) command #endif https://codereview.chromium.org/2617283002/diff/160001/build/toolchain/clang_... File build/toolchain/clang_static_analyzer_wrapper.py (right): https://codereview.chromium.org/2617283002/diff/160001/build/toolchain/clang_... build/toolchain/clang_static_analyzer_wrapper.py:36: assert ((args.clang_cc_path != None) or (args.clang_cxx_path != None)) Isn't this implicit from the != case? If they were both None then != check would fail. https://codereview.chromium.org/2617283002/diff/160001/build/toolchain/clang_... build/toolchain/clang_static_analyzer_wrapper.py:45: env['CLANG_CXX'] = clang_path This differs from the previous version, where you set CLANG - is CLANG_CXX the correct form? https://codereview.chromium.org/2617283002/diff/160001/build/toolchain/clang_... build/toolchain/clang_static_analyzer_wrapper.py:50: temp_dir = tempfile.mkdtemp() Do we want to be dumping stuff in temp files, rather than in some part of the output directory? Or is this purely for intermediate files that the command needs to create? https://codereview.chromium.org/2617283002/diff/160001/build/toolchain/clang_... build/toolchain/clang_static_analyzer_wrapper.py:55: sys.stderr.write(stderr) nit: It seems strange to "capture command stderr", capturing the return code, and stderr output, only to return the return code and write the stderr output to... stderr. :P Is this doing something else valuable?
https://codereview.chromium.org/2617283002/diff/160001/base/logging.h File base/logging.h (right): https://codereview.chromium.org/2617283002/diff/160001/base/logging.h#newcode732 base/logging.h:732: // otherwise false positive errors may be generated by null pointer checks. On 2017/01/11 20:20:05, Nico wrote: > this is getting reviewed separately, right? Yeah, I had patched it in to verify that it is WAI. upstream-branch was giving me errors on git-cl upload so the changes on that CL got into this diff too. I'll remove the patch. https://codereview.chromium.org/2617283002/diff/160001/base/logging.h#newcode732 base/logging.h:732: // otherwise false positive errors may be generated by null pointer checks. On 2017/01/14 00:04:58, Wez wrote: > On 2017/01/11 20:20:05, Nico wrote: > > this is getting reviewed separately, right? > > Rather than have duplicate D[P]CHECK definitions, could we instead define e.g. a > LOGGING_NORETURN(command) macro that depends on __clang_analyzer__, to keep the > analyzer changes more localized? i.e.: > > #if __clang_analyzer__ > #define LOGGING_NORETURN(command) (AnalyzerNoReturn(), command) > #else > #define LOGGING_NORETURN(command) command > #endif Will address this in the other CL. https://codereview.chromium.org/2617283002/diff/160001/build/toolchain/clang_... File build/toolchain/clang_static_analyzer.gni (right): https://codereview.chromium.org/2617283002/diff/160001/build/toolchain/clang_... build/toolchain/clang_static_analyzer.gni:5: # Defines the configuration of Clang static analysis tools. On 2017/01/11 20:20:05, Nico wrote: > Can you > > a) update > https://chromium.googlesource.com/chromium/src/+/master/docs/clang_static_ana... > with the current state of the world (make sure to add a disclaimer that it's in > early bring-up up as of early 2017) > > b) add a link to that in a comment here? ("# See docs/clang_static_analyzer.md", > or the link I pasted) Done. https://codereview.chromium.org/2617283002/diff/160001/build/toolchain/clang_... File build/toolchain/clang_static_analyzer_wrapper.py (right): https://codereview.chromium.org/2617283002/diff/160001/build/toolchain/clang_... build/toolchain/clang_static_analyzer_wrapper.py:22: parser.add_argument('--clang-cc-path', On 2017/01/11 20:20:05, Nico wrote: > can't the script hardcode that to > script_dir/../../third_party/llvm-build/Release+Asserts/bin/clang? But after > reading the rest of the CL, I actually think what you have is better, so please > ignore. Acknowledged. https://codereview.chromium.org/2617283002/diff/160001/build/toolchain/clang_... build/toolchain/clang_static_analyzer_wrapper.py:36: assert ((args.clang_cc_path != None) or (args.clang_cxx_path != None)) On 2017/01/14 00:04:58, Wez wrote: > Isn't this implicit from the != case? If they were both None then != check > would fail. Both of them being set is also a failure case. https://codereview.chromium.org/2617283002/diff/160001/build/toolchain/clang_... build/toolchain/clang_static_analyzer_wrapper.py:45: env['CLANG_CXX'] = clang_path On 2017/01/14 00:04:58, Wez wrote: > This differs from the previous version, where you set CLANG - is CLANG_CXX the > correct form? Yes. https://codereview.chromium.org/2617283002/diff/160001/build/toolchain/clang_... build/toolchain/clang_static_analyzer_wrapper.py:50: temp_dir = tempfile.mkdtemp() On 2017/01/14 00:04:58, Wez wrote: > Do we want to be dumping stuff in temp files, rather than in some part of the > output directory? Or is this purely for intermediate files that the command > needs to create? This is for unsummarized output files. We have some reworking to do to replicate the summarization behavior of scan-build in a GN-friendly way. It's typically run as a post-build step after the Makefile has finished its evaluation, which doesn't map to the granularity of how GN executes its rules. https://codereview.chromium.org/2617283002/diff/160001/build/toolchain/clang_... build/toolchain/clang_static_analyzer_wrapper.py:55: sys.stderr.write(stderr) On 2017/01/14 00:04:58, Wez wrote: > nit: It seems strange to "capture command stderr", capturing the return code, > and stderr output, only to return the return code and write the stderr output > to... stderr. :P Is this doing something else valuable? I think this is the only way to get output to stderr with subprocess.popen - the only other options are to redirect it to stdout, or to suppress it altogether. https://codereview.chromium.org/2617283002/diff/160001/build/toolchain/gcc_to... File build/toolchain/gcc_toolchain.gni (right): https://codereview.chromium.org/2617283002/diff/160001/build/toolchain/gcc_to... build/toolchain/gcc_toolchain.gni:499: "//third_party/llvm/tools/clang/tools/scan-build/libexec/ccc-analyzer", On 2017/01/11 20:20:06, Nico wrote: > We shouldn't check in stuff referring to third_party/llvm. That will make people > think they should have that, and almost everyone shouldn't have that. Can we > DEPS in just the scan-build scripts somewhere and refer to that? Done! third_party/scan-build
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.
Ping
LGTM w/ nits - thanks for doing this, Kevin; looks awesome. https://codereview.chromium.org/2617283002/diff/160001/build/toolchain/clang_... File build/toolchain/clang_static_analyzer_wrapper.py (right): https://codereview.chromium.org/2617283002/diff/160001/build/toolchain/clang_... build/toolchain/clang_static_analyzer_wrapper.py:36: assert ((args.clang_cc_path != None) or (args.clang_cxx_path != None)) On 2017/01/18 23:04:27, Kevin M wrote: > On 2017/01/14 00:04:58, Wez wrote: > > Isn't this implicit from the != case? If they were both None then != check > > would fail. > > Both of them being set is also a failure case. But if both of them are set (or not-set) then (a != None) == (b != None), which will fail the first assert, won't it? For a Boolean value, != is effectively xor. https://codereview.chromium.org/2617283002/diff/160001/build/toolchain/clang_... build/toolchain/clang_static_analyzer_wrapper.py:50: temp_dir = tempfile.mkdtemp() On 2017/01/18 23:04:25, Kevin M wrote: > On 2017/01/14 00:04:58, Wez wrote: > > Do we want to be dumping stuff in temp files, rather than in some part of the > > output directory? Or is this purely for intermediate files that the command > > needs to create? > > This is for unsummarized output files. We have some reworking to do to replicate > the summarization behavior of scan-build in a GN-friendly way. It's typically > run as a post-build step after the Makefile has finished its evaluation, which > doesn't map to the granularity of how GN executes its rules. Acknowledged. https://codereview.chromium.org/2617283002/diff/160001/build/toolchain/clang_... build/toolchain/clang_static_analyzer_wrapper.py:55: sys.stderr.write(stderr) On 2017/01/18 23:04:27, Kevin M wrote: > On 2017/01/14 00:04:58, Wez wrote: > > nit: It seems strange to "capture command stderr", capturing the return code, > > and stderr output, only to return the return code and write the stderr output > > to... stderr. :P Is this doing something else valuable? > > I think this is the only way to get output to stderr with subprocess.popen - the > only other options are to redirect it to stdout, or to suppress it altogether. According to the documentation the default behavious of subprocess.popen() is for the child to inherit the stdio handles of the parent. Failing that, the call does allow the files to output to to be specified explicitly as parameters. https://codereview.chromium.org/2617283002/diff/180001/docs/clang_static_anal... File docs/clang_static_analyzer.md (right): https://codereview.chromium.org/2617283002/diff/180001/docs/clang_static_anal... docs/clang_static_analyzer.md:6: As of early 2017, we have early support for the Clang static analysis tool in nit: Reads oddly to have two "early"s in here; maybe change the second to "experimental" ;) https://codereview.chromium.org/2617283002/diff/180001/docs/clang_static_anal... docs/clang_static_analyzer.md:17: Then run `gn clean` and rebuild your targets. You should see static analysis nit: Is it necessary to run gn clean? Shouldn't the change in command-lines cause ninja to rebuild everything anyway?
https://codereview.chromium.org/2617283002/diff/160001/build/toolchain/clang_... File build/toolchain/clang_static_analyzer_wrapper.py (right): https://codereview.chromium.org/2617283002/diff/160001/build/toolchain/clang_... build/toolchain/clang_static_analyzer_wrapper.py:36: assert ((args.clang_cc_path != None) or (args.clang_cxx_path != None)) On 2017/01/21 01:10:15, Wez wrote: > On 2017/01/18 23:04:27, Kevin M wrote: > > On 2017/01/14 00:04:58, Wez wrote: > > > Isn't this implicit from the != case? If they were both None then != check > > > would fail. > > > > Both of them being set is also a failure case. > > But if both of them are set (or not-set) then (a != None) == (b != None), which > will fail the first assert, won't it? For a Boolean value, != is effectively > xor. Done. https://codereview.chromium.org/2617283002/diff/160001/build/toolchain/clang_... build/toolchain/clang_static_analyzer_wrapper.py:55: sys.stderr.write(stderr) On 2017/01/21 01:10:15, Wez wrote: > On 2017/01/18 23:04:27, Kevin M wrote: > > On 2017/01/14 00:04:58, Wez wrote: > > > nit: It seems strange to "capture command stderr", capturing the return > code, > > > and stderr output, only to return the return code and write the stderr > output > > > to... stderr. :P Is this doing something else valuable? > > > > I think this is the only way to get output to stderr with subprocess.popen - > the > > only other options are to redirect it to stdout, or to suppress it altogether. > > According to the documentation the default behavious of subprocess.popen() is > for the child to inherit the stdio handles of the parent. Failing that, the > call does allow the files to output to to be specified explicitly as parameters. Done.
Friendly ping
Still looks good, but one question: https://codereview.chromium.org/2617283002/diff/200001/build/toolchain/gcc_co... File build/toolchain/gcc_compile_wrapper.py (right): https://codereview.chromium.org/2617283002/diff/200001/build/toolchain/gcc_co... build/toolchain/gcc_compile_wrapper.py:31: used_resources = wrapper_utils.ExtractResourceIdsFromPragmaWarnings(stderr) isn't this broken now? this needs stderr? https://codereview.chromium.org/2617283002/diff/200001/docs/clang_static_anal... File docs/clang_static_analyzer.md (right): https://codereview.chromium.org/2617283002/diff/200001/docs/clang_static_anal... docs/clang_static_analyzer.md:8: to stdout along with other compiler errors at build time. stdout or stderr?
https://codereview.chromium.org/2617283002/diff/200001/build/toolchain/gcc_co... File build/toolchain/gcc_compile_wrapper.py (right): https://codereview.chromium.org/2617283002/diff/200001/build/toolchain/gcc_co... build/toolchain/gcc_compile_wrapper.py:31: used_resources = wrapper_utils.ExtractResourceIdsFromPragmaWarnings(stderr) On 2017/01/24 21:41:33, Nico wrote: > isn't this broken now? this needs stderr? Done. https://codereview.chromium.org/2617283002/diff/200001/docs/clang_static_anal... File docs/clang_static_analyzer.md (right): https://codereview.chromium.org/2617283002/diff/200001/docs/clang_static_anal... docs/clang_static_analyzer.md:8: to stdout along with other compiler errors at build time. On 2017/01/24 21:41:33, Nico wrote: > stdout or stderr? Done.
lgtm https://codereview.chromium.org/2617283002/diff/220001/DEPS File DEPS (right): https://codereview.chromium.org/2617283002/diff/220001/DEPS#newcode304 DEPS:304: nit: no additional newline here
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/2617283002/#ps240001 (title: "rebase")
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": 240001, "attempt_start_ts": 1485294756194210,
"parent_rev": "0f5c7c7e52c0983aaa111f7a6ccc7003ad737fad", "commit_rev":
"520f95147c821e13003362194a5de003a9f5c8f9"}
Message was sent while issue was closed.
Description was changed from ========== Add Clang static analyzer support to Clang toolchain defs in GN. If "use_clang_static_analyzer" is enabled in args.gn, Clang builds will redirect compilation to use Clang's ccc-analyzer and cxx-analyzer for static analysis. ========== to ========== Add Clang static analyzer support to Clang toolchain defs in GN. If "use_clang_static_analyzer" is enabled in args.gn, Clang builds will redirect compilation to use Clang's ccc-analyzer and cxx-analyzer for static analysis. Review-Url: https://codereview.chromium.org/2617283002 Cr-Commit-Position: refs/heads/master@{#445854} Committed: https://chromium.googlesource.com/chromium/src/+/520f95147c821e13003362194a5d... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001) as https://chromium.googlesource.com/chromium/src/+/520f95147c821e13003362194a5d... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
