8 ms·
Android Project Changeset 4f8b683: libc/memset.c
- jared314 16y agoCan someone who lives in the world of C fill in the blanks? If this was a major typo, I have done things along those lines.
- justinweiss 16y agomemset 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.
- mey 16y agoWish there was a cross-link to a bug tracking system where this was initially reported.
- pufuwozu 16y agoYou can just search for the change ID: https://review.source.android.com/#change,14699 https://review.source.android.com/#change,14699
- abstractbill 16y agoSo often, in fact, that this bug where it always initialized the memory to 0 survived so long. Given that the bug went undetected for so long, I think you could almost reasonably claim that calling memset with a nonzero parameter is a "corner case" ;)
- Psyonic 16y agoIt is a "corner case," or close to it, but that's not why it wasn't caught here. I don't think this code is ever called, as explained above. It's default code in case there isn't a platform specific version, but there pretty much always is a platform specific version. Link: http://news.ycombinator.com/item?id=1379639 http://news.ycombinator.com/item?id=1379639
- fserb 16y agoon Google code search: "lang:c memset\(.+,\s[^0]+\s,.+\)" return 32k results and "lang:c memset\(.+,\s0+\s,.+\)" returns 335k.
- wlievens 16y ago10% is a lot more than I would have thought, wow. Good job looking that up.
- amock 16y agoThe function was always setting the memory to 0 instead of what was passed in. I think most uses of memset are to initialize memory to 0, but any other use would have had unexpected results.
- draebek 16y agomemset's job is to fill a block of memory with a given byte. The most common fill byte is 0, to initialize or otherwise clear a buffer. It looks like it is so common, in fact, that a programmer accidentally made Android's memset _always_ fill with 0, no matter what byte was passed in, and presumably no one noticed for a while. (I can't figure out when this patch was introduced.)
- jared314 16y agoThe "Up to change" link takes you to the summary. May 13, 2010
- fourneau 16y agoMemset (according to the c++ reference) is supposed to: "Sets the first num bytes of the block of memory pointed by ptr to the specified value (interpreted as an unsigned char)." So, give it a pointer to some memory, give it a value and the number of bytes you want to set and it'll go wild. In this case, memset was just blindly dropping in zeroes into the memory block
- humbledrone 16y agoThe 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 agoThat sounds like a severe lack of perspective.
- humbledrone 16y agoHuh? 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 agoThe 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 agoWhat's with all the unused function parameters? Shouldn't that be an edge case?
- tedunangst 16y agoJust about every callback interface takes a function pointer where the function expects a void * argument. Half the time it's not needed.
- mahmud 16y agoSome libc hacker has fond memories of bzero(3), tsk tsk ;-)
- niallsmart 16y agoIronic that this makes the homepage the same day as an article bashing unit tests :)
- audidude 16y agoInterestingly, 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;f=libc/arch-x86/string/memset.S;h=1059ccc99516e277af6e04ebc7b078c4f37d2e6d;hb=HEAD http://android.git.kernel.org/?p=platform/bionic.git;a=blob;... http://android.git.kernel.org/?p=platform/bionic.git;a=blob;f=libc/arch-arm/bionic/memset.S;h=93abe15a2f9eb647c5d282310f5c7cf241457cd6;hb=HEAD http://android.git.kernel.org/?p=platform/bionic.git;a=blob;...
- fhars 16y agoIt 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.
- pkaler 16y agoForget the typo. This is the slowest, most naive implementation of memset possible. Is this actual shipping code? I've followed the changeset up to the top and it looks like it's in platform/bootable/bootloader/legacy. Hopefully this isn't actually called anywhere, but the review date says May 13th. What's the point of optimizing the Java VM when functions like memset, malloc, memcmp, memcpy, memchr, etc, etc haven't even been optimized for the platform. EDIT: I understand that 95% of your time will be writing in a language like Ruby, Python, Javascript or a C based language. But dear god, please add Agner Fog's Optimization Manuals to your reading list. http://www.agner.org/optimize/ http://www.agner.org/optimize/
- neilc 16y agoGiven that it is in "platform/bootable/bootloader/legacy", I'm not sure there is much point getting all worked up about performance -- presumably something much more efficient would be used for the vast majority of cases.
- joezydeco 16y agoI haven't updated my source tree in a long time, but the ARM devices were using bionic/libc/arch-arm/bionic/memset.S. This is pure ARM assembly optimized for the pipeline and cache. My ARM assembly is a little rusty, but it looks like they implement memset and bzero, the latter just calling memset with R1=0.
- hoelle 16y agoI'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.
- scott_s 16y agoIf I understand you correctly, you think programmers should say: int i; for (i = 0; i < SIZE; ++i) { buffer[i] = 0; } Instead of: memset(buffer, 0, SIZE) Because it indicates better buffer management.
- spolsky 16y agoAren't there are processor-specific assembler versions of memset for common chips? There are usually assembler versions (for example using REP MOVS on Intel which is light years faster) or even unrolled C versions (typically setting 8 or 16 bytes per iteration) that are used. I suspect this might be "fallback code" that doesn't actually get executed on any chips that anybody really uses.
- spolsky 16y agoMaybe this is a sign that someone is porting android to a new CPU!
- faragon 16y agoFor this case, the required x86 opcode could be "rep stosX" (just write), and not "rep movsX" (copy). Anyway, on x86 processors, "rep movsX" was faster from 8086 to 80386, but since the 80486, it is faster to do assembly unrolling for reducing the jump penalty (as the "rep movsX" doesn't do unrolling, being its conditional jump a penalty). On CPUs with prefetch hardware support -e.g. SSE2-, you could still speed it up a bit more, by fetching/ordering future cache line before needed, avoiding pipeline stalls. The shown example, that just writes to RAM, is probably just fast enough (it is harder to optimize read cases), although a bit of unrolling could reduce jump penalty impact (specially on non superscalar or in in-order superscalar CPUs -typical ARM included on handheld devices-).
- chadaustin 16y agoWhile we're jumping down the architecture pedant rabbit hole, a simple loop like that will be trivially predicted, so the branch will be basically free. In addition, hardware prefetchers do a much better job at predicting linear memory access than manual prefetch instructions. On Core 2, iirc, if you have 2 or 3 L2 misses at fixed offsets either direction from each other, the hardware will automatically begin prefetching memory so it's there when you need. The problem with manual prefetch instructions is there high latency. They're best for hinting to the processor that you're about to make an unpredictable load.
- jacquesm 16y agoThis is a typical cut-and-paste programmer bug, the person took 'memzero', copied it and tried turning it in to memset. Instead of special casing memzero as memset(p,0,count);
- snowbird122 16y agoNow this is hacker news. We don't see too many of these types of posts.
- kelnos 16y agoNo, 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.)
- snowbird122 16y agoIt enriched me to learn what memset was. I love hacker news, but I wouldn't mind a few more posts that contain some code.
- jwr 16y agoThis bug might actually be difficult to find — 99% of the time memset is asked to fill the memory with 0.