8 ms·

The thing that shocks me about this bug is that it should have been caught when the compiler issued a warning about an unused function parameter. Either the wa

The thing that shocks me about this bug is that it should have been caught when the compiler issued a warning about an unused function parameter. Either the warning was not enabled, or the programmer just ignored it; either of those options indicates a pretty severe lack of professionalism.

jared314 16y ago
That sounds like a severe lack of perspective.
- humbledrone 16y ago
  Huh? Any halfway decent software shop will have a policy of compiling with all warnings enabled, and fixing all sources of warnings (with only very rare exceptions). It is extremely unprofessional to disable or ignore warnings such as the "unused function parameter" warning. If you disagree, I respectfully submit that I would never let you near a C project that I was working on.
  - jey 16y ago
    The unused function parameter warning is usually one of the most annoying and least useful. I routinely compile with "-Wall -Wno-unused" because I don't want to litter my code with casts to void.
    - wvenable 16y ago
      What's with all the unused function parameters? Shouldn't that be an edge case?
      - tedunangst 16y ago
        Just about every callback interface takes a function pointer where the function expects a void * argument. Half the time it's not needed.
        kelnos 16y ago
        If the void * is the last parameter, just leave it off your implementation of the callback function.
  - neilc 16y ago
    Any halfway decent software shop will have a policy of compiling with all warnings enabled FWIW, your definition of "halfway decent" would exclude most software shops in the world. It is extremely unprofessional to disable or ignore warnings such as the "unused function parameter" warning Unused function parameter warnings are often noise (e.g., due to #ifdefs, unimplemented APIs, backward compatible APIs, etc). I agree that code should compile w/o warnings, but the benefits of tracking down and squelching unused function argument warnings are pretty marginal in my experience.
    - tptacek 16y ago
      All I can say is than in 15 years (fuck.) I've never worked on a team that would have caught a bug like this in dead code. -Wall has been the gold standard on the teams I've worked on.
      - josephruscio 16y ago
        -Wall unfortunately leaves a lot of warnings disabled. We've used: "-W -Wall -Wcast-align -Wstrict-prototypes -Wmissing -prototypes -Wpointer-arith -Wshadow -Wsign-compare -Wformat=2 -Wno-format-y2k -Wimplicit -Wmissing-braces -Wnested-externs -Wparentheses -Wtrigraphs" Update: Forgot the most important one ... -Werror. Making warnings equivalent to errors is the only way to ensure that they're always taken care of.
      - renata 16y ago
        -Wall doesn't enable -Wunused-parameter. You need -Wall -Wextra for that.
    - humbledrone 16y ago
      I would probably agree that most software shops in the world are not halfway decent. The benefits of tracking down and squelching such warnings would have very likely caused this bug to never exist. The point is that you make "compiles without warnings" a policy, and you build a habit of maintaining that policy -- after a while it begins to take almost no effort at all.
DrSprout 16y ago
You have to know that -Wextra exists, which I for one didn't until just now when I ran a sample program. All my projects just compiled with -Wall, which doesn't catch that error. Also, unless every programmer on the project is being scrupulous about finding and correcting such errors, it quickly becomes line noise, and a lot of it completely unimportant stuff.
- humbledrone 16y ago
  Yeah, that's why every programmer on the project has to be scrupulous about finding and correcting such errors. That's professionalism 101 -- if your team is not keeping the build warning free, things like this slip through.
  - tptacek 16y ago
    Things like what, a blatantly obvious bug that would crash virtually every C program running on the system were it not for the fact that it was dead code? This is practically a documentation bug. [edit] I kind of take it back, after mahmud's bzero() joke... out of oniguruma, Python, hexfiend, Apache, nginx, openssl, redis, regexkit, saxon, memcached, subversion, and valgrind, this hypothetical bug would have broken Python, redis, subversion, several apache modules, and memcached. The contents of my codebase/3p directory, FWIW.
- seliopou 16y ago
  -Wextra is total insanity. I just compiled some C code I've been working on with that flag, and got the following: "warning: signed and unsigned type in conditional expression." I was testing if a variable I declared as an integer was less than 0. To my knowledge, every number literal in C is assumed to be of a signed type unless followed by a 'u'. But now I know better. 0 is the exception.
  - swolchok 16y ago
    Are you sure? $ echo 'int main(int argc, char **argv) { if (argc < 0) { return 1; } else {return 0;}}' > test.c $ gcc -v Using built-in specs. Target: x86_64-linux-gnu Configured with: ../src/configure -v --with-pkgversion='Ubuntu 4.4.3-4ubuntu5' --with-bugurl=file:///usr/share/doc/gcc-4.4/README.Bugs --enable-languages=c,c++,fortran,objc,obj-c++ --prefix=/usr --enable-shared --enable-multiarch --enable-linker-build-id --with-system-zlib --libexecdir=/usr/lib --without-included-gettext --enable-threads=posix --with-gxx-include-dir=/usr/include/c++/4.4 --program-suffix=-4.4 --enable-nls --enable-clocale=gnu --enable-libstdcxx-debug --enable-plugin --enable-objc-gc --disable-werror --with-arch-32=i486 --with-tune=generic --enable-checking=release --build=x86_64-linux-gnu --host=x86_64-linux-gnu --target=x86_64-linux-gnu Thread model: posix gcc version 4.4.3 (Ubuntu 4.4.3-4ubuntu5) $ gcc -Wall -Wextra test.c test.c: In function ‘main’: test.c:1: warning: unused parameter ‘argv’ $ gcc-4.3.4 behaved the same way in my test.
klodolph 16y ago
"Pretty severe" lack of professionalism? It's not unprofessional not to turn the warnings all the way up and sift through each and every one. Some of us work code inherited from other people with different coding guidelines, and some of the warnings will generate loads of messages for code that is not only technically correct but stylistically correct as well. The unused parameter warning is one of the biggest offenders in this area. And if you have a hundred warnings for unused parameters, which ones do you pay attention to? Another potential issue is that the code in the patch might almost never be called in the first place. Most libc implementations (three that I know of, where I've examined the "memset" source) have a "memset" like that as a fallback only for platforms without an assembly version. I'd say the problem is that there are several different ways this error could have been detected sooner: warnings, test, static analysis tools, and review, but none of them worked. But again, it's also possible that the code is never called because assembly versions are always available.
- humbledrone 16y ago
  So what if the code doesn't get called? That doesn't prevent it from being built. Considering that memset() is part of libc, which is a shared library, presumably it's going to get compiled regardless of whether it's called.
