[PATCH] dissect: use built_in_ident() instead of MK_IDENT()

[PATCH] dissect: use built_in_ident() instead of MK_IDENT()

[Luc Van Oostenryck]

The motivation for this patch was to allow sparse to be
compiled with clang which doesn't like what is done
on VLAs in the MK_IDENT() macro.

But also, I can't see any justification for not using the
real thing to create identifiers: built_in_ident().

CC: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 dissect.h      | 13 -------------
 test-dissect.c |  6 +++---
 2 files changed, 3 insertions(+), 16 deletions(-)

diff --git a/dissect.h b/dissect.h
index 3b72b8988..5ac1f4d40 100644
--- a/dissect.h
+++ b/dissect.h
@@ -25,16 +25,3 @@ struct reporter
 };
 
 extern void dissect(struct symbol_list *, struct reporter *);
-
-#define	MK_IDENT(s)	({				\
-	static struct {					\
-		struct ident ident;			\
-		char __[sizeof(s)];			\
-	} ident = {{					\
-		.len  = sizeof(s)-1,			\
-		.name = s,				\
-	}};						\
-	&ident.ident;					\
-})
-
-#endif
diff --git a/test-dissect.c b/test-dissect.c
index a2548b7f2..862318f81 100644
--- a/test-dissect.c
+++ b/test-dissect.c
@@ -47,7 +47,7 @@ static void r_symbol(unsigned mode, struct position *pos, struct symbol *sym)
 	print_usage(pos, sym, mode);
 
 	if (!sym->ident)
-		sym->ident = MK_IDENT("__asm__");
+		sym->ident = built_in_ident("__asm__");
 
 	printf("%-32.*s %s\n",
 		sym->ident->len, sym->ident->name,
@@ -60,10 +60,10 @@ static void r_member(unsigned mode, struct position *pos, struct symbol *sym, st
 
 	print_usage(pos, sym, mode);
 
-	ni = MK_IDENT("?");
+	ni = built_in_ident("?");
 	si = sym->ident ?: ni;
 	/* mem == NULL means entire struct accessed */
-	mi = mem ? (mem->ident ?: ni) : MK_IDENT("*");
+	mi = mem ? (mem->ident ?: ni) : built_in_ident("*");
 
 	printf("%.*s.%-*.*s %s\n",
 		si->len, si->name,
-- 
2.13.0

Re: [PATCH] dissect: use built_in_ident() instead of MK_IDENT()

[Ramsay Jones]

On 19/06/17 22:14, Luc Van Oostenryck wrote:
> The motivation for this patch was to allow sparse to be
> compiled with clang which doesn't like what is done
> on VLAs in the MK_IDENT() macro.
> 
> But also, I can't see any justification for not using the
> real thing to create identifiers: built_in_ident().
> 
> CC: Oleg Nesterov <oleg@redhat.com>
> Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
> ---
>  dissect.h      | 13 -------------
>  test-dissect.c |  6 +++---
>  2 files changed, 3 insertions(+), 16 deletions(-)
> 
> diff --git a/dissect.h b/dissect.h
> index 3b72b8988..5ac1f4d40 100644
> --- a/dissect.h
> +++ b/dissect.h
> @@ -25,16 +25,3 @@ struct reporter
>  };
>  
>  extern void dissect(struct symbol_list *, struct reporter *);
> -
> -#define	MK_IDENT(s)	({				\
> -	static struct {					\
> -		struct ident ident;			\
> -		char __[sizeof(s)];			\
> -	} ident = {{					\
> -		.len  = sizeof(s)-1,			\
> -		.name = s,				\
> -	}};						\
> -	&ident.ident;					\
> -})
> -
> -#endif

Hmm, does this compile?; you seem to have removed an #endif,
leaving an unbalanced '#ifndef DISSECT_H'.

ATB,
Ramsay Jones

