Page MenuHomePhabricator

Add --force-analyze-debug-code option to scan-build to force debug build and hopefully enable more precise warnings.
ClosedPublic

Authored by ygribov on Jan 14 2016, 10:53 AM.

Details

Summary

Static Analyzer is much more efficient when built in debug mode (-UNDEBUG) so we advice users to enable it manually. This may be inconvenient in case of large complex projects (think about Linux distros e.g. Android or Tizen). This patch adds a flag to scan-build which inserts -UNDEBUG automatically.

I've checked this on Android where it removed a bunch of false positives and also uncovered several errors which were present only in assertion code (e.g. use-after-free, etc.).

Diff Detail

Repository
rL LLVM

Event Timeline

ygribov updated this revision to Diff 44905.Jan 14 2016, 10:53 AM
ygribov retitled this revision from to Add --force-debug option to scan-build to force debug build and hopefully enable more precise warnings..
ygribov updated this object.
ygribov added reviewers: zaks.anna, dcoughlin.
ygribov set the repository for this revision to rL LLVM.
ygribov added a subscriber: llvm-commits.
zaks.anna edited edge metadata.Jan 15 2016, 4:55 PM

It would be great if we could make this option more discoverable by the users..

scan-build is being re-implemented in python. Please, add this option to scan-build-py as well; otherwise, it will get lost when we transition.

tools/scan-build/bin/scan-build
1126

How about:
"to hopefully get more precise results" -> "to enable more precise results"

www/analyzer/scan-build.html
230

"which would enable"?

a.sidorin added inline comments.
tools/scan-build/bin/scan-build
1123

How about more informative name like '--force-analyze-debug-code'? (The same for $ForceDebug variable name).

scan-build is being re-implemented in python. Please, add this option to scan-build-py as well; otherwise, it will get lost when we transition.

Anna, how am I supposed to test scan-build-py? It does not seem to be built/installed with default make all and also there are no instructions about manual installation in tools/scan-build-py folder.

ygribov updated this revision to Diff 45157.Jan 18 2016, 1:44 AM
ygribov retitled this revision from Add --force-debug option to scan-build to force debug build and hopefully enable more precise warnings. to Add --force-analyze-debug-code option to scan-build to force debug build and hopefully enable more precise warnings..
ygribov edited edge metadata.

Updated the patch with draft scan-build-py support which I don't know how to test. Also renamed option according to Alexey's comment.

dcoughlin edited edge metadata.Jan 18 2016, 11:58 AM

Updated the patch with draft scan-build-py support which I don't know how to test.

Like the perl scan-build, you shouldn't need to install scan-build-py to run it. You can just run the scan-build script in the 'tools/scan-build-py/bin/' directory in the clang source directory.

As for real tests, scan-build-py has custom unit-ish tests in 'tools/scan-build-py/tests/' that you might extend. These are not run automatically yet.

We have plans to add lit-based tests for scan-build. This will help with qualifying scan-build-py before it replaces the perl scan-build and will also provide a more solid testing foundation moving forward.

Adding Laszlo as a reviewer for the scan-build-py bits.

hey Yury,

nice idea and good work! i personally think that even more aggressive ways to explore bugs. (like call it with and without optimization/debug flags, collect the report from multiple runs and merge it into a single report.) anyway, here it is how to run the python tests. (run it from the scan-build-py directory.)

$ PATH=$(pwd)/bin:$PATH python -m unittest -v
tools/scan-build-py/libscanbuild/runner.py
182 ↗(On Diff #45157)

can you move this logic into a separate method, and write tests around it? (it also will give you better names than new_args or args :)) unit tests for this module are in tests/unit/test_runner.py.

also, iterate through a list in python should do differently. (please take a look at the command module classify_parameters method.)

and i am not sure about the check you do with the line if 'force_analyze_debug_code' in args. you want to check opt instead of args, don't you? and since you were required to pass this parameter, you can write if opts['force_analyze_debug_code'].

ygribov updated this revision to Diff 45578.Jan 21 2016, 12:00 PM
ygribov edited edge metadata.

Addressed some of the comments, seems to work.

Like the perl scan-build, you shouldn't need to install scan-build-py to run it.
You can just run the scan-build script in the 'tools/scan-build-py/bin/' directory in the clang source directory.

Not really, because files in scan-build-py/bin lack executable perms and also scan-build-py uses system clang instead of the one you want (and for some reasone updating PATH does not help). These are probably nits but makes testing kind of inconvenient.

nice idea and good work! i personally think that even more aggressive ways to explore bugs. (like call it with and without optimization/debug flags,
collect the report from multiple runs and merge it into a single report.)

Makes good sense.

anyway, here it is how to run the python tests. (run it from the scan-build-py directory.)

$ PATH=$(pwd)/bin:$PATH python -m unittest -v

I don't think so:

$ python -m unittest -v

----------------------------------------------------------------------
Ran 0 tests in 0.000s

OK

whereas

$ for t in tests/unit/*.py; do t=$(echo $t | sed -e 's/\..*//'); python -m unittest tests.unit.$(basename $t); done

