[PATCH 0/3] reduce noise from unknown attributes

[PATCH 0/3] reduce noise from unknown attributes

[Luc Van Oostenryck]

Currently sparse throws error each time at each unknown attributes.

As GCC often creates new attributes, generaly for specific
usages irrelevant to what sparse is used for. The errors from
these not-yet-known attributes create needless noise and annoyance
for no benefits.

This series aims at reduce this noise by doing 3 things:
* change the error to a warning
* add a new flag to disable or enable warnings from those attributes
* by default set this flag as disabled

The first patch is independent of the others two.

Luc Van Oostenryck (3):

  Warn on unknown attributes instead of throwing errors
  Add a new warning flag: '-Wunknown-attribute'
  By default disable the new warning flag '-Wunknown-attribute'

 lib.c                               |  2 ++
 lib.h                               |  1 +
 parse.c                             |  3 ++-
 validation/Wunknown-attribute-def.c |  8 ++++++++
 validation/Wunknown-attribute-no.c  |  9 +++++++++
 validation/Wunknown-attribute-yes.c | 10 ++++++++++
 6 files changed, 32 insertions(+), 1 deletion(-)
 create mode 100644 validation/Wunknown-attribute-def.c
 create mode 100644 validation/Wunknown-attribute-no.c
 create mode 100644 validation/Wunknown-attribute-yes.c

-- 
2.10.1

[PATCH 1/3] Warn on unknown attributes instead of throwing errors

[Luc Van Oostenryck]