> diff --git a/test-dissect.c b/test-dissect.c
> index a2548b7f2..862318f81 100644
> --- a/test-dissect.c
> +++ b/test-dissect.c
> @@ -47,7 +47,7 @@ static void r_symbol(unsigned mode, struct position *pos, struct symbol *sym)
>  	print_usage(pos, sym, mode);
>  
>  	if (!sym->ident)
> -		sym->ident = MK_IDENT("__asm__");
> +		sym->ident = built_in_ident("__asm__");
>  
>  	printf("%-32.*s %s\n",
>  		sym->ident->len, sym->ident->name,
> @@ -60,10 +60,10 @@ static void r_member(unsigned mode, struct position *pos, struct symbol *sym, st
>  
>  	print_usage(pos, sym, mode);
>  
> -	ni = MK_IDENT("?");
> +	ni = built_in_ident("?");
>  	si = sym->ident ?: ni;
>  	/* mem == NULL means entire struct accessed */
> -	mi = mem ? (mem->ident ?: ni) : MK_IDENT("*");
> +	mi = mem ? (mem->ident ?: ni) : built_in_ident("*");
>  
>  	printf("%.*s.%-*.*s %s\n",
>  		si->len, si->name,
> 

Re: [PATCH] dissect: use built_in_ident() instead of MK_IDENT()

[Luc Van Oostenryck]

On Mon, Jun 19, 2017 at 11:35:30PM +0100, Ramsay Jones wrote:
> On 19/06/17 22:14, Luc Van Oostenryck wrote:
> > The motivation for this patch was to allow sparse to be
> > compiled with clang which doesn't like what is done
> > on VLAs in the MK_IDENT() macro.
> > 
> > But also, I can't see any justification for not using the
> > real thing to create identifiers: built_in_ident().
> > 
> > CC: Oleg Nesterov <oleg@redhat.com>
> > Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
> > ---
> >  dissect.h      | 13 -------------
> >  test-dissect.c |  6 +++---
> >  2 files changed, 3 insertions(+), 16 deletions(-)
> > 
> > diff --git a/dissect.h b/dissect.h
> > index 3b72b8988..5ac1f4d40 100644
> > --- a/dissect.h
> > +++ b/dissect.h
> > @@ -25,16 +25,3 @@ struct reporter
> >  };
> >  
> >  extern void dissect(struct symbol_list *, struct reporter *);
> > -
> > -#define	MK_IDENT(s)	({				\
> > -	static struct {					\
> > -		struct ident ident;			\
> > -		char __[sizeof(s)];			\
> > -	} ident = {{					\
> > -		.len  = sizeof(s)-1,			\
> > -		.name = s,				\
> > -	}};						\
> > -	&ident.ident;					\
> > -})
> > -
> > -#endif
> 
> Hmm, does this compile?; you seem to have removed an #endif,
> leaving an unbalanced '#ifndef DISSECT_H'.

Yes, something (or more probably someone (me)) eat it.
It's already corrected.

But thanks again!

-- Luc 

Re: [PATCH] dissect: use built_in_ident() instead of MK_IDENT()

[Oleg Nesterov]

On 06/19, Luc Van Oostenryck wrote:
>
> The motivation for this patch was to allow sparse to be
> compiled with clang which doesn't like what is done
> on VLAs in the MK_IDENT() macro.

could you spell please? I don't understand why clang can't compile this
code and I am just curious.

> But also, I can't see any justification for not using the
> real thing to create identifiers: built_in_ident().

Simply because MK_IDENT() is faster and simpler to use.

> CC: Oleg Nesterov <oleg@redhat.com>
> Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
> ---
>  dissect.h      | 13 -------------
>  test-dissect.c |  6 +++---

iirc there should be another MK_IDENT user in dissect.c?

Oleg.

Re: [PATCH] dissect: use built_in_ident() instead of MK_IDENT()

[Luc Van Oostenryck]

On Tue, Jun 20, 2017 at 01:17:41PM +0200, Oleg Nesterov wrote:
> On 06/19, Luc Van Oostenryck wrote:
> >
> > The motivation for this patch was to allow sparse to be
> > compiled with clang which doesn't like what is done
> > on VLAs in the MK_IDENT() macro.
> 
> could you spell please? I don't understand why clang can't compile this
> code and I am just curious.

Sure, I should have added in the commit message. Here it is:
	test-dissect.c:50:16: warning: field 'ident' with variable sized type 'struct ident' not at the end of a struct or class is a GNU extension [-Wgnu-variable-sized-type-not-at-end]
	                sym->ident = MK_IDENT("__asm__");
	                             ^
	./dissect.h:31:16: note: expanded from macro 'MK_IDENT'
	                struct ident ident;                     \
	                             ^
	test-dissect.c:50:25: error: initialization of flexible array member is not allowed
	                sym->ident = MK_IDENT("__asm__");
	                                      ^~~~~~~~~
	./dissect.h:35:11: note: expanded from macro 'MK_IDENT'
	                .name = s,                              \
	                        ^
	./token.h:77:7: note: initialized flexible array member 'name' is here
	        char name[];            /* Actual identifier */
	             ^
	test-dissect.c:50:14: error: assigning to 'struct ident *' from incompatible type 'void'
	                sym->ident = MK_IDENT("__asm__");
	                           ^ ~~~~~~~~~~~~~~~~~~~

I quickly looked at it and I didn't seem to be something with MK_IDENT,
just that clang doesn't like initialization of flexible array member
(I said 'VLA' in the patch but there is no VLA here).
 
> > But also, I can't see any justification for not using the
> > real thing to create identifiers: built_in_ident().
> 
> Simply because MK_IDENT() is faster and simpler to use.

