[Cocci] remove typedefs from struct

[Cocci] remove typedefs from struct

[Sören Brinkmann]

Hi,

I'm just getting my feet wet with coccinelle, so I'm probably missing
something really obvious in my problem.

I have a codebase that uses plenty of
  typedef struct foo {
  	...
  } bar;

I want to remove all these typedefs and simply use struct foo instead. I
came up with this patch:

  /// Convert typdefed structs to structs
  
  @ rule1 @
  identifier i;
  type t;
  @@
  -typedef struct i {
  +struct i {
  	...
  - }t;
  + };
  
  @@
  identifier rule1.i;
  type rule1.t;
  @@
  -t
  +struct i
  // end of patch

This seems to work for most occurrences, but there are a couple of
variables defined like this:

  static const t *const baz[MAX] = {
  	...
  };

And also function declarations like: 
  u32 fn(const t* const baz);

In such cases, t isn't replaces as I would expect, but those lines
aren't changed at all.

Other variables and non-const function parameters seem to be replaced as
expected.
Does anybody have an idea what I'm missing?

	Thanks,
	Soren

[Cocci] remove typedefs from struct

[Julia Lawall]

On Sat, 14 Feb 2015, S?ren Brinkmann wrote:

> Hi,
>
> I'm just getting my feet wet with coccinelle, so I'm probably missing
> something really obvious in my problem.
>
> I have a codebase that uses plenty of
>   typedef struct foo {
>   	...
>   } bar;
>
> I want to remove all these typedefs and simply use struct foo instead. I
> came up with this patch:
>
>   /// Convert typdefed structs to structs
>
>   @ rule1 @
>   identifier i;
>   type t;
>   @@
>   -typedef struct i {
>   +struct i {
>   	...
>   - }t;
>   + };
>
>   @@
>   identifier rule1.i;
>   type rule1.t;
>   @@
>   -t
>   +struct i
>   // end of patch
>
> This seems to work for most occurrences, but there are a couple of
> variables defined like this:
>
>   static const t *const baz[MAX] = {
>   	...
>   };
>
> And also function declarations like:
>   u32 fn(const t* const baz);
>
> In such cases, t isn't replaces as I would expect, but those lines
> aren't changed at all.
>
> Other variables and non-const function parameters seem to be replaced as
> expected.
> Does anybody have an idea what I'm missing?

In principle, it should be OK.  Could you send a concrete example of C
conde on which it does not work?  It is possible that there is some
problem in parsing baz, and it is just skipping over it.  For this, you
can run spatch --parse-c on your file.  If there is a BAD or bad in front
of the code that you expect to have changed, then there may be a problem.

julia

[Cocci] remove typedefs from struct

[Sören Brinkmann]

Hi Julia,

On Sat, 2015-02-14 at 07:56PM +0100, Julia Lawall wrote:
> On Sat, 14 Feb 2015, S?ren Brinkmann wrote:
> 
> > Hi,
> >
> > I'm just getting my feet wet with coccinelle, so I'm probably missing
> > something really obvious in my problem.
> >
> > I have a codebase that uses plenty of
> >   typedef struct foo {
> >   	...
> >   } bar;
> >
> > I want to remove all these typedefs and simply use struct foo instead. I
> > came up with this patch:
> >
> >   /// Convert typdefed structs to structs
> >
> >   @ rule1 @
> >   identifier i;
> >   type t;
> >   @@
> >   -typedef struct i {
> >   +struct i {
> >   	...
> >   - }t;
> >   + };
> >
> >   @@
> >   identifier rule1.i;
> >   type rule1.t;
> >   @@
> >   -t
> >   +struct i
> >   // end of patch
> >
> > This seems to work for most occurrences, but there are a couple of
> > variables defined like this:
> >
> >   static const t *const baz[MAX] = {
> >   	...
> >   };
> >
> > And also function declarations like:
> >   u32 fn(const t* const baz);
> >
> > In such cases, t isn't replaces as I would expect, but those lines
> > aren't changed at all.
> >
> > Other variables and non-const function parameters seem to be replaced as
> > expected.
> > Does anybody have an idea what I'm missing?
> 
> In principle, it should be OK.  Could you send a concrete example of C
> conde on which it does not work?  It is possible that there is some
> problem in parsing baz, and it is just skipping over it.  For this, you
> can run spatch --parse-c on your file.  If there is a BAD or bad in front
> of the code that you expect to have changed, then there may be a problem.

When I run with --parse-c, all I get is this:
  init_defs_builtins: /usr/share/coccinelle/standard.h
  
  PARSING: testcase.c
  -----------------------------------------------------------------------
  maybe 10 most problematic tokens
  -----------------------------------------------------------------------
  -----------------------------------------------------------------------
  NB total files = 1; perfect = 1; pbs = 0; timeout = 0; =========> 100%
  nb good = 10,  nb passed = 0 =========> 0.000000% passed
  nb good = 10,  nb bad = 0 =========> 100.000000% good

It doesn't show any code (not sure whether that is correct or not), but no 'BAD' either.
Running that file through gcc -c -Wall works fine as well. So, it should be legal c (just
a warning regarding an unused variable).

I attached the testcase.c and my cocci patch. This is the diff I get:
  init_defs_builtins: /usr/share/coccinelle/standard.h
  HANDLING: testcase.c
  diff =
  --- testcase.c
  +++ /tmp/cocci-output-17008-c37c42-testcase.c
  @@ -1,11 +1,11 @@
  -typedef struct a_struct {
  +struct a_struct {
          int a_member;
  -} a_struct;
  +};
  
   static const a_struct *const a_struct_array[3] = {
   };
  
   unsigned int function(const a_struct* const a_struct_arg);
  
  -static a_struct a_struct_instance;
  +static struct a_struct a_struct_instance;
   static const a_struct a_const_struct_instance;

	Thanks,
	Soren
-------------- next part --------------
A non-text attachment was scrubbed...
Name: testcase.c
Type: text/x-csrc
Size: 251 bytes
Desc: not available
URL: <https://systeme.lip6.fr/pipermail/cocci/attachments/20150214/07a3a4cb/attachment.bin>
-------------- next part --------------
/// Convert typdefed structs to structs

