Re: [Intel-gfx] [PATCH] [v2] Kbuild: move to -std=gnu11
On Mon, Feb 28, 2022 at 11:32 AM Arnd Bergmann wrote: > > -under ``-std=gnu89`` [gcc-c-dialect-options]_: the GNU dialect of ISO C90 > -(including some C99 features). ``clang`` [clang]_ is also supported, see > +under ``-std=gnu11`` [gcc-c-dialect-options]_: the GNU dialect of ISO C11 > +(including some C17 features). ``clang`` [clang]_ is also supported, see I think the "(including some C17)" bit would not make much sense anymore. There were no major changes in C17 and GCC implements `-std=c11` and `-std=c17` as basically the same thing according to the docs (and GNU extensions apply equally to both, I would assume). When I wrote the "(including some C99 features)" I meant that GCC implemented some C99 features as extensions in C90 mode, and the kernel used some of those (e.g. the now gone VLAs). With that changed, for `programming-language.rst`: Reviewed-by: Miguel Ojeda Cheers, Miguel
Re: [Intel-gfx] [PATCH 2/3] fbdev: rework backlight dependencies
On Wed, Oct 27, 2021 at 3:28 PM Arnd Bergmann wrote: > > Rather than having CONFIG_FB_BACKLIGHT select CONFIG_BACKLIGHT_CLASS_DEVICE, > make any driver that needs it have a dependency on the class device > being available, to prevent circular dependencies. Acked-by: Miguel Ojeda Cheers, Miguel
Re: [Intel-gfx] [PATCH 000/141] Fix fall-through warnings for Clang
On Wed, Nov 25, 2020 at 11:44 PM Edward Cree wrote: > > To make the intent clear, you have to first be certain that you > understand the intent; otherwise by adding either a break or a > fallthrough to suppress the warning you are just destroying the > information that "the intent of this code is unknown". If you don't know what the intent of your own code is, then you *already* have a problem in your hands. > Figuring out the intent of a piece of unfamiliar code takes more > than 1 minute; just because > case foo: > thing; > case bar: > break; > produces identical code to > case foo: > thing; > break; > case bar: > break; > doesn't mean that *either* is correct — maybe the author meant What takes 1 minute is adding it *mechanically* by the author, i.e. so that you later compare whether codegen is the same. > to write > case foo: > return thing; > case bar: > break; > and by inserting that break you've destroyed the marker that > would direct someone who knew what the code was about to look > at that point in the code and spot the problem. Then it means you already have a bug. This patchset gives the maintainer a chance to notice it, which is a good thing. The "you've destroyed the market" claim is bogus, because: 1. you were not looking into it 2. you didn't notice the bug so far 3. is implicit -- harder to spot 4. is only useful if you explicitly take a look at this kind of bug. So why don't you do it now? > Thus, you *always* have to look at more than just the immediate > mechanical context of the code, to make a proper judgement that > yes, this was the intent. I find that is the responsibility of the maintainers and reviewers for tree-wide patches like this, assuming they want. They can also keep the behavior (and the bugs) without spending time. Their choice. > If you think that that sort of thing > can be done in an *average* time of one minute, then I hope you > stay away from code I'm responsible for! Please don't accuse others of recklessness or incompetence, especially if you didn't understand what they said. > A warning is only useful because it makes you *think* about the > code. If you suppress the warning without doing that thinking, > then you made the warning useless; and if the warning made you > think about code that didn't *need* it, then the warning was > useless from the start. We are not suppressing the warning. Quite the opposite, in fact. > So make your mind up: does Clang's stricter -Wimplicit-fallthrough > flag up code that needs thought (in which case the fixes take > effort both to author and to review) As I said several times already, it does take time to review if the maintainer wants to take the chance to see if they had a bug to begin with, but it does not require thought for the author if they just go for equivalent codegen. > or does it flag up code > that can be mindlessly "fixed" (in which case the warning is > worthless)? Proponents in this thread seem to be trying to > have it both ways. A warning is not worthless just because you can mindlessly fix it. There are many counterexamples, e.g. many checkpatch/lint/lang-format/indentation warnings, functional ones like the `if (a = b)` warning... Cheers, Miguel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 000/141] Fix fall-through warnings for Clang
On Thu, Nov 26, 2020 at 4:28 PM Geert Uytterhoeven wrote: > > The maintainer is not necessarily the owner/author of the code, and > thus may not know the intent of the code. Agreed, I was not blaming maintainers -- just trying to point out that the problem is there :-) In those cases, it is still very useful: we add the `fallthrough` and a comment saying `FIXME: fallthrough intended? Figure this out...`. Thus a previous unknown unknown is now a known unknown. And no new unknown unknowns will be introduced since we enabled the warning globally. > BTW, you cannot mindlessly fix the latter, as you cannot know if > "(a == b)" or "((a = b))" was intended, without understanding the code > (and the (possibly unavailable) data sheet, and the hardware, ...). That's right, I was referring to the cases where the compiler saves someone time from a typo they just made. Cheers, Miguel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [Intel-wired-lan] [PATCH 000/141] Fix fall-through warnings for Clang
On Wed, Nov 25, 2020 at 5:24 PM Jakub Kicinski wrote: > > And just to spell it out, > > case ENUM_VALUE1: > bla(); > break; > case ENUM_VALUE2: > bla(); > default: > break; > > is a fairly idiomatic way of indicating that not all values of the enum > are expected to be handled by the switch statement. It looks like a benign typo to me -- `ENUM_VALUE2` does not follow the same pattern like `ENUM_VALUE1`. To me, the presence of the `default` is what indicates (explicitly) that not everything is handled. > Applying a real patch set and then getting a few follow ups the next day > for trivial coding things like fallthrough missing or static missing, > just because I didn't have the full range of compilers to check with > before applying makes me feel pretty shitty, like I'm not doing a good > job. YMMV. The number of compilers, checkers, static analyzers, tests, etc. we use keeps going up. That, indeed, means maintainers will miss more things (unless maintainers do more work than before). But catching bugs before they happen is *not* a bad thing. Perhaps we could encourage more rebasing in -next (while still giving credit to bots and testers) to avoid having many fixing commits afterwards, but that is orthogonal. I really don't think we should encourage the feeling that a maintainer is doing a bad job if they don't catch everything on their reviews. Any review is worth it. Maintainers, in the end, are just the "guaranteed" reviewers that decide when the code looks reasonable enough. They should definitely not feel pressured to be perfect. Cheers, Miguel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [Intel-wired-lan] [PATCH 000/141] Fix fall-through warnings for Clang
On Wed, Nov 25, 2020 at 12:53 AM Finn Thain wrote: > > I'm saying that supporting the official language spec makes more sense > than attempting to support a multitude of divergent interpretations of the > spec (i.e. gcc, clang, coverity etc.) Making the kernel strictly conforming is a ship that sailed long ago, for several reasons. Anyway, supporting several compilers and other tools, regardless of extensions, is valuable. > I'm also saying that the reason why we use -std=gnu89 is that existing > code was written in that language, not in ad hoc languages comprised of > collections of extensions that change with every release. No, we aren't particularly tied to `gnu89` or anything like that. We could actually go for `gnu11` already, since the minimum GCC and Clang support it. Even if a bit of code needs fixing, that shouldn't be a problem if someone puts the work. In other words, the kernel code is not frozen, nor are the features it uses from compilers. They do, in fact, change from time to time. > Thank you for checking. I found a free version that's only 6 weeks old: You're welcome! There are quite a few new attributes coming, mostly following C++ ones. > It will be interesting to see whether 6.7.11.5 changes once the various > implementations reach agreement. Not sure what you mean. The standard does not evolve through implementations' agreement (although standardizing existing practice is one of the best arguments to back a change). Cheers, Miguel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 000/141] Fix fall-through warnings for Clang
On Mon, Nov 23, 2020 at 9:38 PM James Bottomley wrote: > > So you think a one line patch should take one minute to produce ... I > really don't think that's grounded in reality. No, I have not said that. Please don't put words in my mouth (again). I have said *authoring* lines of *this* kind takes a minute per line. Specifically: lines fixing the fallthrough warning mechanically and repeatedly where the compiler tells you to, and doing so full-time for a month. For instance, take the following one from Gustavo. Are you really saying it takes 12 minutes (your number) to write that `break;`? diff --git a/drivers/gpu/drm/via/via_irq.c b/drivers/gpu/drm/via/via_irq.c index 24cc445169e2..a3e0fb5b8671 100644 --- a/drivers/gpu/drm/via/via_irq.c +++ b/drivers/gpu/drm/via/via_irq.c @@ -364,6 +364,7 @@ int via_wait_irq(struct drm_device *dev, void *data, struct drm_file *file_priv) irqwait->request.sequence += atomic_read(_irq->irq_received); irqwait->request.type &= ~_DRM_VBLANK_RELATIVE; + break; case VIA_IRQ_ABSOLUTE: break; default: > I suppose a one line > patch only takes a minute to merge with b4 if no-one reviews or tests > it, but that's not really desirable. I have not said that either. I said reviewing and merging those are noise compared to any complex patch. Testing should be done by the author comparing codegen. > Part of what I'm trying to measure is the "and useful" bit because > that's not a given. It is useful since it makes intent clear. It also catches actual bugs, which is even more valuable. > Well, you know, subsystems are very different in terms of the amount of > patches a maintainer has to process per release cycle of the kernel. > If a maintainer is close to capacity, additional patches, however > trivial, become a problem. If a maintainer has spare cycles, trivial > patches may look easy. First of all, voluntary maintainers choose their own workload. Furthermore, we already measure capacity in the `MAINTAINERS` file: maintainers can state they can only handle a few patches. Finally, if someone does not have time for a trivial patch, they are very unlikely to have any time to review big ones. > You seem to be saying that because you find it easy to merge trivial > patches, everyone should. Again, I have not said anything of the sort. Cheers, Miguel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [Intel-wired-lan] [PATCH 000/141] Fix fall-through warnings for Clang
On Tue, Nov 24, 2020 at 11:24 PM Finn Thain wrote: > > These statements are not "missing" unless you presume that code written > before the latest de facto language spec was written should somehow be > held to that spec. There is no "language spec" the kernel adheres to. Even if it did, kernel code is not frozen. If an improvement is found, it should be applied. > If the 'fallthrough' statement is not part of the latest draft spec then > we should ask why not before we embrace it. Being that the kernel still > prefers -std=gnu89 you might want to consider what has prevented > -std=gnu99 or -std=gnu2x etc. The C standard has nothing to do with this. We use compiler extensions of several kinds, for many years. Even discounting those extensions, the kernel is not even conforming to C due to e.g. strict aliasing. I am not sure what you are trying to argue here. But, since you insist: yes, the `fallthrough` attribute is in the current C2x draft. Cheers, Miguel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 000/141] Fix fall-through warnings for Clang
On Tue, Nov 24, 2020 at 1:58 AM Finn Thain wrote:
>
> What I meant was that you've used pessimism as if it was fact.
"future mistakes that it might prevent" is neither pessimism nor states a fact.
> For example, "There is no way to guess what the effect would be if the
> compiler trained programmers to add a knee-jerk 'break' statement to avoid
> a warning".
It is only knee-jerk if you think you are infallible.
> Moreover, what I meant was that preventing programmer mistakes is a
> problem to be solved by development tools
This warning comes from a development tool -- the compiler.
> The idea that retro-fitting new
> language constructs onto mature code is somehow necessary to "prevent
> future mistakes" is entirely questionable.
The kernel is not a frozen codebase.
Further, "mature code vs. risk of change" arguments don't apply here
because the semantics of the program and binary output isn't changing.
> Sure. And if you put -Wimplicit-fallthrough into the Makefile and if that
> leads to well-intentioned patches that cause regressions, it is partly on
> you.
Again: adding a `fallthrough` does not change the program semantics.
If you are a maintainer and want to cross-check, compare the codegen.
> Have you ever considered the overall cost of the countless
> -Wpresume-incompetence flags?
Yeah: negative. On the other hand, the overall cost of the countless
-fI-am-infallible flags is very noticeable.
> Perhaps you pay the power bill for a build farm that produces logs that
> no-one reads? Perhaps you've run git bisect, knowing that the compiler
> messages are not interesting? Or compiled software in using a language
> that generates impenetrable messages? If so, here's a tip:
>
> # grep CFLAGS /etc/portage/make.conf
> CFLAGS="... -Wno-all -Wno-extra ..."
> CXXFLAGS="${CFLAGS}"
>
> Now allow me some pessimism: the hardware upgrades, gigawatt hours and
> wait time attributable to obligatory static analyses are a net loss.
If you really believe compiler warnings and static analysis are
useless and costly, I think there is not much point in continuing the
discussion.
> No, it's not for me to prove that such patches don't affect code
> generation. That's for the patch author and (unfortunately) for reviewers.
I was not asking you to prove it. I am stating that proving it is very easy.
Cheers,
Miguel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 000/141] Fix fall-through warnings for Clang
On Mon, Nov 23, 2020 at 4:58 PM James Bottomley wrote: > > Well, I used git. It says that as of today in Linus' tree we have 889 > patches related to fall throughs and the first series went in in > october 2017 ... ignoring a couple of outliers back to February. I can see ~10k insertions over ~1k commits and 15 years that mention a fallthrough in the entire repo. That is including some commits (like the biggest one, 960 insertions) that have nothing to do with C fallthrough. A single kernel release has an order of magnitude more changes than this... But if we do the math, for an author, at even 1 minute per line change and assuming nothing can be automated at all, it would take 1 month of work. For maintainers, a couple of trivial lines is noise compared to many other patches. In fact, this discussion probably took more time than the time it would take to review the 200 lines. :-) > We're also complaining about the inability to recruit maintainers: > > https://www.theregister.com/2020/06/30/hard_to_find_linux_maintainers_says_torvalds/ > > And burn out: > > http://antirez.com/news/129 Accepting trivial and useful 1-line patches is not what makes a voluntary maintainer quit... Thankless work with demanding deadlines is. > The whole crux of your argument seems to be maintainers' time isn't > important so we should accept all trivial patches I have not said that, at all. In fact, I am a voluntary one and I welcome patches like this. It takes very little effort on my side to review and it helps the kernel overall. Paid maintainers are the ones that can take care of big features/reviews. > What I'm actually trying to articulate is a way of measuring value of > the patch vs cost ... it has nothing really to do with who foots the > actual bill. I understand your point, but you were the one putting it in terms of a junior FTE. In my view, 1 month-work (worst case) is very much worth removing a class of errors from a critical codebase. > One thesis I'm actually starting to formulate is that this continual > devaluing of maintainers is why we have so much difficulty keeping and > recruiting them. That may very well be true, but I don't feel anybody has devalued maintainers in this discussion. Cheers, Miguel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 000/141] Fix fall-through warnings for Clang
On Sun, Nov 22, 2020 at 11:54 PM Finn Thain wrote: > > We should also take into account optimisim about future improvements in > tooling. Not sure what you mean here. There is no reliable way to guess what the intention was with a missing fallthrough, even if you parsed whitespace and indentation. > It is if you want to spin it that way. How is that a "spin"? It is a fact that we won't get *implicit* fallthrough mistakes anymore (in particular if we make it a hard error). > But what we inevitably get is changes like this: > > case 3: > this(); > + break; > case 4: > hmmm(); > > Why? Mainly to silence the compiler. Also because the patch author argued > successfully that they had found a theoretical bug, often in mature code. If someone changes control flow, that is on them. Every kernel developer knows what `break` does. > But is anyone keeping score of the regressions? If unreported bugs count, > what about unreported regressions? Introducing `fallthrough` does not change semantics. If you are really keen, you can always compare the objects because the generated code shouldn't change. Cheers, Miguel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 000/141] Fix fall-through warnings for Clang
On Sun, Nov 22, 2020 at 11:36 PM James Bottomley wrote: > > Well, it seems to be three years of someone's time plus the maintainer > review time and series disruption of nearly a thousand patches. Let's > be conservative and assume the producer worked about 30% on the series > and it takes about 5-10 minutes per patch to review, merge and for > others to rework existing series. So let's say it's cost a person year > of a relatively junior engineer producing the patches and say 100h of > review and application time. The latter is likely the big ticket item > because it's what we have in least supply in the kernel (even though > it's 20x vs the producer time). How are you arriving at such numbers? It is a total of ~200 trivial lines. > It's not about the risk of the changes it's about the cost of > implementing them. Even if you discount the producer time (which > someone gets to pay for, and if I were the engineering manager, I'd be > unhappy about), the review/merge/rework time is pretty significant in > exchange for six minor bug fixes. Fine, when a new compiler warning > comes along it's certainly reasonable to see if we can benefit from it > and the fact that the compiler people think it's worthwhile is enough > evidence to assume this initially. But at some point you have to ask > whether that assumption is supported by the evidence we've accumulated > over the time we've been using it. And if the evidence doesn't support > it perhaps it is time to stop the experiment. Maintainers routinely review 1-line trivial patches, not to mention internal API changes, etc. If some company does not want to pay for that, that's fine, but they don't get to be maintainers and claim `Supported`. Cheers, Miguel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 000/141] Fix fall-through warnings for Clang
Hi Gustavo, On Fri, Nov 20, 2020 at 7:21 PM Gustavo A. R. Silva wrote: > > Hi all, > > This series aims to fix almost all remaining fall-through warnings in > order to enable -Wimplicit-fallthrough for Clang. Thanks for this. Since this warning is reliable in both/all compilers and we are eventually getting rid of all the cases, what about going even further and making it an error right after? Cheers, Miguel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 000/141] Fix fall-through warnings for Clang
On Sun, Nov 22, 2020 at 7:22 PM James Bottomley wrote: > > Well, it's a problem in an error leg, sure, but it's not a really > compelling reason for a 141 patch series, is it? All that fixing this > error will do is get the driver to print "oh dear there's a problem" > under four more conditions than it previously did. > > We've been at this for three years now with nearly a thousand patches, > firstly marking all the fall throughs with /* fall through */ and later > changing it to fallthrough. At some point we do have to ask if the > effort is commensurate with the protection afforded. Please tell me > our reward for all this effort isn't a single missing error print. It isn't that much effort, isn't it? Plus we need to take into account the future mistakes that it might prevent, too. So even if there were zero problems found so far, it is still a positive change. I would agree if these changes were high risk, though; but they are almost trivial. Cheers, Miguel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 14/14] auxdisplay: constify fb ops
On Fri, Nov 29, 2019 at 4:24 PM Daniel Vetter wrote: > > Oh, another display subsystem? Intriguing ... > > Reviewed-by: Daniel Vetter It is intended for displays that are not intended as the usual/main display, e.g. very small LCDs :) Reviewed-by: Miguel Ojeda Cheers, Miguel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 14/14] auxdisplay: constify fb ops
On Fri, Nov 29, 2019 at 9:30 PM Daniel Vetter wrote: > > Well we do have very small lcd display drivers in drm, and before that in > fbdev. And you have a fbdev framebuffer driver in there, which looks a bit > misplaced ... > > Afaiui you also have some even tinier lcd drivers where you don't address > pixels, but just directly upload text, and those obviously don't fit into > drm/fbdev world. But anything where you can address pixels very much does. > -Daniel The first driver in the category used fb.h. At the time (~13 years ago) it was decided that the drivers should go into a different category/folder instead and then the other were added. In any case, I am removing the original ones since I cannot test them anymore and there are likely no user. The only other fb user could be relocated if Robin agrees. Cheers, Miguel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
