| Message ID | CANeU7QnNNL1u3BxhWLebFqzn+9Y7hHKABDo_mkEqfwF+VO963Q@mail.gmail.com (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Headers | show |
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 -- 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, 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 -- 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, 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 -- 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, 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 -- 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, 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 -- 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 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 -- 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, 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 -- 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
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