|
From: <sv...@va...> - 2007-11-02 04:12:49
|
Author: njn Date: 2007-11-02 04:12:48 +0000 (Fri, 02 Nov 2007) New Revision: 7080 Log: Fix 64-bit Massif breakage, caused by problems with integer arithmetic on values of different signs and sizes that only a C language lawyer would spot. Modified: trunk/massif/ms_main.c Modified: trunk/massif/ms_main.c =================================================================== --- trunk/massif/ms_main.c 2007-11-01 18:58:46 UTC (rev 7079) +++ trunk/massif/ms_main.c 2007-11-02 04:12:48 UTC (rev 7080) @@ -395,7 +395,11 @@ } static Bool clo_heap = True; -static UInt clo_heap_admin = 8; + // clo_heap_admin is deliberately a word-sized type. At one point it was + // a UInt, but this caused problems on 64-bit machines when it was + // multiplied by a small negative number and then promoted to a + // word-sized type -- it ended up with a value of 4.2 billion. Sigh. +static SizeT clo_heap_admin = 8; static Bool clo_stacks = False; static UInt clo_depth = 30; static UInt clo_threshold = 100; // 100 == 1% |
|
From: Oswald B. <os...@kd...> - 2007-11-02 08:51:39
|
On Fri, Nov 02, 2007 at 04:12:50AM +0000, sv...@va... wrote: > Author: njn > -static UInt clo_heap_admin = 8; > + // clo_heap_admin is deliberately a word-sized type. At one point it was > + // a UInt, but this caused problems on 64-bit machines when it was > + // multiplied by a small negative number and then promoted to a > + // word-sized type -- it ended up with a value of 4.2 billion. Sigh. > +static SizeT clo_heap_admin = 8; > this is really odd ... the code in update_heap_stats() did this: (unsigned long) = (unsigned long) + (unsigned int) * (signed int); while the actual multiplication is signed, the result is taken to be unsigned and is accordingly zero-extended in the promotion. you converted this to (unsigned long) = (unsigned long) + (unsigned long) * (signed int); which is about the right thing to do. however, for *way* more clarity and less bloat i'd suggest writing the actual expression as update_alloc_stats(heap_szB_delta + (SizeT)clo_heap_admin*n_heap_blocks_delta); instead of making the variable bigger. btw, in ms_post_clo_init() you'll get a warning, because, duh, an unsigned value cannot be smaller than zero. -- Hi! I'm a .signature virus! Copy me into your ~/.signature, please! -- Chaos, panic, and disorder - my work here is done. |
|
From: Nicholas N. <nj...@cs...> - 2007-11-02 21:00:32
|
On Fri, 2 Nov 2007, Oswald Buddenhagen wrote: > this is really odd ... That's what I thought :) > the code in update_heap_stats() did this: > (unsigned long) = (unsigned long) + (unsigned int) * (signed int); > while the actual multiplication is signed, the result is taken to be > unsigned and is accordingly zero-extended in the promotion. > you converted this to > (unsigned long) = (unsigned long) + (unsigned long) * (signed int); > which is about the right thing to do. however, for *way* more clarity > and less bloat i'd suggest writing the actual expression as > update_alloc_stats(heap_szB_delta + (SizeT)clo_heap_admin*n_heap_blocks_delta); > instead of making the variable bigger. I thought about following your suggestion, but your solution only fixes that particular expression, whereas making it a SizeT avoids the possibility of it happening elsewhere. Thanks for the explanation, that's basically what I thought was happening but you explained it more clearly than I had understood it in my head. Nick |
|
From: Oswald B. <os...@kd...> - 2007-11-02 21:07:28
|
On Sat, Nov 03, 2007 at 07:59:38AM +1100, Nicholas Nethercote wrote: > On Fri, 2 Nov 2007, Oswald Buddenhagen wrote: >> for *way* more clarity >> and less bloat i'd suggest writing the actual expression as >> update_alloc_stats(heap_szB_delta + (SizeT)clo_heap_admin*n_heap_blocks_delta); >> instead of making the variable bigger. > > I thought about following your suggestion, but your solution only > fixes that particular expression, whereas making it a SizeT avoids the > possibility of it happening elsewhere. > hmm, but by this logic you have to promote every single variable that is ever used in an offset or size calculation to [S]SizeT, no matter how oversized it seems ... i guess one simply has to keep such issues in mind when doing C. probably *the* reason why Java simply dropped unsigned types. -- Hi! I'm a .signature virus! Copy me into your ~/.signature, please! -- Chaos, panic, and disorder - my work here is done. |