[PATCH v1 0/2] Anonymous inner struct

[PATCH v1 0/2] Anonymous inner struct

[Douglas RAILLARD]

From: Douglas Raillard <douglas.raillard@arm.com>

Add an --inner_anonymous pahole CLI option to make inner
struct/enum/union anonymous. This allows turning this:

    struct foo { ... };
    struct bar {
        struct foo {
         ....
        } a;
    };
    
into this:

    struct foo { ... };
    struct bar {
        struct /* foo */ {
         ....
        } a;
    };

This avoids any conflict between the two definitions of struct foo. The
case arises when dumping multiple types at once with -E, all depending
on a same inner struct:

    struct bar {
        struct foo {
         ....
        } a;
    };
    struct bar2 {
        struct foo {
         ....
        } b;
    };


On top of that, struct foo could already be defined in a public header
already imported in the compilation unit, leading to a redefinition
error, while another nested struct is private and would still need -E
for the type to be usable.

Douglas Raillard (2):
  fprintf: Allow making struct/enum/union anonymous
  pahole.c: Add --inner_anonymous option

 dwarves.h         |  4 +++-
 dwarves_emit.c    |  2 +-
 dwarves_fprintf.c | 52 ++++++++++++++++++++++++++++++-----------------
 pahole.c          |  8 ++++++++
 4 files changed, 45 insertions(+), 21 deletions(-)

-- 
2.25.1

[PATCH v1 1/2] fprintf: Allow making struct/enum/union anonymous

[Douglas RAILLARD]

From: Douglas Raillard <douglas.raillard@arm.com>

Allow making inner struct enums and union anonymous.

Signed-off-by: Douglas Raillard <douglas.raillard@arm.com>
---
 dwarves.h         |  4 +++-
 dwarves_emit.c    |  2 +-
 dwarves_fprintf.c | 52 ++++++++++++++++++++++++++++++-----------------
 3 files changed, 37 insertions(+), 21 deletions(-)

diff --git a/dwarves.h b/dwarves.h
index 30d33fa..be334cb 100644
--- a/dwarves.h
+++ b/dwarves.h
@@ -99,6 +99,7 @@ struct conf_fprintf {
 	uint8_t	   indent;
 	uint8_t	   expand_types:1;
 	uint8_t	   expand_pointers:1;
+	uint8_t	   inner_anonymous:1;
 	uint8_t    rel_offset:1;
 	uint8_t	   emit_stats:1;
 	uint8_t	   suppress_comments:1;
@@ -1327,7 +1328,8 @@ static inline const char *enumerator__name(const struct enumerator *enumerator)
 void enumeration__delete(struct type *type);
 void enumeration__add(struct type *type, struct enumerator *enumerator);
 size_t enumeration__fprintf(const struct tag *tag_enum,
-			    const struct conf_fprintf *conf, FILE *fp);
+			    const struct conf_fprintf *conf, FILE *fp,
+			    bool anonymous);
 
 int dwarves__init(uint16_t user_cacheline_size);
 void dwarves__exit(void);
diff --git a/dwarves_emit.c b/dwarves_emit.c
index 5bf7946..5c67559 100644
--- a/dwarves_emit.c
+++ b/dwarves_emit.c
@@ -91,7 +91,7 @@ static int enumeration__emit_definitions(struct tag *tag,
 		return 0;
 	}
 
-	enumeration__fprintf(tag, conf, fp);
+	enumeration__fprintf(tag, conf, fp, false);
 	fputs(";\n", fp);
 	type_emissions__add_definition(emissions, etype);
 	return 1;
diff --git a/dwarves_fprintf.c b/dwarves_fprintf.c
index c35868a..803cdab 100644
--- a/dwarves_fprintf.c
+++ b/dwarves_fprintf.c
@@ -186,7 +186,8 @@ size_t tag__fprintf_decl_info(const struct tag *tag,
 }
 
 static size_t __class__fprintf(struct class *class, const struct cu *cu,
-			       const struct conf_fprintf *conf, FILE *fp);
+			       const struct conf_fprintf *conf, FILE *fp,
+			       bool anonymous);
 static size_t type__fprintf(struct tag *type, const struct cu *cu,
 			    const char *name, const struct conf_fprintf *conf,
 			    FILE *fp);
@@ -318,7 +319,7 @@ size_t typedef__fprintf(const struct tag *tag, const struct cu *cu,
 		struct conf_fprintf tconf = *pconf;
 
 		tconf.suffix = type__name(type);
-		return fprintf(fp, "typedef ") + __class__fprintf(tag__class(tag_type), cu, &tconf, fp);
+		return fprintf(fp, "typedef ") + __class__fprintf(tag__class(tag_type), cu, &tconf, fp, false);
 	}
 	case DW_TAG_enumeration_type: {
 		struct type *ctype = tag__type(tag_type);
@@ -329,7 +330,7 @@ size_t typedef__fprintf(const struct tag *tag, const struct cu *cu,
 		struct conf_fprintf tconf = *pconf;
 
 		tconf.suffix = type__name(type);
-		return fprintf(fp, "typedef ") + enumeration__fprintf(tag_type, &tconf, fp);
+		return fprintf(fp, "typedef ") + enumeration__fprintf(tag_type, &tconf, fp, false);
 	}
 	}
 
@@ -382,12 +383,16 @@ out:
 	return type->max_tag_name_len;
 }
 
