RE: exit vs. kill on ARM

["Dave Korn" <dave dot korn at artimi dot com>]

----Original Message----
>From: Shaun Jackman
>Sent: 19 July 2005 01:04

> ARM has only one SWI for both _exit and _kill. To give the SWI handler
> (i.e. the kernel) a hope of differentiating the two calls, this patch
> modifies _exit to call the SWI with the second argument set to -1,
> which is an invalid value for signum. This does not affect the RDI
> implementation, since it discards both its arguments anyways.

  I don't know ARM, so this is totally off the top of my head, but ..... is
there a SIGKILL that might be appropriate instead?  If you're running under
GDB or another debugger, wouldn't using it make the debugger DTRT?
 
> This patch also removes _raise. 

  Generally patches should serve one purpose, and something like this should
be submitted separately.  Suppose you were wrong about _raise and someone
needed to revert your patch in CVS in a hurry; it'd be a lot better if it
didn't un-do the _exit/_kill change at the same time.

>  My grepping of the code has shown that
> newlib calls _raise_r, which is implemented in
> newlib/libc/signal/signal.c, but never _raise.

  Also, grepping can be very misleading in a lot of the src/ sources.  Huge
wads of code are generated at either configure or compile time, so you often
need to fully build the software and then grep in the build dir as well; and
in other places, a lot of function calls are obscured by macros and/or token
pasting; for example, trying to find where a function is defined in bfd/ by
grepping is practically impossible.

  This is a general point about the hazards of using grep to try and
determine code coverage, which is not really what grep does!  I don't know
to what extent it may apply to newlib in particular.  At least the code
still links, and we know your change shouldn't affect anything but ARM.  How
many different ARM targets are there?  How many did you try building?  Is
there a non-reentrant newlib build that might want _raise instead of
_raise_r?

  Oh, there's one other very minor point to bear in mind in future patches.
In your patch you've moved the location of _exit from before _kill to after
it.  Given that all the functions are prototyped at the top of the file,
there's no need to do so simply because _exit now calls _kill.  If you had
changed the function definition in place, you get a more minimal delta in
the CVS; the first and last lines of the function would be in the same
place.  Look at the diff and compare the way the before-and-after changes
are immediately visible in _kill, as compared to the way you have to glance
backward and forward or even scroll up and down between the before-and-after
versions of _exit and try and hold one in mind while you compare it to the
other.  It's also better CVS practice; keeping diffs localized and as
minimal as possible reduces the burden on the server, it makes cvs diff
across widely spaced revisions more useful, and reduces the risk of
conflicts when merging or branching is in play.

    cheers,
      DaveK
-- 
Can't think of a witty .sigline today....