Re: [PATCH] analyzer: stash values for CPython plugin [PR107646]
Hi Dave,
Thank you for the feedback! I've incorporated the changes and sent a
revised version of the patch.
On Tue, Aug 1, 2023 at 1:02 PM David Malcolm wrote:
>
> On Tue, 2023-08-01 at 09:52 -0400, Eric Feng wrote:
> > Hi all,
> >
> > This patch adds a hook to the end of ana::on_finish_translation_unit
> > which calls relevant stashing-related callbacks registered during
> > plugin
> > initialization. This feature is used to stash named types and global
> > variables for a CPython analyzer plugin [PR107646].
> >
> > Bootstrapped and tested on aarch64-unknown-linux-gnu. Does it look
> > okay?
>
> Hi Eric, thanks for the patch.
>
> The patch touches the C frontend, so those parts would need approval
> from the C FE maintainers/reviewers; I've CCed them.
>
> Overall, I like the patch, but it's not ready for trunk yet; various
> comments inline below...
>
> >
> > ---
> >
> > gcc/analyzer/ChangeLog:
>
> You could add: PR analyzer/107646 to these ChangeLog entries; have a
> look at how other ChangeLog entries refer to such bugzilla entries.
>
> >
> > * analyzer-language.cc (run_callbacks): New function.
> > (on_finish_translation_unit): New function.
> > * analyzer-language.h (GCC_ANALYZER_LANGUAGE_H): New include.
> > (class translation_unit): New vfuncs.
> >
> > gcc/c/ChangeLog:
> >
> > * c-parser.cc: New functions.
>
> I think this ChangeLog entry needs more detail.
Added in revised version of the patch.
> >
> > gcc/testsuite/ChangeLog:
> >
> > * gcc.dg/plugin/analyzer_cpython_plugin.c: New test.
> >
> > Signed-off-by: Eric Feng
> > ---
> > gcc/analyzer/analyzer-language.cc | 22 ++
> > gcc/analyzer/analyzer-language.h | 9 +
> > gcc/c/c-parser.cc | 26 ++
> > .../gcc.dg/plugin/analyzer_cpython_plugin.c | 224
> > ++
> > 4 files changed, 281 insertions(+)
> > create mode 100644
> > gcc/testsuite/gcc.dg/plugin/analyzer_cpython_plugin.c
> >
> > diff --git a/gcc/analyzer/analyzer-language.cc
> > b/gcc/analyzer/analyzer-language.cc
> > index 2c8910906ee..fc41b9c17b8 100644
> > --- a/gcc/analyzer/analyzer-language.cc
> > +++ b/gcc/analyzer/analyzer-language.cc
> > @@ -35,6 +35,26 @@ static GTY (()) hash_map
> > *analyzer_stashed_constants;
> > #if ENABLE_ANALYZER
> >
> > namespace ana {
> > +static vec
> > +*finish_translation_unit_callbacks;
> > +
> > +void
> > +register_finish_translation_unit_callback (
> > +finish_translation_unit_callback callback)
> > +{
> > + if (!finish_translation_unit_callbacks)
> > +vec_alloc (finish_translation_unit_callbacks, 1);
> > + finish_translation_unit_callbacks->safe_push (callback);
> > +}
> > +
> > +void
> > +run_callbacks (logger *logger, const translation_unit )
>
> This function could be "static" since it's not needed outside of
> analyzer-language.cc
>
> > +{
> > + for (auto const : finish_translation_unit_callbacks)
> > +{
> > + cb (logger, tu);
> > +}
> > +}
> >
> > /* Call into TU to try to find a value for NAME.
> > If found, stash its value within analyzer_stashed_constants. */
> > @@ -102,6 +122,8 @@ on_finish_translation_unit (const
> > translation_unit )
> > the_logger.set_logger (new logger (logfile, 0, 0,
> > *global_dc->printer));
> >stash_named_constants (the_logger.get_logger (), tu);
> > +
> > + run_callbacks (the_logger.get_logger (), tu);
> > }
> >
> > /* Lookup NAME in the named constants stashed when the frontend TU
> > finished.
> > diff --git a/gcc/analyzer/analyzer-language.h
> > b/gcc/analyzer/analyzer-language.h
> > index 00f85aba041..8deea52d627 100644
> > --- a/gcc/analyzer/analyzer-language.h
> > +++ b/gcc/analyzer/analyzer-language.h
> > @@ -21,6 +21,8 @@ along with GCC; see the file COPYING3. If not see
> > #ifndef GCC_ANALYZER_LANGUAGE_H
> > #define GCC_ANALYZER_LANGUAGE_H
> >
> > +#include "analyzer/analyzer-logging.h"
> > +
> > #if ENABLE_ANALYZER
> >
> > namespace ana {
> > @@ -35,8 +37,15 @@ class translation_unit
> > have been seen). If it is defined and an integer (e.g. either
> > as a
> > macro or enum), return the INTEGER_CST value, otherwise return
> > NULL. */
> >virtual tree lookup_constant_by_id (tree id) const = 0;
> > + virtual tree lookup_type_by_id (tree id) const = 0;
> > + virtual tree lookup_global_var_by_id (tree id) const = 0;
> > };
> >
> > +typedef void (*finish_translation_unit_callback)
> > + (logger *, const translation_unit &);
> > +void register_finish_translation_unit_callback (
> > +finish_translation_unit_callback callback);
> > +
> > /* Analyzer hook for frontends to call at the end of the TU. */
> >
> > void on_finish_translation_unit (const translation_unit );
> > diff --git a/gcc/c/c-parser.cc b/gcc/c/c-parser.cc
> > index 80920b31f83..f0ee55e416b 100644
> > --- a/gcc/c/c-parser.cc
> > +++ b/gcc/c/c-parser.cc
> > @@ -1695,6 +1695,32 @@ public:
> > return NULL_TREE;
> >}
> >
> >
Re: [PATCH] analyzer: stash values for CPython plugin [PR107646]
On Tue, 2023-08-01 at 09:52 -0400, Eric Feng wrote:
> Hi all,
>
> This patch adds a hook to the end of ana::on_finish_translation_unit
> which calls relevant stashing-related callbacks registered during
> plugin
> initialization. This feature is used to stash named types and global
> variables for a CPython analyzer plugin [PR107646].
>
> Bootstrapped and tested on aarch64-unknown-linux-gnu. Does it look
> okay?
Hi Eric, thanks for the patch.
The patch touches the C frontend, so those parts would need approval
from the C FE maintainers/reviewers; I've CCed them.
Overall, I like the patch, but it's not ready for trunk yet; various
comments inline below...
>
> ---
>
> gcc/analyzer/ChangeLog:
You could add: PR analyzer/107646 to these ChangeLog entries; have a
look at how other ChangeLog entries refer to such bugzilla entries.
>
> * analyzer-language.cc (run_callbacks): New function.
> (on_finish_translation_unit): New function.
> * analyzer-language.h (GCC_ANALYZER_LANGUAGE_H): New include.
> (class translation_unit): New vfuncs.
>
> gcc/c/ChangeLog:
>
> * c-parser.cc: New functions.
I think this ChangeLog entry needs more detail.
>
> gcc/testsuite/ChangeLog:
>
> * gcc.dg/plugin/analyzer_cpython_plugin.c: New test.
>
> Signed-off-by: Eric Feng
> ---
> gcc/analyzer/analyzer-language.cc | 22 ++
> gcc/analyzer/analyzer-language.h | 9 +
> gcc/c/c-parser.cc | 26 ++
> .../gcc.dg/plugin/analyzer_cpython_plugin.c | 224
> ++
> 4 files changed, 281 insertions(+)
> create mode 100644
> gcc/testsuite/gcc.dg/plugin/analyzer_cpython_plugin.c
>
> diff --git a/gcc/analyzer/analyzer-language.cc
> b/gcc/analyzer/analyzer-language.cc
> index 2c8910906ee..fc41b9c17b8 100644
> --- a/gcc/analyzer/analyzer-language.cc
> +++ b/gcc/analyzer/analyzer-language.cc
> @@ -35,6 +35,26 @@ static GTY (()) hash_map
> *analyzer_stashed_constants;
> #if ENABLE_ANALYZER
>
> namespace ana {
> +static vec
> + *finish_translation_unit_callbacks;
> +
> +void
> +register_finish_translation_unit_callback (
> + finish_translation_unit_callback callback)
> +{
> + if (!finish_translation_unit_callbacks)
> + vec_alloc (finish_translation_unit_callbacks, 1);
> + finish_translation_unit_callbacks->safe_push (callback);
> +}
> +
> +void
> +run_callbacks (logger *logger, const translation_unit )
This function could be "static" since it's not needed outside of
analyzer-language.cc
> +{
> + for (auto const : finish_translation_unit_callbacks)
> + {
> + cb (logger, tu);
> + }
> +}
>
> /* Call into TU to try to find a value for NAME.
> If found, stash its value within analyzer_stashed_constants. */
> @@ -102,6 +122,8 @@ on_finish_translation_unit (const
> translation_unit )
> the_logger.set_logger (new logger (logfile, 0, 0,
> *global_dc->printer));
> stash_named_constants (the_logger.get_logger (), tu);
> +
> + run_callbacks (the_logger.get_logger (), tu);
> }
>
> /* Lookup NAME in the named constants stashed when the frontend TU
> finished.
> diff --git a/gcc/analyzer/analyzer-language.h
> b/gcc/analyzer/analyzer-language.h
> index 00f85aba041..8deea52d627 100644
> --- a/gcc/analyzer/analyzer-language.h
> +++ b/gcc/analyzer/analyzer-language.h
> @@ -21,6 +21,8 @@ along with GCC; see the file COPYING3. If not see
> #ifndef GCC_ANALYZER_LANGUAGE_H
> #define GCC_ANALYZER_LANGUAGE_H
>
> +#include "analyzer/analyzer-logging.h"
> +
> #if ENABLE_ANALYZER
>
> namespace ana {
> @@ -35,8 +37,15 @@ class translation_unit
> have been seen). If it is defined and an integer (e.g. either
> as a
> macro or enum), return the INTEGER_CST value, otherwise return
> NULL. */
> virtual tree lookup_constant_by_id (tree id) const = 0;
> + virtual tree lookup_type_by_id (tree id) const = 0;
> + virtual tree lookup_global_var_by_id (tree id) const = 0;
> };
>
> +typedef void (*finish_translation_unit_callback)
> + (logger *, const translation_unit &);
> +void register_finish_translation_unit_callback (
> + finish_translation_unit_callback callback);
> +
> /* Analyzer hook for frontends to call at the end of the TU. */
>
> void on_finish_translation_unit (const translation_unit );
> diff --git a/gcc/c/c-parser.cc b/gcc/c/c-parser.cc
> index 80920b31f83..f0ee55e416b 100644
> --- a/gcc/c/c-parser.cc
> +++ b/gcc/c/c-parser.cc
> @@ -1695,6 +1695,32 @@ public:
> return NULL_TREE;
> }
>
> + tree
> + lookup_type_by_id (tree id) const final override
> + {
> + if (tree type_decl = lookup_name (id))
> + {
> + if (TREE_CODE (type_decl) == TYPE_DECL)
> + {
> + tree record_type = TREE_TYPE (type_decl);
> + if (TREE_CODE (record_type) == RECORD_TYPE)
> + return record_type;
> + }
It looks like something's wrong with the indentation here, but the idea
seems OK to me (but needs C FE reviewer approval).
> + }
> +
> + return NULL_TREE;
[PATCH] analyzer: stash values for CPython plugin [PR107646]
Hi all,
This patch adds a hook to the end of ana::on_finish_translation_unit
which calls relevant stashing-related callbacks registered during plugin
initialization. This feature is used to stash named types and global
variables for a CPython analyzer plugin [PR107646].
Bootstrapped and tested on aarch64-unknown-linux-gnu. Does it look okay?
---
gcc/analyzer/ChangeLog:
* analyzer-language.cc (run_callbacks): New function.
(on_finish_translation_unit): New function.
* analyzer-language.h (GCC_ANALYZER_LANGUAGE_H): New include.
(class translation_unit): New vfuncs.
gcc/c/ChangeLog:
* c-parser.cc: New functions.
gcc/testsuite/ChangeLog:
* gcc.dg/plugin/analyzer_cpython_plugin.c: New test.
Signed-off-by: Eric Feng
---
gcc/analyzer/analyzer-language.cc | 22 ++
gcc/analyzer/analyzer-language.h | 9 +
gcc/c/c-parser.cc | 26 ++
.../gcc.dg/plugin/analyzer_cpython_plugin.c | 224 ++
4 files changed, 281 insertions(+)
create mode 100644 gcc/testsuite/gcc.dg/plugin/analyzer_cpython_plugin.c
diff --git a/gcc/analyzer/analyzer-language.cc
b/gcc/analyzer/analyzer-language.cc
index 2c8910906ee..fc41b9c17b8 100644
--- a/gcc/analyzer/analyzer-language.cc
+++ b/gcc/analyzer/analyzer-language.cc
@@ -35,6 +35,26 @@ static GTY (()) hash_map
*analyzer_stashed_constants;
#if ENABLE_ANALYZER
namespace ana {
+static vec
+*finish_translation_unit_callbacks;
+
+void
+register_finish_translation_unit_callback (
+finish_translation_unit_callback callback)
+{
+ if (!finish_translation_unit_callbacks)
+vec_alloc (finish_translation_unit_callbacks, 1);
+ finish_translation_unit_callbacks->safe_push (callback);
+}
+
+void
+run_callbacks (logger *logger, const translation_unit )
+{
+ for (auto const : finish_translation_unit_callbacks)
+{
+ cb (logger, tu);
+}
+}
/* Call into TU to try to find a value for NAME.
If found, stash its value within analyzer_stashed_constants. */
@@ -102,6 +122,8 @@ on_finish_translation_unit (const translation_unit )
the_logger.set_logger (new logger (logfile, 0, 0,
*global_dc->printer));
stash_named_constants (the_logger.get_logger (), tu);
+
+ run_callbacks (the_logger.get_logger (), tu);
}
/* Lookup NAME in the named constants stashed when the frontend TU finished.
diff --git a/gcc/analyzer/analyzer-language.h b/gcc/analyzer/analyzer-language.h
index 00f85aba041..8deea52d627 100644
--- a/gcc/analyzer/analyzer-language.h
+++ b/gcc/analyzer/analyzer-language.h
@@ -21,6 +21,8 @@ along with GCC; see the file COPYING3. If not see
#ifndef GCC_ANALYZER_LANGUAGE_H
#define GCC_ANALYZER_LANGUAGE_H
+#include "analyzer/analyzer-logging.h"
+
#if ENABLE_ANALYZER
namespace ana {
@@ -35,8 +37,15 @@ class translation_unit
have been seen). If it is defined and an integer (e.g. either as a
macro or enum), return the INTEGER_CST value, otherwise return NULL. */
virtual tree lookup_constant_by_id (tree id) const = 0;
+ virtual tree lookup_type_by_id (tree id) const = 0;
+ virtual tree lookup_global_var_by_id (tree id) const = 0;
};
+typedef void (*finish_translation_unit_callback)
+ (logger *, const translation_unit &);
+void register_finish_translation_unit_callback (
+finish_translation_unit_callback callback);
+
/* Analyzer hook for frontends to call at the end of the TU. */
void on_finish_translation_unit (const translation_unit );
diff --git a/gcc/c/c-parser.cc b/gcc/c/c-parser.cc
index 80920b31f83..f0ee55e416b 100644
--- a/gcc/c/c-parser.cc
+++ b/gcc/c/c-parser.cc
@@ -1695,6 +1695,32 @@ public:
return NULL_TREE;
}
+ tree
+ lookup_type_by_id (tree id) const final override
+ {
+if (tree type_decl = lookup_name (id))
+ {
+ if (TREE_CODE (type_decl) == TYPE_DECL)
+ {
+ tree record_type = TREE_TYPE (type_decl);
+ if (TREE_CODE (record_type) == RECORD_TYPE)
+ return record_type;
+ }
+ }
+
+return NULL_TREE;
+ }
+
+ tree
+ lookup_global_var_by_id (tree id) const final override
+ {
+if (tree var_decl = lookup_name (id))
+ if (TREE_CODE (var_decl) == VAR_DECL)
+ return var_decl;
+
+return NULL_TREE;
+ }
+
private:
/* Attempt to get an INTEGER_CST from MACRO.
Only handle the simplest cases: where MACRO's definition is a single
diff --git a/gcc/testsuite/gcc.dg/plugin/analyzer_cpython_plugin.c
b/gcc/testsuite/gcc.dg/plugin/analyzer_cpython_plugin.c
new file mode 100644
index 000..285da102edb
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/plugin/analyzer_cpython_plugin.c
@@ -0,0 +1,224 @@
+/* -fanalyzer plugin for CPython extension modules */
+/* { dg-options "-g" } */
+
+#define INCLUDE_MEMORY
+#include "gcc-plugin.h"
+#include "config.h"
+#include "system.h"
+#include "coretypes.h"
+#include "tree.h"
+#include "function.h"
+#include "basic-block.h"
+#include "gimple.h"
+#include "gimple-iterator.h"
+#include "diagnostic-core.h"
+#include