@ rule1 @
identifier i;
type t;
@@
-typedef struct i {
+struct i {
	...
- }t;
+ };

@ rule2 @
identifier rule1.i;
type rule1.t;
@@
-t
+struct i

@ rule3 @
identifier i;
type t;
@@

-typedef struct i t;

[Cocci] remove typedefs from struct

[Julia Lawall]

> When I run with --parse-c, all I get is this:
>   init_defs_builtins: /usr/share/coccinelle/standard.h
>   
>   PARSING: testcase.c
>   -----------------------------------------------------------------------
>   maybe 10 most problematic tokens
>   -----------------------------------------------------------------------
>   -----------------------------------------------------------------------
>   NB total files = 1; perfect = 1; pbs = 0; timeout = 0; =========> 100%
>   nb good = 10,  nb passed = 0 =========> 0.000000% passed
>   nb good = 10,  nb bad = 0 =========> 100.000000% good

OK, so there is no problem in this direction.

> It doesn't show any code (not sure whether that is correct or not), but 
> no 'BAD' either. Running that file through gcc -c -Wall works fine as 
> well. So, it should be legal c (just a warning regarding an unused 
> variable).

gcc -c is not a very useful test because gcc expands macros and Coccinelle 
does not.  So Coccinelle might not be able to parse something that gcc 
can.

> I attached the testcase.c and my cocci patch. This is the diff I get:

Thanks, I will take a look.

julia

>   init_defs_builtins: /usr/share/coccinelle/standard.h
>   HANDLING: testcase.c
>   diff =
>   --- testcase.c
>   +++ /tmp/cocci-output-17008-c37c42-testcase.c
>   @@ -1,11 +1,11 @@
>   -typedef struct a_struct {
>   +struct a_struct {
>           int a_member;
>   -} a_struct;
>   +};
>   
>    static const a_struct *const a_struct_array[3] = {
>    };
>   
>    unsigned int function(const a_struct* const a_struct_arg);
>   
>   -static a_struct a_struct_instance;
>   +static struct a_struct a_struct_instance;
>    static const a_struct a_const_struct_instance;
> 
> 	Thanks,
> 	Soren
> 

[Cocci] remove typedefs from struct

[Julia Lawall]

On Sat, 14 Feb 2015, S?ren Brinkmann wrote:

> Hi Julia,
> 
> On Sat, 2015-02-14 at 07:56PM +0100, Julia Lawall wrote:
> > On Sat, 14 Feb 2015, S?ren Brinkmann wrote:
> > 
> > > Hi,
> > >
> > > I'm just getting my feet wet with coccinelle, so I'm probably missing
> > > something really obvious in my problem.
> > >
> > > I have a codebase that uses plenty of
> > >   typedef struct foo {
> > >   	...
> > >   } bar;
> > >
> > > I want to remove all these typedefs and simply use struct foo instead. I
> > > came up with this patch:
> > >
> > >   /// Convert typdefed structs to structs
> > >
> > >   @ rule1 @
> > >   identifier i;
> > >   type t;
> > >   @@
> > >   -typedef struct i {
> > >   +struct i {
> > >   	...
> > >   - }t;
> > >   + };
> > >
> > >   @@
> > >   identifier rule1.i;
> > >   type rule1.t;
> > >   @@
> > >   -t
> > >   +struct i
> > >   // end of patch
> > >
> > > This seems to work for most occurrences, but there are a couple of
> > > variables defined like this:
> > >
> > >   static const t *const baz[MAX] = {
> > >   	...
> > >   };
> > >
> > > And also function declarations like:
> > >   u32 fn(const t* const baz);
> > >
> > > In such cases, t isn't replaces as I would expect, but those lines
> > > aren't changed at all.
> > >
> > > Other variables and non-const function parameters seem to be replaced as
> > > expected.
> > > Does anybody have an idea what I'm missing?

OK, the issue is that const is considered to be part of the type, so when 
you have a metavariable that contains type foo, it doesn't match an 
occurrence of const foo.  I'm not sure if it is correct to consider const 
to be part of the type, but I'm also not eager to change it.  Could you 
live with the solution of just making one more rule:

@@
identifier rule1.i;
type rule1.t;
@@
 const
-t
+struct i

julia

[Cocci] remove typedefs from struct

[Sören Brinkmann]

On Sun, 2015-02-15 at 10:38AM +0100, Julia Lawall wrote:
> On Sat, 14 Feb 2015, S?ren Brinkmann wrote:
> 
> > Hi Julia,
> > 
> > On Sat, 2015-02-14 at 07:56PM +0100, Julia Lawall wrote:
> > > On Sat, 14 Feb 2015, S?ren Brinkmann wrote:
> > > 
> > > > Hi,
> > > >
> > > > I'm just getting my feet wet with coccinelle, so I'm probably missing
> > > > something really obvious in my problem.
> > > >
> > > > I have a codebase that uses plenty of
> > > >   typedef struct foo {
> > > >   	...
> > > >   } bar;
> > > >
> > > > I want to remove all these typedefs and simply use struct foo instead. I
> > > > came up with this patch:
> > > >
> > > >   /// Convert typdefed structs to structs
> > > >
> > > >   @ rule1 @
> > > >   identifier i;
> > > >   type t;
> > > >   @@
> > > >   -typedef struct i {
> > > >   +struct i {
> > > >   	...
> > > >   - }t;
> > > >   + };
> > > >
> > > >   @@
> > > >   identifier rule1.i;
> > > >   type rule1.t;
> > > >   @@
> > > >   -t
> > > >   +struct i
> > > >   // end of patch
> > > >
> > > > This seems to work for most occurrences, but there are a couple of
> > > > variables defined like this:
> > > >
> > > >   static const t *const baz[MAX] = {
> > > >   	...
> > > >   };
> > > >
> > > > And also function declarations like:
> > > >   u32 fn(const t* const baz);
> > > >
> > > > In such cases, t isn't replaces as I would expect, but those lines
> > > > aren't changed at all.
> > > >
> > > > Other variables and non-const function parameters seem to be replaced as
> > > > expected.
> > > > Does anybody have an idea what I'm missing?
> 
> OK, the issue is that const is considered to be part of the type, so when 
> you have a metavariable that contains type foo, it doesn't match an 
> occurrence of const foo.  I'm not sure if it is correct to consider const 
> to be part of the type, but I'm also not eager to change it.  Could you 
> live with the solution of just making one more rule:
> 
> @@
> identifier rule1.i;
> type rule1.t;
> @@
>  const
> -t
> +struct i

