[m-rev.] diff/review: deep profiler enhancements

[Fergus Henderson]

On 03-Jul-2001, Zoltan Somogyi <zs at cs.mu.OZ.AU> wrote:
> Index: deep_profiler/exclude.m
> +% Author: zs.
> +%
> +% XXX

There should be some comments here - at very least copy the comments
about this file from the log message.

> +:- func even_background = string.
> +:- func odd_background = string.
> +
> +even_background = "rgb(255, 255, 240)".
> +odd_background =  "rgb(240, 240, 255)".

It would be nice to have a couple of comments here indicating what colors
these are, e.g.

	even_background = "rgb(255, 255, 240)". % light magenta
	odd_background =  "rgb(240, 240, 255)". % hot pink

(except that I made up those colors names without regard to the rgb values).

> +++ deep_profiler/timeout.m	2001/06/28 16:04:52
> @@ -43,25 +52,65 @@
>  :- pragma foreign_decl("C",
>  "
>  #include <stdio.h>
> -#include <signal.h>
> +#include <signal.h>	/* for signal numbers */
> +#include <unistd.h>	/* for alarm() */
> +#include <stdlib.h>	/* for atexit() */
...
> +#define	MDPROF_NUM_SIGNAL_NUMBERS	12
> +
> +extern	const int	signal_numbers[MDPROF_NUM_SIGNAL_NUMBERS];
...
>  :- pragma foreign_code("C",
>  "
> +bool	process_is_detached_server = FALSE;
>  char	*MP_timeout_file1;
>  char	*MP_timeout_file2;
>  char	*MP_timeout_file3;
>  
> +/*
> +** SIGALRM alarm signal indicates a timeout. SIGTERM usually indicates the
> +** machine is being shut down. The others are there to catch forceful shutdowns
> +** during development, both intentional ones where the programmer sends the
> +** signal and those caused by bugs in the server code. We would like to include
> +** all catchable, fatal signals in this list, but that set is somewhat OS
> +** dependent. The set here is the widely portable subset.
> +**
> +** We could avoid this problem if we had a version of atexit that executed
> +** its actions even when the program exits after a signal.
> +*/
> +
> +const int	signal_numbers[MDPROF_NUM_SIGNAL_NUMBERS] =
> +{
> +	SIGALRM,
> +	SIGTERM,
> +	SIGHUP,
> +	SIGINT,
> +	SIGQUIT,
> +
> +	SIGILL,
> +	SIGABRT,
> +	SIGIOT,
> +	SIGBUS,
> +	SIGFPE,
> +
> +	SIGSEGV,
> +	SIGPIPE

This set of signals is not portable.  The only ones mandated by the C
standard are SIGABRT, SIGFPE, SIGILL, SIGINT, SIGSEGV, and SIGTERM.
All the others should be inside #ifdefs, e.g.

	#ifdef SIGBUS
		SIGBUS,
	#endif
	#ifdef SIGPIPE
		SIGPIPE,
	#endif

etc.

Get rid of the `#define MDPROF_NUM_SIGNAL_NUMBERS 12'
and replace it with

	extern const int	signal_numbers[];
	extern const int	MDPROF_num_signal_numbers;

and

	#define MDPROF_ARRAY_LEN(a) (sizeof(a)/sizeof((a)[0]))

	const int signal_numbers[] = { ... }
	const int MDPROF_num_signal_numbers = MDPROF_ARRAY_LEN(signal_numbers);

By the way, shouldn't all the externally visible C identifiers in the
deep profiler (e.g. `signal_numbers') have a prefix (e.g. `MDPROF_') on them?

> +	void	(*handler)(void);
...
> +		MR_setup_signal(signal_numbers[i], handler, FALSE,
> +			""Mercury deep profiler: cannot setup signal exit"");

This is a type error; the arguments types to handler() don't
match the types that signal() or sigaction() expects.
It happens to work on many C implementations, but it's not
guaranteed to work.

But the same problem occurs for several other uses of MR_setup_signal()
in the Mercury runtime, in particular the tracer, the time profiler,
and the deep profiler.  So for now just adding an XXX comment would be
fine.

Otherwise that looks fine.  (I only skimmed most of the deep profiling stuff.)

-- 
Fergus Henderson <fjh at cs.mu.oz.au>  |  "I have always known that the pursuit
The University of Melbourne         |  of excellence is a lethal habit"
WWW: <http://www.cs.mu.oz.au/~fjh>  |     -- the last words of T. S. Garp.
--------------------------------------------------------------------------
mercury-reviews mailing list
post:  mercury-reviews at cs.mu.oz.au
administrative address: owner-mercury-reviews at cs.mu.oz.au
unsubscribe: Address: mercury-reviews-request at cs.mu.oz.au Message: unsubscribe
subscribe:   Address: mercury-reviews-request at cs.mu.oz.au Message: subscribe
--------------------------------------------------------------------------