Re: PING: Re: [PATCH] Add analyzer plugin support and CPython GIL example

2020-11-23 Thread Jeff Law via Gcc-patches 



On 11/10/20 2:51 PM, David Malcolm via Gcc-patches wrote:
> I'd like to ping this patch:
>   https://gcc.gnu.org/pipermail/gcc-patches/2020-October/556214.html
> Are the non-analyzer parts OK for master?
Just looking for an ACK on the analyzer_init notification bits in the
generic parts of the compiler?  If so, yea, that's fine.  Sorry for the
delay.

jeff

PING: Re: [PATCH] Add analyzer plugin support and CPython GIL example

2020-11-10 Thread David Malcolm via Gcc-patches 
I'd like to ping this patch:
  https://gcc.gnu.org/pipermail/gcc-patches/2020-October/556214.html
Are the non-analyzer parts OK for master?

Thanks
Dave

On Wed, 2020-10-14 at 21:21 -0400, David Malcolm wrote:
> This patch adds a new GCC plugin event: PLUGIN_ANALYZER_INIT, called
> when -fanalyzer is starting, allowing for GCC plugins to register
> additional state-machine-based checks within -fanalyzer.  The idea
> is that 3rd-party code might want to add domain-specific checks for
> its own APIs - with the caveat that the analyzer is itself still
> rather experimental.
> 
> As an example, the patch adds a proof-of-concept plugin to the
> testsuite
> for checking CPython code: verifying that code that relinquishes
> CPython's global interpreter lock doesn't attempt to do anything with
> PyObjects in the sections where the lock isn't held.  It also adds a
> warning about nested releases of the lock, which is forbidden.
> For example:
> 
> demo.c: In function 'foo':
> demo.c:11:3: warning: use of PyObject '*(obj)' without the GIL
>11 |   Py_INCREF (obj);
>   |   ^
>   'test': events 1-3
> |
> |   15 | void test (PyObject *obj)
> |  |  ^~~~
> |  |  |
> |  |  (1) entry to 'test'
> |   16 | {
> |   17 |   Py_BEGIN_ALLOW_THREADS
> |  |   ~~
> |  |   |
> |  |   (2) releasing the GIL here
> |   18 |   foo (obj);
> |  |   ~
> |  |   |
> |  |   (3) calling 'foo' from 'test'
> |
> +--> 'foo': events 4-5
>|
>|9 | foo (PyObject *obj)
>|  | ^~~
>|  | |
>|  | (4) entry to 'foo'
>|   10 | {
>|   11 |   Py_INCREF (obj);
>|  |   ~
>|  |   |
>|  |   (5) PyObject '*(obj)' used here without the GIL
>|
> 
> Doing so requires adding some logic for ignoring macro expansions in
> analyzer diagnostics, since the insides of Py_INCREF and
> Py_BEGIN_ALLOW_THREADS are not of interest to the user for these
> cases.
> 
> Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.
> 
> Are the non-analyzer parts OK for master?
> 
> gcc/analyzer/ChangeLog:
>   * analyzer-pass.cc (pass_analyzer::execute): Move sorry call
> to...
>   (sorry_no_analyzer): New.
>   * analyzer.h (class state_machine): New forward decl.
>   (class logger): New forward decl.
>   (class plugin_analyzer_init_iface): New.
>   (sorry_no_analyzer): New decl.
>   * checker-path.cc (checker_path::fixup_locations): New.
>   * checker-path.h (checker_event::set_location): New.
>   (checker_path::fixup_locations): New decl.
>   * diagnostic-manager.cc
>   (diagnostic_manager::emit_saved_diagnostic): Call
>   checker_path::fixup_locations, and call fixup_location
>   on the primary location.
>   * engine.cc: Include "plugin.h".
>   (class plugin_analyzer_init_impl): New.
>   (impl_run_checkers): Invoke PLUGIN_ANALYZER_INIT callbacks.
>   * pending-diagnostic.h (pending_diagnostic::fixup_location):
> New
>   vfunc.
> 
> gcc/ChangeLog:
>   * doc/plugins.texi (Plugin callbacks): Add
> PLUGIN_ANALYZER_INIT.
>   * plugin.c (register_callback): Likewise.
>   (invoke_plugin_callbacks_full): Likewise.
>   * plugin.def (PLUGIN_ANALYZER_INIT): New event.
> 
> gcc/testsuite/ChangeLog:
>   * gcc.dg/plugin/analyzer_gil_plugin.c: New test.
>   * gcc.dg/plugin/gil-1.c: New test.
>   * gcc.dg/plugin/gil.h: New header.
>   * gcc.dg/plugin/plugin.exp (plugin_test_list): Add the new
> plugin
>   and test.
> ---
>  gcc/analyzer/analyzer-pass.cc |  18 +-
>  gcc/analyzer/analyzer.h   |  15 +
>  gcc/analyzer/checker-path.cc  |   9 +
>  gcc/analyzer/checker-path.h   |   4 +
>  gcc/analyzer/diagnostic-manager.cc|   9 +-
>  gcc/analyzer/engine.cc|  31 ++
>  gcc/analyzer/pending-diagnostic.h |   8 +
>  gcc/doc/plugins.texi  |   4 +
>  gcc/plugin.c  |   2 +
>  gcc/plugin.def|   4 +
>  .../gcc.dg/plugin/analyzer_gil_plugin.c   | 436
> ++
>  gcc/testsuite/gcc.dg/plugin/gil-1.c   |  90 
>  gcc/testsuite/gcc.dg/plugin/gil.h |  32 ++
>  gcc/testsuite/gcc.dg/plugin/plugin.exp|   2 +
>  14 files changed, 660 insertions(+), 4 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.dg/plugin/analyzer_gil_plugin.c
>  create mode 100644 gcc/testsuite/gcc.dg/plugin/gil-1.c
>  create mode 100644 gcc/testsuite/gcc.dg/plugin/gil.h
> 
> diff --git a/gcc/analyzer/analyzer-pass.cc b/gcc/analyzer/analyzer-
> pass.cc
> index a27421e46d4..1f65bf8b154 100644
> --- a/gcc/analyzer/analyzer-pass.cc
> +++ b/gcc/analyzer/analyzer-pass.cc
> @@ -83,9

[PATCH] Add analyzer plugin support and CPython GIL example

2020-10-14 Thread David Malcolm via Gcc-patches 
This patch adds a new GCC plugin event: PLUGIN_ANALYZER_INIT, called
when -fanalyzer is starting, allowing for GCC plugins to register
additional state-machine-based checks within -fanalyzer.  The idea
is that 3rd-party code might want to add domain-specific checks for
its own APIs - with the caveat that the analyzer is itself still
rather experimental.

As an example, the patch adds a proof-of-concept plugin to the testsuite
for checking CPython code: verifying that code that relinquishes
CPython's global interpreter lock doesn't attempt to do anything with
PyObjects in the sections where the lock isn't held.  It also adds a
warning about nested releases of the lock, which is forbidden.
For example:

demo.c: In function 'foo':
demo.c:11:3: warning: use of PyObject '*(obj)' without the GIL
   11 |   Py_INCREF (obj);
  |   ^
  'test': events 1-3
|
|   15 | void test (PyObject *obj)
|  |  ^~~~
|  |  |
|  |  (1) entry to 'test'
|   16 | {
|   17 |   Py_BEGIN_ALLOW_THREADS
|  |   ~~
|  |   |
|  |   (2) releasing the GIL here
|   18 |   foo (obj);
|  |   ~
|  |   |
|  |   (3) calling 'foo' from 'test'
|
+--> 'foo': events 4-5
   |
   |9 | foo (PyObject *obj)
   |  | ^~~
   |  | |
   |  | (4) entry to 'foo'
   |   10 | {
   |   11 |   Py_INCREF (obj);
   |  |   ~
   |  |   |
   |  |   (5) PyObject '*(obj)' used here without the GIL
   |

Doing so requires adding some logic for ignoring macro expansions in
analyzer diagnostics, since the insides of Py_INCREF and
Py_BEGIN_ALLOW_THREADS are not of interest to the user for these cases.

Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.

Are the non-analyzer parts OK for master?

gcc/analyzer/ChangeLog:
* analyzer-pass.cc (pass_analyzer::execute): Move sorry call to...
(sorry_no_analyzer): New.
* analyzer.h (class state_machine): New forward decl.
(class logger): New forward decl.
(class plugin_analyzer_init_iface): New.
(sorry_no_analyzer): New decl.
* checker-path.cc (checker_path::fixup_locations): New.
* checker-path.h (checker_event::set_location): New.
(checker_path::fixup_locations): New decl.
* diagnostic-manager.cc
(diagnostic_manager::emit_saved_diagnostic): Call
checker_path::fixup_locations, and call fixup_location
on the primary location.
* engine.cc: Include "plugin.h".
(class plugin_analyzer_init_impl): New.
(impl_run_checkers): Invoke PLUGIN_ANALYZER_INIT callbacks.
* pending-diagnostic.h (pending_diagnostic::fixup_location): New
vfunc.

gcc/ChangeLog:
* doc/plugins.texi (Plugin callbacks): Add PLUGIN_ANALYZER_INIT.
* plugin.c (register_callback): Likewise.
(invoke_plugin_callbacks_full): Likewise.
* plugin.def (PLUGIN_ANALYZER_INIT): New event.

gcc/testsuite/ChangeLog:
* gcc.dg/plugin/analyzer_gil_plugin.c: New test.
* gcc.dg/plugin/gil-1.c: New test.
* gcc.dg/plugin/gil.h: New header.
* gcc.dg/plugin/plugin.exp (plugin_test_list): Add the new plugin
and test.
---
 gcc/analyzer/analyzer-pass.cc |  18 +-
 gcc/analyzer/analyzer.h   |  15 +
 gcc/analyzer/checker-path.cc  |   9 +
 gcc/analyzer/checker-path.h   |   4 +
 gcc/analyzer/diagnostic-manager.cc|   9 +-
 gcc/analyzer/engine.cc|  31 ++
 gcc/analyzer/pending-diagnostic.h |   8 +
 gcc/doc/plugins.texi  |   4 +
 gcc/plugin.c  |   2 +
 gcc/plugin.def|   4 +
 .../gcc.dg/plugin/analyzer_gil_plugin.c   | 436 ++
 gcc/testsuite/gcc.dg/plugin/gil-1.c   |  90 
 gcc/testsuite/gcc.dg/plugin/gil.h |  32 ++
 gcc/testsuite/gcc.dg/plugin/plugin.exp|   2 +
 14 files changed, 660 insertions(+), 4 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/plugin/analyzer_gil_plugin.c
 create mode 100644 gcc/testsuite/gcc.dg/plugin/gil-1.c
 create mode 100644 gcc/testsuite/gcc.dg/plugin/gil.h

diff --git a/gcc/analyzer/analyzer-pass.cc b/gcc/analyzer/analyzer-pass.cc
index a27421e46d4..1f65bf8b154 100644
--- a/gcc/analyzer/analyzer-pass.cc
+++ b/gcc/analyzer/analyzer-pass.cc
@@ -83,9 +83,7 @@ pass_analyzer::execute (function *)
 #if ENABLE_ANALYZER
   ana::run_checkers ();
 #else
-  sorry ("%qs was not enabled in this build of GCC"
-" (missing configure-time option %qs)",
-"-fanalyzer", "--enable-analyzer");
+  sorry_no_analyzer ();
 #endif
 
   return 0;
@@ -100,3 +98,17 @@ make_pass_analyzer (gcc::context *ctxt)
 {
   return new pass_analyzer (ctxt);
 }
+