Yes, that works for me. Thanks!

But I just ran into the next controversy:
I extended my patch to also catch typedef forward declarations:

  @ rule4 @
  identifier i;
  type t;
  @@
  -typedef struct i t;
  +struct i;
  
  /* Replace type with struct (forward declarations */
  @ rule5 @
  identifier rule4.i;
  type rule4.t;
  @@
  -t
  +struct i
  
  /* Replace type with struct (forward declarations */
  @ rule6 @
  identifier rule4.i;
  type rule4.t;
  @@
  const
  -t
  +struct i

This works mostly fine, but fails (under certain circumstances) on a
line like this:
  typedef u32 (*const fnptr2)(a_struct_t *strptr);

I did a little bit of testing:
 - the parser chokes on that line unless
   - the u32 typedef is in the same file before the failing line (not
     sure why it's not enough to pull it in through an include)
   - a similar line without the 'const' precedes the failing line e.g.
	typedef u32 (*fnptr)(a_struct_t *strptr);

I attach a patch and c file to reproduce this. Running
'spatch --parse-c --spfile test.cocci testcase2.c' reveals some
parsing issues.  If the annotated line int he c file is commented
in, parsing goes through fine.

	Thanks,
	Soren
-------------- next part --------------
@ rule4 @
identifier i;
type t;
@@
-typedef struct i t;
+struct i;

/* Replace type with struct (forward declarations */
@ rule5 @
identifier rule4.i;
type rule4.t;
@@
-t
+struct i

/* Replace type with struct (forward declarations */
@ rule6 @
identifier rule4.i;
type rule4.t;
@@
const
-t
+struct i
-------------- next part --------------
A non-text attachment was scrubbed...
Name: testcase2.c
Type: text/x-csrc
Size: 285 bytes
Desc: not available
URL: <https://systeme.lip6.fr/pipermail/cocci/attachments/20150215/0ff2dc93/attachment.bin>

[Cocci] remove typedefs from struct

[Julia Lawall]

> This works mostly fine, but fails (under certain circumstances) on a
> line like this:
>   typedef u32 (*const fnptr2)(a_struct_t *strptr);
> 
> I did a little bit of testing:
>  - the parser chokes on that line unless
>    - the u32 typedef is in the same file before the failing line (not
>      sure why it's not enough to pull it in through an include)

What options do you use?  Coccinelle will just ignore some part of your 
includes, depending on the command line options.  If you give no options 
related to this issue, then the only include for eg foo.c that is 
processed will be an include of foo.h, on the assumption that the contains 
very specific foo definitions that are probably very relevant for foo.c.

Coccinelle doesn't process Makefiles, so it doesn't know about complex 
include paths.  But if you want it to try as hard as possible with respect 
to include files (which would probably be necessary to find a definition 
of u32) then you can give the options --recursive-includes 
--relax-include-path.  The first means that if included file foo.h itself 
includes bar.h, then includ bar.h too.  The second means that if there is 
eg only one occurrence of bar.h in your source tree, use that one, even if 
the various -I arguments on the command line don't lead to it.  This will 
be extremely slow.  You might be able to use the option 
--include-headers-for-types which only takes the type information from the 
header files, and doesn't actually try applying your transformation to 
them.  The intent is to have type information, when you have eg a->b and 
the type of a is defined in a header file.  But to collect type 
information, one has to parse first, so it should be able to use the u32 
definition.

The parsing also tries to accumulate information along the way.  So if it 
sees u32 in a place where it can only be a type, then it infer that it is 
a type without actually seeing the typedef definition.  This probably 
results in your second point.  I'm not sure to understand the impact of 
const in this case.  Perhaps the heuristic that recognizes u32 as a type 
is thrown off in this case.

>    - a similar line without the 'const' precedes the failing line e.g.
> 	typedef u32 (*fnptr)(a_struct_t *strptr);
> 
> I attach a patch and c file to reproduce this. Running
> 'spatch --parse-c --spfile test.cocci testcase2.c' reveals some
> parsing issues.  If the annotated line int he c file is commented
> in, parsing goes through fine.

Thanks, I will take a look.

julia

[Cocci] remove typedefs from struct

[Sören Brinkmann]

On Sun, 2015-02-15 at 10:16PM +0100, Julia Lawall wrote:
> > This works mostly fine, but fails (under certain circumstances) on a
> > line like this:
> >   typedef u32 (*const fnptr2)(a_struct_t *strptr);
> > 
> > I did a little bit of testing:
> >  - the parser chokes on that line unless
> >    - the u32 typedef is in the same file before the failing line (not
> >      sure why it's not enough to pull it in through an include)
> 
> What options do you use?  Coccinelle will just ignore some part of your 
> includes, depending on the command line options.  If you give no options 
> related to this issue, then the only include for eg foo.c that is 
> processed will be an include of foo.h, on the assumption that the contains 
> very specific foo definitions that are probably very relevant for foo.c.

If you look at my example, I tested by creating types.h (in the same
directory), that did include the single line 'typedef unsigned int u32;'
only. I then ran
  spatch --all-includes -I . --sp-file test.cocci testcase2.c
But that doesn't seem to make any difference. If I move the typedef into
the c file, it works though.

> 
> Coccinelle doesn't process Makefiles, so it doesn't know about complex 
> include paths.  But if you want it to try as hard as possible with respect 
> to include files (which would probably be necessary to find a definition 
> of u32) then you can give the options --recursive-includes 
> --relax-include-path.  The first means that if included file foo.h itself 
> includes bar.h, then includ bar.h too.  The second means that if there is 
> eg only one occurrence of bar.h in your source tree, use that one, even if 
> the various -I arguments on the command line don't lead to it.  This will 
> be extremely slow.  You might be able to use the option 
> --include-headers-for-types which only takes the type information from the 
> header files, and doesn't actually try applying your transformation to 
> them.  The intent is to have type information, when you have eg a->b and 
> the type of a is defined in a header file.  But to collect type 
> information, one has to parse first, so it should be able to use the u32 
> definition.

Thanks for those tips. They may become helpful when running on the
actual code base.
My spatch doesn't seem to understand the '--include-headers-for-types'
switch. I might have to upgrade. I'm currently running what I get
through my distro. That is version 1.0.0-rc21 with Python support and
with PCRE support.

> 
> The parsing also tries to accumulate information along the way.  So if it 
> sees u32 in a place where it can only be a type, then it infer that it is 
> a type without actually seeing the typedef definition.  This probably 
> results in your second point.  I'm not sure to understand the impact of 
> const in this case.  Perhaps the heuristic that recognizes u32 as a type 
> is thrown off in this case.

Yep, that might explain why adding some lines helps.

> 
> >    - a similar line without the 'const' precedes the failing line e.g.
> > 	typedef u32 (*fnptr)(a_struct_t *strptr);
> > 
> > I attach a patch and c file to reproduce this. Running
> > 'spatch --parse-c --spfile test.cocci testcase2.c' reveals some
> > parsing issues.  If the annotated line int he c file is commented
> > in, parsing goes through fine.
> 
> Thanks, I will take a look.

Thanks, much appreciated.

	Soren

[Cocci] remove typedefs from struct

[Julia Lawall]

> If you look at my example, I tested by creating types.h (in the same
> directory), that did include the single line 'typedef unsigned int u32;'
> only. I then ran
>   spatch --all-includes -I . --sp-file test.cocci testcase2.c
> But that doesn't seem to make any difference. If I move the typedef into
> the c file, it works though.

OK, maybe types in header files are indeed treated differently.

> > 
> > Coccinelle doesn't process Makefiles, so it doesn't know about complex 
> > include paths.  But if you want it to try as hard as possible with respect 
> > to include files (which would probably be necessary to find a definition 
> > of u32) then you can give the options --recursive-includes 
> > --relax-include-path.  The first means that if included file foo.h itself 
> > includes bar.h, then includ bar.h too.  The second means that if there is 
> > eg only one occurrence of bar.h in your source tree, use that one, even if 
> > the various -I arguments on the command line don't lead to it.  This will 
> > be extremely slow.  You might be able to use the option 
> > --include-headers-for-types which only takes the type information from the 
> > header files, and doesn't actually try applying your transformation to 
> > them.  The intent is to have type information, when you have eg a->b and 
> > the type of a is defined in a header file.  But to collect type 
> > information, one has to parse first, so it should be able to use the u32 
> > definition.
> 
> Thanks for those tips. They may become helpful when running on the
> actual code base.
> My spatch doesn't seem to understand the '--include-headers-for-types'
> switch. I might have to upgrade. I'm currently running what I get
> through my distro. That is version 1.0.0-rc21 with Python support and
> with PCRE support.

Yes, it would be good to upgrade.

A patch that adds a few more parsing hacks is below.  It may apply to 
your version or you may need to upgrade.

julia

diff --git a/parsing_c/parsing_hacks.ml b/parsing_c/parsing_hacks.ml
index e6824b4..96b1995 100644
--- a/parsing_c/parsing_hacks.ml
+++ b/parsing_c/parsing_hacks.ml
@@ -2496,6 +2496,26 @@ let lookahead2 ~pass next before =
       msg_typedef s i1 35; LP.add_typedef_root s;
       TypedefIdent (s, i1)
 
+  (* x ( *y )(params),  function pointer *)
+  | (TIdent (s, i1)::TOPar _::TMul _::Tconst _::TIdent _::TCPar _::TOPar _::_,
+     _)
+      when not_struct_enum before
+      && ok_typedef s
+        ->
+
+      msg_typedef s i1 34; LP.add_typedef_root s;
+      TypedefIdent (s, i1)
+
+  (* x* ( *y )(params),  function pointer 2 *)
+  | (TIdent (s, i1)::TMul _::TOPar _::TMul _::Tconst _::TIdent _::TCPar _::
+     TOPar _::_,  _)
+      when not_struct_enum before
+      && ok_typedef s
+        ->
+
+      msg_typedef s i1 35; LP.add_typedef_root s;
+      TypedefIdent (s, i1)
+
 
   (*-------------------------------------------------------------*)
   (* CPP *)

[Cocci] remove typedefs from struct

[Sören Brinkmann]

On Sun, 2015-02-15 at 10:41PM +0100, Julia Lawall wrote:
> > If you look at my example, I tested by creating types.h (in the same
> > directory), that did include the single line 'typedef unsigned int u32;'
> > only. I then ran
> >   spatch --all-includes -I . --sp-file test.cocci testcase2.c
> > But that doesn't seem to make any difference. If I move the typedef into
> > the c file, it works though.
> 
> OK, maybe types in header files are indeed treated differently.

I even tried --preprocess without seeing different behavior. No clue
what's going on there.

> 
> > > 
> > > Coccinelle doesn't process Makefiles, so it doesn't know about complex 
> > > include paths.  But if you want it to try as hard as possible with respect 
> > > to include files (which would probably be necessary to find a definition 
> > > of u32) then you can give the options --recursive-includes 
> > > --relax-include-path.  The first means that if included file foo.h itself 
> > > includes bar.h, then includ bar.h too.  The second means that if there is 
> > > eg only one occurrence of bar.h in your source tree, use that one, even if 
> > > the various -I arguments on the command line don't lead to it.  This will 
> > > be extremely slow.  You might be able to use the option 
> > > --include-headers-for-types which only takes the type information from the 
> > > header files, and doesn't actually try applying your transformation to 
> > > them.  The intent is to have type information, when you have eg a->b and 
> > > the type of a is defined in a header file.  But to collect type 
> > > information, one has to parse first, so it should be able to use the u32 
> > > definition.
> > 
> > Thanks for those tips. They may become helpful when running on the
> > actual code base.
> > My spatch doesn't seem to understand the '--include-headers-for-types'
> > switch. I might have to upgrade. I'm currently running what I get
> > through my distro. That is version 1.0.0-rc21 with Python support and
> > with PCRE support.
> 
> Yes, it would be good to upgrade.

Already got that done in the meantime :)

> 
> A patch that adds a few more parsing hacks is below.  It may apply to 
> your version or you may need to upgrade.

Applied and rebuilt. That seems to do the trick. My testcase passes and
even on the real code base everything seems to get replaced correctly
now.
Thanks a lot!

	Soren

[Cocci] remove typedefs from struct

[Julia Lawall]

On Sun, 15 Feb 2015, S?ren Brinkmann wrote:

> On Sun, 2015-02-15 at 10:41PM +0100, Julia Lawall wrote:
> > > If you look at my example, I tested by creating types.h (in the same
> > > directory), that did include the single line 'typedef unsigned int u32;'
> > > only. I then ran
> > >   spatch --all-includes -I . --sp-file test.cocci testcase2.c
> > > But that doesn't seem to make any difference. If I move the typedef into
> > > the c file, it works though.
> > 
> > OK, maybe types in header files are indeed treated differently.
> 
> I even tried --preprocess without seeing different behavior. No clue
> what's going on there.

No idea...  I can try to check.

> > A patch that adds a few more parsing hacks is below.  It may apply to 
> > your version or you may need to upgrade.
> 
> Applied and rebuilt. That seems to do the trick. My testcase passes and
> even on the real code base everything seems to get replaced correctly
> now.

Sounds good.  I'm not sure that complicated const constructions are tested 
as well as they should be...

julia

[Cocci] remove typedefs from struct

[Sören Brinkmann]

On Sun, 2015-02-15 at 10:56PM +0100, Julia Lawall wrote:
> On Sun, 15 Feb 2015, S?ren Brinkmann wrote:
> 
> > On Sun, 2015-02-15 at 10:41PM +0100, Julia Lawall wrote:
> > > > If you look at my example, I tested by creating types.h (in the same
> > > > directory), that did include the single line 'typedef unsigned int u32;'
> > > > only. I then ran
> > > >   spatch --all-includes -I . --sp-file test.cocci testcase2.c
> > > > But that doesn't seem to make any difference. If I move the typedef into
> > > > the c file, it works though.
> > > 
> > > OK, maybe types in header files are indeed treated differently.
> > 
> > I even tried --preprocess without seeing different behavior. No clue
> > what's going on there.
> 
> No idea...  I can try to check.
> 
> > > A patch that adds a few more parsing hacks is below.  It may apply to 
> > > your version or you may need to upgrade.
> > 
> > Applied and rebuilt. That seems to do the trick. My testcase passes and
> > even on the real code base everything seems to get replaced correctly
> > now.
> 
> Sounds good.  I'm not sure that complicated const constructions are tested 
> as well as they should be...

With your help I did some more adjustments and my semantic patch replaces
all the typedefed structs correctly, with a single exception. The sources
contain one line like this:
  static const foo_t const foo_arry[] = {
  	...
  };
Similar as before, foo_t should be replaced with 'struct foo_t',
but it's not.

Again, some complex const usage, though this time I'm almost certain that
the second const is meaningless. gcc consumes it, though, so does
'spatch --parse-c'.

	Soren

[Cocci] remove typedefs from struct

[Julia Lawall]

> With your help I did some more adjustments and my semantic patch replaces
> all the typedefed structs correctly, with a single exception. The sources
> contain one line like this:
>   static const foo_t const foo_arry[] = {

Could you check if things work when the [] is not empty?

julia

>   	...
>   };
> Similar as before, foo_t should be replaced with 'struct foo_t',
> but it's not.
>
> Again, some complex const usage, though this time I'm almost certain that
> the second const is meaningless. gcc consumes it, though, so does
> 'spatch --parse-c'.
>
> 	Soren
>

[Cocci] remove typedefs from struct

[Sören Brinkmann]

On Mon, 2015-02-16 at 10:10AM +0100, Julia Lawall wrote:
> > With your help I did some more adjustments and my semantic patch replaces
> > all the typedefed structs correctly, with a single exception. The sources
> > contain one line like this:
> >   static const foo_t const foo_arry[] = {
> 
> Could you check if things work when the [] is not empty?

No, doesn't make a difference. It works though, when I remove the second
const (regardless of the size specifier).

	Soren

[Cocci] remove typedefs from struct

[Julia Lawall]

Another patch is below.  With this the code parses, and the transformation 
can be made.  However, it considers this to be a duplicate const 
annotation, so I'm not sure that the const is being associated in the 
right way by the parser.

julia

diff --git a/parsing_c/parsing_hacks.ml b/parsing_c/parsing_hacks.ml
index 4e8335e..2dd867e 100644
--- a/parsing_c/parsing_hacks.ml
+++ b/parsing_c/parsing_hacks.ml
@@ -1997,6 +1997,14 @@ let lookahead2 ~pass next before =
     when !Flag.c_plus_plus ->
 	  TCommentCpp (Token_c.CppDirective, i1)
 
+  (* const ident const: ident must be a type *)
+  | (TIdent (s, i1)::Tconst _::_,  Tconst _::_)
+      when not_struct_enum before
+      && ok_typedef s
+        ->
+      msg_typedef s i1 38; LP.add_typedef_root s;
+      TypedefIdent (s, i1)
+
 	(* xx const tt *)
   | (TIdent (s, i1)::(Tconst _|Tvolatile _|Trestrict _)::type_::_  , _)
     when not_struct_enum before

[Cocci] remove typedefs from struct

[Sören Brinkmann]

On Tue, 2015-02-17 at 07:56AM +0100, Julia Lawall wrote:
> Another patch is below.  With this the code parses, and the transformation 
> can be made.  However, it considers this to be a duplicate const 
> annotation, so I'm not sure that the const is being associated in the 
> right way by the parser.

I applied the patch. I do see the mentioned warning now:
  Warning: PARSING: duplicate 'const'; value = [1]

The replacement still doesn't happen though. I'm not sure whether I
need another rule to catch these declarations. But I think I will
just make people fix their code. I think having 'const' appear twice
is not right, despite it being parseable.

	S?ren

[Cocci] remove typedefs from struct

[Julia Lawall]

On Tue, 17 Feb 2015, S?ren Brinkmann wrote:

> On Tue, 2015-02-17 at 07:56AM +0100, Julia Lawall wrote:
> > Another patch is below.  With this the code parses, and the transformation 
> > can be made.  However, it considers this to be a duplicate const 
> > annotation, so I'm not sure that the const is being associated in the 
> > right way by the parser.
> 
> I applied the patch. I do see the mentioned warning now:
>   Warning: PARSING: duplicate 'const'; value = [1]
> 
> The replacement still doesn't happen though. I'm not sure whether I
> need another rule to catch these declarations. But I think I will
> just make people fix their code. I think having 'const' appear twice
> is not right, despite it being parseable.

Could you send a complete example?  It worked in the example I tried.

julia

[Cocci] remove typedefs from struct

[Sören Brinkmann]

On Tue, 2015-02-17 at 09:31PM +0100, Julia Lawall wrote:
> On Tue, 17 Feb 2015, S?ren Brinkmann wrote:
> 
> > On Tue, 2015-02-17 at 07:56AM +0100, Julia Lawall wrote:
> > > Another patch is below.  With this the code parses, and the transformation 
> > > can be made.  However, it considers this to be a duplicate const 
> > > annotation, so I'm not sure that the const is being associated in the 
> > > right way by the parser.
> > 
> > I applied the patch. I do see the mentioned warning now:
> >   Warning: PARSING: duplicate 'const'; value = [1]
> > 
> > The replacement still doesn't happen though. I'm not sure whether I
> > need another rule to catch these declarations. But I think I will
> > just make people fix their code. I think having 'const' appear twice
> > is not right, despite it being parseable.
> 
> Could you send a complete example?  It worked in the example I tried.

Sure. Please fine a test c file and my cocci patch (which grew a bit)
attached. This is the diff I get:
  init_defs_builtins: /usr/local/share/coccinelle/standard.h
  HANDLING: testcase4.c
  diff = 
  --- testcase4.c
  +++ /tmp/cocci-output-7912-c5f1e0-testcase4.c
  @@ -1,21 +1,21 @@
   
  -typedef struct foo {
  +struct foo {
   	int member;
  -} foo_t;
  +};
   
  -static foo_t foo;
  -static const foo_t stco_foo;
  +static struct foo foo;
  +static const struct foo stco_foo;
   const foo_t const co_foo_co;
   static const foo_t const stco_foo_co;
   static const foo_t const stco_foo_coarr[] = {
   };
   
  -typedef struct {
  +struct bar {
   	int member;
  -} bar_t;
  +};
   
  -static bar_t bar;
  -static const bar_t stco_bar;
  +static struct bar bar;
  +static const struct bar stco_bar;
   const bar_t const co_bar_co;
   static const bar_t const stco_bar_co;
   static const bar_t const stco_bar_coarr[] = {

	Thanks,
	S?ren
-------------- next part --------------
A non-text attachment was scrubbed...
Name: testcase4.c
Type: text/x-csrc
Size: 412 bytes
Desc: not available
URL: <https://systeme.lip6.fr/pipermail/cocci/attachments/20150217/17db5dc0/attachment.bin>
-------------- next part --------------
/* Convert typdefed structs to structs */

/* Convert typdef to struct declaration */
@ rule1 @
identifier i;
type t;
@@
-typedef struct i {
+struct i {
	...
}
- t
;

/* Replace type with struct */
@ rule2 @
identifier rule1.i;
type rule1.t;
@@
-t
+struct i

/* Catch occurences of 'const' instances/pointers */
@ rule3 @
identifier rule1.i;
type rule1.t;
@@
const
-t
+struct i

/* Replace typdef forward declarations with struct declaration */
@ rule4 @
identifier i;
type t;
@@
-typedef struct i t;
+struct i;

/* Replace type with struct (forward declarations */
@ rule5 @
identifier rule4.i;
type rule4.t;
@@
-t
+struct i

/* Replace type with struct (forward declarations */
@ rule6 @
identifier rule4.i;
type rule4.t;
@@
const
-t
+struct i

/* Convert unnamed structs */
@ rule7 @
type t;
@@
typedef struct {
	...
}
t
;

/* Make typename usable as identifier */
@ script:python rule8 @
t << rule7.t;
i;
@@
coccinelle.i = t.rstrip('_t')

@ rule9 @
type rule7.t;
identifier rule8.i;
@@
-typedef struct {
+struct i {
	...
}
- t
;

/* Replace type with struct */
@ rule10 @
type rule7.t;
identifier rule8.i;
@@
-t
+struct i

/* Catch occurences of 'const' instances/pointers */
@ rule11 @
type rule7.t;
identifier rule8.i;
@@
const
-t
+struct i

[Cocci] remove typedefs from struct

[Julia Lawall]

On Tue, 17 Feb 2015, S?ren Brinkmann wrote:

> On Tue, 2015-02-17 at 09:31PM +0100, Julia Lawall wrote:
> > On Tue, 17 Feb 2015, S?ren Brinkmann wrote:
> > 
> > > On Tue, 2015-02-17 at 07:56AM +0100, Julia Lawall wrote:
> > > > Another patch is below.  With this the code parses, and the transformation 
> > > > can be made.  However, it considers this to be a duplicate const 
> > > > annotation, so I'm not sure that the const is being associated in the 
> > > > right way by the parser.
> > > 
> > > I applied the patch. I do see the mentioned warning now:
> > >   Warning: PARSING: duplicate 'const'; value = [1]
> > > 
> > > The replacement still doesn't happen though. I'm not sure whether I
> > > need another rule to catch these declarations. But I think I will
> > > just make people fix their code. I think having 'const' appear twice
> > > is not right, despite it being parseable.
> > 
> > Could you send a complete example?  It worked in the example I tried.
> 
> Sure. Please fine a test c file and my cocci patch (which grew a bit)
> attached. This is the diff I get:
>   init_defs_builtins: /usr/local/share/coccinelle/standard.h
>   HANDLING: testcase4.c
>   diff = 
>   --- testcase4.c
>   +++ /tmp/cocci-output-7912-c5f1e0-testcase4.c
>   @@ -1,21 +1,21 @@
>    
>   -typedef struct foo {
>   +struct foo {
>    	int member;
>   -} foo_t;
>   +};
>    
>   -static foo_t foo;
>   -static const foo_t stco_foo;
>   +static struct foo foo;
>   +static const struct foo stco_foo;
>    const foo_t const co_foo_co;
>    static const foo_t const stco_foo_co;

OK, the problem seems to be that the representation of the type in C code 
involves two occurrences of the token "const" and the representation of 
the type in SmPL involves at most one.  So there is no way to match one of 
these double const things.  Telling the developer to remove the second 
const is probably the reasonable solution.

julia

[Cocci] Finding several type qualifiers with SmPL?

[SF Markus Elfring]

> So there is no way to match one of these double const things.

Is the situation different for the semantic patch language
if multiple qualifiers would be syntactically required?

Another source code example:
static const struct data const* my_pointer;


> Telling the developer to remove the second const is probably
> the reasonable solution.

Would you like to support additional type qualifiers there?

Regards,
Markus

[Cocci] Finding several type qualifiers with SmPL?

[Sören Brinkmann]

On Thu, 2015-02-19 at 10:08AM +0100, SF Markus Elfring wrote:
> > So there is no way to match one of these double const things.
> 
> Is the situation different for the semantic patch language
> if multiple qualifiers would be syntactically required?
> 
> Another source code example:
> static const struct data const* my_pointer;

That would still be just a duplicate, wouldn't it. To actually make
sense it would have to be:
  static const struct data * const my_pointer;

That works I think (at least with spatch patched with the first patch
Julia sent in the thread that started this).

> 
> 
> > Telling the developer to remove the second const is probably
> > the reasonable solution.
> 
> Would you like to support additional type qualifiers there?

My personal view, if gcc can parse&compile it, spatch should correctly parse
it. There seem to be some discrepancies currently.

	S?ren

[Cocci] Finding several type qualifiers with SmPL?

[Julia Lawall]

> My personal view, if gcc can parse&compile it, spatch should correctly parse
> it. There seem to be some discrepancies currently.

Parsing is hard.  Currently, our view is if it is in the Linux kernel
source code, we would like to try to parse it maybe.  There will be some
work on the parser in the next few months, however.

Actually, the current issue is not a parsing problem but a matching
problem.  I guess it could become a parsing problem by just considering
the second const to be a comment.

julia

[Cocci] Finding several type qualifiers with SmPL?

[SF Markus Elfring]

> I guess it could become a parsing problem by just considering
> the second const to be a comment.

Would you like to reconsider consequences from a C99 rule like "6.7.3/4"?
http://stackoverflow.com/questions/5781222/duplicate-const-qualifier-allowed-in-c-but-not-in-c

Regards,
Markus

[Cocci] Finding several type qualifiers with SmPL?

[SF Markus Elfring]

>> Another source code example:
>> static const struct data const* my_pointer;
> 
> That would still be just a duplicate, wouldn't it.

Would it make sense that the semantic patch language can also handle
such misplaced stars or type qualifiers?


> To actually make sense it would have to be:
>   static const struct data * const my_pointer;

This kind of source code will be more useful together with an
initialisation expression, won't it?


Can you imagine more nice transformations here?

Regards,
Markus

[Cocci] remove typedefs from struct

[SF Markus Elfring]

>   /// Convert typdefed structs to structs
>   
>   @ rule1 @
>   identifier i;
>   type t;
>   @@
>   -typedef struct i {
>   +struct i {
>   	...
>   - }t;
>   + };

How do you think about a bit more fine-tuning for your SmPL approach?

 ...
 }
-t
 ;

Is it sufficient to express the desired deletion at this place?

Regards,
Markus

[Cocci] remove typedefs from struct

[Julia Lawall]

On Sun, 15 Feb 2015, SF Markus Elfring wrote:

> >   /// Convert typdefed structs to structs
> >   
> >   @ rule1 @
> >   identifier i;
> >   type t;
> >   @@
> >   -typedef struct i {
> >   +struct i {
> >   	...
> >   - }t;
> >   + };
> 
> How do you think about a bit more fine-tuning for your SmPL approach?
> 
>  ...
>  }
> -t
>  ;
> 
> Is it sufficient to express the desired deletion at this place?

It is possible, but perhaps not necessary.  It would depend on whether 
there are any comments or attributes between the } and the ; that need to 
be preserved.  Currently they will be removed.

Another issue is that the typedef name might be (closer to) the one that 
ou want to preserve.  And i is not even necessary, I believe.  You could 
have another rule for the case where it is not present.

julia

[Cocci] remove typedefs from struct

[Sören Brinkmann]

On Sun, 2015-02-15 at 06:32PM +0100, Julia Lawall wrote:
> On Sun, 15 Feb 2015, SF Markus Elfring wrote:
> 
> > >   /// Convert typdefed structs to structs
> > >   
> > >   @ rule1 @
> > >   identifier i;
> > >   type t;
> > >   @@
> > >   -typedef struct i {
> > >   +struct i {
> > >   	...
> > >   - }t;
> > >   + };
> > 
> > How do you think about a bit more fine-tuning for your SmPL approach?
> > 
> >  ...
> >  }
> > -t
> >  ;
> > 
> > Is it sufficient to express the desired deletion at this place?
> 
> It is possible, but perhaps not necessary.  It would depend on whether 
> there are any comments or attributes between the } and the ; that need to 
> be preserved.  Currently they will be removed.

Good point. I hadn't looked into such cases yet and the codebase I'm
looking at doesn't have attributes or comments in that line. But in
general, it seems to make sense changing the patch this way.
I didn't even expect such a change would have any effect on the result. I
have a lot to learn still :)

