Differential D72982
[Clang] Un-break scan-build after integrated-cc1 change Authored by aganea on Jan 18 2020, 12:21 PM.
Details The issue was reported by @xazax.hun here. "This patch breaks scan-build-py which parses the output of "-###" to get -cc1 command. There might be other tools with the same problems. Could we either remove (in-process) from CC1Command::Print or add a line break? Having the last line as a valid invocation is valuable and there might be tools relying on that."
Diff Detail
Event TimelineComment Actions Do you have commit access or need someone to commit on your behalf? Also, in case your change made it into the clang 10 release branch this will need to be cherry picked there as well. Comment Actions Wait, do we really want the "(in-process)" marker to be written to a separate line? I'm not sure that we do. The change description doesn't explain how scan-build was broken or how this fixes it. And, maybe most importantly, it seems scan-build doesn't have any tests that catches this? Should it have one? I've put it on my clang 10 todo list, but I'm also not sure this is the right fix, see above. Comment Actions Since the -### command had this property of emitting copy pastable -cc1 invocations I would be surprised if scan-build would be the only tool/script to rely on this. Whatever fix we come up with I think it would be great to maintain this property.
The fact that scan-build does not have a lit-test that is (un)broken by this change is definitely a bug and we should fix this. Unfortunately, no one had time to do so yet.
Thanks! Comment Actions Do you see value in keeping "(in-process)" at all? I added it in the first place for testing.
Fixed in the summary above. Should I revert the patch and recommit with a proper description/proper fix? Comment Actions The way I see it, the purpose of -### was only ever for debugging and testing. If tools are parsing it, they need to deal with changes to the format. The cc1 interface is also not stable, so such tools should probably be used to adapting. I think in this case, it would be better to fix scan-build to handle the prefix. Also, doesn't it need to do that anyway if it's printed on a separate line? Yes, I think it's good for testing and debugging, which is the purpose of -###.
I think we should revert the patch and fix scan-build instead. Comment Actions Due to some unfortunate historical reasons fixing scan-build means fixing 3 separate code bases. We have the original scan-build written in perl, a python rewrite both checked-in in the LLVM repository, and on github. And the two versions are not the same as the author lost interest upstreaming it halfway through. Since the github version is available in pip (and it has a relatively large number of downloads), we are likely to have customers from all three sources. That being said it is not unreasonable to fix all of them it is just a bit more complicated than committing one more patch to the LLVM repository. In case it is printed on a separate line the current parsing happens to work in all versions of scan-build and this seemed to be an easier way forward because of the reasons I described above (no need to push patches to 3rd party github repository to keep users happy). Comment Actions
Sigh, what a mess.
Okay, let's keep it on a separate line then. Comment Actions I totally agree and sorry for that :(
Thanks! Alternatively we could try to push the changes to all three versions and revert this patch once all of them are accepted and a new pip package is published. Comment Actions @xazax.hun In that case do you think it'd be possible for scan-build to handle a more structured format like a CDB? (and inherently make clang emit that) Comment Actions In case you refer to compilation database, scan-build-py already does handle that. The cc1 command line is used to avoid duplicating driver logic in scan-build. So sometimes, when scan-build want to peak the cc1 commands it will get it using -### instead of trying to infer them from the (driver) command that is available in the compilation database. Comment Actions I think the point is to provide a machine-readable alternative to -### - that would, say, write down one line of compilation database. The alternative solution would be to teach scan-build to wrap its extra flags with -Xclang, which is how it should have behaved to begin with. I'm not sure we're actually removing any -cc1 flags. Comment Actions We're seeing Driver/cc-print-options.c test failures after this change: ******************** TEST 'Clang :: Driver/cc-print-options.c' FAILED ********************
Script:
--
: 'RUN: at line 1'; env CC_PRINT_OPTIONS=1 CC_PRINT_OPTIONS_FILE=/b/s/w/ir/k/recipe_cleanup/clangpfY9av/llvm_build_dir/tools/clang/test/Driver/Output/cc-print-options.c.tmp.log /b/s/w/ir/k/recipe_cleanup/clangpfY9av/llvm_build_dir/bin/clang -no-canonical-prefixes -S -o /b/s/w/ir/k/recipe_cleanup/clangpfY9av/llvm_build_dir/tools/clang/test/Driver/Output/cc-print-options.c.tmp.s /b/s/w/ir/k/llvm-project/clang/test/Driver/cc-print-options.c
: 'RUN: at line 4'; /b/s/w/ir/k/recipe_cleanup/clangpfY9av/llvm_build_dir/bin/FileCheck /b/s/w/ir/k/llvm-project/clang/test/Driver/cc-print-options.c < /b/s/w/ir/k/recipe_cleanup/clangpfY9av/llvm_build_dir/tools/clang/test/Driver/Output/cc-print-options.c.tmp.log
--
Exit Code: 1
Command Output (stderr):
--
/b/s/w/ir/k/llvm-project/clang/test/Driver/cc-print-options.c:6:11: error: CHECK: expected string not found in input
// CHECK: [Logging clang options]{{.*}}clang{{.*}}"-S"
^
<stdin>:1:1: note: scanning from here
[Logging clang options] (in-process)
^Comment Actions I've pushed that to the 10.x branch in 54acc20e6da529f8ab6183d912a75a94b25d2bd9. Thanks! |