- queensnake 16y ago
  | The unused parameter warning is one of the biggest offenders in this area. And if you have a hundred warnings for unused parameters, which ones do you pay attention to? Well, so then you're exposed to this error. Do: void foo(int x) { (void)x; } to silence unused parameter warnings.
  - tptacek 16y ago
    Go look at Google code search, which includes vast quantities of code written by professional teams and (often later) open sourced. Very little of it casts expressions to (void). When I started at Arbor, I got rid of all the (void) casts. What an eyesore. Very little C code is const-correct either. Most of the C devs I know make fun of const-correctness.
jmillikin 16y ago
The "unused parameter" error is one of the most useless of C compiler errors. There are dozens of perfectly valid reasons why a parameter would go unused, so the occasional error just gets lost in the noise. It's more helpful in C++, which can omit parameter names to indicate that it's intentionally unused. But then you're using C++; not worth the tradeoff.
- humbledrone 16y ago
  It's not hard to type a statement like "(void)v;" to explicitly say, "v is intentionally unused." I fail to see the drawback of this approach. Of course you should only do this when (1) you can't remove the parameter from the function's prototype and (2) there is a good reason for it to be unused. But once you develop that habit, the compiler can help you catch bugs that are otherwise easy to miss. Sounds like a worthwhile tradeoff to me.
  - tedunangst 16y ago
    Then you get a "statement with no effect" warning, which is usually more serious. I'll take a bogus warning in real code over a bogus warning in contrived code any day.
    - cperciva 16y ago
      I've never seen a compiler issue a warning about "(void)foo;". Even compilers which issue 'statement with no effect' warnings normally recognize deliberate casts to void as a way of saying "shut up, I know what I'm doing".
  - leif 16y ago
    What you want is __attribute__((unused)), assuming you're compiling with gcc.