> 
> Another issue is that the typedef name might be (closer to) the one that 
> ou want to preserve.  And i is not even necessary, I believe.  You could 
> have another rule for the case where it is not present.

Right. In my case, the typedefs consistently use i and t with i and t
even being identical. I guess it depends on your sources which one you
want to preserve. My assumption is that we'd have something like
 typedef struct foo {...} foo_t;
which would make i more appropriate to preserve than t. But depending on
the coding style your sources follow things can look quite different, I
suppose.

	Thanks,
	Soren

[Cocci] remove typedefs from struct

[Sören Brinkmann]

On Sun, 2015-02-15 at 06:32PM +0100, Julia Lawall wrote:
> On Sun, 15 Feb 2015, SF Markus Elfring wrote:
> 
> > >   /// Convert typdefed structs to structs
> > >   
> > >   @ rule1 @
> > >   identifier i;
> > >   type t;
> > >   @@
> > >   -typedef struct i {
> > >   +struct i {
> > >   	...
> > >   - }t;
> > >   + };
> > 
> > How do you think about a bit more fine-tuning for your SmPL approach?
> > 
> >  ...
> >  }
> > -t
> >  ;
> > 
> > Is it sufficient to express the desired deletion at this place?
> 
> It is possible, but perhaps not necessary.  It would depend on whether 
> there are any comments or attributes between the } and the ; that need to 
> be preserved.  Currently they will be removed.
> 
> Another issue is that the typedef name might be (closer to) the one that 
> ou want to preserve.  And i is not even necessary, I believe.  You could 
> have another rule for the case where it is not present.

