16 ms·
Linus on compiler warnings and code reviews
- Someone1234 11y agoThat was actually darn polite for Linus.
- fit2rule 11y agoYes, I was going to say something along those lines too .. seems our Benevolent Leader has gotten a bit soft. Frankly, the use of array function arguments in code like this would set me off into a minor rage .. I'm impressed at Linus' restraint this time around. Whats interesting though is that we're still dealing with these kinds of issues, even now in the 21st Century, from C programmers who should know better. Seems that the more things change the more they stay the same ..
- jordigh 11y agoYeah, "bovine manure" instead of "bullshit". He censored himself!
- mikekchar 11y agoIt's really quite a good post, though I suppose it is embarrassing for the people involved. It spells out clearly what the problem is (i.e. what bugs have been introduced and what are likely to be introduced), what actions he wants changed in the future (i.e. don't ignore new compiler warnings) and says that he is not satisfied with the level of quality. He even indicates specifically which people he wants to pay particular attention to the issue. That he can get away with saying these things shows that he has built a good team. I know I am not alone in experiencing the frustration of pointing out problems only to be greeted with, "Yeah, whatever. That's just your opinion." Or conversely having people complain vociferously about something without knowing the actual problems it will create (very often it is merely "You aren't doing it my way!"). I think the important thing is that, no matter how OTT Linus's posts can seem to someone outside the process, he has built trust. His complaints are very nearly always, "You are going to break X" and almost never "You aren't doing it my way". Even if it is difficult to understand, the people working on the kernel trust him and will accept the criticism. Just musing here, but I think that if you want to build something of quality, you need a person on the team who is willing to be critical of failure. But that person has to engender trust in the rest of the team. It's quite a difficult balance because nobody likes to be criticised (or have their work criticised). Getting it right is hard and is probably the biggest failure I see of people trying to make the jump from being a good solid dev to being a good leader of people.
- Animats 11y agoBiggest flaw in the design of C: no real array parameters. Source of most buffer overflow vulnerabilities for thirty years. It's a fixable problem [1] but it's not going to get fixed. We need to move forward to Rust, or something like it, and get rid of C for anything for which security matters. [1] http://animats.com/papers/languages/safearraysforc43.pdf http://animats.com/papers/languages/safearraysforc43.pdf
- kachnuv_ocasek 11y agoIt's not a flaw IMO, it's just confusing. Especially for those who come from higher languages and don't read a solid intro into C. It's actually explained quite well in K&R.
- Animats 11y agoNo, it's a design flaw. It's a consequence of K&R C for the PDP-11, which had to be crammed into a small address space machine and didn't even have function prototypes. In K&R C, which is one notch above assembly code, there was no cross-module type checking and everything was really an int or a float. Even in the 1970s, this was kind of lame. Pascal could pass array sizes through a call.
- pjmlp 11y agoIt is a big flaw to ignore what other language communities were doing. Other systems programming languages older than C did it properly.
- tosseraccount 11y agoHow many programs running on your computer were written in these languages?
- pjmlp 11y agoWhat does it matter?!? UNIX killed those languages.
- acconsta 11y agoDoes anyone know why array arguments decay to pointers?
- doppelganger27 11y agoIn C (and in all other languages I can think of), an array is actually a pointer to a bunch of memory. Indexing into the array calculates offset from the array pointer, and grabs the item at that offset.
- nickcw 11y agoFYI the equivalent Go code would actually copy the whole array on the stack, and so it would be valid if possibly inefficient depending on the size of the array.
- ashearer 11y ago...and for anything larger than a small fixed-size array (like the example), you'd avoid data-copying overhead by passing a slice, which is a bounds-checked bundle of pointer and length.
- 36erhefg 11y agoArray is not a pointer. Your description of indexing is not correct, neither is the phrase "array pointer" you invented. The machine code produced will calculate the offset differently for a pointer and an array.
- doppelganger27 11y agoLet me clarify with an example. Suppose you have an array: int a[10] There is a section of memory of size sizeof(int) * 10 somewhere, and the variable a is a * int that points to that section. When someone does this: int x = a[2] It is equivalent to: int x = *(a+(sizeof(int)*2)) When I said "array pointer" in my previous comment, all it meant was the memory address that the variable (which is an array) points to (in the example, the variable "a") Edit: missing paren
- meesterdude 11y agoI often feel a bit guilty when I spent time looking through something instead of just giving it a quick glance; but knowing that Linus has to fight the same fight really gives me confidence that it's better to know whats going on than to just ship code out the door.
- kybernetyk 11y agoI tend to treat warnings as errors. (Even set up my IDE to do so). In the short run it maybe is annoying but in the long run it really pays off. (Have been burned by a huge pile of crap code base with hundreds of warnings + 64bit transition).
- halosghost 11y agoThis! I religiously use clang's -Weverything -Werror. Does it mean I spend more time dealing with false-positives? yes (though honestly, not that much). Does it also mean I end up writing much better code? yes, absolutely.
- blazespin 11y agoIt doesn't take long before you naturally write warning free code. (Which ends up making you a better coder, as well. Such a simple thing..)
- halosghost 11y agoI completely agree. In general, the only warnings I get on my code anymore are ones related to how a library I use does something. In those cases, I surround those particular library calls with no-warn pragmas for that particular warning and then I go about my day. It feels so much cleaner.
- nickpsecurity 11y agoI didn't use a lot of C or C++ but I agree from my limited experience. I noticed the warnings and errors kicked in when I did certain things. They aggravated or worried me. So, I figured out how to not cause them. Some of those became a habit even during my brief stints working in those languages. Same thing happens in other languages with good compilers. Won't stop lazy people from ignoring them but that says more about them than the compiler. ;)
- MereInterest 11y agoAgreed. New code shouldn't be committed unless it compiles without error. The build server should then compile with -Werror, with all relevant shame being healed on the developer who breaks the build by letting a warning through.
- zwp 11y ago"I tried - and failed - to come up with a reasonable grep pattern" Is there a nice, light, better-than-betterthangrep[1] static analysis tool that would help with this sort of question? (I suspect the decay to pointer would, for example, elide this detail from llvm's IR?). [1] http://beyondgrep.com/ http://beyondgrep.com/
- klodolph 11y agoCreate a new compiler warning. My guess is that it wouldn't be that difficult to add: add a warning for using sizeof() on a value whose type is the result of array-pointer decay. If you really want the size of the pointer, then you can change the function parameter's type to the pointer type, or use a cast.
- chris_wot 11y agoClang plugins are pretty cool. The LibreOffice project uses them reasonably extensively.
- nitrogen 11y agoThere is/was a refactoring DSL for parsing and modifying C code accurately, but I don't recall the name. If I remember correctly, it was produced by a French organization or university.
- krajaratnam 11y agoYou're probably thinking of http://coccinelle.lip6.fr/sp.php http://coccinelle.lip6.fr/sp.php?
- nitrogen 11y agoYes, I think that's right.
- chengiz 11y agoWhat is the warning?
- deleted 11y ago[deleted]
- myth_buster 11y agoIt's in the subsequent response [0] from Linus. [0] https://lkml.org/lkml/2015/9/3/494 https://lkml.org/lkml/2015/9/3/494
- zamalek 11y agoIn the age and day of static analysis, someone actually took the time to question the rudimentary analysis the compiler does? It boggles the mind. I'm with Linus on this one.
- vmorgulis 11y agoThorvalds still runs Fedora (22) and the GCC warning is a new one coming from clang. https://lkml.org/lkml/2015/9/3/494 https://lkml.org/lkml/2015/9/3/494 https://lkml.org/lkml/2015/9/3/499 https://lkml.org/lkml/2015/9/3/499
- pcvarmint 11y agoC does have pointers to arrays: static bool rate_control_cap_mask(struct ieee80211_sub_if_data *sdata, struct ieee80211_supported_band *sband, struct ieee80211_sta *sta, u32 *mask, u8 (*mcs_mask)[IEEE80211_HT_MCS_MASK_LEN]) for (i = 0; i < sizeof(*mcs_mask)/sizeof(**mcs_mask); i++) { // Use (*mcs_mask)[i] or mcs_mask[0][i] }
- anthay 11y agoI've made this mistake in the past. In C++ I used a reference to fix it like so: #include <cassert> typedef char array_xyz_type[42]; void test(array_xyz_type & a, array_xyz_type b, char c[42], char (&d)[42]) { assert(sizeof(a) == 42); assert(sizeof(b) == 4); assert(sizeof(c) == 4); assert(sizeof(d) == 42); } int main() { array_xyz_type x = {1,2,3}; test(x, x, x, x); } I'd add these comments: 1. The reference makes the code somewhat less flexible in that you can't call the function with anything other than a char[42] type, e.g. test(new char[42]...) won't compile. (This may or may not be what you want.) 2. As Linus said, don't ignore compiler warnings. 3. Write some code, then watch every line execute in the debugger so you see if what you wrote is what you thought you wrote. Repeat.