[PLUGIN] Fix PLUGIN_FINISH_TYPE

Hi,

This patch solves some lacks with the PLUGIN_FINISH_TYPE event
triggering. For now, both C and C++ parser won't trigger it for enums or
for typedef. In C++, when a struct, class or union declaration is parsed
(without definition), the given event data is an error mark instead of
the parsed type node.

Bootstrapped and tested on x86_64.

Romain Geissler

gcc/
2011-09-12  Romain Geissler  <romain.geissler@gmail.com>

	* c-decl.c (grokdeclarator): Trigger PLUGIN_FINISH_TYPE for typedefs.
	* c-parser.c (cp_parser_type_specifier: Trigger PLUGIN_FINISH_TYPE for
	enums.


gcc/cp/
2011-09-12  Romain Geissler  <romain.geissler@gmail.com>

	* decl.c (grokdeclarator): Trigger PLUGIN_FINISH_TYPE for typedefs.
	* parser.c (cp_parser_type_specifier: Trigger PLUGIN_FINISH_TYPE for
	enums.
	Correctly trigger PLUGIN_FINISH_TYPE for unions, structs or classes.


gcc/testsuite/
2011-09-12  Romain Geissler  <romain.geissler@gmail.com>

	* gcc.dg/plugin/dumb-plugin-test-1.c: New File.
	* gcc.dg/plugin/dumb_plugin.c: Likewise.
	* gcc.dg/plugin/plugin.exp: Add above testcase.
	* g++.dg/plugin/dumb-plugin-test-1.C: New tests.
	* g++.dg/plugin/dumb_plugin.c (handle_type): Renamed from handle_struct.
	Add warnings for all kind of types.

Hello Romain,

Romain Geissler <romain.geissler@st.com> a écrit:

> This patch solves some lacks with the PLUGIN_FINISH_TYPE event
> triggering. For now, both C and C++ parser won't trigger it for enums or
> for typedef.

AFAICT, typedef declarations are reported by the PLUGIN_FINISH_DECL
event.  The predicate is_typedef_decl then returns TRUE when passed the
decl carried by the event.  Maybe the plugin's documentation could use
an addendum for this.

[...]

> gcc/cp/
> 2011-09-12  Romain Geissler  <romain.geissler@gmail.com>
>
> 	* decl.c (grokdeclarator): Trigger PLUGIN_FINISH_TYPE for typedefs.

[...]

> Index: gcc/cp/decl.c
> ===================================================================
> --- gcc/cp/decl.c	(revision 178252)
> +++ gcc/cp/decl.c	(working copy)
> @@ -9682,6 +9682,9 @@ grokdeclarator (const cp_declarator *dec
>  		      memfn_quals != TYPE_UNQUALIFIED,
>  		      inlinep, friendp, raises != NULL_TREE);
>  
> +      if (TREE_TYPE (decl) != error_mark_node && !DECL_ARTIFICIAL (decl))
> +        invoke_plugin_callbacks (PLUGIN_FINISH_TYPE, decl);
> +
>        return decl;
>      }
>  

So I think this change is not necessary.

[...]


> Index: gcc/testsuite/gcc.dg/plugin/dumb-plugin-test-1.c
> ===================================================================
> --- gcc/testsuite/gcc.dg/plugin/dumb-plugin-test-1.c	(revision 0)
> +++ gcc/testsuite/gcc.dg/plugin/dumb-plugin-test-1.c	(revision 0)
> @@ -0,0 +1,56 @@
> +// Test case for the dumb plugin.
> +// { dg-do compile }
> +// { dg-options "-O -fplugin-arg-dumb_plugin-ref-pass-name=ccp -fplugin-arg-dumb_plugin-ref-pass-instance-num=1" }
> +

Here, it would be nice to add a comment saying which plugin acts on this
plugin's test and what the plugin does (what the output is supposed to
be).  That redundant information eases the review, at very least.

Thanks.

Hi,

2011/9/12 Dodji Seketeli <dodji@seketeli.org>:
> Hello Romain,
>
> Romain Geissler <romain.geissler@st.com> a écrit:
>
>> This patch solves some lacks with the PLUGIN_FINISH_TYPE event
>> triggering. For now, both C and C++ parser won't trigger it for enums or
>> for typedef.
>
> AFAICT, typedef declarations are reported by the PLUGIN_FINISH_DECL
> event.  The predicate is_typedef_decl then returns TRUE when passed the
> decl carried by the event.  Maybe the plugin's documentation could use
> an addendum for this.