-size_t enumeration__fprintf(const struct tag *tag, const struct conf_fprintf *conf, FILE *fp)
+size_t enumeration__fprintf(const struct tag *tag, const struct conf_fprintf *conf, FILE *fp, bool anonymous)
 {
 	struct type *type = tag__type(tag);
 	struct enumerator *pos;
 	int max_entry_name_len = enumeration__max_entry_name_len(type);
-	size_t printed = fprintf(fp, "enum%s%s {\n", type__name(type) ? " " : "", type__name(type) ?: "");
+	size_t printed = fprintf(fp, "enum%s%s%s%s {\n",
+				 type__name(type) ? " " : "",
+				 type__name(type) && anonymous ? "/* " : "",
+				 type__name(type) ? type__name(type) : "",
+				 type__name(type) && anonymous ? " */" : "");
 	int indent = conf->indent;
 
 	if (indent >= (int)sizeof(tabs))
@@ -622,7 +627,8 @@ static size_t type__fprintf_stats(struct type *type, const struct cu *cu,
 }
 
 static size_t union__fprintf(struct type *type, const struct cu *cu,
-			     const struct conf_fprintf *conf, FILE *fp);
+			     const struct conf_fprintf *conf, FILE *fp,
+			     bool anonymous);
 
 static size_t type__fprintf(struct tag *type, const struct cu *cu,
 			    const char *name, const struct conf_fprintf *conf,
@@ -638,6 +644,7 @@ static size_t type__fprintf(struct tag *type, const struct cu *cu,
 	};
 	size_t printed = 0;
 	int expand_types = conf->expand_types;
+	int inner_anonymous = conf->inner_anonymous;
 	int suppress_offset_comment = conf->suppress_offset_comment;
 
 	if (type == NULL)
@@ -795,7 +802,7 @@ print_default:
 				class__find_holes(cclass);
 
 			tconf.type_spacing -= 8;
-			printed += __class__fprintf(cclass, cu, &tconf, fp);
+			printed += __class__fprintf(cclass, cu, &tconf, fp, inner_anonymous);
 		}
 		break;
 	case DW_TAG_union_type:
@@ -805,7 +812,7 @@ print_default:
 			printed += fprintf(fp, "union %-*s %s", tconf.type_spacing - 6, type__name(ctype), name);
 		} else {
 			tconf.type_spacing -= 8;
-			printed += union__fprintf(ctype, cu, &tconf, fp);
+			printed += union__fprintf(ctype, cu, &tconf, fp, inner_anonymous);
 		}
 		break;
 	case DW_TAG_enumeration_type:
@@ -814,7 +821,7 @@ print_default:
 		if (type__name(ctype) != NULL)
 			printed += fprintf(fp, "enum %-*s %s", tconf.type_spacing - 5, type__name(ctype), name);
 		else
-			printed += enumeration__fprintf(type, &tconf, fp);
+			printed += enumeration__fprintf(type, &tconf, fp, inner_anonymous);
 		break;
 	}
 out:
