login
Search
"In no case is it ok to just 'shut up the warning'," Linus Torvalds exclaimed in response to a patch that stifled a compiler warning. Reminiscent of a thread on the lkml last year [story], Linus pointed out that it is very important to understand and properly fix compiler warnings [story]:
"Please, we do NOT fix compiler warnings without understanding the code! That's a sure way to just introduce _new_ bugs, rather than fix old ones. So please, please, please, realize that the compiler is _stupid_, and fixing warnings without understanding the code is bad.
"In this case, anybody who actually spends 5 seconds looking at the code should have realized that the warning is just another way of saying that the author of the code was on some bad drugs, and the warnings WERE BETTER OFF REMAINING! Because that code _should_ have warnings. Big fat warnings about incompetence!?"
From: Bartlomiej Zolnierkiewicz [email blocked] To: Linus Torvalds [email blocked] Subject: [git patches] IDE fixes Date: Thu, 24 May 2007 02:46:18 +0200 Please pull from: master.kernel.org:/pub/scm/linux/kernel/git/bart/ide-2.6.git/ to receive the following updates: drivers/ide/ide-dma.c | 4 +++- drivers/ide/ide-proc.c | 2 ++ drivers/ide/pci/atiixp.c | 1 + drivers/ide/pci/serverworks.c | 14 ++++++++++---- include/linux/pci_ids.h | 1 + 5 files changed, 17 insertions(+), 5 deletions(-)
From: Linus Torvalds [email blocked] Subject: Re: [git patches] IDE fixes Date: Wed, 23 May 2007 18:56:45 -0700 (PDT) On Thu, 24 May 2007, Bartlomiej Zolnierkiewicz wrote: > diff --git a/drivers/ide/pci/serverworks.c b/drivers/ide/pci/serverworks.c > index 6234f80..47bcd91 100644 > --- a/drivers/ide/pci/serverworks.c > +++ b/drivers/ide/pci/serverworks.c > @@ -173,7 +179,7 @@ dma_pio: > ((dma_stat&(1<<(5+unit)))==(1<<(5+unit)))) { > u8 dmaspeed = dma_timing; > > - dma_timing &= ~0xFF; > + dma_timing &= ~0xFFU; This is just crap. The old code was _also_ crap, but the new code just is worse. What's the point of this, really? dma_timing is a 8-bit value, so the above is just a *really* stupid and bad way of saying dma_timing = 0; and whoever wrote that code is just terminally confused. I pulled, but that driver is CRAP. Please don't add new crap blindly like that. Andrew, that was your change, it appears. Tssk. Please, we do NOT fix compiler warnings without understanding the code! That's a sure way to just introduce _new_ bugs, rather than fix old ones. So please, please, please, realize that the compiler is _stupid_, and fixing warnings without understanding the code is bad. In this case, anybody who actually spends 5 seconds looking at the code should have realized that the warning is just another way of saying that the author of the code was on some bad drugs, and the warnings WERE BETTER OFF REMAINING! Because that code _should_ have warnings. Big fat warnings about incompetence! Linus
From: Linus Torvalds [email blocked] Subject: Re: [git patches] IDE fixes Date: Wed, 23 May 2007 19:16:33 -0700 (PDT) On Wed, 23 May 2007, Linus Torvalds wrote: > > So please, please, please, realize that the compiler is _stupid_, and > fixing warnings without understanding the code is bad. Btw, this is fundamental. If you don't need to understand the code, then the compiler could have just fixed it for you, and there was no need to warn. So compiler warnings have two cases: - the compiler is being a complete a**hole, and the warning should exist. It happens. - it can be fixed, but only by understanding what the code wants to do. In no case is it ok to just "shut up the warning". Linus
Dude, that code IS b0rken!!!
I went to look at that particular code. I don't see how the function could POSSIBLY be right. Take a look:
if ((dmaspeed & 0x20) == 0x20) dmaspeed = XFER_MW_DMA_2; else if ((dmaspeed & 0x21) == 0x21) dmaspeed = XFER_MW_DMA_1; else if ((dmaspeed & 0x77) == 0x77) dmaspeed = XFER_MW_DMA_0; else goto dma_pio;This code will set dmaspeed = XFER_MW_DMA_2, or it will go to the piospeed setup. It will NEVER set dmaspeed to XFER_MW_DMA_1 or XFER_MW_DMA_2. It's simply not possible. Why? If (dmaspeed & 0x21) == 0x21, then (dmaspeed & 0x20) == 0x20. The first 'if' will fire before the second is looked at. A similar statement can be said about the third 'if' statement with respect to the first.
The piospeed nested-if is similarly bad. WTF?
Given that the comments say that this came from the ASIC architect, I'm guessing it's a clumsy attempt at writing C while staring at the VHDL / Verilog.
Edit: Gahh!!! My eyes! I looked through more of that code. The PAIN! I blame YOU KernelTrap, I blame YOU for showing me this! ;-) ;-)
Seriously, why are there arrays of values of allowed register settings, and yet still bare constants in a nested if-else that test against these same constants? Hmmm? This is initialization code. Does it really need its loops unrolled like that?
I love how noobs call others
I love how noobs call others code "WTF" because they have no clue.
Such if-else sequence a very common thing to do.
Beware of trolls
That code is plainly broken, and the comment demonstrated what and why.
And it's not about if-else-if stuff it's about the truth values that lies there in:
0x20: 00100000
0x21: 00100001
So 0x20 & 0x21 == 0x20, which was eloquently stated in the original comment.
Beware of lack of sleep.
Ok, so i needed more coffee.
It is correct.
0x21 & 0x20 will never equal 0x21.
Sigh...
Please stop using bad drugs.
Please stop using bad drugs.
Ok, let's try something.
First, let me say I'm hardly a n00b. Second, let's try an experiment. Here's the original code:
if ((dmaspeed & 0x20) == 0x20) dmaspeed = XFER_MW_DMA_2; else if ((dmaspeed & 0x21) == 0x21) dmaspeed = XFER_MW_DMA_1; else if ((dmaspeed & 0x77) == 0x77) dmaspeed = XFER_MW_DMA_0; else goto dma_pio;I'm going to modify it *slightly* and flesh it out into a complete program:
#include <stdio.h> int main() { int i; unsigned char dmaspeed; for (i = 0; i < 256; i++) { dmaspeed = i; if ((dmaspeed & 0x20) == 0x20) dmaspeed = 2; else if ((dmaspeed & 0x21) == 0x21) dmaspeed = 1; else if ((dmaspeed & 0x77) == 0x77) dmaspeed = 0; else dmaspeed = 255; if ((i & 0xF) == 0) printf("\n%2X: ", i); printf(" %2X", dmaspeed); } printf("\n"); }And let's just see what it outputs, shall we?
Oh gee, it only selected between 2 and nothing! Hmmm... Go back and read my original post for an explanation of why. If it doesn't make sense to you, then perhaps "n00b" doesn't mean what you think it means.
Bad code
Concerning the output of your program, it is too bloated by far. You probably can do better than that, come on.
Que?
Go away troll. :-) You either don't understand the principle of brute force, or are just looking to get a rise out of people.
Common but not good.
Usually the people who code these else-ifs learned 'C' in a couple of weeks and their coding skills did not progress any more from that point on. Usually they learned Pascal before 'C' and then try to turn 'C' into Pascal. Thank goodness for departure of such "programmers".
As the kernel matures, it is time to rewrite some junky/quickly-written code to improve quality. The original writer will always defend it -- "it works, most of the time, and I wrote it so quickly, and so long ago!". Such coders belong at Microsoft, not in the kernel.
Neither common nor good.
The code idiom that this code has is *not* common precisely because it is *not* good. That said, it bears a striking resemblance to a good idiom. That's the problem.
The good idiom looks something like this:
if ((status & (FLAG1 | FLAG2 | FLAG3)) == (FLAG1 | FLAG2 | FLAG3)) { /* do something */ }This idiom ensures that all three flags are set, ignoring other possible flags. Putting this in an if-else, though, where the least restrictive comparison precedes the other comparisons? Not quite idiomatic, and far from good.
Now, a better (but not necessarily great) way to write this might be:
switch (status & (FLAG1 | FLAG2 | FLAG3 | FLAG4)) { case (FLAG1 | FLAG3): case (FLAG1 | FLAG2 | FLAG3): /* handle FLAG1 && FLAG3 && ~FLAG4 */; break; case (FLAG1 | FLAG2): case (FLAG1 | FLAG2 | FLAG4): /* hangle FLAG1 && FLAG2 && ~FLAG3 */; break; /* etc */Still, not great.
The code had lookup tables that could be looped over. Given that this is initialization code, why aren't those structs w/ just a little more information in them? The whole thing could be boiled down to a pretty simple for loop.
Maybe that's what the coder wanted:
Why not just do it that way, rather than masturbating?
I _HATE_ warnings
I agree with Linus that a warning is the result of bad thinking brought on by incompetence. What brings on incompetence I’ll leave up to others; but: Is there any such thing a bad drug? ;)
Should not _all_ warnings in the kernel be investigated to ensure that they are not the harbingers of doom? Should new code be rejected if it contains warnings? Of course, this could make developers fix the symptoms by hiding the warnings rather that address the real problem. But, I suspect that people who do this would not long remain part of the kernel-developer community.
Just my 2c (GST included) worth.
----
Problem Solving Algorithm:
1) Write down problem
2) Think really hard
3) Write down answer
- Richard Feynman
Shutting up warnings
> Should not _all_ warnings in the kernel be investigated to ensure that they are not the harbingers of doom?
Sure, they should all be *investigated*, but they should not all be "shut up". Some warnings are simply the result of GCC not being very smart and unable to see that whatever it is warning about can never happen. Some warnings are just plain silly.
> Should new code be rejected if it contains warnings?
No, not if the code is fine and shutting up the warning would make it worse. When the warning is just a case of compiler stupidity the warning should just be ignored.
> this could make developers fix the symptoms by hiding the warnings rather that address the real problem.
Sometimes there really is no "real problem".
Compiler warnings
When I developed code, I never allowed compiler warnings to be turned off with a compiler switch or with #pragmas.
When warnings were produced, I investigated, and investigated, and resolved the warning(s) and retested until I was sure the code was correct. Often the warnings signaled me that I should rewrite the code. I usually found a better way to solve business problem and eliminate the warning.
If the code was correct but the warning was valid, then and only then would I put a comment with a date stamp as the side comment or a "before the code" comment to explain that the following warning was to be ignored.
Sometimes, early in my career I too have been guilty of putting #pragmas nowarnings and #pragma warnings around code that I explained in the penultimate paragraph.This was limited to code warnings due to macro expansions. But then 6 months later, when I had to support my code, I went crazy to discover the masked error. Once bitten, twice shy. One must use brains, and not be lazy to recode, and test, if necessary.
Every person, can make errors, is it due to incompetence, or lack of sleep (16 hour days)? Actually, the author of the code should always pass the code to a peer for a walkthrough. Haven't developers ever heard of code walk throughs?
Urgl... typo above.
"It will NEVER set dmaspeed to XFER_MW_DMA_1 or XFER_MW_DMA_2." I clearly meant "It will NEVER set dmaspeed to XFER_MW_DMA_1 or XFER_MW_DMA_0." Mea culpa.
Bugreport
I suppose you've posted to the listed maintainer about this issue? This is one of those bugs that will remain lingering because most users will probably have no problems and those that do don't complain. And there is little chance in somebody stumbling on it again except when somebody someday decides to revise the code.
And that's probably not gonna happen any time soon :-)
I haven't, actually.
I looked for email addresses in the source file, and didn't see any. I was posting from work, and so really didn't have the time to dig in and find out who actually maintains that. I also haven't a clue what the correct code is supposed to be (though I can guess).
Even just reversing the order of the tests would be a step in the right direction, even if it's not truly correct.
If I remember over the weekend I might dig around for the maintainer.
Edit: Just sent a friendly email to Bartlomiej Zolnierkiewicz pointing him over to this thread, and describing to him the issue as I saw it.
you can always write
you can always write directly to lkml list or according to MAINTAINERS file in kernel sources, ide subsystem is maintained by
IDE SUBSYSTEM
P: Bartlomiej Zolnierkiewicz
M: bzolnier@gmail.com
L: linux-ide@vger.kernel.org
T: quilt kernel.org/pub/linux/kernel/people/bart/pata-2.6/
S: Maintained
re: report
did you reported this code issue to authorized kernel personel? i'm just interested in followup :)
Like I said in my edit...
Like I said in my edit above, I emailed Bartlomiej. I haven't heard anything back though.
Hi, are you Mr Z from the
Hi,
are you Mr Z from the C-64 "scene"? You sure seem like a capable coder.
Sorry...
I'm not, though I am also Mr Z on Slashdot and Wikipedia. I am a part of the "Intellivision scene" these days, though, and I certainly know my way around assembler hacks. Just not C64. For my day job, I'm a CPU architect for Texas Instruments.
do not use chip diagnostics you dont design yourself
there has been a fad of integrating a govt designed chip diagnostics system into processing chips since the 8088.
the 8086 did not have this in its design, thus the reason the 8088 was introduced; to flood the market with this replacement chip, of which the sole purpose of its inception was to introduce the hardcased gps failsafe into the diagnostics portion of the central processor.
ever since the 8088, in all 8x86 since, this integrated circuitry, allows for any chip in the world to be controlled using wireless satelite communications with the GPS network. this system affords for computation errors to be introduced remotely, in order to eventually crash a system, and provide Cryptos AG components (yes a federal satellite of the united states as a classified front company for the nsa and cia in order to gain control over cyptography around the world under a seemingly legit company in sweden. the bubble of secrecy burst during a scandal involving iranians seizing employess from the Cryptos AG company with demands for an extreme dollar amount as reparation for their us backdoored cryptography systems, of which a master key controlled by the united states was inserted secretly into all such cryptograpy devices)
another purpose was to be able to reverse engineer any such computer system using these modified chips, in addition to tracking their globally unique location. Windows simply sucks because these systems are in full force, not due to issues with the os, the os is actually virtually flawless, these integrated systems are the cause, not the software.
in the next 5 years, Linux itself will be shown to have such interuptions in performance as well. The only reason linux is so stable, is due to the restraint of the future federal american empire in interfering with such systems, as to watch quietly its development in a passive manner.
the use of quantum tunneling and particle/wave pairing is in use by such communications in order to make use of quantum decision splits in order to monitor said traffic and interfere when deemed neccessary.
Mr_Z, you must ensure these systems do not encur in your chips, however most likely you will have no choice, due to a change in design outside your awareness, pre-production.
Sincerely,
TRK (MIB)
mediaplague.com
Wow, that's a good one.
Tell us about how it's a plot of the Illuminati to hijack all of our computers in order to create an ultrapowerful Megabrain that will communicate with ancient astronauts to help refloat Atlantis!
Sheesh.
Nearly had me there, except
Nearly had me there, except for your claim that "Windows [...] is actually virtually flawless". Anyone claiming that is clearly delusional.
And when is your novel due?
You seem to know how to write fiction.