I just checked again, and PLUGIN_FINISH_DECL is triggered for a
typedef in C mode, but not in C++ mode. I'll patch that.
When i recontributed the PLUGIN_FINISH_DECL patch from the original
Brian Hackett, i didn't exactly checked  what may or may not trigger
this new event. I know for example that declaring a function triggers
this event, but defining it does not.

I don't really know when those event should be triggered, we should
clarify the meaning of those.

According to me:
PLUGIN_FINISH_DECL should be triggered anytime the parser parse a
declaration (which includes declaration + definition of function,
typedef definition, namespaces, ..., all DECL_P trees built by the
parser).
For, PLUGIN_FINISH_TYPE i don't really know it means a new type
declaration (or declaration + definition) or if it means usage of a
type (in a function prototype, the type of a local variable. I would
rather vote for new type definition (including typedefs) but for
special cases of struct, you can declare and use them at the same
time:

#include <stdlib.h>

struct my_struct x;
struct my_struct *my_function ();

struct my_struct {
  int field_1;
  int field_2;
};

struct my_struct *my_function () {
>---return NULL;
}

which GCC accepts even with -pedantic.

That's why in this patch i launch a PLUGIN_FINISH_TYPE event every
time the parser meet the "struct" keyword.


So, what the real meaning of all those events ?

>
> [...]
>
>> gcc/cp/
>> 2011-09-12  Romain Geissler  <romain.geissler@gmail.com>
>>
>>       * decl.c (grokdeclarator): Trigger PLUGIN_FINISH_TYPE for typedefs.
>
> [...]
>
>> Index: gcc/cp/decl.c
>> ===================================================================
>> --- gcc/cp/decl.c     (revision 178252)
>> +++ gcc/cp/decl.c     (working copy)
>> @@ -9682,6 +9682,9 @@ grokdeclarator (const cp_declarator *dec
>>                     memfn_quals != TYPE_UNQUALIFIED,
>>                     inlinep, friendp, raises != NULL_TREE);
>>
>> +      if (TREE_TYPE (decl) != error_mark_node && !DECL_ARTIFICIAL (decl))
>> +        invoke_plugin_callbacks (PLUGIN_FINISH_TYPE, decl);
>> +
>>        return decl;
>>      }
>>
>
> So I think this change is not necessary.
>
> [...]
>
>
>> Index: gcc/testsuite/gcc.dg/plugin/dumb-plugin-test-1.c
>> ===================================================================
>> --- gcc/testsuite/gcc.dg/plugin/dumb-plugin-test-1.c  (revision 0)
>> +++ gcc/testsuite/gcc.dg/plugin/dumb-plugin-test-1.c  (revision 0)
>> @@ -0,0 +1,56 @@
>> +// Test case for the dumb plugin.
>> +// { dg-do compile }
>> +// { dg-options "-O -fplugin-arg-dumb_plugin-ref-pass-name=ccp -fplugin-arg-dumb_plugin-ref-pass-instance-num=1" }
>> +
>
> Here, it would be nice to add a comment saying which plugin acts on this
> plugin's test and what the plugin does (what the output is supposed to
> be).  That redundant information eases the review, at very least.

Ok

>
> Thanks.
>
> --
>                Dodji
>

Romain Geissler <romain.geissler@gmail.com> a écrit:

> I just checked again, and PLUGIN_FINISH_DECL is triggered for a
> typedef in C mode, but not in C++ mode. I'll patch that.

Correct.  This is because the event is emitted at the end of
cp_finish_decl, but that function has many return points.  Maybe
unifying all the those return points by replacing them with "goto out;"
and adding an "out" label right at the end, before emitting the event.

> When i recontributed the PLUGIN_FINISH_DECL patch from the original
> Brian Hackett, i didn't exactly checked  what may or may not trigger
> this new event. I know for example that declaring a function triggers
> this event, but defining it does not.
>
> I don't really know when those event should be triggered, we should
> clarify the meaning of those.
>
> According to me:
> PLUGIN_FINISH_DECL should be triggered anytime the parser parse a
> declaration (which includes declaration + definition of function,
> typedef definition, namespaces, ..., all DECL_P trees built by the
> parser).

