On Tuesday 27 February 2007, Maynard Johnson wrote:
> I have applied the "cleanup" patch that Arnd sent, but had to fix up a
> few things:
> - Bug fix: Initialize retval in spu_task_sync.c, line 95, otherwise
> OProfile this function returns non-zero and OProfile fails.
> - Remove unused codes in include/linux/oprofile.h
> - Compile warnings: Initialize offset and spu_cookie at lines 283
> and 284 in spu_task_sync.c
>
> With these changes and some userspace changes that were necessary to
> correspond with Arnd's changes, our testing was successful.
>
> A fixup patch is attached.
>
The patch does not contain any of the metadata I need to apply it
(subject, description, signed-off-by).
> @@ -280,8 +280,8 @@ static int process_context_switch(struct
> {
> unsigned long flags;
> int retval;
> - unsigned int offset;
> - unsigned long spu_cookie, app_dcookie;
> + unsigned int offset = 0;
> + unsigned long spu_cookie = 0, app_dcookie;
> retval = prepare_cached_spu_info(spu, objectId);
> if (retval)
> goto out;
No, this is wrong. Leaving the variables uninitialized at least warns
you about the bug you have in this function: when there is anything wrong,
you just continue writing the record with zero offset and dcookie values
in it. Instead, you should get handle the error condition somewhere
down the code.
It's harmless most of the time, but you really should not be painting
over your bugs by blindly initializing variables.
> diff -paur linux-orig/include/linux/oprofile.h linux-new/include/linux/oprofile.h
> --- linux-orig/include/linux/oprofile.h 2007-02-27 14:41:29.000000000 -0600
> +++ linux-new/include/linux/oprofile.h 2007-02-27 14:43:18.000000000 -0600
> @@ -36,9 +36,6 @@
> #define XEN_ENTER_SWITCH_CODE 10
> #define SPU_PROFILING_CODE 11
> #define SPU_CTX_SWITCH_CODE 12
> -#define SPU_OFFSET_CODE 13
> -#define SPU_COOKIE_CODE 14
> -#define SPU_SHLIB_COOKIE_CODE 15
>
> struct super_block;
> struct dentry;
>
Right, I forgot about this.
Arnd <><