On Fri, Oct 21, 2011 at 09:37:02AM +0200, Paolo Bonzini wrote: > On 10/21/2011 02:26 AM, Stuart Brady wrote: > >>> They all look okay, perhaps the include path you passed to > >>> Coccinelle is incomplete? > >Ah, good point! I'm not sure what include dirs are needed, though... > >anyone have any advice? > > > >Blue Swirl, I gather you're one of the few other people to have used > >Coccinelle with Qemu's source... > > I played a bit yesterday and it turns out that Coccinelle is a bit > limited WRT handling headers, because they are very expensive. I > used "-I . -I +build -I hw" but it didn't help much. > > Stuart/Blue, do you have a macro file? Mine was simply "#define > coroutine_fn".
I didn't even have that, but Coccinelle didn't seem to mind... It did occur to me that since a lot of Qemu's source is recompiled with different macro definitions for different targets, we need to be really careful about what we do regarding includes. Hopefully the names of types that are used won't vary between targets, though. Submitting what Coccinelle could process successfully and fixing up the rest manually seemed reasonable, but I'd like to be as confident as possible of these changes. BTW, I'd thought that noone would ever do E = (T *)g_malloc(sizeof(*E)), but from looking hw/blizzard.c, hw/cbus.c and hw/nseries.c, it seems that this isn't quite the case afterall! I'll be sure to include this in my second attempt, once QEMU 1.0 has been released. One thing that did not occur to me is use of E = malloc(sizeof(*E1)) or E = malloc(sizeof(T1)) where E is of type void *, but E1 or T1 is not what was intended. I'm also somewhat astonished to find that sizeof(void) and sizeof(*E) where E is of type void * both compile! It would probably make sense to check for these. Any remaining calls to g_malloc() would be then be reviewed to make sure that they're all correct. We could also perhaps search for places where free() is called on memory that is allocated with g_malloc(), as g_free() should be used instead. --- Some background on my thinking before sending the patch series: (T *)g_malloc(sizeof(T)) can obviously be safely replaced with g_new(T, 1) since that's what g_new(T, 1) expands to. Replacing E = g_malloc(sizeof(*E)) with E = g_new(T, 1) adds a cast, but the cast does not provide any extra safety, since sizeof(*T) is pretty much certain to be the correct size (unless T = void *). There seems to be some agreement that this is more readable, though. Replacing E = g_malloc(sizeof(T)) without a cast with E = g_new(T, 1) effectively just adds a cast to T *, which might result in additional compilation warnings (which are turned into errors) but should have no other effect, so this should be perfectly safe. Other cases where g_malloc(sizeof(*E)) or g_malloc(sizeof(T)) is used will either be due to Coccinelle not understanding the types, or due to a bug in Qemu, and both of these cases need special consideration. Cheers, -- Stuart