Sorry for bugging you again, but I tried to create a rule for cases that
don't name the struct. E.g.
  typedef struct {
  	int bar;
  } foo;

My problem seems to be, that a struct identifier is an identifier, while
the newly created type is a type. Hence, my naive approach:
@@
type t;
@@
-typedef struct {
+struct t {
	...
}
- t
;

does not work. And thus far, I couldn't find a way to get around this. I
suspect there is some subtle trick to make it do what I want but I'm
just not seeing it.

	Soren

[Cocci] remove typedefs from struct

[Julia Lawall]

> Sorry for bugging you again, but I tried to create a rule for cases that
> don't name the struct. E.g.
>   typedef struct {
>   	int bar;
>   } foo;
> 
> My problem seems to be, that a struct identifier is an identifier, while
> the newly created type is a type. Hence, my naive approach:
> @@
> type t;
> @@
> -typedef struct {
> +struct t {
> 	...
> }
> - t
> ;
> 
> does not work. And thus far, I couldn't find a way to get around this. I
> suspect there is some subtle trick to make it do what I want but I'm
> just not seeing it.

It's a hack, but you can pass through python or ocaml.

@r@
type t;
@@
typedef struct {
	...
} t ;

@script:python s@
t << r.t;
i;
@@

coccinelle.i = t

@@
tpye r.t;
identifier s.i;
@@

...

Now in the last rule, you have all the information you need.  You might even
remove eg any trailing _t from t along the way.

julia

[Cocci] remove typedefs from struct

[Sören Brinkmann]

On Mon, 2015-02-16 at 07:19AM +0100, Julia Lawall wrote:
> > Sorry for bugging you again, but I tried to create a rule for cases that
> > don't name the struct. E.g.
> >   typedef struct {
> >   	int bar;
> >   } foo;
> > 
> > My problem seems to be, that a struct identifier is an identifier, while
> > the newly created type is a type. Hence, my naive approach:
> > @@
> > type t;
> > @@
> > -typedef struct {
> > +struct t {
> > 	...
> > }
> > - t
> > ;
> > 
> > does not work. And thus far, I couldn't find a way to get around this. I
> > suspect there is some subtle trick to make it do what I want but I'm
> > just not seeing it.
> 
> It's a hack, but you can pass through python or ocaml.
> 
> @r@
> type t;
> @@
> typedef struct {
> 	...
> } t ;
> 
> @script:python s@
> t << r.t;
> i;
> @@
> 
> coccinelle.i = t
> 
> @@
> tpye r.t;
> identifier s.i;
> @@
> 
> ...
> 
> Now in the last rule, you have all the information you need.  You might even
> remove eg any trailing _t from t along the way.

Awesome, that's it. I was pretty close. At some time I had pretty much
exactly that. I think all I missed was the 'coccinelle.' in the python
code.

	Thanks,
	Soren

[Cocci] remove typedefs from struct

[Julia Lawall]

On Sun, 15 Feb 2015, S?ren Brinkmann wrote:

> On Mon, 2015-02-16 at 07:19AM +0100, Julia Lawall wrote:
> > > Sorry for bugging you again, but I tried to create a rule for cases that
> > > don't name the struct. E.g.
> > >   typedef struct {
> > >   	int bar;
> > >   } foo;
> > >
> > > My problem seems to be, that a struct identifier is an identifier, while
> > > the newly created type is a type. Hence, my naive approach:
> > > @@
> > > type t;
> > > @@
> > > -typedef struct {
> > > +struct t {
> > > 	...
> > > }
> > > - t
> > > ;
> > >
> > > does not work. And thus far, I couldn't find a way to get around this. I
> > > suspect there is some subtle trick to make it do what I want but I'm
> > > just not seeing it.
> >
> > It's a hack, but you can pass through python or ocaml.
> >
> > @r@
> > type t;
> > @@
> > typedef struct {
> > 	...
> > } t ;
> >
> > @script:python s@
> > t << r.t;
> > i;
> > @@
> >
> > coccinelle.i = t
> >
> > @@
> > tpye r.t;
> > identifier s.i;
> > @@
> >
> > ...
> >
> > Now in the last rule, you have all the information you need.  You might even
> > remove eg any trailing _t from t along the way.
>
> Awesome, that's it. I was pretty close. At some time I had pretty much
> exactly that. I think all I missed was the 'coccinelle.' in the python
> code.

In the demos subdirectory there are two examples:

pythontococci.cocci
camltococci.cocci

They are very helpful for remembering the syntax.

julia