|
From: Josef W. <Jos...@gm...> - 2004-03-15 17:57:32
Attachments:
vgstore.patch
|
Oops, here is the patch... Josef |
|
From: Julian S. <js...@ac...> - 2004-03-15 20:55:54
|
> Can somebody (Jeremy?) review it and commit it if it is OK? Just discovered "Kompare". Nice tool! At least in the details, it looks OK. I am a somewhat concerned about the type system abuse that's already there, though (nothing to do with your patch). static void emit_movb_reg_regmem ( Bool bounds, Int reg1, Int reg2 ) First arg is Bool, conceptually 1-bit and in fact unsigned char (see include/vg_skin.h). But then there are calls emit_movb_reg_regmem ( size & DO_BOUNDSCHECK, reg1, reg2 ); This just can't be right, and I'm not even sure what the caller intends here. Not to mention DO_BOUNDSCHECK is 1 << 8 so is zero when interpreted as Bool. If the bounds check always happens for instructions emitted by this call, it's because size just happens to always be 1, 2 or 4 but never zero -- and completely nothing to do with DO_BOUNDSCHECK. I guess the C "type system" (so-called) gets to wear the pointy hat and stand in the corner :-) Anybody know of a way to define a Bool type in C in such a way the compiler can't silently confuse it with any flavour of int? J |
|
From: Jeremy F. <je...@go...> - 2004-03-15 22:31:10
|
On Mon, 2004-03-15 at 13:05, Julian Seward wrote: > First arg is Bool, conceptually 1-bit and in fact unsigned char > (see include/vg_skin.h). But then there are calls > > emit_movb_reg_regmem ( size & DO_BOUNDSCHECK, reg1, reg2 ); > > This just can't be right, and I'm not even sure what the caller > intends here. Yeah, it's a bit of a nasty hack. I snarfed a bit in the "size" parameter to mean "this should be a checked memory operation". This test masks out the size part, and just grabs the flag. The code should probably be something like ((size & DO_BOUNDSCHECK) != 0) to generate a proper boolean result. I guess the cleaner approach is to add a separate flag argument rather than encoding the bounds-checking flag in the size. > Not to mention DO_BOUNDSCHECK is 1 << 8 so is > zero when interpreted as Bool. If the bounds check always happens > for instructions emitted by this call, it's because size just > happens to always be 1, 2 or 4 but never zero -- and completely > nothing to do with DO_BOUNDSCHECK. Well, the expression is evaluated as an int, so you'll get 0x100 or 0x0 as the result. It will also probably be passed as an int; I'm not sure when the bits are masked off (if ever). > I guess the C "type system" (so-called) gets to wear the pointy > hat and stand in the corner :-) Anybody know of a way to define > a Bool type in C in such a way the compiler can't silently confuse > it with any flavour of int? Not really. C has a boolean type, and it's called "int". The interpretation is 0 = False, non-0 = True. The cleanest way of making a "proper" boolean type would be to use an enum, but C still lets you easily cast between int and enum types. J |
|
From: Jeremy F. <je...@go...> - 2004-03-15 22:50:16
|
On Mon, 2004-03-15 at 09:56, Josef Weidendorfer wrote: > here is the patch... That looks simple enough. The really simple approach would be to just set VG_(clo_pointercheck) = False in SK_(post_clo_init). Since cachegrind is intended for basically working programs, wild pointers shouldn't be a huge problem... J |