Page MenuHomePhabricator

[Clang] Un-break scan-build after integrated-cc1 change
ClosedPublic

Authored by aganea on Jan 18 2020, 12:21 PM.

Details

Summary

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 Timeline

aganea created this revision.Jan 18 2020, 12:21 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 18 2020, 12:21 PM
xazax.hun accepted this revision.Jan 19 2020, 9:30 AM

LGTM, thanks!

This revision is now accepted and ready to land.Jan 19 2020, 9:30 AM

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.

This revision was automatically updated to reflect the committed changes.
hans added a comment.Jan 21 2020, 9:31 AM

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?

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.

I've put it on my clang 10 todo list, but I'm also not sure this is the right fix, see above.

xazax.hun added a comment.EditedJan 21 2020, 9:40 AM

Wait, do we really want the "(in-process)" marker to be written to a separate line? I'm not sure that we do.

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 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?

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.

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.

I've put it on my clang 10 todo list, but I'm also not sure this is the right fix, see above.

Thanks!

aganea edited the summary of this revision. (Show Details)Jan 21 2020, 9:45 AM

Wait, do we really want the "(in-process)" marker to be written to a separate line? I'm not sure that we do.

Do you see value in keeping "(in-process)" at all? I added it in the first place for testing.

The change description doesn't explain how scan-build was broken or how this fixes it.

Fixed in the summary above. Should I revert the patch and recommit with a proper description/proper fix?

hans added a comment.Jan 21 2020, 10:19 AM

Wait, do we really want the "(in-process)" marker to be written to a separate line? I'm not sure that we do.

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 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?

Wait, do we really want the "(in-process)" marker to be written to a separate line? I'm not sure that we do.

Do you see value in keeping "(in-process)" at all? I added it in the first place for testing.

Yes, I think it's good for testing and debugging, which is the purpose of -###.

The change description doesn't explain how scan-build was broken or how this fixes it.

Fixed in the summary above. Should I revert the patch and recommit with a proper description/proper fix?

I think we should revert the patch and fix scan-build instead.

Wait, do we really want the "(in-process)" marker to be written to a separate line? I'm not sure that we do.

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 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?

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).

hans added a comment.Jan 21 2020, 11:09 AM

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.

Sigh, what a mess.

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).

Okay, let's keep it on a separate line then.

Sigh, what a mess.

I totally agree and sorry for that :(

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).

Okay, let's keep it on a separate line then.

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.

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.

@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)

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.

@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)

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.

NoQ added a comment.Jan 21 2020, 11:58 AM

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.

@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)

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.

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.

phosek added a subscriber: phosek.Jan 21 2020, 12:42 PM

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)
^

We're seeing Driver/cc-print-options.c test failures after this change:

Yes, seeing it too, I will commit a fix in a minute!

We're seeing Driver/cc-print-options.c test failures after this change:

Yes, seeing it too, I will commit a fix in a minute!

Fixed by rG133a7e631cee97965e310f0d110739217427fd3d

hans added a comment.Jan 22 2020, 9:34 AM

We're seeing Driver/cc-print-options.c test failures after this change:

Yes, seeing it too, I will commit a fix in a minute!

Fixed by rG133a7e631cee97965e310f0d110739217427fd3d

I've pushed that to the 10.x branch in 54acc20e6da529f8ab6183d912a75a94b25d2bd9. Thanks!

NoQ added a comment.Jan 23 2020, 8:46 AM

Thank you everybody for unbreaking us! 🙏