GCC creates new attributes quite often, generaly for specific
usages irrelevant to what sparse is used for.
Throwing errors on these create needless noise and annoyance
which is better to avoid.


Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 parse.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/parse.c b/parse.c
index 205e1264..212fae3a 100644
--- a/parse.c
+++ b/parse.c
@@ -1230,7 +1230,8 @@ static struct token *recover_unknown_attribute(struct token *token)
 {
 	struct expression *expr = NULL;
 
-	sparse_error(token->pos, "attribute '%s': unknown attribute", show_ident(token->ident));
+	if (Wunknown_attribute)
+		warning(token->pos, "attribute '%s': unknown attribute", show_ident(token->ident));
 	token = token->next;
 	if (match_op(token, '('))
 		token = parens_expression(token, &expr, "in attribute");
-- 
2.10.1

Re: [PATCH 1/3] Warn on unknown attributes instead of throwing errors

[Ramsay Jones]

On 02/11/16 21:59, Luc Van Oostenryck wrote:
> GCC creates new attributes quite often, generaly for specific
> usages irrelevant to what sparse is used for.
> Throwing errors on these create needless noise and annoyance
> which is better to avoid.
> 
> 
> Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
> ---
>  parse.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/parse.c b/parse.c
> index 205e1264..212fae3a 100644
> --- a/parse.c
> +++ b/parse.c
> @@ -1230,7 +1230,8 @@ static struct token *recover_unknown_attribute(struct token *token)
>  {
>  	struct expression *expr = NULL;
>  
> -	sparse_error(token->pos, "attribute '%s': unknown attribute", show_ident(token->ident));
> +	if (Wunknown_attribute)

Since Wunknown_attribute is not declared yet, this won't even compile.
Simply squash this into patch #2.

Apart from that, looks good.

Thanks!

ATB,
Ramsay Jones

Re: [PATCH 1/3] Warn on unknown attributes instead of throwing errors

[Luc Van Oostenryck]

On Wed, Nov 02, 2016 at 10:29:41PM +0000, Ramsay Jones wrote:
> 
> 
> On 02/11/16 21:59, Luc Van Oostenryck wrote:
> > GCC creates new attributes quite often, generaly for specific
> > usages irrelevant to what sparse is used for.
> > Throwing errors on these create needless noise and annoyance
> > which is better to avoid.
> > 
> > 
> > Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
> > ---
> >  parse.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/parse.c b/parse.c
> > index 205e1264..212fae3a 100644
> > --- a/parse.c
> > +++ b/parse.c
> > @@ -1230,7 +1230,8 @@ static struct token *recover_unknown_attribute(struct token *token)
> >  {
> >  	struct expression *expr = NULL;
> >  
> > -	sparse_error(token->pos, "attribute '%s': unknown attribute", show_ident(token->ident));
> > +	if (Wunknown_attribute)
> 
> Since Wunknown_attribute is not declared yet, this won't even compile.
> Simply squash this into patch #2.
> 
> Apart from that, looks good.
> 
> Thanks!
> 
> ATB,
> Ramsay Jones
> --

Grrr yes.
A leftover of the patch splitting. Sorry for that.

Thanks for noticing that.

Luc

[PATCH 2/3] Add a new warning flag: '-Wunknown-attribute'

[Luc Van Oostenryck]

So, we can choose if we're interested by those warnings or not.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 lib.c                               |  2 ++
 lib.h                               |  1 +
 validation/Wunknown-attribute-def.c |  9 +++++++++
 validation/Wunknown-attribute-no.c  |  9 +++++++++
 validation/Wunknown-attribute-yes.c | 10 ++++++++++
 5 files changed, 31 insertions(+)
 create mode 100644 validation/Wunknown-attribute-def.c
 create mode 100644 validation/Wunknown-attribute-no.c
 create mode 100644 validation/Wunknown-attribute-yes.c

diff --git a/lib.c b/lib.c
index d5b56b01..138736e7 100644
--- a/lib.c
+++ b/lib.c
@@ -240,6 +240,7 @@ int Wtransparent_union = 0;
 int Wtypesign = 0;
 int Wundef = 0;
 int Wuninitialized = 1;
+int Wunknown_attribute = 1;
 int Wvla = 1;
 
 int dbg_entry = 0;
@@ -463,6 +464,7 @@ static const struct warning {
 	{ "typesign", &Wtypesign },
 	{ "undef", &Wundef },
 	{ "uninitialized", &Wuninitialized },
+	{ "unknown-attribute", &Wunknown_attribute },
 	{ "vla", &Wvla },
 };
 
diff --git a/lib.h b/lib.h
index 15b69fa2..b778bdcd 100644
--- a/lib.h
+++ b/lib.h
@@ -126,6 +126,7 @@ extern int Wtransparent_union;
 extern int Wtypesign;
 extern int Wundef;
 extern int Wuninitialized;
+extern int Wunknown_attribute;
 extern int Wvla;
 
 extern int dbg_entry;
diff --git a/validation/Wunknown-attribute-def.c b/validation/Wunknown-attribute-def.c
new file mode 100644
index 00000000..0c0868d6
--- /dev/null
+++ b/validation/Wunknown-attribute-def.c
@@ -0,0 +1,9 @@
+static int foo(void) __attribute__((unknown_attribute));
+
+/*
+ * check-name: warn-unknown-attribute
+ *
+ * check-error-start
+Wunknown-attribute-def.c:1:37: warning: attribute 'unknown_attribute': unknown attribute
+ * check-error-end
+ */
diff --git a/validation/Wunknown-attribute-no.c b/validation/Wunknown-attribute-no.c
new file mode 100644
index 00000000..87951699
--- /dev/null
+++ b/validation/Wunknown-attribute-no.c
@@ -0,0 +1,9 @@
+static int foo(void) __attribute__((unknown_attribute));
+
+/*
+ * check-name: warn-unknown-attribute-no
+ * check-command: sparse -Wno-unknown-attribute $file
+ *
+ * check-error-start
+ * check-error-end
+ */
diff --git a/validation/Wunknown-attribute-yes.c b/validation/Wunknown-attribute-yes.c
new file mode 100644
index 00000000..72538cf5
--- /dev/null
+++ b/validation/Wunknown-attribute-yes.c
@@ -0,0 +1,10 @@
+static int foo(void) __attribute__((unknown_attribute));
+
+/*
+ * check-name: warn-unknown-attribute-yes
+ * check-command: sparse -Wunknown-attribute $file
+ *
+ * check-error-start
+Wunknown-attribute-yes.c:1:37: warning: attribute 'unknown_attribute': unknown attribute
+ * check-error-end
+ */
-- 
2.10.1

[PATCH 3/3] By default disable '-Wunknown-attribute'

[Luc Van Oostenryck]

Generally, we won't be interested by the warnings from this flag,
but we can always explicitly ask for them if needed.


Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 lib.c                               | 2 +-
 validation/Wunknown-attribute-def.c | 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/lib.c b/lib.c
index 138736e7..b4b09a78 100644
--- a/lib.c
+++ b/lib.c
@@ -240,7 +240,7 @@ int Wtransparent_union = 0;
 int Wtypesign = 0;
 int Wundef = 0;
 int Wuninitialized = 1;
-int Wunknown_attribute = 1;
+int Wunknown_attribute = 0;
 int Wvla = 1;
 
 int dbg_entry = 0;
diff --git a/validation/Wunknown-attribute-def.c b/validation/Wunknown-attribute-def.c
index 0c0868d6..defe643d 100644
--- a/validation/Wunknown-attribute-def.c
+++ b/validation/Wunknown-attribute-def.c
@@ -4,6 +4,5 @@ static int foo(void) __attribute__((unknown_attribute));
  * check-name: warn-unknown-attribute
  *
  * check-error-start
-Wunknown-attribute-def.c:1:37: warning: attribute 'unknown_attribute': unknown attribute
  * check-error-end
  */
-- 
2.10.1

Re: [PATCH 3/3] By default disable '-Wunknown-attribute'

[Christopher Li]

On Thu, Nov 3, 2016 at 5:59 AM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
> Generally, we won't be interested by the warnings from this flag,
> but we can always explicitly ask for them if needed.

I don't want to disable it by default. I think default should be on so
that we can collect test case and prepare to add them later one.

I would much rather see a patch to make those attribute parsed
eventually. I think gcc or clang should have a collection of all
the attribute stash some where. I haven't take a closer look myself.

Most of the attribute are trivial to parse correctly. We should have
some thing like empty_int_attribute instead of blindly ignore them.

Also ideally the attribute should be a list instead of member in the
ctype structure. Most of the symbol has at most one attribute any
way. That is a much bigger change though.

Chris

Re: [PATCH 3/3] By default disable '-Wunknown-attribute'

[Luc Van Oostenryck]

On Fri, Nov 18, 2016 at 12:52:37AM +0800, Christopher Li wrote:
> On Thu, Nov 3, 2016 at 5:59 AM, Luc Van Oostenryck
> <luc.vanoostenryck@gmail.com> wrote:
> > Generally, we won't be interested by the warnings from this flag,
> > but we can always explicitly ask for them if needed.
> 
> I don't want to disable it by default. I think default should be on so
> that we can collect test case and prepare to add them later one.
> 
> I would much rather see a patch to make those attribute parsed
> eventually. I think gcc or clang should have a collection of all
> the attribute stash some where. I haven't take a closer look myself.
> 
> Most of the attribute are trivial to parse correctly. We should have
> some thing like empty_int_attribute instead of blindly ignore them.
> 
> Also ideally the attribute should be a list instead of member in the
> ctype structure. Most of the symbol has at most one attribute any
> way. That is a much bigger change though.


I'm not surprised that you don't want to have it disabled by default.
But leaving it on would, in my opinion, miss the point entirely.
Why would sparses *users* be annoyed with warnings that doesn't
concern them? What kind of useful information it gives to them that
sparse doesn't know about this or this new attribute?
Even worse, what the difference for sparse between having this option
disabled by default and just adding yet another attribute to the lists
of known ones (even if it is not in the format of a list)?

I have taken a look at such gcc list before I wrote this serie.
There is a whole bunch of them for exotic uses on exotic architectures
and a more limited number fo them for more common use. But
don't forget that it's also not only the question of the existing
attributes but the fact that each gcc version (and maybe clang) may
add a few new ones and soon or later someone will use them.

Don't take me wrong, if it would make a visible semantic difference
I would be the first to say that such or such attribute need to be
handled correctly. And I think that patches doing the parsing of
some more attributes should be welcomed.
But given what sparse curently do and is used, leaving this option
enabled by default is, in my opinion, useless and just annoyance for
its users.

Also, I don't exagerate too much by saying that during the last 2 years
or so there has been almost as much patches for these new attributes
than anything else. It's a bit ridiculous.


Luc