I find this very weak. For the simplicity of use, in both cases
there is just a single call with the same argument, for the speed,
I really really doubt there is any measurable difference.
But what annoys me is that the macro is doing another kind
of ident (other init code, not being hashed, different lifetime),
not that it matters here though but ...

> > CC: Oleg Nesterov <oleg@redhat.com>
> > Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
> > ---
> >  dissect.h      | 13 -------------
> >  test-dissect.c |  6 +++---
> 
> iirc there should be another MK_IDENT user in dissect.c?

No, it doesn't seem to, and the history tell that there it
never had been the case, just the three in test-dissect.c

-- Luc

Re: [PATCH] dissect: use built_in_ident() instead of MK_IDENT()

[Oleg Nesterov]

On 06/20, Luc Van Oostenryck wrote:
>
> On Tue, Jun 20, 2017 at 01:17:41PM +0200, Oleg Nesterov wrote:
> > On 06/19, Luc Van Oostenryck wrote:
> > >
> > > The motivation for this patch was to allow sparse to be
> > > compiled with clang which doesn't like what is done
> > > on VLAs in the MK_IDENT() macro.
> >
> > could you spell please? I don't understand why clang can't compile this
> > code and I am just curious.
>
> Sure, I should have added in the commit message. Here it is:
> 	test-dissect.c:50:16: warning: field 'ident' with variable sized type 'struct ident' not at the end of a struct or class is a GNU extension [-Wgnu-variable-sized-type-not-at-end]
> 	                sym->ident = MK_IDENT("__asm__");
> 	                             ^
> 	./dissect.h:31:16: note: expanded from macro 'MK_IDENT'
> 	                struct ident ident;                     \
> 	                             ^
> 	test-dissect.c:50:25: error: initialization of flexible array member is not allowed
> 	                sym->ident = MK_IDENT("__asm__");
> 	                                      ^~~~~~~~~
> 	./dissect.h:35:11: note: expanded from macro 'MK_IDENT'
> 	                .name = s,                              \
> 	                        ^

Hmm. Now that I git-cloned the code I understand. And I can't recall why MK_IDENT() was
written this way, it simply doesn't look right. Probably can be fixed but I agree, lets
remove it.

> > > But also, I can't see any justification for not using the
> > > real thing to create identifiers: built_in_ident().
> >
> > Simply because MK_IDENT() is faster and simpler to use.
>
> I find this very weak. For the simplicity of use, in both cases
> there is just a single call with the same argument, for the speed,
> I really really doubt there is any measurable difference.

Yes, I didn't try to convince you this optimization actually makes sense,
just explained the motivation I (probably) had many years ago.

> > >  dissect.h      | 13 -------------
> > >  test-dissect.c |  6 +++---
> > 
> > iirc there should be another MK_IDENT user in dissect.c?
> 
> No, it doesn't seem to, and the history tell that there it
> never had been the case, just the three in test-dissect.c

OK, my memory fooled me.

Thanks,

Oleg.

Re: [PATCH] dissect: use built_in_ident() instead of MK_IDENT()

[Luc Van Oostenryck]

On Tue, Jun 20, 2017 at 06:07:11PM +0200, Oleg Nesterov wrote:
> Hmm. Now that I git-cloned the code I understand. And I can't recall why MK_IDENT() was
> written this way, it simply doesn't look right.

It looks to me as you simply wanted to avoid doing dynamic allocation
and/or the cost of the hashing.

> Probably can be fixed but I agree, lets
> remove it.

Probably yes, but yes btter to remove it.


Thanks for your reply.

-- Luc

Re: [PATCH] dissect: use built_in_ident() instead of MK_IDENT()

[Oleg Nesterov]

On 06/20, Luc Van Oostenryck wrote:
>
> On Tue, Jun 20, 2017 at 06:07:11PM +0200, Oleg Nesterov wrote:
> > Hmm. Now that I git-cloned the code I understand. And I can't recall why MK_IDENT() was
> > written this way, it simply doesn't look right.
>
> It looks to me as you simply wanted to avoid doing dynamic allocation
> and/or the cost of the hashing.

This is clear ;) what is not clear is why I didn't do, say,

	#define	MK_IDENT(s)	({				\
		static struct {					\
			struct ident ident;			\
			char name[sizeof(s)];			\
		} ident = {					\
			.ident.len  = sizeof(s)-1,		\
			.name = s,				\
		};						\
		&ident.ident;					\
	})

probably because I wanted to avoid __attribute__((packed)) in struct ident,
not sure.

> > Probably can be fixed but I agree, lets
> > remove it.
>
> Probably yes, but yes btter to remove it.

Agreed.

Oleg.