[PATCH] Adding ignored attribute optimize

[PATCH] Adding ignored attribute optimize

[Christopher Li]

[-- Attachment #1: Type: text/plain, Size: 123 bytes --]

I notice this unknow attribute "__optimize__"when I am looking
sparse checking result on the recent kernel compile.

Chris

[-- Attachment #2: 0001-Adding-ignored-attribute-optimize.patch --]
[-- Type: text/x-patch, Size: 1612 bytes --]

From d8925f125ee8b1cc6eb29f43312b81f3f859374d Mon Sep 17 00:00:00 2001
From: Christopher Li <sparse@chrisli.org>
Date: Fri, 23 Jun 2017 17:13:17 -0700
Subject: [PATCH] Adding ignored attribute optimize

It was used in kvm/vmx.c

Signed-of-By: Christopher Li <sparse@chrisli.org>
---
 parse.c                    |  6 ++++--
 validation/attr-optimize.c | 16 ++++++++++++++++
 2 files changed, 20 insertions(+), 2 deletions(-)
 create mode 100644 validation/attr-optimize.c

diff --git a/parse.c b/parse.c
index 2145479..f665f05 100644
--- a/parse.c
+++ b/parse.c
@@ -607,6 +607,8 @@ static const char *ignored_attributes[] = {
 	"__naked__",
 	"no_instrument_function",
 	"__no_instrument_function__",
+	"no_sanitize_address",
+	"__no_sanitize_address__",
 	"noclone",
 	"__noclone",
 	"__noclone__",
@@ -618,6 +620,8 @@ static const char *ignored_attributes[] = {
 	"nothrow",
 	"__nothrow",
 	"__nothrow__",
+	"optimize",
+	"__optimize__",
 	"regparm",
 	"__regparm__",
 	"section",
@@ -646,8 +650,6 @@ static const char *ignored_attributes[] = {
 	"__warning__",
 	"weak",
 	"__weak__",
-	"no_sanitize_address",
-	"__no_sanitize_address__",
 };
 
 
diff --git a/validation/attr-optimize.c b/validation/attr-optimize.c
new file mode 100644
index 0000000..c45cbe8
--- /dev/null
+++ b/validation/attr-optimize.c
@@ -0,0 +1,16 @@
+
+#define __noclone	__attribute__((__noclone__, __optimize__("no-tracer")))
+
+struct kvm_vcpu;
+
+static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu)
+{
+	__asm__("");
+}
+
+extern void *run;
+void *run = vmx_vcpu_run;
+
+/*
+ * check-name: optimize attributes
+ */
-- 
2.9.4

Re: [PATCH] Adding ignored attribute optimize

[Luc Van Oostenryck]

On Tue, Jun 27, 2017 at 10:21:43AM -0700, Christopher Li wrote:
> I notice this unknow attribute "__optimize__"when I am looking
> sparse checking result on the recent kernel compile.

There is also a long time 'require_context' in drivers/block/drbd,/
a failed attempt to fix or extend sparse's handling of lock context.
Why is it still there after all those years, I have no idea.
 
Anyway, my position about these unknown attributes is still the same
of course: we should simply ignore them instead of annoying people
with useless warnings.

-- Luc

Re: [PATCH] Adding ignored attribute optimize

[Christopher Li]

On Thu, Jun 29, 2017 at 12:04 AM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
> There is also a long time 'require_context' in drivers/block/drbd,/
> a failed attempt to fix or extend sparse's handling of lock context.
> Why is it still there after all those years, I have no idea.

Yes, I notice that one too. But that one require to fix in the kernel side.
Sparse does not support that kind of syntax.

>
> Anyway, my position about these unknown attributes is still the same
> of course: we should simply ignore them instead of annoying people
> with useless warnings.

I have carefully consider your idea. It will have other unwanted behavior.
That is why I did not apply your patch to change the default as no warning.

Let say some one typo the sparse specific attribute "__contxt__" instead
of "__context__". Sparse will not give warning about those typo. The statement
become an no-op. It will be very hard for developer to find out what
is going on.
In this case, we do want to give out warning on unknown sparse
specific attributes.

Does it make sense?

Chris

Re: [PATCH] Adding ignored attribute optimize

[Luc Van Oostenryck]

On Thu, Jun 29, 2017 at 02:46:09PM -0700, Christopher Li wrote:
> On Thu, Jun 29, 2017 at 12:04 AM, Luc Van Oostenryck
> <luc.vanoostenryck@gmail.com> wrote:
> > There is also a long time 'require_context' in drivers/block/drbd,/
> > a failed attempt to fix or extend sparse's handling of lock context.
> > Why is it still there after all those years, I have no idea.
> 
> Yes, I notice that one too. But that one require to fix in the kernel side.
> Sparse does not support that kind of syntax.
> 
> >
> > Anyway, my position about these unknown attributes is still the same
> > of course: we should simply ignore them instead of annoying people
> > with useless warnings.
> 
> I have carefully consider your idea. It will have other unwanted behavior.
> That is why I did not apply your patch to change the default as no warning.
> 
> Let say some one typo the sparse specific attribute "__contxt__" instead
> of "__context__". Sparse will not give warning about those typo. The statement
> become an no-op. It will be very hard for developer to find out what
> is going on.
> In this case, we do want to give out warning on unknown sparse
> specific attributes.
> 
> Does it make sense?

Sure it make some sense.

But how often it happen and when it happen how much developers
are impacted and for how long?

Now compare that with the current situation with, for example,
this '__optimize__' or the '__alloc_align__' of last year, not
talking of the ones only used for less common architecture and
code other than Linux's kernel, where it happens all the time,
and when it happens it happens for *all* developpers who doesn't
even made any mistake, just wanting to check their code, and who
will receive this 'unknown attribute' warning, ad nauseum,
every time they want to check the code and this *until sparse
is updated to have this new attribute in its list* and worse,
for most people, *until their version of sparse in their distro
is updated*.

The last two points are higly critical in my eyes. It's one thing
to receive false-positive warnings (the attribute is only unknown
to sparse, it's a defect of sparse, not with the kernel or
the developer's code) just once or for a short while. And if sparse
could be updated within days and upgraded for everyone impacted,
then I would think exactly the same as you.  But as you know,
the situation is totally different.

Guess which version of sparse most distros are shipping?
sparse v0.5.0, of course, a three years old version.
Look at Debian, for example, they just released their new version
10 days ago, thus for the next two years or so, Debian users
(who don't use Sid or compile & install sparse themselves) will
still use sparse 0.5.0 with all its bad warnings.

In other words, this list of known attributes is totally
useless because it's never complete and always updated too
late for developers.

For me it's clear that we must here look after forward-compatibility.
And when we have, on one side, some rare legitimate warnings because
of a typo which will happen once for a single developer, and on the
other side, tons of false warnings which will confuse and annoy a
lot of devs for a very long time, I know which side must be choosen.

-- Luc

Re: [PATCH] Adding ignored attribute optimize

[Christopher Li]

On Sat, Jul 1, 2017 at 2:24 AM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
>
> Sure it make some sense.
>
> But how often it happen and when it happen how much developers
> are impacted and for how long?

It actually happen right now with the kernel using require_context
which is not supported by sparse. Sparse should report error
on the sparse related syntax which it does not support. Gcc is
no going to report error on those sparse related attributes because
gcc never see them.

Because we have noway to tell an unknown attribute is belong
to sparse or gcc. Sparse checker should report it. It is very bad
silence on those sparse specific error and developer have no way
to know it.

I am fine with other use of sparse like sparse-llvm turn
it off by default. But for sparse checker program itself. I think
report the error is the right thing to do.

As for the distro version of sparse is too old.
I think the right thing to do is just get into the habit of release
more often.

I am guilty of not cutting a release for 3 years. On the other
hand, things are getting better now. Thanks to you, sparse
has merge more change in the recent half years than all
of the two years before it. I do expect to see another release to
merge your huge number of patches.


Chris

Re: [PATCH] Adding ignored attribute optimize

[Luc Van Oostenryck]

On Sat, Jul 01, 2017 at 03:21:36PM -0700, Christopher Li wrote:
> On Sat, Jul 1, 2017 at 2:24 AM, Luc Van Oostenryck
> <luc.vanoostenryck@gmail.com> wrote:
> >
> > Sure it make some sense.
> >
> > But how often it happen and when it happen how much developers
> > are impacted and for how long?
> 
> It actually happen right now with the kernel using require_context
> which is not supported by sparse. Sparse should report error
> on the sparse related syntax which it does not support. Gcc is
> no going to report error on those sparse related attributes because
> gcc never see them.

"should report error" ...
What error exactly? Is there something that doesn't work on sparse
or on GCC's side regarding this 'require_context'? What need to be
fixed exactly?
 
> Because we have noway to tell an unknown attribute is belong
> to sparse or gcc.

That's *exactly* why sparse should *not* report. You have no way to
tell if what you're reporting is legitimate or not. And in most
cases it's legitimate on GCC's side but you still want to report it.

> Sparse checker should report it. It is very bad
> silence on those sparse specific error and developer have no way
> to know it.

It is very bad to report about all those false warnings.
It make sparse less usefull, not more.

> As for the distro version of sparse is too old.
> I think the right thing to do is just get into the habit of release
> more often.

Yes, it makes the problem worse, but even doing a release every
week won't change the problem when you take in account the lifetime
of distros.

If you want sparse to be usefull, you need to seek forward
compatibility and not blindly reporting lots of false warnings
for some very rare true ones.

But well ... let's agree we disagree.

-- Luc

Re: [PATCH] Adding ignored attribute optimize

[Christopher Li]

On Sun, Jul 2, 2017 at 12:58 PM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
> "should report error" ...
> What error exactly? Is there something that doesn't work on sparse
> or on GCC's side regarding this 'require_context'? What need to be
> fixed exactly?

Have you look at the require_context code causing the warning in the
kernel? That code only issue require_context when compiling with sparse.

The fix should be just either convert to context parsing sparse supported,
implement in sparse, or just remove the require_context in kernel. It is
not doing any thing there right now. What else do you thing it is the
right fix for it?

>
> That's *exactly* why sparse should *not* report. You have no way to
> tell if what you're reporting is legitimate or not. And in most
> cases it's legitimate on GCC's side but you still want to report it.

I don't like sparse warn on the legit gcc attribute either.
I have some patch should make both of us happy.

GCC is open. It is not that hard to get a full list of gcc supported
attribute names.  I add the full list of gcc supported attribute list
into the sparse. Sparse should not warn on any known gcc attributes
any more. Patch on the way.

Let's be done with this.

Chris

Re: [PATCH] Adding ignored attribute optimize

[Luc Van Oostenryck]

On Mon, Jul 03, 2017 at 11:23:48AM -0700, Christopher Li wrote:
> On Sun, Jul 2, 2017 at 12:58 PM, Luc Van Oostenryck
> <luc.vanoostenryck@gmail.com> wrote:
> > "should report error" ...
> > What error exactly? Is there something that doesn't work on sparse
> > or on GCC's side regarding this 'require_context'? What need to be
> > fixed exactly?
> 
> Have you look at the require_context code causing the warning in the
> kernel? That code only issue require_context when compiling with sparse.

Yes, I have looked at the code. Not only looked at but also searching how
it is *used*. And the result is that the defined macros are aither:
* not used (__protexted_{read,write}_by()
* always used with a macro that *also* use __context__ (__must_hold())
* a single exception: a member is declared with __protect_by() but that
  change exactly nothing.
In other words, this __require_context() has exactly zero effect and
doesn't need any fixes. It can simply be removed.

> The fix should be just either convert to context parsing sparse supported,
> implement in sparse, or just remove the require_context in kernel. It is
> not doing any thing there right now. What else do you thing it is the
> right fix for it?

There is nothing to fix since nothing is broken and everything works
as it should, including sparse's context checking. At worse, someone
use for this code a version of sparse that support other kind of context
checking that was never officially supported.

> > That's *exactly* why sparse should *not* report. You have no way to
> > tell if what you're reporting is legitimate or not. And in most
> > cases it's legitimate on GCC's side but you still want to report it.
> 
> I don't like sparse warn on the legit gcc attribute either.
> I have some patch should make both of us happy.
> 
> GCC is open. It is not that hard to get a full list of gcc supported
> attribute names.  I add the full list of gcc supported attribute list
> into the sparse. Sparse should not warn on any known gcc attributes
> any more. Patch on the way.
> 
> Let's be done with this.

Will your patch also cover the new attributes in GCC8? And the ones
that clang use now or will use in the short future? Or those used
by some othesr compilers that use the same syntax, like does the
ARM's compiler?

-- Luc