Live data from Hacker News

Android Project Changeset 4f8b683: libc/memset.c

review.source.android.com

71–80 of 93 posts

Re: Android Project Changeset 4f8b683: libc/memset.c

#71

Interestingly, they have the OpenBSD version that does word-size assignments in asm for x86/arm. wonder why it wasn't being used. http://android.git.kernel.org/?p=platform/bionic.git;a=blob;... http://android.git.kernel.org/?p=platform/bionic.git;a=blob;...

It was. That's why this bug hasn't been caught before: the buggy code was never actually compiled as part of a production build. Real builds always use the version optimized for the actual hardware platform.

Re: Android Project Changeset 4f8b683: libc/memset.c

#72
post #30
post #24

Earlier quoted context omitted.

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 ag…

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.

-Wall doesn't enable -Wunused-parameter. You need -Wall -Wextra for that.

Re: Android Project Changeset 4f8b683: libc/memset.c

#73
post #25

Earlier quoted context omitted.

memset sets a block of memory to a given value. It is often used to initialize an uninitialized block of memory to 0. So often, in fact, that this bug where it always initialized the memory to 0 survived so long.

on Google code search: "lang:c memset\(.+,\s [^0]+\s ,.+\)" return 32k results and "lang:c memset\(.+,\s 0+\s ,.+\)" returns 335k.

10% is a lot more than I would have thought, wow. Good job looking that up.

Re: Android Project Changeset 4f8b683: libc/memset.c

#74

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.

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.

>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?

Re: Android Project Changeset 4f8b683: libc/memset.c

#75
post #56
post #31

Earlier quoted context omitted.

I'll offer the tip that even if you have a fast memset, you can and should avoid using it when possible. Seeing memset is usually a sign that someone is being lazy about watching the length of their buffers and/or verifying their copies aren't off-by-one.

If I understand you correctly, you think programmers should say: int i; for (i = 0; i Instead of: memset(buffer, 0, SIZE) Because it indicates better buffer management.

If I understand hoelle's point, it is that you shouldn't need to fill the buffer with a constant value in the first place. Why do people do it? The only time you generally need to prefill something is if you're using some algorithm whose initial state includes a prefilled array. It's hard to find a place where a byte array needs to be memset.

If you need an array zeroed on first use, why didn't you use calloc? The only reason memset has for existing is when you need to reinitialize an existing array to zero. If things are so tight that you're wiping out an existing array and reusing it, which usually involves being deliberately less abstract than you'd naturally be, then, well, that's a pretty unusual situation.

(While we're on memset, I'd like to point out that the weirdest thing is that while memset initializes value byte by byte, the places where you do need a prefilled array are always places where the array is an array of larger values.)

Re: Android Project Changeset 4f8b683: libc/memset.c

#76

Earlier quoted context omitted.

| 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.

(void)x does not work on all compilers to silence the warning. gcc + cl != C.

What's cl?

Re: Android Project Changeset 4f8b683: libc/memset.c

#78
post #74

Earlier quoted context omitted.

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.

>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?

I can only think of a few, but they're valid:

1. An API-defined callback signature where you need to use (for example) the first and third parameters, and not the second.

2. A public API function that has become deprecated and the previous behavior is emulated in a way that doesn't require all the parameters.

3. Stub/wrapper functions.

4. Init/cleanup functions for modules written to a plugin API, when all parameters passed are not needed in all cases.

5. Language binding closure/thunk functions.

Most of the uses, I think, boil down to backwards compat and fixed callback/plugin interfaces. In those cases I think an __attribute__((unused)) or void cast aren't a huge burden. I usually turn on -Wunused-parameter just in case.

Re: Android Project Changeset 4f8b683: libc/memset.c

#79

Earlier quoted context omitted.

What's with all the unused function parameters? Shouldn't that be an edge case?

Just about every callback interface takes a function pointer where the function expects a void * argument. Half the time it's not needed.

If the void * is the last parameter, just leave it off your implementation of the callback function.

Re: Android Project Changeset 4f8b683: libc/memset.c

#80

Now this is hacker news. We don't see too many of these types of posts.

No, this is The Daily WTF. "Oh look, someone wrote some really stupid code!" Who cares? How does it enrich us to know that someone mis-implemented memset() in a bootloader that possibly never even calls it, or if it does, probably passes zero as the argument anyway? (Or is dead code because on any arch anyone uses, memset() is implemented in asm.)
Post reply on HN