- torpor 16y ago
  >There are dozens of perfectly valid reasons why a parameter would go unused, so the occasional error just gets lost in the noise. Such as?

jared314 16y ago

That sounds like a severe lack of perspective.

humbledrone 16y ago
Huh? Any halfway decent software shop will have a policy of compiling with all warnings enabled, and fixing all sources of warnings (with only very rare exceptions). It is extremely unprofessional to disable or ignore warnings such as the "unused function parameter" warning. If you disagree, I respectfully submit that I would never let you near a C project that I was working on.
- jey 16y ago
  The unused function parameter warning is usually one of the most annoying and least useful. I routinely compile with "-Wall -Wno-unused" because I don't want to litter my code with casts to void.
  - wvenable 16y ago
    What's with all the unused function parameters? Shouldn't that be an edge case?
    - tedunangst 16y ago
      Just about every callback interface takes a function pointer where the function expects a void * argument. Half the time it's not needed.
      - kelnos 16y ago
        If the void * is the last parameter, just leave it off your implementation of the callback function.
- neilc 16y ago
  Any halfway decent software shop will have a policy of compiling with all warnings enabled FWIW, your definition of "halfway decent" would exclude most software shops in the world. It is extremely unprofessional to disable or ignore warnings such as the "unused function parameter" warning Unused function parameter warnings are often noise (e.g., due to #ifdefs, unimplemented APIs, backward compatible APIs, etc). I agree that code should compile w/o warnings, but the benefits of tracking down and squelching unused function argument warnings are pretty marginal in my experience.
  - tptacek 16y ago
    All I can say is than in 15 years (fuck.) I've never worked on a team that would have caught a bug like this in dead code. -Wall has been the gold standard on the teams I've worked on.
    - josephruscio 16y ago
      -Wall unfortunately leaves a lot of warnings disabled. We've used: "-W -Wall -Wcast-align -Wstrict-prototypes -Wmissing -prototypes -Wpointer-arith -Wshadow -Wsign-compare -Wformat=2 -Wno-format-y2k -Wimplicit -Wmissing-braces -Wnested-externs -Wparentheses -Wtrigraphs" Update: Forgot the most important one ... -Werror. Making warnings equivalent to errors is the only way to ensure that they're always taken care of.
    - renata 16y ago
      -Wall doesn't enable -Wunused-parameter. You need -Wall -Wextra for that.
  - humbledrone 16y ago
    I would probably agree that most software shops in the world are not halfway decent. The benefits of tracking down and squelching such warnings would have very likely caused this bug to never exist. The point is that you make "compiles without warnings" a policy, and you build a habit of maintaining that policy -- after a while it begins to take almost no effort at all.

DrSprout 16y ago

You have to know that -Wextra exists, which I for one didn't until just now when I ran a sample program. All my projects just compiled with -Wall, which doesn't catch that error. Also, unless every programmer on the project is being scrupulous about finding and correcting such errors, it quickly becomes line noise, and a lot of it completely unimportant stuff.

humbledrone 16y ago
Yeah, that's why every programmer on the project has to be scrupulous about finding and correcting such errors. That's professionalism 101 -- if your team is not keeping the build warning free, things like this slip through.
- tptacek 16y ago
  Things like what, a blatantly obvious bug that would crash virtually every C program running on the system were it not for the fact that it was dead code? This is practically a documentation bug. [edit] I kind of take it back, after mahmud's bzero() joke... out of oniguruma, Python, hexfiend, Apache, nginx, openssl, redis, regexkit, saxon, memcached, subversion, and valgrind, this hypothetical bug would have broken Python, redis, subversion, several apache modules, and memcached. The contents of my codebase/3p directory, FWIW.
seliopou 16y ago
-Wextra is total insanity. I just compiled some C code I've been working on with that flag, and got the following: "warning: signed and unsigned type in conditional expression." I was testing if a variable I declared as an integer was less than 0. To my knowledge, every number literal in C is assumed to be of a signed type unless followed by a 'u'. But now I know better. 0 is the exception.
- swolchok 16y ago
  Are you sure? $ echo 'int main(int argc, char **argv) { if (argc < 0) { return 1; } else {return 0;}}' > test.c $ gcc -v Using built-in specs. Target: x86_64-linux-gnu Configured with: ../src/configure -v --with-pkgversion='Ubuntu 4.4.3-4ubuntu5' --with-bugurl=file:///usr/share/doc/gcc-4.4/README.Bugs --enable-languages=c,c++,fortran,objc,obj-c++ --prefix=/usr --enable-shared --enable-multiarch --enable-linker-build-id --with-system-zlib --libexecdir=/usr/lib --without-included-gettext --enable-threads=posix --with-gxx-include-dir=/usr/include/c++/4.4 --program-suffix=-4.4 --enable-nls --enable-clocale=gnu --enable-libstdcxx-debug --enable-plugin --enable-objc-gc --disable-werror --with-arch-32=i486 --with-tune=generic --enable-checking=release --build=x86_64-linux-gnu --host=x86_64-linux-gnu --target=x86_64-linux-gnu Thread model: posix gcc version 4.4.3 (Ubuntu 4.4.3-4ubuntu5) $ gcc -Wall -Wextra test.c test.c: In function ‘main’: test.c:1: warning: unused parameter ‘argv’ $ gcc-4.3.4 behaved the same way in my test.

klodolph 16y ago

"Pretty severe" lack of professionalism? It's not unprofessional not to turn the warnings all the way up and sift through each and every one. Some of us work code inherited from other people with different coding guidelines, and some of the warnings will generate loads of messages for code that is not only technically correct but stylistically correct as well. The unused parameter warning is one of the biggest offenders in this area. And if you have a hundred warnings for unused parameters, which ones do you pay attention to? Another potential issue is that the code in the patch might almost never be called in the first place. Most libc implementations (three that I know of, where I've examined the "memset" source) have a "memset" like that as a fallback only for platforms without an assembly version. I'd say the problem is that there are several different ways this error could have been detected sooner: warnings, test, static analysis tools, and review, but none of them worked. But again, it's also possible that the code is never called because assembly versions are always available.

