| Message ID | 20170619211454.45244-1-luc.vanoostenryck@gmail.com (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Headers | show |
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, > -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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,
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(-)