| Message ID | 1438216001-8862-4-git-send-email-tcamuso@redhat.com (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Headers | show |
On Wed, Jul 29, 2015 at 08:26:41PM -0400, Tony Camuso wrote: > Allows building sparse to avert reporting semantic problems, e.g. > using sparse as a tokenizer to create a graph of KABI symbols and > their dependencies. > > Signed-off-by: Tony Camuso <tcamuso@redhat.com> This doesn't seem like something that should be determined at compile time. Ideally, this should be determined at runtime. > lib.c | 21 +++++++++++++++++++++ > parse.c | 12 ++++++++++++ > 2 files changed, 33 insertions(+) > > diff --git a/lib.c b/lib.c > index 8dc5bcf..cb10bce 100644 > --- a/lib.c > +++ b/lib.c > @@ -48,6 +48,13 @@ > int verbose, optimize, optimize_size, preprocessing; > int die_if_error = 0; > > +#ifdef NOWARN > +#pragma message "Built with NOWARN" > +#endif > +#ifdef NOERR > +#pragma message "Built with NOERR" > +#endif > + > #ifndef __GNUC__ > # define __GNUC__ 2 > # define __GNUC_MINOR__ 95 > @@ -103,6 +110,9 @@ unsigned int hexval(unsigned int c) > > static void do_warn(const char *type, struct position pos, const char * fmt, va_list args) > { > +#ifdef NOWARN > + return; > +#else > static char buffer[512]; > const char *name; > > @@ -111,9 +121,12 @@ static void do_warn(const char *type, struct position pos, const char * fmt, va_ > > fprintf(stderr, "%s:%d:%d: %s%s\n", > name, pos.line, pos.pos, type, buffer); > +#endif > } > > +#ifndef NOWARN > static int max_warnings = 100; > +#endif > static int show_info = 1; > > void info(struct position pos, const char * fmt, ...) > @@ -129,6 +142,9 @@ void info(struct position pos, const char * fmt, ...) > > static void do_error(struct position pos, const char * fmt, va_list args) > { > +#ifdef NOERR > + return; > +#else > static int errors = 0; > die_if_error = 1; > show_info = 1; > @@ -145,10 +161,14 @@ static void do_error(struct position pos, const char * fmt, va_list args) > > do_warn("error: ", pos, fmt, args); > errors++; > +#endif > } > > void warning(struct position pos, const char * fmt, ...) > { > +#ifdef NOWARN > + return; > +#else > va_list args; > > if (Wsparse_error) { > @@ -171,6 +191,7 @@ void warning(struct position pos, const char * fmt, ...) > va_start(args, fmt); > do_warn("warning: ", pos, fmt, args); > va_end(args); > +#endif > } > > void sparse_error(struct position pos, const char * fmt, ...) > diff --git a/parse.c b/parse.c > index 02275d8..f773fe8 100644 > --- a/parse.c > +++ b/parse.c > @@ -44,6 +44,13 @@ > #include "expression.h" > #include "target.h" > > +#ifdef NOWARN > +#pragma message "Built with NOWARN" > +#endif > +#ifdef NOERR > +#pragma message "Built with NOERR" > +#endif > + > static struct symbol_list **function_symbol_list; > struct symbol_list *function_computed_target_list; > struct statement_list *function_computed_goto_list; > @@ -2746,8 +2753,13 @@ struct token *external_declaration(struct token *token, struct symbol_list **lis > > /* Just a type declaration? */ > if (!ident) { > + > +#if defined NOWARN || defined NOERR > + return token->next; > +#else > warning(token->pos, "missing identifier in declaration"); > return expect(token, ';', "at the end of type declaration"); > +#endif > } > > /* type define declaration? */ > -- > 2.4.3 > > -- > 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 -- 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 07/29/2015 10:55 PM, Josh Triplett wrote: > On Wed, Jul 29, 2015 at 08:26:41PM -0400, Tony Camuso wrote: >> Allows building sparse to avert reporting semantic problems, e.g. >> using sparse as a tokenizer to create a graph of KABI symbols and >> their dependencies. >> >> Signed-off-by: Tony Camuso <tcamuso@redhat.com> > > This doesn't seem like something that should be determined at compile > time. Ideally, this should be determined at runtime. I thought it would be less intrusive, since I don't know how useful this would be to others. If you prefer a switch, I will do that. -- 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 07/29/2015 10:55 PM, Josh Triplett wrote: > On Wed, Jul 29, 2015 at 08:26:41PM -0400, Tony Camuso wrote: >> Allows building sparse to avert reporting semantic problems, e.g. >> using sparse as a tokenizer to create a graph of KABI symbols and >> their dependencies. >> >> Signed-off-by: Tony Camuso <tcamuso@redhat.com> > > This doesn't seem like something that should be determined at compile > time. Ideally, this should be determined at runtime. > I will be sending the new patch as a reply to the original, though the subject line is different, because I'm using the input switches instead of build-time constants. -- 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 Thu, Jul 30, 2015 at 4:45 AM, Tony Camuso <tcamuso@redhat.com> wrote: > > I thought it would be less intrusive, since I don't know how useful > this would be to others. > > If you prefer a switch, I will do that. I agree that this should be run time behavior. + +#if defined NOWARN || defined NOERR + return token->next; +#else What is up with this change? It is not output warning or not. It affect the parsing as well. If sparse can't bail out properly, this should be a separate patch. Chris -- 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 07/31/2015 07:46 PM, Christopher Li wrote: > On Thu, Jul 30, 2015 at 4:45 AM, Tony Camuso <tcamuso@redhat.com> wrote: >> >> I thought it would be less intrusive, since I don't know how useful >> this would be to others. >> >> If you prefer a switch, I will do that. > > I agree that this should be run time behavior. > > + > +#if defined NOWARN || defined NOERR > + return token->next; > +#else > > What is up with this change? It is not output warning or not. > It affect the parsing as well. If sparse can't bail out properly, > this should be a separate patch. > > Chris > Hi, Chris. I've since submitted a runtime patch (3/3 V3) with a switch as a response to this patch, but it basically does the same thing here. Consider the case where the source contains something like this... struct foo { union { int number; int *pointer; }; }; There being no ident for the union within the struct, we get the warning, "missing identifier in declaration" etc. Code without the patch. /* Just a type declaration? */ if (!ident) { warning(token->pos, "missing identifier in declaration"); return expect(token, ';', "at the end of type declaration"); } Regards, Tony -- 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 Sat, Aug 01, 2015 at 07:09:06AM -0400, Tony Camuso wrote: > On 07/31/2015 07:46 PM, Christopher Li wrote: > >On Thu, Jul 30, 2015 at 4:45 AM, Tony Camuso <tcamuso@redhat.com> wrote: > >> > >>I thought it would be less intrusive, since I don't know how useful > >>this would be to others. > >> > >>If you prefer a switch, I will do that. > > > >I agree that this should be run time behavior. > > > >+ > >+#if defined NOWARN || defined NOERR > >+ return token->next; > >+#else > > > >What is up with this change? It is not output warning or not. > >It affect the parsing as well. If sparse can't bail out properly, > >this should be a separate patch. > > > >Chris > > > > Hi, Chris. > > I've since submitted a runtime patch (3/3 V3) with a switch as a > response to this patch, but it basically does the same thing here. > > Consider the case where the source contains something like this... > > struct foo { > union { > int number; > int *pointer; > }; > }; > > There being no ident for the union within the struct, we get the warning, > "missing identifier in declaration" etc. If so, that's actually a bug in Sparse; anonymous unions should be allowed without warning: $ cat test.c struct foo { union { int number; int *pointer; }; }; $ gcc -Wall -Wextra -c test.c -o /dev/null $ They have a well-defined semantic meaning, and they're standardized in C11, just not in C89 or C99. - Josh Triplett -- 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 Sat, Aug 1, 2015 at 4:09 AM, Tony Camuso <tcamuso@redhat.com> wrote: > > I've since submitted a runtime patch (3/3 V3) with a switch as a > response to this patch, but it basically does the same thing here. > > Consider the case where the source contains something like this... > > struct foo { > union { > int number; > int *pointer; > }; > }; > > There being no ident for the union within the struct, we get the warning, > "missing identifier in declaration" etc. > I can't reproduce the warning with the chrisl repo master branch. That being said, if there is indeed a bug in sparse, it should be a separate patch. It changes the parsing behavior. Thanks Chris -- 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 08/01/2015 02:45 PM, Christopher Li wrote: > On Sat, Aug 1, 2015 at 4:09 AM, Tony Camuso <tcamuso@redhat.com> wrote: >> >> I've since submitted a runtime patch (3/3 V3) with a switch as a >> response to this patch, but it basically does the same thing here. >> >> Consider the case where the source contains something like this... >> >> struct foo { >> union { >> int number; >> int *pointer; >> }; >> }; >> >> There being no ident for the union within the struct, we get the warning, >> "missing identifier in declaration" etc. >> > > I can't reproduce the warning with the chrisl repo master branch. > > That being said, if there is indeed a bug in sparse, it should be a > separate patch. It changes the parsing behavior. > > Thanks > > Chris > I'll isolate exactly where the report is coming from, but it is tripping over this in parse.c warning(token->pos, "missing identifier in declaration"); return expect(token, ';', "at the end of type declaration"); -- 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 08/01/2015 02:45 PM, Christopher Li wrote: > On Sat, Aug 1, 2015 at 4:09 AM, Tony Camuso <tcamuso@redhat.com> wrote: >> >> I've since submitted a runtime patch (3/3 V3) with a switch as a >> response to this patch, but it basically does the same thing here. >> >> Consider the case where the source contains something like this... >> >> struct foo { >> union { >> int number; >> int *pointer; >> }; >> }; >> >> There being no ident for the union within the struct, we get the warning, >> "missing identifier in declaration" etc. >> > > I can't reproduce the warning with the chrisl repo master branch. > > That being said, if there is indeed a bug in sparse, it should be a > separate patch. It changes the parsing behavior. > > Thanks > > Chris > This doubt this is a sparse bug. It is an most likely caused by some RHEL specific macros with encapsulated anonymous unions devised to protect the Kernel Application Binary Interface while backporting upstream changes from point release to point release. I can try to convince the KABI macro maintainers to make their macros sparse-compliant, but I'm hoping you agree that it might not be a bad idea to provide an option to disable the sparse error reporting in the meantime. If you're interested in pursuing this further, these are the errors I'm seeing, followed by the source that generates them. /work/linux/fs/bio.i:9137:130: error: Expected ) in function declarator /work/linux/fs/bio.i:9137:130: error: got ( builtin:0:0: error: expected ; at end of declaration builtin:0:0: error: expected ; at end of declaration builtin:0:0: error: expected ; at end of declaration /work/linux/fs/bio.i:9137:172: error: Expected ; at the end of type declaration /work/linux/fs/bio.i:9137:172: error: got } /work/linux/fs/bio.i:9137:209: error: Expected ; at the end of type declaration /work/linux/fs/bio.i:9137:209: error: got } /work/linux/fs/bio.i:9137:266: error: Expected ) in function declarator /work/linux/fs/bio.i:9137:266: error: got ( /work/linux/fs/bio.i:9137:308: error: Expected ; at the end of type declaration /work/linux/fs/bio.i:9137:308: error: got } /work/linux/fs/bio.i:9137:350: error: Expected ; at the end of type declaration /work/linux/fs/bio.i:9137:350: error: got } /work/linux/fs/bio.i:9137:382: error: Expected ; at the end of type declaration /work/linux/fs/bio.i:9137:382: error: got } /work/linux/fs/bio.i:9137:385: error: Expected ; at the end of type declaration /work/linux/fs/bio.i:9137:385: error: got } /work/linux/fs/bio.i:9139:1: error: Expected ; at the end of type declaration /work/linux/fs/bio.i:9139:1: error: got } The anonymous unions giving us those error messages are encapsulated in a RHEL-specific macro. # define _RH_KABI_REPLACE(_orig, _new) \ union { \ _new; \ struct { \ _orig; \ } __UNIQUE_ID(rh_kabi_hide); \ __RH_KABI_CHECK_SIZE_ALIGN(_orig, _new); \ } Here's an example of the macro invocation. RH_KABI_REPLACE(void *spin_mlock, /* Spinner MCS lock */ struct optimistic_spin_queue *osq) /* Spinner MCS lock */ The preprocessor expands it as follows. union { struct optimistic_spin_queue *osq; struct { void *spin_mlock; } __UNIQUE_ID_rh_kabi_hide0; union { _Static_assert(sizeof(struct{struct optimistic_spin_queue *osq;}) <= sizeof(struct{void * spin_mlock;}), "kabi sizeof test panic"); _Static_assert(__alignof__(struct{struct optimistic_spin_queue *osq;}) <= __alignof__(struct{void *spin_mlock;}), "kabi alignof test panic"); }; }; -- 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 08/01/2015 02:45 PM, Christopher Li wrote: > On Sat, Aug 1, 2015 at 4:09 AM, Tony Camuso <tcamuso@redhat.com> wrote: >> >> I've since submitted a runtime patch (3/3 V3) with a switch as a >> response to this patch, but it basically does the same thing here. >> >> Consider the case where the source contains something like this... >> >> struct foo { >> union { >> int number; >> int *pointer; >> }; >> }; >> >> There being no ident for the union within the struct, we get the warning, >> "missing identifier in declaration" etc. >> > > I can't reproduce the warning with the chrisl repo master branch. > > That being said, if there is indeed a bug in sparse, it should be a > separate patch. It changes the parsing behavior. > > Thanks > > Chris > Here's one that might be a sparse bug. /work/linux/fs/bio.i:5368:26: error: Expected ) at end of cast operator /work/linux/fs/bio.i:5368:26: error: got __int128 Here is the kernel source in include/linux/math64.h that causes the error. static inline __attribute__((no_instrument_function)) u64 mul_u64_u32_shr(u64 a, u32 mul, unsigned int shift) { return (u64)(((unsigned __int128)a * mul) >> shift); } -- 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
Hi Tony, Tony Camuso <tcamuso@redhat.com> writes: > Here's one that might be a sparse bug. > > /work/linux/fs/bio.i:5368:26: error: Expected ) at end of cast operator > /work/linux/fs/bio.i:5368:26: error: got __int128 > > Here is the kernel source in include/linux/math64.h that causes the error. > > static inline __attribute__((no_instrument_function)) u64 mul_u64_u32_shr(u64 a, u32 mul, unsigned int shift) > { > return (u64)(((unsigned __int128)a * mul) >> shift); > } This one is probably triggered by running sparse on gcc -E output. The mul_u64_u32_shr() implementation cited by you is protected by a #if ... && defined(__SIZEOF_INT128__) in include/linux/math64.h __SIZEOF_INT128__ is #define'd by gcc but not by sparse. The reason for your parsing error messages is probably that declaration_specifiers() does not eat up the __int128, leaving this one to cast_expression(). Could you please confirm that you did indeed run sparse on gcc -E output? Thank you, Nicolai -- 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 08/03/2015 07:23 AM, Nicolai Stange wrote: > Hi Tony, > > Tony Camuso <tcamuso@redhat.com> writes: >> Here's one that might be a sparse bug. >> >> /work/linux/fs/bio.i:5368:26: error: Expected ) at end of cast operator >> /work/linux/fs/bio.i:5368:26: error: got __int128 >> >> Here is the kernel source in include/linux/math64.h that causes the error. >> >> static inline __attribute__((no_instrument_function)) u64 mul_u64_u32_shr(u64 a, u32 mul, unsigned int shift) >> { >> return (u64)(((unsigned __int128)a * mul) >> shift); >> } > > This one is probably triggered by running sparse on gcc -E output. > > The mul_u64_u32_shr() implementation cited by you is protected by a > #if ... && defined(__SIZEOF_INT128__) > in include/linux/math64.h > > __SIZEOF_INT128__ is #define'd by gcc but not by sparse. > > > The reason for your parsing error messages is probably that > declaration_specifiers() does not eat up the __int128, leaving this one > to cast_expression(). > > Could you please confirm that you did indeed run sparse on gcc -E > output? > > Thank you, > > Nicolai Yes, we are using -E to generate preprocessor output. -- 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/lib.c b/lib.c index 8dc5bcf..cb10bce 100644 --- a/lib.c +++ b/lib.c @@ -48,6 +48,13 @@ int verbose, optimize, optimize_size, preprocessing; int die_if_error = 0; +#ifdef NOWARN +#pragma message "Built with NOWARN" +#endif +#ifdef NOERR +#pragma message "Built with NOERR" +#endif + #ifndef __GNUC__ # define __GNUC__ 2 # define __GNUC_MINOR__ 95 @@ -103,6 +110,9 @@ unsigned int hexval(unsigned int c) static void do_warn(const char *type, struct position pos, const char * fmt, va_list args) { +#ifdef NOWARN + return; +#else static char buffer[512]; const char *name; @@ -111,9 +121,12 @@ static void do_warn(const char *type, struct position pos, const char * fmt, va_ fprintf(stderr, "%s:%d:%d: %s%s\n", name, pos.line, pos.pos, type, buffer); +#endif } +#ifndef NOWARN static int max_warnings = 100; +#endif static int show_info = 1; void info(struct position pos, const char * fmt, ...) @@ -129,6 +142,9 @@ void info(struct position pos, const char * fmt, ...) static void do_error(struct position pos, const char * fmt, va_list args) { +#ifdef NOERR + return; +#else static int errors = 0; die_if_error = 1; show_info = 1; @@ -145,10 +161,14 @@ static void do_error(struct position pos, const char * fmt, va_list args) do_warn("error: ", pos, fmt, args); errors++; +#endif } void warning(struct position pos, const char * fmt, ...) { +#ifdef NOWARN + return; +#else va_list args; if (Wsparse_error) { @@ -171,6 +191,7 @@ void warning(struct position pos, const char * fmt, ...) va_start(args, fmt); do_warn("warning: ", pos, fmt, args); va_end(args); +#endif } void sparse_error(struct position pos, const char * fmt, ...) diff --git a/parse.c b/parse.c index 02275d8..f773fe8 100644 --- a/parse.c +++ b/parse.c @@ -44,6 +44,13 @@ #include "expression.h" #include "target.h" +#ifdef NOWARN +#pragma message "Built with NOWARN" +#endif +#ifdef NOERR +#pragma message "Built with NOERR" +#endif + static struct symbol_list **function_symbol_list; struct symbol_list *function_computed_target_list; struct statement_list *function_computed_goto_list; @@ -2746,8 +2753,13 @@ struct token *external_declaration(struct token *token, struct symbol_list **lis /* Just a type declaration? */ if (!ident) { + +#if defined NOWARN || defined NOERR + return token->next; +#else warning(token->pos, "missing identifier in declaration"); return expect(token, ';', "at the end of type declaration"); +#endif } /* type define declaration? */
Allows building sparse to avert reporting semantic problems, e.g. using sparse as a tokenizer to create a graph of KABI symbols and their dependencies. Signed-off-by: Tony Camuso <tcamuso@redhat.com> --- lib.c | 21 +++++++++++++++++++++ parse.c | 12 ++++++++++++ 2 files changed, 33 insertions(+)