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

[Arnaldo Carvalho de Melo <acme@kernel.org>]

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
$