----------------------------------------------------------------------
Ran 0 tests in 0.000s

OK
...............................................
----------------------------------------------------------------------
Ran 47 tests in 0.452s

OK
etc. etc.

leaving me puzzled about what exactly needs to be tested.

As for adding the unit test, is there currently a way to analyze C file and verify that output contains particular analyzer warning? I haven't found anything like this in tests/.

ygribov marked 4 inline comments as done.Jan 21 2016, 12:07 PM

$ PATH=$(pwd)/bin:$PATH python -m unittest -v

I don't think so:

the command i gave to you works fine on my environment. would be curious to know what OS/python do you use to have the differences. anyway, to run python unittest see more on this link https://docs.python.org/2/library/unittest.html

leaving me puzzled about what exactly needs to be tested.

As for adding the unit test, is there currently a way to analyze C file and verify that output contains particular analyzer warning? I haven't found anything like this in tests/.

i would make this method do only the release flag filtering. (maybe call the method something like that too. eg.: debug_compile_flags) and call it only if the force_analyze_debug_code was set. this way you have a method you can test easy. pass some input flags, and check are they as expected.... assertEquals(['-Dkey=value'], debug_compile_flags(['-Dkey=value', '-DNDEBUG'])). or you can extend the create_commands method with special cases.

minor comments: you should extend the required decorator with force_analyze_debug_code on create_commands method... please call the new_args as result which is more descriptive... please add documentation to this new method...

thanks

$ PATH=$(pwd)/bin:$PATH python -m unittest -v
I don't think so:

the command i gave to you works fine on my environment.
would be curious to know what OS/python do you use to have the differences.

I'm on Ubuntu 14.04 with it's Python 2.7.6 (default, Jun 22 2015, 17:58:13).

anyway, to run python unittest see more on this link https://docs.python.org/2/library/unittest.html

Ah, thanks, it works now but only if I ran it as

$ python -m unittest  tests.__init__

and add

unittest.TextTestRunner(verbosity=2).run(suite())

to tests/__init__.py (and this has 41 error).

i would make this method do only the release flag filtering.

Got it.

minor comments: you should extend the required decorator with force_analyze_debug_code on create_commands method...
please call the new_args as result which is more descriptive... please add documentation to this new method...

Right.

ygribov updated this revision to Diff 45683.Jan 22 2016, 6:23 AM

Addressed Laszlo's comments.

Laszlo, Anna, does this look better?

thanks Yury for the update.

tools/scan-build-py/tests/unit/test_runner.py
216 ↗(On Diff #45683)

interesting to see that the method is changing the flags. (glue them together) my expectation would be that it only filters, and do not modify it. (i understand that semantically it is the same.)

and would like to see a few more tests... ['-Dkey=val'], ['-DNDEBUG'], ['-D', 'NDEBUG'], ['-DNDEBUG=1'], ['-DDEBUG'], ['-Dkey=val', '-DNDEBUG'], ['-Dkey=val', '-D', 'NDEBUG'], ['-DNDEBUG', '-Dkey=val'], ['-Dkey1=val1', '-DNDEBUG=1', '-Dkey2=val2']

ygribov updated this revision to Diff 46376.Jan 29 2016, 5:35 AM

Addressed Laszlo's comments.

rizsotto.mailinglist edited edge metadata.

thanks Yury!

This revision is now accepted and ready to land.Jan 29 2016, 3:36 PM
zaks.anna accepted this revision.Jan 29 2016, 10:34 PM
zaks.anna edited edge metadata.

Thanks!

sorry for it, but was thinking on the weekend and i have a lame question after the hard work: why not just append a -UNDEBUG to the end of the argument list? would not it make the same effect?

why not just append a -UNDEBUG to the end of the argument list? would not it make the same effect?

I think for Perl version of scan-build I was just following the general practice of stripping/modifying interesting arguments. And scan-build-py then just followed the same pattern. -UNDEBUG is supported by all major compiler so it's probably ok to use it instead. Should I switch?

to me it appears a more reasonable solution. (it does not require a separate method and unit test around it.) maybe the same effort could be focus to write a functional test for it... that's only my thoughts. Devin or Anna, could you share your opinion?

If the implementation is simpler and there are no other downsides, we should go for it.

ygribov updated this revision to Diff 47184.Feb 8 2016, 5:59 AM
ygribov edited edge metadata.

Append -UNDEBUG instead of filtering flags, per Laszlo's advice.

See some nits below. Otherwise, LGTM.

Thank you!
Anna.

tools/scan-build-py/libscanbuild/analyze.py
463 ↗(On Diff #47184)

"(to enable more precise results)" -> enabling more precise results

tools/scan-build-py/libscanbuild/runner.py
169 ↗(On Diff #47184)

Please, update or remove the comment.

tools/scan-build-py/tests/unit/test_runner.py
222 ↗(On Diff #47184)

Is '-' missing in front of NDEBUG?

tools/scan-build/bin/scan-build
1126

please remove '('.

This revision was automatically updated to reflect the committed changes.

LGTM.
Thank you.