[Cbe-oss-dev] [PATCH] libspe, libspe2: remove CHECK_nnn_OPCODE to fix waitpid

[Patrick Mansfield]

On Tue, May 15, 2007 at 01:14:46PM +0900, Kazunori Asayama wrote:
> Patrick Mansfield <patmans at us.ibm.com> wrote:
> > The waitpid handler is checking for the wrong opcode, it should be
> > CHECK_POSIX1_OPCODE(WAITPID) not CHECK_POSIX1_OPCODE(WAIT).
> > 
> > That could be simply fixed, but on failure returning 1 from the handler
> > does not do anything - neither the return code (rc) nor errno are set -
> > and the SPE/newlib code ends up with whatever values were passed to the
> > assist call (garbage data for errno, and whatever the first argument was
> > for the rc). There is no common rc value that can be used as the rc, like
> > -1 (for the C99 or POSIX functions).
> > 
> > So just remove the opcode checks both CHECK_C99_OPCODE and
> > CHECK_POSIX1_OPCODE from libspe and libspe2.
> 
> The checks are helpful to detect bugs when modify function pointer
> tables (default_c99_funcs[] and default_posix1_funcs[]), because the
> tables can be easily broken.
> 
> So I'd like to leave the checks or to have more 'robust' initializers
> of the tables. I propose two options:
> 
>   a. add a new argument which means 'error rc', to CHEKC_*_OPCODE macros:
> 
> #define CHECK_C99_OPCODE(_op, __error_rc)                               \
> 	if (SPE_C99_OP(opdata) != SPE_C99_ ## _op) { 			\
> 		DEBUG_PRINTF("OPCODE (0x%x) mismatch.\n", SPE_C99_OP(opdata)); 	\
> 		PUT_LS_RC(__error_rc, 0, 0, ENOSYS);			\
> 		return 1;                                       	\
> 	}

I don't like this, you still don't know if the _op is wrong or if the
wrong function was called. The SPE context should probably die if they
don't match (similar to the handling of a bad/missing callback or opcode).

>   b. remove the opcode checks and modify the initializers:
> 
> int (*default_c99_funcs[SPE_C99_NR_OPCODES]) (char *, unsigned long) = {
> 	[SPE_C99_CLEARERR]	= default_c99_handler_clearerr,
> 	[SPE_C99_FCLOSE]	= default_c99_handler_fclose,
> 
> What do you think ?

b looks OK. The enum/defines aren't even used if we don't have this
change, and the values could be comments, and then we could have that much
less C code. But I can implement b.

Is that fine with everyone?

Do you want a separate patch, or repost with the initializer changed? 

If repost is it OK to change only libspe2?

-- Patrick Mansfield