@@ -968,7 +975,8 @@ static size_t union_member__fprintf(struct class_member *member,
 }
 
 static size_t union__fprintf(struct type *type, const struct cu *cu,
-			     const struct conf_fprintf *conf, FILE *fp)
+			     const struct conf_fprintf *conf, FILE *fp,
+			     bool anonymous)
 {
 	struct class_member *pos;
 	size_t printed = 0;
@@ -982,8 +990,11 @@ static size_t union__fprintf(struct type *type, const struct cu *cu,
 
 	if (conf->prefix != NULL)
 		printed += fprintf(fp, "%s ", conf->prefix);
-	printed += fprintf(fp, "union%s%s {\n", type__name(type) ? " " : "",
-			   type__name(type) ?: "");
+	printed += fprintf(fp, "union%s%s%s%s {\n",
+			   type__name(type) ? " " : "",
+			   type__name(type) && anonymous ? "/* " : "",
+			   type__name(type) ?: "",
+			   type__name(type) && anonymous ? " */" : "");
 
 	uconf = *conf;
 	uconf.indent = indent + 1;
@@ -1341,7 +1352,8 @@ out:
 }
 
 static size_t __class__fprintf(struct class *class, const struct cu *cu,
-			       const struct conf_fprintf *conf, FILE *fp)
+			       const struct conf_fprintf *conf, FILE *fp,
+			       bool anonymous)
 {
 	struct type *type = &class->type;
 	size_t last_size = 0, size;
@@ -1361,14 +1373,16 @@ static size_t __class__fprintf(struct class *class, const struct cu *cu,
 	const char *current_accessibility = NULL;
 	struct conf_fprintf cconf = conf ? *conf : conf_fprintf__defaults;
 	const uint16_t t = type->namespace.tag.tag;
-	size_t printed = fprintf(fp, "%s%s%s%s%s",
+	size_t printed = fprintf(fp, "%s%s%s%s%s%s%s",
 				 cconf.prefix ?: "", cconf.prefix ? " " : "",
 				 ((cconf.classes_as_structs ||
 				   t == DW_TAG_structure_type) ? "struct" :
 				  t == DW_TAG_class_type ? "class" :
 							"interface"),
 				 type__name(type) ? " " : "",
-				 type__name(type) ?: "");
+				 anonymous && type__name(type) ? "/* " : "",
+				 type__name(type) ?: "",
+				 anonymous && type__name(type) ? " */" : "");
 	int indent = cconf.indent;
 
 	if (indent >= (int)sizeof(tabs))
@@ -1785,7 +1799,7 @@ out:
 
 size_t class__fprintf(struct class *class, const struct cu *cu, FILE *fp)
 {
-	return __class__fprintf(class, cu, NULL, fp);
+	return __class__fprintf(class, cu, NULL, fp, false);
 }
 
 static size_t variable__fprintf(const struct tag *tag, const struct cu *cu,
@@ -1872,7 +1886,7 @@ size_t tag__fprintf(struct tag *tag, const struct cu *cu,
 		printed += array_type__fprintf(tag, cu, "array", pconf, fp);
 		break;
 	case DW_TAG_enumeration_type:
-		printed += enumeration__fprintf(tag, pconf, fp);
+		printed += enumeration__fprintf(tag, pconf, fp, false);
 		break;
 	case DW_TAG_typedef:
 		printed += typedef__fprintf(tag, cu, pconf, fp);
@@ -1880,7 +1894,7 @@ size_t tag__fprintf(struct tag *tag, const struct cu *cu,
 	case DW_TAG_class_type:
 	case DW_TAG_interface_type:
 	case DW_TAG_structure_type:
-		printed += __class__fprintf(tag__class(tag), cu, pconf, fp);
+		printed += __class__fprintf(tag__class(tag), cu, pconf, fp, false);
 		break;
 	case DW_TAG_subroutine_type:
 		printed += ftype__fprintf(tag__ftype(tag), cu, NULL, false, false, 0, true, pconf, fp);
@@ -1892,7 +1906,7 @@ size_t tag__fprintf(struct tag *tag, const struct cu *cu,
 		printed += function__fprintf(tag, cu, pconf, fp);
 		break;
 	case DW_TAG_union_type:
-		printed += union__fprintf(tag__type(tag), cu, pconf, fp);
+		printed += union__fprintf(tag__type(tag), cu, pconf, fp, false);
 		break;
 	case DW_TAG_variable:
 		printed += variable__fprintf(tag, cu, pconf, fp);
-- 
2.25.1

[PATCH v1 2/2] pahole.c: Add --inner_anonymous option

[Douglas RAILLARD]

From: Douglas Raillard <douglas.raillard@arm.com>

Allow making the inner struct/enum/union anonymous. This permits using
the header to inspect pointer values using -E, without having to care
about avoiding duplicate type definitions such as:

    struct foo { ... };
    struct bar {
        struct foo {
	     ....
	} a;
    };

With --inner_anonymous, the conflict between the two definitions of
struct foo is gone:

    struct foo { ... };
    struct bar {
        struct {
	     ....
	} a;
    };

Signed-off-by: Douglas Raillard <douglas.raillard@arm.com>
---
 pahole.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/pahole.c b/pahole.c
index 80271b5..b9c6305 100644
--- a/pahole.c
+++ b/pahole.c
@@ -1125,6 +1125,7 @@ ARGP_PROGRAM_VERSION_HOOK_DEF = dwarves_print_version;
 #define ARGP_hashbits		   329
 #define ARGP_devel_stats	   330
 #define ARGP_skip_encoding_btf_tag 331
+#define ARGP_inner_anonymous       332
 
 static const struct argp_option pahole__options[] = {
 	{
@@ -1232,6 +1233,11 @@ static const struct argp_option pahole__options[] = {
 		.key  = 'E',
 		.doc  = "expand class members",
 	},
+	{
+		.name = "inner_anonymous",
+		.key  = ARGP_inner_anonymous,
+		.doc  = "expanded class members are anonymous",
+	},
 	{
 		.name = "nr_members",
 		.key  = 'n',
@@ -1650,6 +1656,8 @@ static error_t pahole__options_parser(int key, char *arg,
 		conf_load.ptr_table_stats = true;	break;
 	case ARGP_skip_encoding_btf_tag:
 		conf_load.skip_encoding_btf_tag = true;	break;
+	case ARGP_inner_anonymous:
+		conf.inner_anonymous = true;            break;
 	default:
 		return ARGP_ERR_UNKNOWN;
 	}
-- 
2.25.1

Re: [PATCH v1 0/2] Anonymous inner struct

[Douglas Raillard]

Hi Arnaldo,

Any opinion on this series ?

Cheers,
Douglas

On 10/19/21 11:07 AM, Douglas RAILLARD wrote:
> From: Douglas Raillard <douglas.raillard@arm.com>
> 
> Add an --inner_anonymous pahole CLI option to make inner
> struct/enum/union anonymous. This allows turning this:
> 
>      struct foo { ... };
>      struct bar {
>          struct foo {
>           ....
>          } a;
>      };
>      
> into this:
> 
>      struct foo { ... };
>      struct bar {
>          struct /* foo */ {
>           ....
>          } a;
>      };
> 
> This avoids any conflict between the two definitions of struct foo. The
> case arises when dumping multiple types at once with -E, all depending
> on a same inner struct:
> 
>      struct bar {
>          struct foo {
>           ....
>          } a;
>      };
>      struct bar2 {
>          struct foo {
>           ....
>          } b;
>      };
> 
> 
> On top of that, struct foo could already be defined in a public header
> already imported in the compilation unit, leading to a redefinition
> error, while another nested struct is private and would still need -E
> for the type to be usable.
> 
> Douglas Raillard (2):
>    fprintf: Allow making struct/enum/union anonymous
>    pahole.c: Add --inner_anonymous option
> 
>   dwarves.h         |  4 +++-
>   dwarves_emit.c    |  2 +-
>   dwarves_fprintf.c | 52 ++++++++++++++++++++++++++++++-----------------
>   pahole.c          |  8 ++++++++
>   4 files changed, 45 insertions(+), 21 deletions(-)
> 

Re: [PATCH v1 0/2] Anonymous inner struct

[Arnaldo Carvalho de Melo]

Em Tue, Nov 16, 2021 at 02:09:44PM +0000, Douglas Raillard escreveu:
> Hi Arnaldo,
> 
> Any opinion on this series ?

I'll try and get back to processing pahole patches tomorrow,

- Arnaldo
 
> Cheers,
> Douglas
> 
> On 10/19/21 11:07 AM, Douglas RAILLARD wrote:
> > From: Douglas Raillard <douglas.raillard@arm.com>
> > 
> > Add an --inner_anonymous pahole CLI option to make inner
> > struct/enum/union anonymous. This allows turning this:
> > 
> >      struct foo { ... };
> >      struct bar {
> >          struct foo {
> >           ....
> >          } a;
> >      };
> > into this:
> > 
> >      struct foo { ... };
> >      struct bar {
> >          struct /* foo */ {
> >           ....
> >          } a;
> >      };
> > 
> > This avoids any conflict between the two definitions of struct foo. The
> > case arises when dumping multiple types at once with -E, all depending
> > on a same inner struct:
> > 
> >      struct bar {
> >          struct foo {
> >           ....
> >          } a;
> >      };
> >      struct bar2 {
> >          struct foo {
> >           ....
> >          } b;
> >      };
> > 
> > 
> > On top of that, struct foo could already be defined in a public header
> > already imported in the compilation unit, leading to a redefinition
> > error, while another nested struct is private and would still need -E
> > for the type to be usable.
> > 
> > Douglas Raillard (2):
> >    fprintf: Allow making struct/enum/union anonymous
> >    pahole.c: Add --inner_anonymous option
> > 
> >   dwarves.h         |  4 +++-
> >   dwarves_emit.c    |  2 +-
> >   dwarves_fprintf.c | 52 ++++++++++++++++++++++++++++++-----------------
> >   pahole.c          |  8 ++++++++
> >   4 files changed, 45 insertions(+), 21 deletions(-)
> > 

-- 

- Arnaldo

Re: [PATCH v1 1/2] fprintf: Allow making struct/enum/union anonymous

[Arnaldo Carvalho de Melo]

Em Tue, Oct 19, 2021 at 11:07:23AM +0100, Douglas RAILLARD escreveu:
> From: Douglas Raillard <douglas.raillard@arm.com>
> 
> Allow making inner struct enums and union anonymous.

So I had to apply it by hand due to changes in the areas it touches, see
below, and I expanded a bit the commit message, please review:


------ 8< ------------------------

fprintf: Allow making struct/enum/union anonymous

Allow making inner struct enums and union anonymous, so that when using
-E to expand types we don't end up with multiple definitions for
expanded inner structs, allowing the resulting expanded struct to be
compilable.

Signed-off-by: Douglas Raillard <douglas.raillard@arm.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
------ 8< ------------------------

⬢[acme@toolbox pahole]$ patch -p1 < ~/wb/1.patch
patching file dwarves.h
Hunk #1 succeeded at 105 (offset 6 lines).
Hunk #2 succeeded at 1372 with fuzz 2 (offset 44 lines).
patching file dwarves_emit.c
patching file dwarves_fprintf.c
Hunk #1 succeeded at 198 (offset 12 lines).
Hunk #2 succeeded at 331 (offset 12 lines).
Hunk #3 succeeded at 342 (offset 12 lines).
Hunk #4 succeeded at 395 (offset 12 lines).
Hunk #5 succeeded at 639 (offset 12 lines).
Hunk #6 succeeded at 656 (offset 12 lines).
Hunk #7 succeeded at 814 (offset 12 lines).
Hunk #8 succeeded at 824 with fuzz 1 (offset 12 lines).
Hunk #9 succeeded at 833 with fuzz 2 (offset 12 lines).
Hunk #10 succeeded at 998 (offset 23 lines).
Hunk #11 succeeded at 1013 (offset 23 lines).
Hunk #12 succeeded at 1375 (offset 23 lines).
Hunk #13 succeeded at 1396 (offset 23 lines).
Hunk #14 succeeded at 1822 (offset 23 lines).
Hunk #15 succeeded at 1909 (offset 23 lines).
Hunk #16 succeeded at 1917 (offset 23 lines).
Hunk #17 succeeded at 1929 (offset 23 lines).
⬢[acme@toolbox pahole]$

Re: [PATCH v1 1/2] fprintf: Allow making struct/enum/union anonymous

[Douglas Raillard]

On 11/26/21 6:27 PM, Arnaldo Carvalho de Melo wrote:
> Em Tue, Oct 19, 2021 at 11:07:23AM +0100, Douglas RAILLARD escreveu:
>> From: Douglas Raillard <douglas.raillard@arm.com>
>>
>> Allow making inner struct enums and union anonymous.
> 
> So I had to apply it by hand due to changes in the areas it touches, see
> below, and I expanded a bit the commit message, please review:

Thanks for the review, it seems to be working as well as before. That said I found a couple of issues:

1. CLI options seem to abbreviate "anonymous" into "anon" so maybe we should do the same for this one ?
2. I just found a case that generates a broken header:

	struct /* hrtimer */ {
		struct /* timerqueue_node */ {
			struct /* rb_node */ {
				long unsigned int __rb_parent_color;                     /*  3328     8 */
				struct rb_node * rb_right;                               /*  3336     8 */
				struct rb_node * rb_left;                                /*  3344     8 */
			}node; /*  3328    24 */
			/* typedef ktime_t -> s64 -> __s64 */ long long int expires;     /*  3352     8 */
		}node; /*  3328    32 */
		/* typedef ktime_t -> s64 -> __s64 */ long long int      _softexpires;   /*  3360     8 */
		enum hrtimer_restart (*function)(struct hrtimer *);                      /*  3368     8 */
		struct hrtimer_clock_base * base;                                        /*  3376     8 */
		/* typedef u8 -> __u8 */ unsigned char      state;                       /*  3384     1 */
		/* typedef u8 -> __u8 */ unsigned char      is_rel;                      /*  3385     1 */
		/* typedef u8 -> __u8 */ unsigned char      is_soft;                     /*  3386     1 */
		/* typedef u8 -> __u8 */ unsigned char      is_hard;                     /*  3387     1 */
	}hrtick_timer; /*  3328    64 */

Here we can see that "struct rb_node" is a recursive type, so since the type
definition is now anonymous it will not compile. Detecting recursive types and
printing the name would avoid that but defeats the original purpose
of --inner_anonymous.

I can see two solutions:

1. Detecting recursive types and appending a user-defined prefix to create a
    unique name.
2. Detecting recursive types and replacing the recursive references by "void *".

Solution #2 is the least invasive but will require a bit more work for the
end-user of the header:

  struct hrtimer foo = *ptr;
  typeof(foo.node.node) node = foo.node.node;

  // Extra cast using typeof in order to change the type of tb_right from void*
  // to "struct rb_node*"
  ((typeof(node))node.rb_right)->rb_left


Thanks,
Douglas

> 
> ------ 8< ------------------------
> 
> fprintf: Allow making struct/enum/union anonymous
> 
> Allow making inner struct enums and union anonymous, so that when using
> -E to expand types we don't end up with multiple definitions for
> expanded inner structs, allowing the resulting expanded struct to be
> compilable.
> 
> Signed-off-by: Douglas Raillard <douglas.raillard@arm.com>
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> ------ 8< ------------------------
> 
> ⬢[acme@toolbox pahole]$ patch -p1 < ~/wb/1.patch
> patching file dwarves.h
> Hunk #1 succeeded at 105 (offset 6 lines).
> Hunk #2 succeeded at 1372 with fuzz 2 (offset 44 lines).
> patching file dwarves_emit.c
> patching file dwarves_fprintf.c
> Hunk #1 succeeded at 198 (offset 12 lines).
> Hunk #2 succeeded at 331 (offset 12 lines).
> Hunk #3 succeeded at 342 (offset 12 lines).
> Hunk #4 succeeded at 395 (offset 12 lines).
> Hunk #5 succeeded at 639 (offset 12 lines).
> Hunk #6 succeeded at 656 (offset 12 lines).
> Hunk #7 succeeded at 814 (offset 12 lines).
> Hunk #8 succeeded at 824 with fuzz 1 (offset 12 lines).
> Hunk #9 succeeded at 833 with fuzz 2 (offset 12 lines).
> Hunk #10 succeeded at 998 (offset 23 lines).
> Hunk #11 succeeded at 1013 (offset 23 lines).
> Hunk #12 succeeded at 1375 (offset 23 lines).
> Hunk #13 succeeded at 1396 (offset 23 lines).
> Hunk #14 succeeded at 1822 (offset 23 lines).
> Hunk #15 succeeded at 1909 (offset 23 lines).
> Hunk #16 succeeded at 1917 (offset 23 lines).
> Hunk #17 succeeded at 1929 (offset 23 lines).
> ⬢[acme@toolbox pahole]$
> 

Re: [PATCH v1 1/2] fprintf: Allow making struct/enum/union anonymous

[Arnaldo Carvalho de Melo]

Em Tue, Nov 30, 2021 at 04:42:55PM +0000, Douglas Raillard escreveu:
> On 11/26/21 6:27 PM, Arnaldo Carvalho de Melo wrote:
> > Em Tue, Oct 19, 2021 at 11:07:23AM +0100, Douglas RAILLARD escreveu:
> > > From: Douglas Raillard <douglas.raillard@arm.com>
> > > 
> > > Allow making inner struct enums and union anonymous.
> > 
> > So I had to apply it by hand due to changes in the areas it touches, see
> > below, and I expanded a bit the commit message, please review:
> 
> Thanks for the review, it seems to be working as well as before. That said I found a couple of issues:
> 
> 1. CLI options seem to abbreviate "anonymous" into "anon" so maybe we should do the same for this one ?

Ok, I can shorten the option name here.

> 2. I just found a case that generates a broken header:
> 
> 	struct /* hrtimer */ {
> 		struct /* timerqueue_node */ {
> 			struct /* rb_node */ {
> 				long unsigned int __rb_parent_color;                     /*  3328     8 */
> 				struct rb_node * rb_right;                               /*  3336     8 */
> 				struct rb_node * rb_left;                                /*  3344     8 */
> 			}node; /*  3328    24 */
> 			/* typedef ktime_t -> s64 -> __s64 */ long long int expires;     /*  3352     8 */
> 		}node; /*  3328    32 */
> 		/* typedef ktime_t -> s64 -> __s64 */ long long int      _softexpires;   /*  3360     8 */
> 		enum hrtimer_restart (*function)(struct hrtimer *);                      /*  3368     8 */
> 		struct hrtimer_clock_base * base;                                        /*  3376     8 */
> 		/* typedef u8 -> __u8 */ unsigned char      state;                       /*  3384     1 */
> 		/* typedef u8 -> __u8 */ unsigned char      is_rel;                      /*  3385     1 */
> 		/* typedef u8 -> __u8 */ unsigned char      is_soft;                     /*  3386     1 */
> 		/* typedef u8 -> __u8 */ unsigned char      is_hard;                     /*  3387     1 */
> 	}hrtick_timer; /*  3328    64 */
> 
> Here we can see that "struct rb_node" is a recursive type, so since the type
> definition is now anonymous it will not compile. Detecting recursive types and
> printing the name would avoid that but defeats the original purpose
> of --inner_anonymous.
> 
> I can see two solutions:
> 
> 1. Detecting recursive types and appending a user-defined prefix to create a
>    unique name.
> 2. Detecting recursive types and replacing the recursive references by "void *".
 
> Solution #2 is the least invasive but will require a bit more work for the
> end-user of the header:
> 
>  struct hrtimer foo = *ptr;
>  typeof(foo.node.node) node = foo.node.node;
 
>  // Extra cast using typeof in order to change the type of tb_right from void*
>  // to "struct rb_node*"
>  ((typeof(node))node.rb_right)->rb_left

A third solution and probably the easiest to implement. If you don't
punch a hole on it:

Track if the struct is being printed the first time and then flip a bit
not to print it again?

$ cat inner_anon.c
struct foo {
	struct baz {
		struct rb_node {
			struct rb_node *prev, *next;
		} yy;
	} y;
	struct /* rb_node */ {
		struct rb_node *prev, *next;
	} x;
} i;
$ cc  -c  inner_anon.c   -o inner_anon.o
$ vim inner_anon.c
$ cat inner_anon.c
struct foo {
	struct rb_node {
		struct rb_node *prev, *next;
	} x;
	struct baz {
		struct /* rb_node */ {
			struct rb_node *prev, *next;
		} yy;
	} y;
} i;
$ cc  -c  inner_anon.c   -o inner_anon.o
$

Re: [PATCH v1 1/2] fprintf: Allow making struct/enum/union anonymous

[Douglas Raillard]

On 11/30/21 6:49 PM, Arnaldo Carvalho de Melo wrote:
> Em Tue, Nov 30, 2021 at 04:42:55PM +0000, Douglas Raillard escreveu:
>> On 11/26/21 6:27 PM, Arnaldo Carvalho de Melo wrote:
>>> Em Tue, Oct 19, 2021 at 11:07:23AM +0100, Douglas RAILLARD escreveu:
>>>> From: Douglas Raillard <douglas.raillard@arm.com>
>>>>
>>>> Allow making inner struct enums and union anonymous.
>>>
>>> So I had to apply it by hand due to changes in the areas it touches, see
>>> below, and I expanded a bit the commit message, please review:
>>
>> Thanks for the review, it seems to be working as well as before. That said I found a couple of issues:
>>
>> 1. CLI options seem to abbreviate "anonymous" into "anon" so maybe we should do the same for this one ?
> 
> Ok, I can shorten the option name here.
> 
>> 2. I just found a case that generates a broken header:
>>
>> 	struct /* hrtimer */ {
>> 		struct /* timerqueue_node */ {
>> 			struct /* rb_node */ {
>> 				long unsigned int __rb_parent_color;                     /*  3328     8 */
>> 				struct rb_node * rb_right;                               /*  3336     8 */
>> 				struct rb_node * rb_left;                                /*  3344     8 */
>> 			}node; /*  3328    24 */
>> 			/* typedef ktime_t -> s64 -> __s64 */ long long int expires;     /*  3352     8 */
>> 		}node; /*  3328    32 */
>> 		/* typedef ktime_t -> s64 -> __s64 */ long long int      _softexpires;   /*  3360     8 */
>> 		enum hrtimer_restart (*function)(struct hrtimer *);                      /*  3368     8 */
>> 		struct hrtimer_clock_base * base;                                        /*  3376     8 */
>> 		/* typedef u8 -> __u8 */ unsigned char      state;                       /*  3384     1 */
>> 		/* typedef u8 -> __u8 */ unsigned char      is_rel;                      /*  3385     1 */
>> 		/* typedef u8 -> __u8 */ unsigned char      is_soft;                     /*  3386     1 */
>> 		/* typedef u8 -> __u8 */ unsigned char      is_hard;                     /*  3387     1 */
>> 	}hrtick_timer; /*  3328    64 */
>>
>> Here we can see that "struct rb_node" is a recursive type, so since the type
>> definition is now anonymous it will not compile. Detecting recursive types and
>> printing the name would avoid that but defeats the original purpose
>> of --inner_anonymous.
>>
>> I can see two solutions:
>>
>> 1. Detecting recursive types and appending a user-defined prefix to create a
>>     unique name.
>> 2. Detecting recursive types and replacing the recursive references by "void *".
>   
>> Solution #2 is the least invasive but will require a bit more work for the
>> end-user of the header:
>>
>>   struct hrtimer foo = *ptr;
>>   typeof(foo.node.node) node = foo.node.node;
>   
>>   // Extra cast using typeof in order to change the type of tb_right from void*
>>   // to "struct rb_node*"
>>   ((typeof(node))node.rb_right)->rb_left
> 
> A third solution and probably the easiest to implement. If you don't
> punch a hole on it:
> 
> Track if the struct is being printed the first time and then flip a bit
> not to print it again?
> 
> $ cat inner_anon.c
> struct foo {
> 	struct baz {
> 		struct rb_node {
> 			struct rb_node *prev, *next;
> 		} yy;
> 	} y;
> 	struct /* rb_node */ {
> 		struct rb_node *prev, *next;
> 	} x;
> } i;
> $ cc  -c  inner_anon.c   -o inner_anon.o
> $ vim inner_anon.c
> $ cat inner_anon.c
> struct foo {
> 	struct rb_node {
> 		struct rb_node *prev, *next;
> 	} x;
> 	struct baz {
> 		struct /* rb_node */ {
> 			struct rb_node *prev, *next;
> 		} yy;
> 	} y;
> } i;
> $ cc  -c  inner_anon.c   -o inner_anon.o
> $


"struct baz" and "struct foo" could very well be private and needing pahole to be exposed,
but "struct rb_node" could be already exposed in a public header necessary for e.g. macros,
leading to a name clash. It's quite frustrating that the language leaves you with nothing
to deal with that sort of issues, C++ at least gives the options of namespaces and does
proper scoping for nested type definitions, but it's unfortunately not an option for kernel modules ...

Another alternative is to just manually "namespace" all the definitions with a user-given prefix
(not just for the recursive types).

Re: [PATCH v1 1/2] fprintf: Allow making struct/enum/union anonymous

[Arnaldo Carvalho de Melo]

Em Wed, Dec 01, 2021 at 10:56:49AM +0000, Douglas Raillard escreveu:
> On 11/30/21 6:49 PM, Arnaldo Carvalho de Melo wrote:
> > Em Tue, Nov 30, 2021 at 04:42:55PM +0000, Douglas Raillard escreveu:
> > > On 11/26/21 6:27 PM, Arnaldo Carvalho de Melo wrote:
> > > > Em Tue, Oct 19, 2021 at 11:07:23AM +0100, Douglas RAILLARD escreveu:
> > > 2. I just found a case that generates a broken header:

> > > 	struct /* hrtimer */ {
> > > 		struct /* timerqueue_node */ {
> > > 			struct /* rb_node */ {
> > > 				long unsigned int __rb_parent_color;                     /*  3328     8 */
> > > 				struct rb_node * rb_right;                               /*  3336     8 */
> > > 				struct rb_node * rb_left;                                /*  3344     8 */
> > > 			}node; /*  3328    24 */
> > > 			/* typedef ktime_t -> s64 -> __s64 */ long long int expires;     /*  3352     8 */
> > > 		}node; /*  3328    32 */
> > > 		/* typedef ktime_t -> s64 -> __s64 */ long long int      _softexpires;   /*  3360     8 */
> > > 		enum hrtimer_restart (*function)(struct hrtimer *);                      /*  3368     8 */
> > > 		struct hrtimer_clock_base * base;                                        /*  3376     8 */
> > > 		/* typedef u8 -> __u8 */ unsigned char      state;                       /*  3384     1 */
> > > 		/* typedef u8 -> __u8 */ unsigned char      is_rel;                      /*  3385     1 */
> > > 		/* typedef u8 -> __u8 */ unsigned char      is_soft;                     /*  3386     1 */
> > > 		/* typedef u8 -> __u8 */ unsigned char      is_hard;                     /*  3387     1 */
> > > 	}hrtick_timer; /*  3328    64 */

> > > Here we can see that "struct rb_node" is a recursive type, so since the type
> > > definition is now anonymous it will not compile. Detecting recursive types and
> > > printing the name would avoid that but defeats the original purpose
> > > of --inner_anonymous.

> > > I can see two solutions:

> > > 1. Detecting recursive types and appending a user-defined prefix to create a
> > >     unique name.
> > > 2. Detecting recursive types and replacing the recursive references by "void *".
> > > Solution #2 is the least invasive but will require a bit more work for the
> > > end-user of the header:
> > > 
> > >   struct hrtimer foo = *ptr;
> > >   typeof(foo.node.node) node = foo.node.node;
> > >   // Extra cast using typeof in order to change the type of tb_right from void*
> > >   // to "struct rb_node*"
> > >   ((typeof(node))node.rb_right)->rb_left

> > A third solution and probably the easiest to implement. If you don't
> > punch a hole on it:

> > Track if the struct is being printed the first time and then flip a bit
> > not to print it again?

> > $ cat inner_anon.c
> > struct foo {
> > 	struct baz {
> > 		struct rb_node {
> > 			struct rb_node *prev, *next;
> > 		} yy;
> > 	} y;
> > 	struct /* rb_node */ {
> > 		struct rb_node *prev, *next;
> > 	} x;
> > } i;
> > $ cc  -c  inner_anon.c   -o inner_anon.o
> > $ vim inner_anon.c
> > $ cat inner_anon.c
> > struct foo {
> > 	struct rb_node {
> > 		struct rb_node *prev, *next;
> > 	} x;
> > 	struct baz {
> > 		struct /* rb_node */ {
> > 			struct rb_node *prev, *next;
> > 		} yy;
> > 	} y;
> > } i;
> > $ cc  -c  inner_anon.c   -o inner_anon.o
> > $
 
> "struct baz" and "struct foo" could very well be private and needing pahole to be exposed,
> but "struct rb_node" could be already exposed in a public header necessary for e.g. macros,
> leading to a name clash. It's quite frustrating that the language leaves you with nothing
> to deal with that sort of issues, C++ at least gives the options of namespaces and does
> proper scoping for nested type definitions, but it's unfortunately not an option for kernel modules ...

> Another alternative is to just manually "namespace" all the definitions with a user-given prefix
> (not just for the recursive types).

Yeah, that namespacing can be an optional argument to --inner_anon, then
having what I suggested + the namespace prefix we can get closer to what
we want?

- Arnaldo

Re: [PATCH v1 1/2] fprintf: Allow making struct/enum/union anonymous

[Douglas Raillard]

On 12/1/21 12:01 PM, Arnaldo Carvalho de Melo wrote:
> Em Wed, Dec 01, 2021 at 10:56:49AM +0000, Douglas Raillard escreveu:
>> On 11/30/21 6:49 PM, Arnaldo Carvalho de Melo wrote:
>>> Em Tue, Nov 30, 2021 at 04:42:55PM +0000, Douglas Raillard escreveu:
>>>> On 11/26/21 6:27 PM, Arnaldo Carvalho de Melo wrote:
>>>>> Em Tue, Oct 19, 2021 at 11:07:23AM +0100, Douglas RAILLARD escreveu:
>>>> 2. I just found a case that generates a broken header:
> 
>>>> 	struct /* hrtimer */ {
>>>> 		struct /* timerqueue_node */ {
>>>> 			struct /* rb_node */ {
>>>> 				long unsigned int __rb_parent_color;                     /*  3328     8 */
>>>> 				struct rb_node * rb_right;                               /*  3336     8 */
>>>> 				struct rb_node * rb_left;                                /*  3344     8 */
>>>> 			}node; /*  3328    24 */
>>>> 			/* typedef ktime_t -> s64 -> __s64 */ long long int expires;     /*  3352     8 */
>>>> 		}node; /*  3328    32 */
>>>> 		/* typedef ktime_t -> s64 -> __s64 */ long long int      _softexpires;   /*  3360     8 */
>>>> 		enum hrtimer_restart (*function)(struct hrtimer *);                      /*  3368     8 */
>>>> 		struct hrtimer_clock_base * base;                                        /*  3376     8 */
>>>> 		/* typedef u8 -> __u8 */ unsigned char      state;                       /*  3384     1 */
>>>> 		/* typedef u8 -> __u8 */ unsigned char      is_rel;                      /*  3385     1 */
>>>> 		/* typedef u8 -> __u8 */ unsigned char      is_soft;                     /*  3386     1 */
>>>> 		/* typedef u8 -> __u8 */ unsigned char      is_hard;                     /*  3387     1 */
>>>> 	}hrtick_timer; /*  3328    64 */
> 
>>>> Here we can see that "struct rb_node" is a recursive type, so since the type
>>>> definition is now anonymous it will not compile. Detecting recursive types and
>>>> printing the name would avoid that but defeats the original purpose
>>>> of --inner_anonymous.
> 
>>>> I can see two solutions:
> 
>>>> 1. Detecting recursive types and appending a user-defined prefix to create a
>>>>      unique name.
>>>> 2. Detecting recursive types and replacing the recursive references by "void *".
>>>> Solution #2 is the least invasive but will require a bit more work for the
>>>> end-user of the header:
>>>>
>>>>    struct hrtimer foo = *ptr;
>>>>    typeof(foo.node.node) node = foo.node.node;
>>>>    // Extra cast using typeof in order to change the type of tb_right from void*
>>>>    // to "struct rb_node*"
>>>>    ((typeof(node))node.rb_right)->rb_left
> 
>>> A third solution and probably the easiest to implement. If you don't
>>> punch a hole on it:
> 
>>> Track if the struct is being printed the first time and then flip a bit
>>> not to print it again?
> 
>>> $ cat inner_anon.c
>>> struct foo {
>>> 	struct baz {
>>> 		struct rb_node {
>>> 			struct rb_node *prev, *next;
>>> 		} yy;
>>> 	} y;
>>> 	struct /* rb_node */ {
>>> 		struct rb_node *prev, *next;
>>> 	} x;
>>> } i;
>>> $ cc  -c  inner_anon.c   -o inner_anon.o
>>> $ vim inner_anon.c
>>> $ cat inner_anon.c
>>> struct foo {
>>> 	struct rb_node {
>>> 		struct rb_node *prev, *next;
>>> 	} x;
>>> 	struct baz {
>>> 		struct /* rb_node */ {
>>> 			struct rb_node *prev, *next;
>>> 		} yy;
>>> 	} y;
>>> } i;
>>> $ cc  -c  inner_anon.c   -o inner_anon.o
>>> $
>   
>> "struct baz" and "struct foo" could very well be private and needing pahole to be exposed,
>> but "struct rb_node" could be already exposed in a public header necessary for e.g. macros,
>> leading to a name clash. It's quite frustrating that the language leaves you with nothing
>> to deal with that sort of issues, C++ at least gives the options of namespaces and does
>> proper scoping for nested type definitions, but it's unfortunately not an option for kernel modules ...
> 
>> Another alternative is to just manually "namespace" all the definitions with a user-given prefix
>> (not just for the recursive types).
> 
> Yeah, that namespacing can be an optional argument to --inner_anon, then
> having what I suggested + the namespace prefix we can get closer to what
> we want?

Yes I'll try to make a new version based on that:
* print only the first time it's encountered
* add option to prefix types for namespacing
* add an option to add typedef for original name for the types that were
   specified explicitly (or maybe simply do not apply the namespace on them)

This should fix the recursive type issue and allow using explicitly given types directly.

Douglas
> 
> - Arnaldo
> 

Re: [PATCH v1 1/2] fprintf: Allow making struct/enum/union anonymous

[Arnaldo Carvalho de Melo]

Em Mon, Dec 06, 2021 at 10:28:10AM +0000, Douglas Raillard escreveu:
> On 12/1/21 12:01 PM, Arnaldo Carvalho de Melo wrote:
> > Em Wed, Dec 01, 2021 at 10:56:49AM +0000, Douglas Raillard escreveu:
> > > On 11/30/21 6:49 PM, Arnaldo Carvalho de Melo wrote:
> > > > Em Tue, Nov 30, 2021 at 04:42:55PM +0000, Douglas Raillard escreveu:
> > > > > On 11/26/21 6:27 PM, Arnaldo Carvalho de Melo wrote:
> > > > > > Em Tue, Oct 19, 2021 at 11:07:23AM +0100, Douglas RAILLARD escreveu:
> > > > > 2. I just found a case that generates a broken header:
> > 
> > > > > 	struct /* hrtimer */ {
> > > > > 		struct /* timerqueue_node */ {
> > > > > 			struct /* rb_node */ {
> > > > > 				long unsigned int __rb_parent_color;                     /*  3328     8 */
> > > > > 				struct rb_node * rb_right;                               /*  3336     8 */
> > > > > 				struct rb_node * rb_left;                                /*  3344     8 */
> > > > > 			}node; /*  3328    24 */
> > > > > 			/* typedef ktime_t -> s64 -> __s64 */ long long int expires;     /*  3352     8 */
> > > > > 		}node; /*  3328    32 */
> > > > > 		/* typedef ktime_t -> s64 -> __s64 */ long long int      _softexpires;   /*  3360     8 */
> > > > > 		enum hrtimer_restart (*function)(struct hrtimer *);                      /*  3368     8 */
> > > > > 		struct hrtimer_clock_base * base;                                        /*  3376     8 */
> > > > > 		/* typedef u8 -> __u8 */ unsigned char      state;                       /*  3384     1 */
> > > > > 		/* typedef u8 -> __u8 */ unsigned char      is_rel;                      /*  3385     1 */
> > > > > 		/* typedef u8 -> __u8 */ unsigned char      is_soft;                     /*  3386     1 */
> > > > > 		/* typedef u8 -> __u8 */ unsigned char      is_hard;                     /*  3387     1 */
> > > > > 	}hrtick_timer; /*  3328    64 */
> > 
> > > > > Here we can see that "struct rb_node" is a recursive type, so since the type
> > > > > definition is now anonymous it will not compile. Detecting recursive types and
> > > > > printing the name would avoid that but defeats the original purpose
> > > > > of --inner_anonymous.
> > 
> > > > > I can see two solutions:
> > 
> > > > > 1. Detecting recursive types and appending a user-defined prefix to create a
> > > > >      unique name.
> > > > > 2. Detecting recursive types and replacing the recursive references by "void *".
> > > > > Solution #2 is the least invasive but will require a bit more work for the
> > > > > end-user of the header:
> > > > > 
> > > > >    struct hrtimer foo = *ptr;
> > > > >    typeof(foo.node.node) node = foo.node.node;
> > > > >    // Extra cast using typeof in order to change the type of tb_right from void*
> > > > >    // to "struct rb_node*"
> > > > >    ((typeof(node))node.rb_right)->rb_left
> > 
> > > > A third solution and probably the easiest to implement. If you don't
> > > > punch a hole on it:
> > 
> > > > Track if the struct is being printed the first time and then flip a bit
> > > > not to print it again?
> > 
> > > > $ cat inner_anon.c
> > > > struct foo {
> > > > 	struct baz {
> > > > 		struct rb_node {
> > > > 			struct rb_node *prev, *next;
> > > > 		} yy;
> > > > 	} y;
> > > > 	struct /* rb_node */ {
> > > > 		struct rb_node *prev, *next;
> > > > 	} x;
> > > > } i;
> > > > $ cc  -c  inner_anon.c   -o inner_anon.o
> > > > $ vim inner_anon.c
> > > > $ cat inner_anon.c
> > > > struct foo {
> > > > 	struct rb_node {
> > > > 		struct rb_node *prev, *next;
> > > > 	} x;
> > > > 	struct baz {
> > > > 		struct /* rb_node */ {
> > > > 			struct rb_node *prev, *next;
> > > > 		} yy;
> > > > 	} y;
> > > > } i;
> > > > $ cc  -c  inner_anon.c   -o inner_anon.o
> > > > $
> > > "struct baz" and "struct foo" could very well be private and needing pahole to be exposed,
> > > but "struct rb_node" could be already exposed in a public header necessary for e.g. macros,
> > > leading to a name clash. It's quite frustrating that the language leaves you with nothing
> > > to deal with that sort of issues, C++ at least gives the options of namespaces and does
> > > proper scoping for nested type definitions, but it's unfortunately not an option for kernel modules ...
> > 
> > > Another alternative is to just manually "namespace" all the definitions with a user-given prefix
> > > (not just for the recursive types).
> > 
> > Yeah, that namespacing can be an optional argument to --inner_anon, then
> > having what I suggested + the namespace prefix we can get closer to what
> > we want?
> 
> Yes I'll try to make a new version based on that:
> * print only the first time it's encountered
> * add option to prefix types for namespacing
> * add an option to add typedef for original name for the types that were
>   specified explicitly (or maybe simply do not apply the namespace on them)
> 
> This should fix the recursive type issue and allow using explicitly given types directly.

Great, looking forward to the patch,

Cheers,

- Arnaldo