The general idea sounds sensible, IMHO.  However, we must keep in mind
that there are cases like, e.g, 'struct C;' where G++ creates a typedef
'typedef struct C C;' so that you can name that type 'C' instead of
having to type "struct C' all the time.  For these cases, I don't think
the PLUGIN_FINISH_DECL event should be emitted.

> For, PLUGIN_FINISH_TYPE i don't really know it means a new type
> declaration (or declaration + definition) or if it means usage of a
> type (in a function prototype, the type of a local variable.

I'd say it's a definition of a new type, IMHO.

> I would rather vote for new type definition (including typedefs)

My understanding is that a typedef declaration doesn't define a new
type.  Rather, it declares an alias for an existing type.  As such, I
would think that notifying typedef declarations via PLUGIN_FINISH_DECL
would be the way to go.

> but for special cases of struct, you can declare and use them at the
> same time:

Just to be sure I understand, do you need to be notified about *uses* of
types and decls as well?  If so, maybe a new kind of event should
probably be defined, because PLUGIN_FINISH_DECL and PLUGIN_FINISH_TYPE
seem to have more to do with declaring/defining decls and types than
using them.

> So, what the real meaning of all those events ?

From plugin.def, I read:

 /* After finishing parsing a type.  */
 DEFEVENT (PLUGIN_FINISH_TYPE)

 /* After finishing parsing a declaration. */
 DEFEVENT (PLUGIN_FINISH_DECL)

I agree that's rather terse, but it doesn't seem to be related to the
use of the type/decls.

Hi

2011/9/12 Dodji Seketeli <dodji@seketeli.org>:
 > Romain Geissler <romain.geissler@gmail.com> a écrit:
 >> When i recontributed the PLUGIN_FINISH_DECL patch from the original
 >> Brian Hackett, i didn't exactly checked  what may or may not trigger
 >> this new event. I know for example that declaring a function triggers
 >> this event, but defining it does not.
 >>
 >> I don't really know when those event should be triggered, we should
 >> clarify the meaning of those.
 >>
 >> According to me:
 >> PLUGIN_FINISH_DECL should be triggered anytime the parser parse a
 >> declaration (which includes declaration + definition of function,
 >> typedef definition, namespaces, ..., all DECL_P trees built by the
 >> parser).
 >
 > The general idea sounds sensible, IMHO.  However, we must keep in mind
 > that there are cases like, e.g, 'struct C;' where G++ creates a typedef
 > 'typedef struct C C;' so that you can name that type 'C' instead of
 > having to type "struct C' all the time.  For these cases, I don't think
 > the PLUGIN_FINISH_DECL event should be emitted.

I agree.

 >
 >> For, PLUGIN_FINISH_TYPE i don't really know it means a new type
 >> declaration (or declaration + definition) or if it means usage of a
 >> type (in a function prototype, the type of a local variable.
 >
 > I'd say it's a definition of a new type, IMHO.

Ok, so it only involves struct, unions, enum and class declaration / definitions.

 >
 >> I would rather vote for new type definition (including typedefs)
 >
 > My understanding is that a typedef declaration doesn't define a new
 > type.  Rather, it declares an alias for an existing type.  As such, I
 > would think that notifying typedef declarations via PLUGIN_FINISH_DECL
 > would be the way to go.

Ok

 >
 >> but for special cases of struct, you can declare and use them at the
 >> same time:
 >
 > Just to be sure I understand, do you need to be notified about *uses* of
 > types and decls as well?  If so, maybe a new kind of event should
 > probably be defined, because PLUGIN_FINISH_DECL and PLUGIN_FINISH_TYPE
 > seem to have more to do with declaring/defining decls and types than
 > using them.
 >

Personally i don't need to catch all struct uses, but i need to catch struct declarations.
I want to apply some __attributes__ to a given struct type, for example let's say i need 
to mark *struct my_struct* as deprecated thanks to a plugin. I know how to apply some 
attributes to a type or a decl. See the plugin (test_plugin.c) and the test files attached.

With test_pass.c, my_struct is declared first, then defined after, applying the deprecated 
attribute works.
With test_fail.c, my_struct is declared and defined at the same time, applying the 
deprecated attribute doesn't work with the current trunk (and also with my patch).
I got:
test_fail.c:4:1: warning: type attributes ignored after type is already defined [-Wattributes]

So i may need a PLUGIN_FINISH_TYPE_DECLARATION triggered when the type is declared but 
before it is finally defined.

Does two different events PLUGIN_FINISH_TYPE_DECLARATION and PLUGIN_FINISH_TYPE_DEFINITION 
make sens to you ?

 > --
 >                Dodji
 >

Romain Geissler
struct my_struct {
	int field_1;
	int field_2;
};

struct my_struct *my_function();

struct my_struct x = {1, 2};

struct my_struct *my_function(){
	return &x;
}
struct my_struct *my_function();

struct my_struct {
	int field_1;
	int field_2;
};

struct my_struct x = {1, 2};

struct my_struct *my_function(){
	return &x;
}
#include "gcc-plugin.h"
#include "plugin-version.h"
#include "tree.h"
#include "cp/cp-tree.h"

#pragma weak cplus_decl_attributes

bool my_struct_found = false;
void finish_type_callback (void *gcc_data, void *data ATTRIBUTE_UNUSED) {
	tree type = (tree)gcc_data;
	const char *type_name;

	if (my_struct_found || type == error_mark_node || DECL_P (type)) {
		//current PLUGIN_FINISH_TYPE is buggy in C++
		//and my current patch may return TYPE_DECL for typedefs

		return;
	}

	if (TREE_CODE (TYPE_NAME (type)) == IDENTIFIER_NODE) {
		type_name = IDENTIFIER_POINTER (TYPE_NAME (type));
	} else {
		type_name = IDENTIFIER_POINTER (DECL_NAME (TYPE_NAME (type)));
	}

	if (strcmp (type_name, "my_struct")) {
		//only affect my_struct type
		return;
	}

	my_struct_found = true;
	tree deprecated_attribute = tree_cons (get_identifier ("deprecated"), NULL_TREE, NULL_TREE);
	inform(input_location,  "my_struct found");


	//apply the deprecated attribute to the my_struct type.
	if (cplus_decl_attributes) {
		cplus_decl_attributes (&type, deprecated_attribute, ATTR_FLAG_TYPE_IN_PLACE);
	} else {
		decl_attributes (&type, deprecated_attribute, ATTR_FLAG_TYPE_IN_PLACE);
	}
}

int plugin_is_GPL_compatible;

int plugin_init (struct plugin_name_args *plugin_info,
	struct plugin_gcc_version *version ATTRIBUTE_UNUSED)
{
	register_callback (plugin_info->base_name, PLUGIN_FINISH_TYPE, &finish_type_callback, NULL);

	return 0;
}

Hi,

I tried to fix PLUGIN_FINISH_DECL as well to include typedefs in C++.

The followings does not currently trigger the PLUGIN_FINISH_DECL
(or not in all cases), but should them ?
 - function parameters (in the function prototype)
 - definition (with a function body) of a top-level function (while the exact
   same function definition enclosed in a class definition will trigger
   PLUGIN_FINISH_DECL)
 - label declaration
 - constants defined by enums
 - namespace

Romain.

Hello Romain,

Sorry for my late reply to this thread.

Romain Geissler <romain.geissler@st.com> a écrit:

>> Just to be sure I understand, do you need to be notified about *uses* of
>> types and decls as well?  If so, maybe a new kind of event should
>> probably be defined, because PLUGIN_FINISH_DECL and PLUGIN_FINISH_TYPE
>> seem to have more to do with declaring/defining decls and types than
>> using them.
>>
>
> Personally i don't need to catch all struct uses, but i need to catch struct declarations.
> I want to apply some __attributes__ to a given struct type, for
> example let's say i need to mark *struct my_struct* as deprecated
> thanks to a plugin. I know how to apply some attributes to a type or a
> decl. See the plugin (test_plugin.c) and the test files attached.
>
> With test_pass.c, my_struct is declared first, then defined after,
> applying the deprecated attribute works.
> With test_fail.c, my_struct is declared and defined at the same time,
> applying the deprecated attribute doesn't work with the current trunk
> (and also with my patch).
> I got:
> test_fail.c:4:1: warning: type attributes ignored after type is already defined [-Wattributes]

Right, you cannot use (cplus_)decl_attributes on type that is fully
created.  The function warns in that case.

> So i may need a PLUGIN_FINISH_TYPE_DECLARATION triggered when the type
> is declared but before it is finally defined.

Hmmh.  For this specific case, maybe just setting the TREE_DEPRECATED
flag on the tree node of type could do what you want?

Romain Geissler <romain.geissler@gmail.com> a écrit:

> I tried to fix PLUGIN_FINISH_DECL as well to include typedefs in C++.
>
> The followings does not currently trigger the PLUGIN_FINISH_DECL
> (or not in all cases), but should them ?
>  - function parameters (in the function prototype)
>  - definition (with a function body) of a top-level function (while the exact
>    same function definition enclosed in a class definition will trigger
>    PLUGIN_FINISH_DECL)
>  - label declaration
>  - constants defined by enums
>  - namespace

Indeed.  finish_decl is not called in those cases.  As to if the
PLUGIN_FINISH_DECL event should be emitted for those, I'd say yes, at
least if I believe what the description in plugin.def says:

    /* After finishing parsing a declaration. */
    DEFEVENT (PLUGIN_FINISH_DECL)

But I'd rather ask what the maintainers think about it.

Jason, Diego?

On 11-09-22 09:40 , Dodji Seketeli wrote:
> Romain Geissler<romain.geissler@gmail.com>  a écrit:
>
>> I tried to fix PLUGIN_FINISH_DECL as well to include typedefs in C++.
>>
>> The followings does not currently trigger the PLUGIN_FINISH_DECL
>> (or not in all cases), but should them ?
>>   - function parameters (in the function prototype)
>>   - definition (with a function body) of a top-level function (while the exact
>>     same function definition enclosed in a class definition will trigger
>>     PLUGIN_FINISH_DECL)
>>   - label declaration
>>   - constants defined by enums
>>   - namespace
>
> Indeed.  finish_decl is not called in those cases.  As to if the
> PLUGIN_FINISH_DECL event should be emitted for those, I'd say yes, at
> least if I believe what the description in plugin.def says:
>
>      /* After finishing parsing a declaration. */
>      DEFEVENT (PLUGIN_FINISH_DECL)
>
> But I'd rather ask what the maintainers think about it.
>
> Jason, Diego?

Yes, those events should trigger a PLUGIN_FINISH_DECL call.


Diego.

Hello,

Le 22 sept. 2011 à 15:22, Dodji Seketeli a écrit :
> 
>> So i may need a PLUGIN_FINISH_TYPE_DECLARATION triggered when the type
>> is declared but before it is finally defined.
> 
> Hmmh.  For this specific case, maybe just setting the TREE_DEPRECATED
> flag on the tree node of type could do what you want


The deprecated attribute was just an example. I may need to apply any type attribute
when a new type is parsed, which is currently not possible with the current C++
front-end.

Romain

Le 22 sept. 2011 à 16:18, Diego Novillo a écrit :

> On 11-09-22 09:40 , Dodji Seketeli wrote:
>> Romain Geissler<romain.geissler@gmail.com>  a écrit:
>> 
>>> I tried to fix PLUGIN_FINISH_DECL as well to include typedefs in C++.
>>> 
>>> The followings does not currently trigger the PLUGIN_FINISH_DECL
>>> (or not in all cases), but should them ?
>>>  - function parameters (in the function prototype)
>>>  - definition (with a function body) of a top-level function (while the exact
>>>    same function definition enclosed in a class definition will trigger
>>>    PLUGIN_FINISH_DECL)
>>>  - label declaration
>>>  - constants defined by enums
>>>  - namespace
>> 
>> Indeed.  finish_decl is not called in those cases.  As to if the
>> PLUGIN_FINISH_DECL event should be emitted for those, I'd say yes, at
>> least if I believe what the description in plugin.def says:
>> 
>>     /* After finishing parsing a declaration. */
>>     DEFEVENT (PLUGIN_FINISH_DECL)
>> 
>> But I'd rather ask what the maintainers think about it.
>> 
>> Jason, Diego?
> 
> Yes, those events should trigger a PLUGIN_FINISH_DECL call.

Ok, i've already implemented it in the C front-end. I'll post the whole patch soon.

Romain

Romain Geissler <romain.geissler@gmail.com> a écrit:

> The deprecated attribute was just an example. I may need to apply any type attribute
> when a new type is parsed, which is currently not possible with the current C++
> front-end.

So then it seems to me like you might need to emit an event when a new
declared named is about to be put in the symbol table for a given
scope.  For the c++ FE, that would be in pushdecl_maybe_friend.  That
way, you can see the DECL even for the declaration of a type, before the
type is fully parsed.