Re: [PATCH] D9600: Add scan-build python implementation
rizsotto.mailinglist added a comment. Thank you guys for your review and help! The latest patch is commited. Let's do the next steps too. ;) @danielmarjamaki the standalone Bear will be maintained. and your comments will also be considered. http://reviews.llvm.org/D9600 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D9600: Add scan-build python implementation
dcoughlin closed this revision. dcoughlin added a comment. This was committed in r257533. Thanks Laszlo! http://reviews.llvm.org/D9600 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D9600: Add scan-build python implementation
zaks.anna accepted this revision. zaks.anna added a comment. Laszlo, I am very excited about having the new and much improved scan-build in tree! It will serve as a solid foundation for moving forward. Thank you for all your hard work! Anna. http://reviews.llvm.org/D9600 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D9600: Add scan-build python implementation
dcoughlin added a comment. Hi Laszlo, I've run scan-build-py with both environment-variable-based and library-based interposition on our open source benchmark suite and it looks like it is in great shape on Darwin! There are still some remaining issues with xcodebuild, but I will help fix these after scan-build-py is committed. Please apply for commit access (the instructions are at http://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access), update the commit message, and commit. Thanks for all your hard work on this! Comment at: tools/scan-build-py/libscanbuild/intercept.py:261 @@ +260,3 @@ + +Same problem on linux when SELinux is enabled. The status query program +'sestatus' and the output when it's enabled 'SELinux status: enabled'. """ Great! Thanks for checking for this. Comment at: tools/scan-build-py/libscanbuild/runner.py:42 @@ +41,3 @@ + +@require(['command', 'directory', 'file', # an entry from compilation database + 'clang', 'direct_args', # compiler name, and arguments from command Thanks for documenting these. http://reviews.llvm.org/D9600 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D9600: Add scan-build python implementation
danielmarjamaki added a comment. I like the standalone bear tool. should it really be embedded into clang? Will the standalone tool still be maintained? Comment at: tools/scan-build-py/libear/ear.c:1 @@ +1,2 @@ +/* -*- coding: utf-8 -*- +// The LLVM Compiler Infrastructure why should these be added in these source files when no other file in LLVM/Clang has it. http://reviews.llvm.org/D9600 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D9600: Add scan-build python implementation
rizsotto.mailinglist added a comment.
sorry for the delay, hard to get free time these days. ;)
Comment at: tools/scan-build-py/libscanbuild/intercept.py:146
@@ +145,3 @@
+})
+elif sys.platform == 'darwin':
+logging.debug('intercept gonna preload libear on OSX')
dcoughlin wrote:
> Can you change this to default to compiler-wrapper interposition on Darwin
> with a command-line flag to force library-based interposition?
>
> Library-based interposition will fail silently if SIP is enabled, so this
> should be detected when that flag is passed so the user is informed. You can
> detect whether SIP is enabled on Darwin by checking whether (1) there is a
> binary called 'csrutil' in the path and, if so, (2) whether the output of
> executing 'csrutil status' contains 'System Integrity Protection status:
> enabled'.
>
>
ok, implemented the SIP check. (also added SELinux check, which behaves the
same as SIP.)
about the default behavior: `scan-build` command has the other compiler-wrapper
(run compiler & analyzer) and flags can turn this module on. `intercept-build`
default is the library-based and flag can turn the compiler-wrapper
interposition on... i would keep it this way. (and also not make platform
dependent switches into the code.)
http://reviews.llvm.org/D9600
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D9600: Add scan-build python implementation
rgov added a subscriber: rgov. rgov added a comment. I tried out intercept-build and found it very easy to use. I tried running it on projects that compile with both make and xcodebuild and it worked for both. Thanks for contributing this tool. http://reviews.llvm.org/D9600 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D9600: Add scan-build python implementation
dcoughlin added a comment.
Thanks for adding `bear_get_environment()` to handle the environment weirdness
on Darwin.
Comment at: tools/scan-build-py/libscanbuild/intercept.py:146
@@ +145,3 @@
+})
+elif sys.platform == 'darwin':
+logging.debug('intercept gonna preload libear on OSX')
Can you change this to default to compiler-wrapper interposition on Darwin with
a command-line flag to force library-based interposition?
Library-based interposition will fail silently if SIP is enabled, so this
should be detected when that flag is passed so the user is informed. You can
detect whether SIP is enabled on Darwin by checking whether (1) there is a
binary called 'csrutil' in the path and, if so, (2) whether the output of
executing 'csrutil status' contains 'System Integrity Protection status:
enabled'.
http://reviews.llvm.org/D9600
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D9600: Add scan-build python implementation
rizsotto.mailinglist added inline comments. Comment at: tools/scan-build-py/libear/ear.c:142 @@ +141,3 @@ +#endif +if (!initialized) +initialized = bear_capture_env_t(_env); dcoughlin wrote: > rizsotto.mailinglist wrote: > > rizsotto.mailinglist wrote: > > > to run the full test set > > > > > > > PATH=$(pwd)/bin:$PATH python -m unittest discover > > > > > > to run that specific test > > > > > > > PATH=$(pwd)/bin:$PATH python -m unittest -v > > > > tests.functional.cases.test_create_cdb.CompilationDatabaseTest.test_successful_build_on_empty_env > > > > > > to more about run tests > > > > > > https://docs.python.org/2/library/unittest.html > > my understanding on the `_NSGetEnviron` is, that it shall be used when the > > library is during the load process. later when the build process calls > > `execv` the load process is over, and `environ` variable is available. an > > earlier version of this code had a `get_environ` method, which were either > > return the `environ` variable or called the `_NSGetEnviron`. then i made > > this change and the tests were still passing, so i don't see where your > > issue is coming from. please tell me what kind of test you run against it > > to find it as problem. would like to add it to the test suite. > Aaah, I had an ancient libscanbuild in my global site-packages, which seemed > to cause `unittest discover` to not work. > > When running the above on your latest patch on a machine without SIP I get. > > ``` > test_successful_build_on_empty_env > (tests.functional.cases.test_create_cdb.CompilationDatabaseTest) ... ERROR > > == > ERROR: test_successful_build_on_empty_env > (tests.functional.cases.test_create_cdb.CompilationDatabaseTest) > -- > Traceback (most recent call last): > File "tests/functional/cases/test_create_cdb.py", line 58, in > test_successful_build_on_empty_env > 'env', '-'] + make) > File "tests/functional/cases/__init__.py", line 38, in silent_check_call > return subprocess.check_call(cmd, *args, **kwargs) > File > "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/subprocess.py", > line 540, in check_call > raise CalledProcessError(retcode, cmd) > CalledProcessError: Command '['intercept-build', '--cdb', > '/var/folders/mq/46lsd1kx65v5702dzrkgzdlrgn/T/scan-build-test-cKXza1/cdb.json', > 'env', '-', 'make', 'SRCDIR=tests/functional/src', > 'OBJDIR=/var/folders/mq/46lsd1kx65v5702dzrkgzdlrgn/T/scan-build-test-cKXza1', > '-f', 'tests/functional/src/build/Makefile', 'CC=clang', 'build_regular']' > returned non-zero exit status 2 > > -- > Ran 1 test in 0.554s > > FAILED (errors=1) > ``` > This goes away if you use my suggested *_NSGetEnviron() fix above. > After applying that fix, all but 6 test past on OS X without SIP. The > remaining 6 failures are due to your use of mknod() in tests, which requires > superuser privileges on OS X: > > > ``` > == > ERROR: test_interposition_cxx_works > (tests.functional.cases.test_from_cmd.RunAnalyzerTest) > -- > Traceback (most recent call last): > File > "/Volumes/Data/Clangs/OpenSourceGit/clang/tools/scan-build-py/tests/functional/cases/test_from_cmd.py", > line 102, in test_interposition_cxx_works > self.compile_empty_source_file(tmpdir, True)) > File > "/Volumes/Data/Clangs/OpenSourceGit/clang/tools/scan-build-py/tests/functional/cases/test_from_cmd.py", > line 87, in compile_empty_source_file > os.mknod(src_file) > OSError: [Errno 1] Operation not permitted > ``` > > Is there a more portable way to create an empty file? Perhaps open a file for > writing and then closing it? > > > With SIP I see different failures: > > ``` > workzilla:scan-build-py dcoughlin$ PATH=$(pwd)/bin:$PATH python -m unittest > -v > tests.functional.cases.test_create_cdb.CompilationDatabaseTest.test_successful_build_on_empty_env > test_successful_build_on_empty_env > (tests.functional.cases.test_create_cdb.CompilationDatabaseTest) ... FAIL > > == > FAIL: test_successful_build_on_empty_env > (tests.functional.cases.test_create_cdb.CompilationDatabaseTest) > -- > Traceback (most recent call last): > File "tests/functional/cases/test_create_cdb.py", line 60, in > test_successful_build_on_empty_env > self.assertEqual(5, self.count_entries(result)) > AssertionError: 5 != 0 > > -- > Ran 1 test in 1.069s > > FAILED (failures=1) > ``` > > Running the entire
Re: [PATCH] D9600: Add scan-build python implementation
kimgr added a subscriber: kimgr. kimgr added a comment. > > Comment at: tools/scan-build-py/bin/analyze-cc:14 > @@ +13,2 @@ > +from libscanbuild.analyze import wrapper > +sys.exit(wrapper(False)) > > > > It is hard to figure out/search which functions actually get called from the > top level tools. Could you rename all of the public functions so that they > have unique names? For example, "wrapper" -> "scan_build_wrapper". This > would greatly improve searchability! A nice pattern in Python is to avoid importing individual functions/classes, and instead use the module name as a namespace, e.g. from libscanbuild import analyze sys.exit(analyze.wrapper(False)) I don't know if that addresses your concern? I haven't looked at the code in more detail. - Kim http://reviews.llvm.org/D9600 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D9600: Add scan-build python implementation
dcoughlin added inline comments. Comment at: tools/scan-build-py/libear/ear.c:142 @@ +141,3 @@ +#endif +if (!initialized) +initialized = bear_capture_env_t(_env); rizsotto.mailinglist wrote: > rizsotto.mailinglist wrote: > > to run the full test set > > > > > PATH=$(pwd)/bin:$PATH python -m unittest discover > > > > to run that specific test > > > > > PATH=$(pwd)/bin:$PATH python -m unittest -v > > > tests.functional.cases.test_create_cdb.CompilationDatabaseTest.test_successful_build_on_empty_env > > > > to more about run tests > > > > https://docs.python.org/2/library/unittest.html > my understanding on the `_NSGetEnviron` is, that it shall be used when the > library is during the load process. later when the build process calls > `execv` the load process is over, and `environ` variable is available. an > earlier version of this code had a `get_environ` method, which were either > return the `environ` variable or called the `_NSGetEnviron`. then i made this > change and the tests were still passing, so i don't see where your issue is > coming from. please tell me what kind of test you run against it to find it > as problem. would like to add it to the test suite. Aaah, I had an ancient libscanbuild in my global site-packages, which seemed to cause `unittest discover` to not work. When running the above on your latest patch on a machine without SIP I get. ``` test_successful_build_on_empty_env (tests.functional.cases.test_create_cdb.CompilationDatabaseTest) ... ERROR == ERROR: test_successful_build_on_empty_env (tests.functional.cases.test_create_cdb.CompilationDatabaseTest) -- Traceback (most recent call last): File "tests/functional/cases/test_create_cdb.py", line 58, in test_successful_build_on_empty_env 'env', '-'] + make) File "tests/functional/cases/__init__.py", line 38, in silent_check_call return subprocess.check_call(cmd, *args, **kwargs) File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/subprocess.py", line 540, in check_call raise CalledProcessError(retcode, cmd) CalledProcessError: Command '['intercept-build', '--cdb', '/var/folders/mq/46lsd1kx65v5702dzrkgzdlrgn/T/scan-build-test-cKXza1/cdb.json', 'env', '-', 'make', 'SRCDIR=tests/functional/src', 'OBJDIR=/var/folders/mq/46lsd1kx65v5702dzrkgzdlrgn/T/scan-build-test-cKXza1', '-f', 'tests/functional/src/build/Makefile', 'CC=clang', 'build_regular']' returned non-zero exit status 2 -- Ran 1 test in 0.554s FAILED (errors=1) ``` This goes away if you use my suggested *_NSGetEnviron() fix above. After applying that fix, all but 6 test past on OS X without SIP. The remaining 6 failures are due to your use of mknod() in tests, which requires superuser privileges on OS X: ``` == ERROR: test_interposition_cxx_works (tests.functional.cases.test_from_cmd.RunAnalyzerTest) -- Traceback (most recent call last): File "/Volumes/Data/Clangs/OpenSourceGit/clang/tools/scan-build-py/tests/functional/cases/test_from_cmd.py", line 102, in test_interposition_cxx_works self.compile_empty_source_file(tmpdir, True)) File "/Volumes/Data/Clangs/OpenSourceGit/clang/tools/scan-build-py/tests/functional/cases/test_from_cmd.py", line 87, in compile_empty_source_file os.mknod(src_file) OSError: [Errno 1] Operation not permitted ``` Is there a more portable way to create an empty file? Perhaps open a file for writing and then closing it? With SIP I see different failures: ``` workzilla:scan-build-py dcoughlin$ PATH=$(pwd)/bin:$PATH python -m unittest -v tests.functional.cases.test_create_cdb.CompilationDatabaseTest.test_successful_build_on_empty_env test_successful_build_on_empty_env (tests.functional.cases.test_create_cdb.CompilationDatabaseTest) ... FAIL == FAIL: test_successful_build_on_empty_env (tests.functional.cases.test_create_cdb.CompilationDatabaseTest) -- Traceback (most recent call last): File "tests/functional/cases/test_create_cdb.py", line 60, in test_successful_build_on_empty_env self.assertEqual(5, self.count_entries(result)) AssertionError: 5 != 0 -- Ran 1 test in 1.069s FAILED (failures=1) ``` Running the entire test suite with SIP yields: ``` F...EEFFE..FEEF.EE... ... Ran 85 tests in 15.688s FAILED (failures=9, errors=7) ``` I tried changing intercept.py to always use compiler wrappers on
Re: [PATCH] D9600: Add scan-build python implementation
rizsotto.mailinglist marked 6 inline comments as done. Comment at: tools/scan-build-py/README.md:86 @@ +85,3 @@ +The 2. mode is available only on FreeBSD, Linux and OSX. Where library preload +is available from the dynamic loader. On OSX System Integrity Protection security +feature enabled prevents library preload, so this method will not work in such zaks.anna wrote: > This is very unfortunate! > > We should call out that library interposition is "not supported on OS X > (unless System Integrity Protection feature is turned off)" and return an > error if people are trying to use it (and System Integrity Protection feature > is turned on). comment changed. please hint me with some code how can the script detect SIP status. (or postpone the check implementation after the patch is accepted) Comment at: tools/scan-build-py/bin/analyze-c++:3 @@ +2,3 @@ +# -*- coding: utf-8 -*- +# The LLVM Compiler Infrastructure +# zaks.anna wrote: > I searched the code and did not see it being called. By looking back at the > previous revision I see that that libscanbuild.analyze.main used to call > 'analyze-cxx' not 'analyze-c++'. Looks like you've also fixed the same bug > with 'intercept-c++'. > > Is this something that could/would be caught by the tests? test harness contains only C files, that was the reason this could have gone unnoticed. added specific check for it. http://reviews.llvm.org/D9600 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D9600: Add scan-build python implementation
dcoughlin added inline comments.
Comment at: tools/scan-build-py/README.md:85
@@ +84,3 @@
+
+The 2. mode is available only on FreeBSD, Linux and OSX. Where library preload
+is available from the dynamic loader. On OSX System Integrity Protection
security
My comment about SIP on 10.11 not preventing library interposition on build
tools is not correct. My apologies for the bad information. SIP does not
prevent DYLD_INSERT_LIBRARIES on the build tools themselves but it *does*
prevent interposition on the build tool shims in /usr/bin. These are thin shims
that call the appropriate tools elsewhere and it is these shims that typical
build scripts call. SIP also prevents library interposition on perl/python/etc.
-- which are commonly used to drive build scripts. (For example, openssl's
build script uses make to call perl to call cc). I think on Darwin we should
use your wrapper-based approach (intercept-c/intercept-c++) instead. In my
tests on openssl it produces the same (modulo ordering of entries) compilation
database as running with library-interposition on a machine with SIP turned off.
Comment at: tools/scan-build-py/libear/ear.c:141
@@ +140,3 @@
+environ = *_NSGetEnviron();
+#endif
+if (!initialized)
rizsotto.mailinglist wrote:
> this call just sets up the `environ` variables for the `bear_capture_env_t`
> method, two lines down. that call saves the relevant variables... so, if
> before execv'ing somebody modify the variables we still have it saved. (by
> the way `Scons` is the tool which does this.) and when `call_execve` called
> it recovers from the saved environment.
>
> there is a test against it. (tests/functional/cases/test_create_cdb.py:53)
>
> > The issue I am seeing with library-interposition on OS X [...]
>
> can you give me details what did you see? what was the test you were running?
You also later use 'environ' in `execv()`. But at that point the environment
stashed in environ is different than the environment given to children. So in
execv() rather than using 'environ' you on Darwin you would need to use
'*_NSGetEnviron()'. (The behavior of 'environ' on Darwin in dylibs is different
than in main executables, which is why _NSGetEnviron() exists). In any event,
if only support using wrapper-based interposition (intercept-build-cc, etc.) on
Darwin then perhaps we don't need to deal with this.
>> there is a test against it. (tests/functional/cases/test_create_cdb.py:53)
Interesting. Have you run the tests on OS X? Can you tell me how to run them?
Comment at: tools/scan-build-py/libscanbuild/intercept.py:143
@@ +142,3 @@
+})
+elif sys.platform == 'darwin':
+logging.debug('intercept gonna preload libear on OSX')
I think it would be better to use the fall-back compiler-wrapper interposition
on darwin. As I noted above (apologies again for the bad information) System
Integrity Protection will make library-based interposition unreliable for build
scripts that employ restricted executables.
http://reviews.llvm.org/D9600
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D9600: Add scan-build python implementation
rizsotto.mailinglist marked an inline comment as done. Comment at: tools/scan-build-py/bin/analyze-c++:2 @@ +1,3 @@ +#!/usr/bin/env python +# -*- coding: utf-8 -*- +# The LLVM Compiler Infrastructure zaks.anna wrote: > Where/How is analyze-c++ used? this is the compiler wrapper which runs the real compiler + the static analyzer. (you guys were call it interposition mode) there is 'analyze-c++' and 'analyze-cc' for C++ and C compilers. it is used from the libscanbuild.analyze.main method. http://reviews.llvm.org/D9600 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D9600: Add scan-build python implementation
dcoughlin added inline comments. Comment at: tools/scan-build-py/libear/ear.c:140 @@ +139,3 @@ +#ifdef HAVE_NSGETENVIRON +environ = *_NSGetEnviron(); +#endif The issue I am seeing with library-interposition on OS X seems to be caused by eagerly stashing *_NSGetEnviron() in `environ`. This causes processes that modify their environment before execv'ing to drop the modified environments because interposition forwards execv to call_execve() with the stale environment. To avoid this, you can replace all references to `environ` on Darwin with (*_NSGetEnviron()). http://reviews.llvm.org/D9600 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D9600: Add scan-build python implementation
zaks.anna added inline comments. Comment at: tools/scan-build-py/README.md:86 @@ +85,3 @@ +The 2. mode is available only on FreeBSD, Linux and OSX. Where library preload +is available from the dynamic loader. On OSX System Integrity Protection security +feature enabled prevents library preload, so this method will not work in such This is very unfortunate! We should call out that library interposition is "not supported on OS X (unless System Integrity Protection feature is turned off)" and return an error if people are trying to use it (and System Integrity Protection feature is turned on). Comment at: tools/scan-build-py/bin/analyze-build:17 @@ +16,2 @@ +from libscanbuild.analyze import main +sys.exit(main(this_dir, False)) Please rename 'main'. Comment at: tools/scan-build-py/bin/analyze-c++:3 @@ +2,3 @@ +# -*- coding: utf-8 -*- +# The LLVM Compiler Infrastructure +# I searched the code and did not see it being called. By looking back at the previous revision I see that that libscanbuild.analyze.main used to call 'analyze-cxx' not 'analyze-c++'. Looks like you've also fixed the same bug with 'intercept-c++'. Is this something that could/would be caught by the tests? Comment at: tools/scan-build-py/bin/analyze-cc:14 @@ +13,2 @@ +from libscanbuild.analyze import scan_build_wrapper +sys.exit(scan_build_wrapper(False)) Could you rename **all** of the public functions, not just this one? I am talking about the other wrapper and several main functions used as the entry points in the scripts. Comment at: tools/scan-build-py/bin/intercept-build:17 @@ +16,2 @@ +from libscanbuild.intercept import main +sys.exit(main(this_dir)) Please rename 'main'. Comment at: tools/scan-build-py/bin/intercept-c++:14 @@ +13,2 @@ +from libscanbuild.intercept import wrapper +sys.exit(wrapper(True)) Please rename 'wrapper'. Comment at: tools/scan-build-py/bin/intercept-cc:14 @@ +13,2 @@ +from libscanbuild.intercept import wrapper +sys.exit(wrapper(False)) Please rename 'wrapper'. Comment at: tools/scan-build-py/bin/scan-build:17 @@ +16,2 @@ +from libscanbuild.analyze import main +sys.exit(main(this_dir, True)) Please rename 'main'. http://reviews.llvm.org/D9600 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D9600: Add scan-build python implementation
rizsotto.mailinglist added inline comments. Comment at: tools/scan-build-py/libear/ear.c:141 @@ +140,3 @@ +environ = *_NSGetEnviron(); +#endif +if (!initialized) this call just sets up the `environ` variables for the `bear_capture_env_t` method, two lines down. that call saves the relevant variables... so, if before execv'ing somebody modify the variables we still have it saved. (by the way `Scons` is the tool which does this.) and when `call_execve` called it recovers from the saved environment. there is a test against it. (tests/functional/cases/test_create_cdb.py:53) > The issue I am seeing with library-interposition on OS X [...] can you give me details what did you see? what was the test you were running? http://reviews.llvm.org/D9600 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D9600: Add scan-build python implementation
rizsotto.mailinglist marked 7 inline comments as done. rizsotto.mailinglist added a comment. thanks for the comments. Comment at: tools/scan-build-py/bin/analyze-c++:1 @@ +1,2 @@ +#!/usr/bin/env python +# -*- coding: utf-8 -*- zaks.anna wrote: > What calls this script? sorry, don't get the question Comment at: tools/scan-build-py/libscanbuild/runner.py:23 @@ +22,3 @@ +""" Decorator for checking the required values in state. + +It checks the required attributes in the passed state and stop when dcoughlin wrote: > Ok. If I create a patch with additional documentation for these fields, would > you be willing to take a look at it to make sure the comments are correct? sure http://reviews.llvm.org/D9600 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D9600: Add scan-build python implementation
rizsotto.mailinglist added inline comments. Comment at: tools/scan-build-py/libear/ear.c:282 @@ +281,3 @@ +DLSYM(func, fp, "execve"); + +char const **const menvp = bear_update_environment(envp, _env); okay, since i don't have access for OSX machines can you help me to make it right? http://reviews.llvm.org/D9600 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D9600: Add scan-build python implementation
rizsotto.mailinglist marked an inline comment as done.
rizsotto.mailinglist added a comment.
> Also, what do you think about renaming intercept-build to "log-build" or some
> of the other alternatives I proposed above? I think it is important for the
> name of the executable to communicate its purpose.
i do think differently. :) first, i think you can rename executables to
whatever you like. previously i made this functionality in a tool called Bear.
within 4 years got more than 100 bug reports, improvement ideas, but nobody did
care about the executable name. second, i'm not a native speaker, but to me
'intercept compiler calls from build' can be shortened to 'intercept-build'.
third, if i rename the executable from intercept-build, shall rename the
intercept module too? and to be honest, the executable name scan-build just as
bad as intercept-build. :) if i did not convinced you, give me a name and i
will rename to it.
Comment at: tools/scan-build-py/MANIFEST.in:2
@@ +1,3 @@
+include README.md
+include *.txt
+recursive-include libear *
dcoughlin wrote:
> I see that at https://pypi.python.org/pypi/scan-build it lists clang as a
> prerequisite. But since scan-build-py will now be distributed as part of
> clang I don't understand what the point of the standalone tool is. Can you
> explain why this is needed?
thought to have more distribution channel is better. but if that so confusing,
i've deleted it.
Comment at: tools/scan-build-py/libscanbuild/driver.py:67
@@ +66,3 @@
+except Exception:
+logging.exception("Something unexpected had happened.")
+return 127
jroelofs wrote:
> dcoughlin wrote:
> > rizsotto.mailinglist wrote:
> > > jroelofs wrote:
> > > > rizsotto.mailinglist wrote:
> > > > > dcoughlin wrote:
> > > > > > I think this error message can be improved. Perhaps "Unexpected
> > > > > > error running intercept-build"?
> > > > > this line is printed as:
> > > > >
> > > > > intercept-build: ERROR: Something unexpected had happened.
> > > > > (and the stack-trace)
> > > > >
> > > > > because the logging formating. so, 'intercept-build' and 'error' will
> > > > > be part of the message anyway.
> > > > Is there a pythonic way of doing llvm crash handlers? I.e. the "here's
> > > > the steps to reproduce this, a stack trace, and a bug report url"
> > > > things that clang spits out.
> > > this crash/exception is not a clang crash. it's more like this program's
> > > fault. clang crash reports are recorded already in crash report files
> > > (and linked into the html report file).
> > Maybe this should ask the user to file a bug against scan-build? Can it
> > point to the bugzilla and tell the user what information, files, etc. they
> > should attach to the bug report to help us reproduce the problem? Just
> > saying "Something unexpected happened" is not as user-friendly as it could
> > be because it does not tell the user that the problem is not their fault
> > nor what they should do about it.
> Oh, I don't mean to handle clang crashes... I mean for handling python
> crashes. I meant to draw an analogy to a feature that clang has: when it
> crashes, it prints a stack trace, and some steps to reproduce the problem. I
> was wondering if a similar thing existed in python, for when a python app
> crashes.
good point. added some implementation. if you don't like the text, please
suggest one instead and will replace.
Comment at: tools/scan-build-py/libscanbuild/runner.py:22
@@ +21,3 @@
+def require(required):
+""" Decorator for checking the required values in state.
+
the comment says exactly the value coming from the compilation database. anyway
added more explanations. if you have something in mind, please write the text
and will copy it.
http://reviews.llvm.org/D9600
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D9600: Add scan-build python implementation
dcoughlin added a comment. In http://reviews.llvm.org/D9600#305980, @rizsotto.mailinglist wrote: > > Also, what do you think about renaming intercept-build to "log-build" or > > some of the other alternatives I proposed above? I think it is important > > for the name of the executable to communicate its purpose. > > > ... and to be honest, the executable name scan-build just as bad as > intercept-build. :) There are many things about the original scan-build that are bad. As you have seen, the perl-based scan-build is poorly named, poorly factored, insufficiently documented and poorly tested. This has made it very difficult to fix bugs, support new platforms, and extend scan-build's functionality -- and is exactly the reason why we are so excited about a scan-build reimplementation. With this reimplementation, we have the opportunity to do so much better! If we live up to the high standards in the rest of the clang/llvm codebase about naming, factoring, documentation, and testing then the new scan-build will be much easier to maintain and can serve as a solid foundation to add new features. Comment at: tools/scan-build-py/README.md:84 @@ +83,3 @@ +The 2. mode is available only on FreeBSD, Linux and OSX. Where library preload +is available from the dynamic loader. On OSX System Integrity Protection security +feature enabled prevents library preload, so this method will not work in such With respect to dynamic library-interposition still not working on OS X, the System Integrity Protection in OS X 10.11 should not prevent interposition on build tools so in theory intercept-build should work. I'll look into it. Comment at: tools/scan-build-py/libscanbuild/runner.py:23 @@ +22,3 @@ +""" Decorator for checking the required values in state. + +It checks the required attributes in the passed state and stop when Ok. If I create a patch with additional documentation for these fields, would you be willing to take a look at it to make sure the comments are correct? http://reviews.llvm.org/D9600 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D9600: Add scan-build python implementation
zaks.anna added a comment.
Hi Laszlo,
Here are some comments from me.
Should we be worried about the name conflicts (between old scan-build and this
tool) during rollout? I think it would be beneficial to rename the tools, but
let's discuss the names later. (If we integrate Codecheck, that will affect
naming too.)
The libear library should be built as part of clang cmake build; looks like
@jroelofs offered to do this.
We should also integrate your tests into lit, which is the way we test all of
the other tools in clang.
Comment at: tools/scan-build-py/README.md:1
@@ +1,2 @@
+[](https://travis-ci.org/rizsotto/scan-build)
+
What's this? Please, remove.
Comment at: tools/scan-build-py/README.md:6
@@ +5,3 @@
+
+It's a static analyzer wrapper for [Clang][1]. The original `scan-build`
+is written in Perl. This package contains reimplementation of that scripts
I do not think you need to mention the old scan-build. I'd just describe the
tool. Here is a slightly modified copy and paste from scan-build:
"A package designed to wrap a build so that all calls to gcc/clang are
intercepted and logged into a compilation database [2] and/or piped to the
clang static analyzer. Includes intercept-build tool, which logs the build, as
well as scan-build tool, which logs the build and runs the clang static
analyzer on it."
I have a bunch of comments about other parts of the documentation. However, I
can rewrite parts of it as a separate commit. (It will be more efficient than
going back and forth.)
Comment at: tools/scan-build-py/README.md:59
@@ +58,3 @@
+
+1. Use compiler wrappers to make actions.
+The compiler wrappers does run the real compiler and the analyzer.
This is a good place to point out how each mode can be activated. (Which
parameters should be used.)
Comment at: tools/scan-build-py/README.md:85
@@ +84,3 @@
+is available from the dynamic loader. On OSX System Integrity Protection
security
+feature enabled prevents library preload, so this method will not work in such
+environment.
Would be good to find out which specific binaries used by the build we are not
allowed to interpose on. It would be very unfortunate if this does not work
with the System Integrity Protection feature, which is turned on by default.
Comment at: tools/scan-build-py/README.md:114
@@ +113,3 @@
+
+The project is licensed under University of Illinois/NCSA Open Source License.
+
Please, refer to the LICENSE.TXT file like you do in the other files.
Comment at: tools/scan-build-py/bin/analyze-c++:1
@@ +1,2 @@
+#!/usr/bin/env python
+# -*- coding: utf-8 -*-
What calls this script?
Comment at: tools/scan-build-py/bin/analyze-cc:14
@@ +13,2 @@
+from libscanbuild.analyze import wrapper
+sys.exit(wrapper(False))
It is hard to figure out/search which functions actually get called from the
top level tools. Could you rename all of the public functions so that they have
unique names? For example, "wrapper" -> "scan_build_wrapper". This would
greatly improve searchability!
Comment at: tools/scan-build-py/libear/ear.c:406
@@ +405,3 @@
+if (0 == fd) {
+perror("bear: fopen");
+exit(EXIT_FAILURE);
It will be hard for the users to interpret these error messages; especially if
the tool is widely distributed. (Can be addressed after the initial commit.)
Comment at: tools/scan-build-py/libscanbuild/__init__.py:11
@@ +10,3 @@
+This work is derived from the original 'scan-build' Perl implementation and
+from an independent project 'bear'. """
+
Not sure if this comment helps to understand the functionality, please, remove.
Comment at: tools/scan-build-py/libscanbuild/analyze.py:55
@@ +54,3 @@
+exit_code = capture(args, bin_dir)
+# next step to run the analyzer against the captured commands
+if need_analyzer(args.build):
http://llvm.org/docs/CodingStandards.html#commenting
Comment at: tools/scan-build-py/libscanbuild/intercept.py:71
@@ +70,3 @@
+
+def post_processing(commands):
+# run post processing only if that was requested
What does post_processing do? Looks like it allows to append to the compilation
database and more.. Could you add a comment or rename the function?
Comment at: tools/scan-build-py/libscanbuild/intercept.py:287
@@ +286,3 @@
+action='store_true',
+help="""Disable filter, unformated output.""")
+
Do you explain what the "filter" means somewhere visible to the user? When is
filter useful and when it is not?
Comment
Re: [PATCH] D9600: Add scan-build python implementation
zaks.anna added a comment. Hi Laszlo, Comment at: tools/scan-build-py/libear/ear.c:281 @@ +280,3 @@ + +DLSYM(func, fp, "execve"); + This is not the recommended way of interposing on Darwin. All you need to do is provide your function, which can call the function you are interposing on and tell the linker about it as described in this example: http://www.opensource.apple.com/source/dyld/dyld-97.1/include/mach-o/dyld-interposing.h You would only need to set DYLD_INSERT_LIBRARIES and would not need to require flat namespace. We should not be setting DYLD_FORCE_FLAT_NAMESPACE; some of the problems it causes are described on this thread: https://mikeash.com/pyblog/friday-qa-2009-01-30-code-injection.html compiler-rt (sanitizers) also intercept on all platforms including Windows (see interception.h); that code is very platform dependent. http://reviews.llvm.org/D9600 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D9600: Add scan-build python implementation
dcoughlin added a comment.
Thanks Laszlo.
I'm still not convinced that all the python package stuff is needed because
scan-build is distributed with clang (see my question inline).
Also, what do you think about renaming intercept-build to "log-build" or some
of the other alternatives I proposed above? I think it is important for the
name of the executable to communicate its purpose.
Comment at: tools/scan-build-py/MANIFEST.in:2
@@ +1,3 @@
+include README.md
+include *.txt
+recursive-include libear *
I see that at https://pypi.python.org/pypi/scan-build it lists clang as a
prerequisite. But since scan-build-py will now be distributed as part of clang
I don't understand what the point of the standalone tool is. Can you explain
why this is needed?
Comment at: tools/scan-build-py/libscanbuild/analyze.py:131
@@ +130,3 @@
+def setup_environment(args, destination, wrapper_dir):
+""" Set up environment for build command to intrepose compiler wrapper. """
+
Typo: 'intrepose'
Comment at: tools/scan-build-py/libscanbuild/driver.py:67
@@ +66,3 @@
+except Exception:
+logging.exception("Something unexpected had happened.")
+return 127
rizsotto.mailinglist wrote:
> jroelofs wrote:
> > rizsotto.mailinglist wrote:
> > > dcoughlin wrote:
> > > > I think this error message can be improved. Perhaps "Unexpected error
> > > > running intercept-build"?
> > > this line is printed as:
> > >
> > > intercept-build: ERROR: Something unexpected had happened.
> > > (and the stack-trace)
> > >
> > > because the logging formating. so, 'intercept-build' and 'error' will be
> > > part of the message anyway.
> > Is there a pythonic way of doing llvm crash handlers? I.e. the "here's the
> > steps to reproduce this, a stack trace, and a bug report url" things that
> > clang spits out.
> this crash/exception is not a clang crash. it's more like this program's
> fault. clang crash reports are recorded already in crash report files (and
> linked into the html report file).
Maybe this should ask the user to file a bug against scan-build? Can it point
to the bugzilla and tell the user what information, files, etc. they should
attach to the bug report to help us reproduce the problem? Just saying
"Something unexpected happened" is not as user-friendly as it could be because
it does not tell the user that the problem is not their fault nor what they
should do about it.
Comment at: tools/scan-build-py/libscanbuild/runner.py:21
@@ +20,3 @@
+
+def run(opts):
+""" Entry point to run (or not) static analyzer against a single entry
dcoughlin wrote:
> This needs documentation on what opts can/should contain.
What I was asking for here is documentation about the 'opts' parameter. Where
does 'opts' come from? Is it json input from a compilation database? What keys
should it contain, what is meaning of their values, etc.
It is important to at least document the expected structure of the dictionary.
This documentation will help people understand your design and make sure they
don't accidentally break some invariant when fixing bugs or adding features in
the future.
http://reviews.llvm.org/D9600
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D9600: Add scan-build python implementation
dcoughlin added a comment. Thanks Laszlo! Is there a more descriptive name than "intercept-build" (I realize scan-build is pretty general too). It seems to me the point of the intercept-build tool is to generate the compilation database. I think it would be helpful if the tool name indicated that rather than the *method* used to to generate the database. I don't have any great suggestions: "compilation-database-build", "log-build", "log-compilation-build", ... A couple more comments inline. Comment at: tools/scan-build-py/MANIFEST.in:1 @@ +1,2 @@ +include README.md +include *.txt rizsotto.mailinglist wrote: > dcoughlin wrote: > > How about this one? Is it needed in clang trunk? > in order to make this code a standalone python tool tool, we need this file. > (see llvm/utils/lit directory for example.) Can you explain why this is needed? We didn't need one for the perl scan-build nor for SATestBuild.py/SATestAdd.py? A difference between lit and scan-build is that scan-build is not a standalone tool. Are you envisioning users installing scan-build without installing clang? Comment at: tools/scan-build-py/libear/__init__.py:1 @@ +1,2 @@ +# -*- coding: utf-8 -*- +# The LLVM Compiler Infrastructure jroelofs wrote: > rizsotto.mailinglist wrote: > > dcoughlin wrote: > > > How does this file fit into the overall build picture? Will this file go > > > away once scan-build-py is built with the common clang cmake? > > this is quiet confusing me. previously you were asking make it work without > > installation. this file makes it possible to compile the `ear` library > > compiled before the build runs to use as preloaded library. the thing which > > is not needed is the CMakefile actually. > I think the best way forward would be to teach the CMake build how to run > `setup.py`. Then this would work both with and without installation, and > it'd use the same code paths for both. > previously you were asking make it work without installation. Yes. It is very important to support that workflow since many users have multiple versions of clang on their systems at the same time. > this file makes it possible to compile the ear library compiled before the > build runs to use as preloaded library. Shouldn't this be done as part of the build process and not as part of installation? Can the interception library (eventually) be built as part of the global CMake build? It seems quite fragile to have custom build logic in a python file. Comment at: tools/scan-build-py/libear/__init__.py:1 @@ +1,2 @@ +# -*- coding: utf-8 -*- +# The LLVM Compiler Infrastructure dcoughlin wrote: > jroelofs wrote: > > rizsotto.mailinglist wrote: > > > dcoughlin wrote: > > > > How does this file fit into the overall build picture? Will this file > > > > go away once scan-build-py is built with the common clang cmake? > > > this is quiet confusing me. previously you were asking make it work > > > without installation. this file makes it possible to compile the `ear` > > > library compiled before the build runs to use as preloaded library. the > > > thing which is not needed is the CMakefile actually. > > I think the best way forward would be to teach the CMake build how to run > > `setup.py`. Then this would work both with and without installation, and > > it'd use the same code paths for both. > > previously you were asking make it work without installation. > > Yes. It is very important to support that workflow since many users have > multiple versions of clang on their systems at the same time. > > > this file makes it possible to compile the ear library compiled before the > > build runs to use as preloaded library. > > Shouldn't this be done as part of the build process and not as part of > installation? > > Can the interception library (eventually) be built as part of the global > CMake build? It seems quite fragile to have custom build logic in a python > file. jroelofs wrote: > I think the best way forward would be to teach the CMake build how to run > setup.py What is the benefit of running `setup.py` and treating scan-build as a python package? It seems to me that the fact that scan-build is written in python should be an implementation detail. Most importantly, installing scan-build shouldn't muck up the user's python environment. In my opinion, it would be more user-friendly to install into the new scan-build install hierarchy that jroelofs recently added. This way all the scan-build stuff is under one directory that the user can blow away rather than hidden deep in the bowels of some python environment. This makes it much easier to ensure that scan-build and clang are in sync and would prevent the user from accidentally installing scan-build into a virtualenv (which doesn't make sense because clang doesn't live there).
Re: [PATCH] D9600: Add scan-build python implementation
On 11/21/15 9:50 AM, Devin Coughlin wrote: dcoughlin added a comment. Thanks Laszlo! Is there a more descriptive name than "intercept-build" (I realize scan-build is pretty general too). It seems to me the point of the intercept-build tool is to generate the compilation database. I think it would be helpful if the tool name indicated that rather than the *method* used to to generate the database. I don't have any great suggestions: "compilation-database-build", "log-build", "log-compilation-build", ... A couple more comments inline. Comment at: tools/scan-build-py/MANIFEST.in:1 @@ +1,2 @@ +include README.md +include *.txt rizsotto.mailinglist wrote: dcoughlin wrote: How about this one? Is it needed in clang trunk? in order to make this code a standalone python tool tool, we need this file. (see llvm/utils/lit directory for example.) Can you explain why this is needed? We didn't need one for the perl scan-build nor for SATestBuild.py/SATestAdd.py? A difference between lit and scan-build is that scan-build is not a standalone tool. Are you envisioning users installing scan-build without installing clang? Comment at: tools/scan-build-py/libear/__init__.py:1 @@ +1,2 @@ +# -*- coding: utf-8 -*- +# The LLVM Compiler Infrastructure jroelofs wrote: rizsotto.mailinglist wrote: dcoughlin wrote: How does this file fit into the overall build picture? Will this file go away once scan-build-py is built with the common clang cmake? this is quiet confusing me. previously you were asking make it work without installation. this file makes it possible to compile the `ear` library compiled before the build runs to use as preloaded library. the thing which is not needed is the CMakefile actually. I think the best way forward would be to teach the CMake build how to run `setup.py`. Then this would work both with and without installation, and it'd use the same code paths for both. previously you were asking make it work without installation. Yes. It is very important to support that workflow since many users have multiple versions of clang on their systems at the same time. this file makes it possible to compile the ear library compiled before the build runs to use as preloaded library. Shouldn't this be done as part of the build process and not as part of installation? Can the interception library (eventually) be built as part of the global CMake build? It seems quite fragile to have custom build logic in a python file. Oh, good point. This is also important if we're cross building the toolchain... this library should get built with the same compiler that's building clang. Comment at: tools/scan-build-py/libear/__init__.py:1 @@ +1,2 @@ +# -*- coding: utf-8 -*- +# The LLVM Compiler Infrastructure dcoughlin wrote: jroelofs wrote: rizsotto.mailinglist wrote: dcoughlin wrote: How does this file fit into the overall build picture? Will this file go away once scan-build-py is built with the common clang cmake? this is quiet confusing me. previously you were asking make it work without installation. this file makes it possible to compile the `ear` library compiled before the build runs to use as preloaded library. the thing which is not needed is the CMakefile actually. I think the best way forward would be to teach the CMake build how to run `setup.py`. Then this would work both with and without installation, and it'd use the same code paths for both. previously you were asking make it work without installation. Yes. It is very important to support that workflow since many users have multiple versions of clang on their systems at the same time. this file makes it possible to compile the ear library compiled before the build runs to use as preloaded library. Shouldn't this be done as part of the build process and not as part of installation? Can the interception library (eventually) be built as part of the global CMake build? It seems quite fragile to have custom build logic in a python file. jroelofs wrote: I think the best way forward would be to teach the CMake build how to run setup.py What is the benefit of running `setup.py` and treating scan-build as a python package? It seems to me that the fact that scan-build is written in python should be an implementation detail. Most importantly, installing scan-build shouldn't muck up the user's python environment. In my opinion, it would be more user-friendly to install into the new scan-build install hierarchy that jroelofs recently added. This way all the scan-build stuff is under one directory that the user can blow away rather than hidden deep in the bowels of some python environment. This makes it much easier to ensure that scan-build and clang are in sync and would prevent the user from accidentally installing scan-build into a virtualenv (which doesn't make sense because clang doesn't
Re: [PATCH] D9600: Add scan-build python implementation
jroelofs added inline comments.
Comment at: tools/scan-build-py/libear/__init__.py:1
@@ +1,2 @@
+# -*- coding: utf-8 -*-
+# The LLVM Compiler Infrastructure
rizsotto.mailinglist wrote:
> dcoughlin wrote:
> > How does this file fit into the overall build picture? Will this file go
> > away once scan-build-py is built with the common clang cmake?
> this is quiet confusing me. previously you were asking make it work without
> installation. this file makes it possible to compile the `ear` library
> compiled before the build runs to use as preloaded library. the thing which
> is not needed is the CMakefile actually.
I think the best way forward would be to teach the CMake build how to run
`setup.py`. Then this would work both with and without installation, and it'd
use the same code paths for both.
Comment at: tools/scan-build-py/libscanbuild/driver.py:67
@@ +66,3 @@
+except Exception:
+logging.exception("Something unexpected had happened.")
+return 127
rizsotto.mailinglist wrote:
> dcoughlin wrote:
> > I think this error message can be improved. Perhaps "Unexpected error
> > running intercept-build"?
> this line is printed as:
>
> intercept-build: ERROR: Something unexpected had happened.
> (and the stack-trace)
>
> because the logging formating. so, 'intercept-build' and 'error' will be part
> of the message anyway.
Is there a pythonic way of doing llvm crash handlers? I.e. the "here's the
steps to reproduce this, a stack trace, and a bug report url" things that clang
spits out.
http://reviews.llvm.org/D9600
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D9600: Add scan-build python implementation
rizsotto.mailinglist marked 13 inline comments as done.
rizsotto.mailinglist added a comment.
thanks Devin for your comments. made some changes already (will upload it
tonight after some tests).
Comment at: tools/scan-build-py/CHANGES.txt:1
@@ +1,1 @@
+v, -- Initial release.
dcoughlin wrote:
> Is this one needed too?
in order to make this code a standalone python tool tool, we need this file.
(see llvm/utils/lit directory for example.)
Comment at: tools/scan-build-py/MANIFEST.in:1
@@ +1,2 @@
+include README.md
+include *.txt
dcoughlin wrote:
> How about this one? Is it needed in clang trunk?
in order to make this code a standalone python tool tool, we need this file.
(see llvm/utils/lit directory for example.)
Comment at: tools/scan-build-py/libear/__init__.py:1
@@ +1,2 @@
+# -*- coding: utf-8 -*-
+# The LLVM Compiler Infrastructure
dcoughlin wrote:
> How does this file fit into the overall build picture? Will this file go away
> once scan-build-py is built with the common clang cmake?
this is quiet confusing me. previously you were asking make it work without
installation. this file makes it possible to compile the `ear` library compiled
before the build runs to use as preloaded library. the thing which is not
needed is the CMakefile actually.
Comment at: tools/scan-build-py/libear/__init__.py:99
@@ +98,3 @@
+def dl_libraries(self):
+pass
+
dcoughlin wrote:
> I gather the intent is that subclasses will override to provide their own
> versions of these methods? If so, these methods need to be documented so that
> people adding new support for additional platforms know what they should
> provide in their subclasses.
>
> If there are reasonable defaults (for example., `[]` for `dl_libraries`),
> those should be returned here rather than `pass`. If not, these should
> probably raise an exception indicating they must be implemented rather than
> silently doing nothing.
now rise `NotImplementedError` runtime exception.
Comment at: tools/scan-build-py/libear/__init__.py:166
@@ +165,3 @@
+self.ctx = context
+self.results = {'APPLE': sys.platform == 'darwin'}
+
dcoughlin wrote:
> What does this do? Why is it hard-coded?
this is added to mimic `cmake` behaviour. it is used in the `config.h.in` file.
Comment at: tools/scan-build-py/libscanbuild/command.py:20
@@ +19,3 @@
+
+def classify_parameters(command):
+""" Parses the command line arguments of the given invocation. """
dcoughlin wrote:
> I think it would be good to document the keys and meaning of the returned
> dictionary. Or perhaps it would be better represented as class?
now documented when create the return value. (creating class would not bring
much to the kitchen i think.)
Comment at: tools/scan-build-py/libscanbuild/command.py:23
@@ +22,3 @@
+
+ignored = {
+'-g': 0,
dcoughlin wrote:
> I think it would good to document what the value in this mapping means
> (number of expected parameters). I realize ccc-analyzer in the original
> scan-build is similarly un-documented, but we should do better here!
>
> Also: should this include all the arguments `IgnoredOptionMap` in
> ccc-analyzer? It is missing `-u' and adds '-g'. Or are these changes
> intentional?
`-u` is part of ignored linker flags. (see a few line above)
`-g` is added to mimic the `ccc-analyzer` results.
comment about key, value is added.
Comment at: tools/scan-build-py/libscanbuild/driver.py:1
@@ +1,2 @@
+# -*- coding: utf-8 -*-
+# The LLVM Compiler Infrastructure
dcoughlin wrote:
> Why is this file called "driver"?
any recommendation? it was the only entry point before the interposition was
introduced. so it was the driver of the libscanbuild library.
Comment at: tools/scan-build-py/libscanbuild/driver.py:34
@@ +33,3 @@
+def main(bin_dir):
+""" Entry point for 'scan-build'. """
+
dcoughlin wrote:
> Should this be 'intercept-build'?
can be anything, but would make it rhyme with the module name...
Comment at: tools/scan-build-py/libscanbuild/driver.py:67
@@ +66,3 @@
+except Exception:
+logging.exception("Something unexpected had happened.")
+return 127
dcoughlin wrote:
> I think this error message can be improved. Perhaps "Unexpected error running
> intercept-build"?
this line is printed as:
intercept-build: ERROR: Something unexpected had happened.
(and the stack-trace)
because the logging formating. so, 'intercept-build' and 'error' will be part
of the message anyway.
Comment at: tools/scan-build-py/libscanbuild/intercept.py:98
@@ +97,3 @@
+
+if args.override_compiler or not ear_library_path:
Re: [PATCH] D9600: Add scan-build python implementation
dcoughlin added a comment.
Hi Laszlo, thanks for the update!
Some high-level questions/comments (and various small things I noticed inline).
- I tried running scan-build in interposition mode (i.e., uncommenting out
#"$SCRIPT_DIR/analyze-build" $@ in scan-build) and got python compile errors.
When you did your testing to compare output with the old scan-build, did you
use this mode?
- When I run scan-build in intercept-build mode on openssl-1.0 on OS X I get
compile errors. Is this one of the projects you tested with? When I run in it
in analyze-build mode I get diffs with CmpRuns.py -- it looks like might be
because some paths are absolute. What kind of fidelity with the old scan-build
do expect at this point?
- What is the point of intercept-cc/intercept-c++? I thought libear didn't need
to set CC/CXX environmental variables.
- Do all the files in this patch need to be committed into the clang repo?
There seem to be some extraneous files (I've asked inline about the ones that
don't seem necessary.)
- I found it difficult to understand how all the modules and command-line tools
fit together. Can the module names be made more harmonious with the
command-line tool names(the scripts in bin). It is not at all obvious which
modules belong to which tools, etc. I think it would be good to document this
in the code, as well.
Comment at: tools/scan-build-py/.travis.yml:1
@@ +1,2 @@
+language: python
+
Is this file needed in clang trunk?
Comment at: tools/scan-build-py/CHANGES.txt:1
@@ +1,1 @@
+v, -- Initial release.
Is this one needed too?
Comment at: tools/scan-build-py/MANIFEST.in:1
@@ +1,2 @@
+include README.md
+include *.txt
How about this one? Is it needed in clang trunk?
Comment at: tools/scan-build-py/README.md:70
@@ +69,3 @@
+---
+If you find a bug in this documentation or elsewhere in the program or would
+like to propose an improvement, please use the project's [github issue
This should probably point to llvm bugzilla.
https://llvm.org/bugs/enter_bug.cgi?product=clang
Comment at: tools/scan-build-py/bin/scan-build:13
@@ +12,3 @@
+#
+#"$SCRIPT_DIR/analyze-build" $@
+#"$SCRIPT_DIR/intercept-build" all $@
These should be controlled by flags rather than by commenting the required
behavior in or out.
I also think analyze-build should be the default behavior, to maintain
compatibility with the existing scan-build.
Comment at: tools/scan-build-py/libear/__init__.py:1
@@ +1,2 @@
+# -*- coding: utf-8 -*-
+# The LLVM Compiler Infrastructure
How does this file fit into the overall build picture? Will this file go away
once scan-build-py is built with the common clang cmake?
Comment at: tools/scan-build-py/libear/__init__.py:79
@@ +78,3 @@
+class Context(object):
+""" Abstract class to represent different toolset. """
+
Maybe "Toolset" would be a more descriptive name than "Context"?
Comment at: tools/scan-build-py/libear/__init__.py:99
@@ +98,3 @@
+def dl_libraries(self):
+pass
+
I gather the intent is that subclasses will override to provide their own
versions of these methods? If so, these methods need to be documented so that
people adding new support for additional platforms know what they should
provide in their subclasses.
If there are reasonable defaults (for example., `[]` for `dl_libraries`), those
should be returned here rather than `pass`. If not, these should probably raise
an exception indicating they must be implemented rather than silently doing
nothing.
Comment at: tools/scan-build-py/libear/__init__.py:151
@@ +150,3 @@
+@contextlib.contextmanager
+def make_context(src_dir):
+platform = sys.platform
Why is this a generator? There is no functionality after yield.
Comment at: tools/scan-build-py/libear/__init__.py:166
@@ +165,3 @@
+self.ctx = context
+self.results = {'APPLE': sys.platform == 'darwin'}
+
What does this do? Why is it hard-coded?
Comment at: tools/scan-build-py/libear/__init__.py:224
@@ +223,3 @@
+def do_configure(context):
+yield Configure(context)
+
Why is this a generator?
Comment at: tools/scan-build-py/libear/__init__.py:262
@@ +261,2 @@
+def create_shared_library(name, context):
+yield SharedLibrary(name, context)
Why is this a generator?
Comment at: tools/scan-build-py/libscanbuild/clang.py:33
@@ +32,3 @@
+
+This method receives the full command line for direct compilation. And
+it generates the command for indirect compilation. """
I think it would be more accurate to say that this method returns the front-end
Re: [PATCH] D9600: Add scan-build python implementation
jroelofs accepted this revision. jroelofs added a reviewer: jroelofs. jroelofs added a comment. This revision is now accepted and ready to land. Sounds reasonable and I can help with setting up the CMake & LIT goop once you've got it committed. FWIW, I was able to get a simple example working by doing: $ mkdir -p $build/lib/python2.7/site-packages $ python setup.py build $ PYTHONPATH=$build/lib/python2.7/site-packages python setup.py install --prefix=$build $ cd path/to/example $ PYTHONPATH=$build/lib/python2.7/site-packages $build/bin/scan-build --override-compiler --use-cc $build/bin/clang --use-analyzer $build/bin/clang make I wasn't able to get the libear interceptor to work on my Darwin machine, but that can be debugged later. http://reviews.llvm.org/D9600 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D9600: Add scan-build python implementation
rizsotto.mailinglist added a comment. In http://reviews.llvm.org/D9600#290031, @jroelofs wrote: > Thanks for re-uploading! thanks for your comments! :) most of your comments are about to embed it more into the clang build, test infrastructure. i think these are valid points! i would like to have these code in the repository first and to do -the work you've proposed- afterwards. i might need help for some of those too. (like using lint to run python unit tests.) http://reviews.llvm.org/D9600 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D9600: Add scan-build python implementation
jroelofs added a comment. Thanks for re-uploading! Comment at: tools/scan-build-py/README.md:39 @@ +38,3 @@ + +$ python setup.py build +$ python setup.py install Mind adding a CMakeLists.txt to drive these from the clang build itself? Comment at: tools/scan-build-py/libscanbuild/__init__.py:7 @@ +6,3 @@ +""" +This module responsible to run the Clang static analyzer against any build +and generate reports. I think most of this block comment belongs in a new file: clang/docs/ScanBuild.rst Comment at: tools/scan-build-py/tests/functional/src/build/Makefile:1 @@ +1,2 @@ +SRCDIR := .. +OBJDIR := . It'd probably be a good idea to structure these test inputs into their own "projects" so that support for more build system can be added later. I'd suggest something like: tools/scan-build-py/tests/functional/ simple_makefile src clean-one.c broken-two.c clean-one.c clean-two.c build Makefile cmake_makefiles src ... build CMakeLists.txt cmake_ninja src ... build CMakeLists.txt Comment at: tools/scan-build-py/tests/unit/__init__.py:16 @@ +15,3 @@ + +def load_tests(loader, suite, pattern): +suite.addTests(loader.loadTestsFromModule(test_command)) Mind hooking these up so that a LIT test harness can run them too? We use LIT to test pretty much everything else in the llvm project, and it'd be a shame to have a tool with a completely different way of running tests... http://reviews.llvm.org/D9600 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D9600: Add scan-build python implementation
rizsotto.mailinglist added a comment. In http://reviews.llvm.org/D9600#287157, @jroelofs wrote: > Would you mind re-uploading this patch as a diff against upstream trunk with > full context? i'm not sure i do understand what do you ask. i wish i could upload these changes as a single patch, but don't know is that possible? http://reviews.llvm.org/D9600 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D9600: Add scan-build python implementation
jroelofs added a comment. If you you're familiar with git, I'm asking you to: squash, rebase, and `git diff -U999`, then upload the resulting patch. If you use svn, I'm asking you to: `svn diff -r$(FirstCommit):$(LastCommit) --diff-cmd=diff -x -U999`, and upload that. The reason to squash all your patches together is that partial diffs from a patch series don't work well with Phabricator's "Revision Update History" tool. Also, it makes it hard to pull out a patch that I can apply on my local tree to try it out. http://reviews.llvm.org/D9600 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D9600: Add scan-build python implementation
jroelofs added a subscriber: jroelofs. jroelofs added a comment. Would you mind re-uploading this patch as a diff against upstream trunk with full context? http://reviews.llvm.org/D9600 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D9600: Add scan-build python implementation
rizsotto.mailinglist updated this revision to Diff 39703.
rizsotto.mailinglist added a comment.
findings from compare from older Perl implementation
http://reviews.llvm.org/D9600
Files:
tools/scan-build-py/README.md
tools/scan-build-py/libear/CMakeLists.txt
tools/scan-build-py/libear/__init__.py
tools/scan-build-py/libear/config.h.in
tools/scan-build-py/libear/ear.c
tools/scan-build-py/libscanbuild/clang.py
tools/scan-build-py/libscanbuild/command.py
tools/scan-build-py/libscanbuild/driver.py
tools/scan-build-py/libscanbuild/intercept.py
tools/scan-build-py/libscanbuild/interposition.py
tools/scan-build-py/libscanbuild/options.py
tools/scan-build-py/libscanbuild/report.py
tools/scan-build-py/libscanbuild/runner.py
tools/scan-build-py/libscanbuild/shell.py
tools/scan-build-py/setup.py
tools/scan-build-py/tests/functional/cases/__init__.py
tools/scan-build-py/tests/functional/cases/test_from_cdb.py
tools/scan-build-py/tests/functional/cases/test_from_cmd.py
tools/scan-build-py/tests/functional/exec/main.c
tools/scan-build-py/tests/functional/src/build/Makefile
tools/scan-build-py/tests/unit/__init__.py
tools/scan-build-py/tests/unit/test_clang.py
tools/scan-build-py/tests/unit/test_command.py
tools/scan-build-py/tests/unit/test_intercept.py
tools/scan-build-py/tests/unit/test_runner.py
tools/scan-build-py/tests/unit/test_shell.py
Index: tools/scan-build-py/tests/unit/test_shell.py
===
--- /dev/null
+++ tools/scan-build-py/tests/unit/test_shell.py
@@ -0,0 +1,42 @@
+# -*- coding: utf-8 -*-
+# The LLVM Compiler Infrastructure
+#
+# This file is distributed under the University of Illinois Open Source
+# License. See LICENSE.TXT for details.
+
+import libscanbuild.shell as sut
+import unittest
+
+
+class ShellTest(unittest.TestCase):
+
+def test_encode_decode_are_same(self):
+def test(value):
+self.assertEqual(sut.encode(sut.decode(value)), value)
+
+test("")
+test("clang")
+test("clang this and that")
+
+def test_decode_encode_are_same(self):
+def test(value):
+self.assertEqual(sut.decode(sut.encode(value)), value)
+
+test([])
+test(['clang'])
+test(['clang', 'this', 'and', 'that'])
+test(['clang', 'this and', 'that'])
+test(['clang', "it's me", 'again'])
+test(['clang', 'some "words" are', 'quoted'])
+
+def test_encode(self):
+self.assertEqual(sut.encode(['clang', "it's me", 'again']),
+ 'clang "it\'s me" again')
+self.assertEqual(sut.encode(['clang', "it(s me", 'again)']),
+ 'clang "it(s me" "again)"')
+self.assertEqual(sut.encode(['clang', 'redirect > it']),
+ 'clang "redirect > it"')
+self.assertEqual(sut.encode(['clang', '-DKEY="VALUE"']),
+ 'clang -DKEY=\\"VALUE\\"')
+self.assertEqual(sut.encode(['clang', '-DKEY="value with spaces"']),
+ 'clang -DKEY=\\"value with spaces\\"')
Index: tools/scan-build-py/tests/unit/test_runner.py
===
--- tools/scan-build-py/tests/unit/test_runner.py
+++ tools/scan-build-py/tests/unit/test_runner.py
@@ -141,18 +141,19 @@
def test(expected, input):
spy = fixtures.Spy()
self.assertEqual(spy.success, sut.language_check(input, spy.call))
-self.assertEqual(expected, spy.arg)
+self.assertEqual(expected, spy.arg['language'])
l = 'language'
f = 'file'
-i = 'cxx'
-test({f: 'file.c', l: 'c'}, {f: 'file.c', l: 'c'})
-test({f: 'file.c', l: 'c++'}, {f: 'file.c', l: 'c++'})
-test({f: 'file.c', l: 'c++', i: True}, {f: 'file.c', i: True})
-test({f: 'file.c', l: 'c'}, {f: 'file.c'})
-test({f: 'file.cxx', l: 'c++'}, {f: 'file.cxx'})
-test({f: 'file.i', l: 'c-cpp-output'}, {f: 'file.i'})
-test({f: 'f.i', l: 'c-cpp-output'}, {f: 'f.i', l: 'c-cpp-output'})
+i = 'c++'
+test('c', {f: 'file.c', l: 'c', i: False})
+test('c++', {f: 'file.c', l: 'c++', i: False})
+test('c++', {f: 'file.c', i: True})
+test('c', {f: 'file.c', i: False})
+test('c++', {f: 'file.cxx', i: False})
+test('c-cpp-output', {f: 'file.i', i: False})
+test('c++-cpp-output', {f: 'file.i', i: True})
+test('c-cpp-output', {f: 'f.i', l: 'c-cpp-output', i: True})
def test_arch_loop(self):
def test(input):
@@ -163,16 +164,16 @@
input = {'key': 'value'}
self.assertEqual(input, test(input))
-input = {'archs_seen': ['-arch', 'i386']}
+input = {'archs_seen': ['i386']}
self.assertEqual({'arch': 'i386'}, test(input))
-input = {'archs_seen': ['-arch', 'ppc']}
+input =
Re: [PATCH] D9600: Add scan-build python implementation
rizsotto.mailinglist added a comment. the latest update has the following improvements: - makes it work without installation. - pass the `SATestBuild.py` test harness against different projects with `--strictness 2` arguments. (coreutils-8.23, gawk-4.1.1, make-4.0, openssl-1.0.0s, patch-2.7.4, tar-1.28) these minor adjustments were needed. - get silents when it runs against `configure` or `autogen`. - implements `--override-compiler` which enforce to use compiler wrappers. - a couple of minor fixes to get rid of the differences. - small number of improvement for compilation database generation (mainly shell escaping when that's needed). which were learnt form my other project Bear. http://reviews.llvm.org/D9600 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D9600: Add scan-build python implementation
aaron.ballman added a subscriber: aaron.ballman. aaron.ballman added a comment. I'm wondering about the status of this, since I've not heard much in the past few months. Is this patch still progressing? (I hope so, I would really love to see us drop our reliance on Perl!) http://reviews.llvm.org/D9600 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
