This is an archive of the discontinued LLVM Phabricator instance.

Improvements to scan-build.
ClosedPublic

Authored by ayartsev on Dec 5 2014, 7:19 AM.

Details

Summary

Refactoring:
+ rename 'Options' to 'EnvVars' to better reflect the variables contents.
+ keep all scan-build arguments in the hash rather then in scattered variables.

All arguments were tested with the debugger. Please review!

Diff Detail

Event Timeline

ayartsev updated this revision to Diff 16987.Dec 5 2014, 7:19 AM
ayartsev retitled this revision from to Improvements to scan-build..
ayartsev updated this object.
ayartsev edited the test plan for this revision. (Show Details)
ayartsev added a reviewer: jordan_rose.
ayartsev added subscribers: zaks.anna, krememek, Unknown Object (MLST).
ayartsev updated this revision to Diff 22727.Mar 26 2015, 9:02 AM

+ refactoring: moved processing of command-line arguments to a subroutine.

Please review!

Anna, if you want, as I am a big user of scan-build, I can do the testing of this for you.

Anton,
Is all of this just a refactoring? I've noticed a couple of places where new functionality is being added. If that is the case, could you split that into a separate patch, explaining why that is needed.

Sylvestre,

Thank you for the offer! It would be really great if you could help us with testing this. Which platforms are you using this on?

Anton has another patch for scan-build in review. See cfe-commits "Prevent ccc/c++-analyzer from hanging on Windows." It might be best if you test all of them together. (Maybe Anton could send you the cumulative patch to save you time.)

Anna, I am testing on Debian.

I am going to wait for the patch to be split then!

Anna, just a refactoring, except line 1254 where missing "-Xclang" added before "-load". I'll make a separate patch for this.

Anna, sorry, mistaken, all of this is just a refactoring, including the line 1254.

Ok. Sounds good.

Sylvestre,
Could you test this patch and the one labeled "Prevent ccc/c++-analyzer from hanging on Windows."?

Anton,
Does Sylvestre need an updated diff for the other patch?

Anna, Sylvestre,
"Prevent ccc/c++-analyzer from hanging on Windows." is up to date:


Note, this patch is not included in the refactoring patch.

The second patch does not apply on top of the first.
Hunk #1 succeeded at 1258 (offset 26 lines).
Hunk #2 FAILED at 1242.
Hunk #3 succeeded at 1301 (offset 31 lines).
Hunk #4 succeeded at 1338 (offset 31 lines).

could you please use phabricator for the second patch and add a dependency on this one? Thanks

and, last but not least, the CR are broken. I don't know what you use but it seems that patch detects it as binary.

The second patch does not apply on top of the first.

Yes, these patches are not cumulative. After one of the patches gets in the other must be modified.

http://reviews.llvm.org/D8774

could you please use phabricator for the second patch and add a dependency on this one? Thanks

Done! Added the second patch as http://reviews.llvm.org/D8774

and, last but not least, the CR are broken. I don't know what you use but it seems that patch detects it as binary.

I'll address this a bit later.

ayartsev updated this revision to Diff 23214.Apr 3 2015, 4:36 AM

Fixed CRs.

and, last but not least, the CR are broken. I don't know what you use but it seems that patch detects it as binary.

Weird.. Use 'svn diff --diff-cmd=diff -x -U999999' as usual, no specific properties are set on 'scan-build', 'svn proplist -v scan-build' gives:
Properties on 'scan-build':

svn:executable
  *
svn:keywords
  Id

Fixed CRs manually, updated the patch.

The second patch does not apply on top of the first.

Could you, please, first test http://reviews.llvm.org/D8774 in order to merge it with the refactoring patch?

Gentle ping.

Ping!
Tested the patch more carefully - tested combinations of different options. Also launched scan-build over real codebases - LLVM and OGRE. No regressions found.

sylvestre.ledru requested changes to this revision.May 3 2015, 12:08 PM
sylvestre.ledru added a reviewer: sylvestre.ledru.

This is breaking the build of Firefox with scan-build with:

sh: 1: Syntax error: "(" unexpected
could not find clang line

Full log:
http://relman-ci.mozilla.org/job/firefox-scan-build/77/consoleFull

By the way, this error message should be improved.

This revision now requires changes to proceed.May 3 2015, 12:08 PM
ayartsev updated this revision to Diff 29031.Jul 3 2015, 9:35 AM
ayartsev edited edge metadata.

Updated the patch.

I rebuilt Firefox code with this change under Debian and it works great.
So, LGTM (but I am not the owner of this code)

(if you commit it, please update the description, "Improvements to scan-build" is too general

Hi all,

I have also tested this patch by analyzing ARM version of Linux kernel with my patch D10356.
I successfully got the result after resolving a simple indentation conflict by modifying my patch.
If this patch is committed first then I will update D10356 based on it.

Honggyu

Hi Anton,

Since D10356 is committed first, your patch(D6551) has to be modified based on it.
So I just did some modification to resolve a conflict.

I was trying to upload the patch in phabricator, but I don't have a permission to update your patch.
I just attached the modified version so that you can easily upload it.

Thanks,
Honggyu

ayartsev updated this revision to Diff 31868.Aug 11 2015, 2:57 PM
ayartsev added a reviewer: zaks.anna.
ayartsev removed a subscriber: zaks.anna.

Hi Honggyu,
thanks for updating the patch!

The new patch contains changes introduced by r244400 (D10356) and r244673 plus some simplifications.
Ok to commit?

ayartsev updated this revision to Diff 32969.Aug 24 2015, 9:20 AM

Updated the patch with r245621.
OK to commit?

Anton, I think you should just commit it. We can always iterate on this later if it breaks anything.
The patch is big and rebasing is time consuming.

dcoughlin accepted this revision.Aug 28 2015, 12:17 PM
dcoughlin edited edge metadata.

I ran our build-bot scripts with this and it looks good to me, although I've noted two places with trailing whitespace. Thanks for your patience!

tools/scan-build/scan-build
30–1

Trailing whitespace here.

32

Trailing whitespace here.

zaks.anna edited edge metadata.Aug 28 2015, 12:22 PM

Please, make sure to include a very specific commit title/message.

zaks.anna accepted this revision.Aug 28 2015, 12:22 PM
zaks.anna edited edge metadata.

Removed trailing whitespaces and committed as r246710.
Thanks for reviews and assistance!

sylvestre.ledru accepted this revision.Sep 16 2015, 4:39 AM
sylvestre.ledru edited edge metadata.
This revision is now accepted and ready to land.Sep 16 2015, 4:39 AM

To close this review.