humbledrone 16y ago
So what if the code doesn't get called? That doesn't prevent it from being built. Considering that memset() is part of libc, which is a shared library, presumably it's going to get compiled regardless of whether it's called.
queensnake 16y ago
| The unused parameter warning is one of the biggest offenders in this area. And if you have a hundred warnings for unused parameters, which ones do you pay attention to? Well, so then you're exposed to this error. Do: void foo(int x) { (void)x; } to silence unused parameter warnings.
- tptacek 16y ago
  Go look at Google code search, which includes vast quantities of code written by professional teams and (often later) open sourced. Very little of it casts expressions to (void). When I started at Arbor, I got rid of all the (void) casts. What an eyesore. Very little C code is const-correct either. Most of the C devs I know make fun of const-correctness.

jmillikin 16y ago

The "unused parameter" error is one of the most useless of C compiler errors. There are dozens of perfectly valid reasons why a parameter would go unused, so the occasional error just gets lost in the noise. It's more helpful in C++, which can omit parameter names to indicate that it's intentionally unused. But then you're using C++; not worth the tradeoff.

humbledrone 16y ago
It's not hard to type a statement like "(void)v;" to explicitly say, "v is intentionally unused." I fail to see the drawback of this approach. Of course you should only do this when (1) you can't remove the parameter from the function's prototype and (2) there is a good reason for it to be unused. But once you develop that habit, the compiler can help you catch bugs that are otherwise easy to miss. Sounds like a worthwhile tradeoff to me.
- tedunangst 16y ago
  Then you get a "statement with no effect" warning, which is usually more serious. I'll take a bogus warning in real code over a bogus warning in contrived code any day.
  - cperciva 16y ago
    I've never seen a compiler issue a warning about "(void)foo;". Even compilers which issue 'statement with no effect' warnings normally recognize deliberate casts to void as a way of saying "shut up, I know what I'm doing".
- leif 16y ago
  What you want is __attribute__((unused)), assuming you're compiling with gcc.
torpor 16y ago
>There are dozens of perfectly valid reasons why a parameter would go unused, so the occasional error just gets lost in the noise. Such as?