[m-dev.] for review: use Win32 exceptions in the runtime

[Fergus Henderson]

On 27-Jun-2000, Peter Ross <petdr at cs.mu.OZ.AU> wrote:
> On Tue, Jun 27, 2000 at 10:56:20AM +1000, Fergus Henderson wrote:
> > Where does MR_WIN32 get defined?
> > 
> It is defined in mercury_conf.h, which is generate by configure.

Ah, I see.  Thanks.

I think the documentation of MR_WIN32 is not clear.

For example, if MS port MSVC++ to Linux, should MR_WIN32
be defined when using the Linux version of MSVC++?

> > > +#ifdef MR_PROTECTPAGE
> > > +
> > > +  #ifdef MR_WIN32
> > > +    #define PROT_NONE  0x0000
> > > +    #define PROT_READ  0x0001
> > > +    #define PROT_WRITE 0x0002
> > > +  #endif
> > 
> > That could cause problems on systems that offer both Win32 and Unix APIs,
> > and thus define MR_WIN32 but also include a definition of PROT_NONE in
> > the system header files.  I suggest using MR_PROT_NONE etc.
> > and defining MR_PROT_NONE as PROT_NONE on systems for which MR_WIN32
> > is not defined.
> 
> If PROT_* are not defined, I define them as above, this is all that
> needs to be done.

But the above definitions are not protected by `#ifndef'.
So you define PROT_* regardless of whether or not they are already defined.
This might cause problems on systems that define both MR_WIN32 and PROT_NONE.
I suppose you could change them to

	#ifdef MR_PROTECTPAGE
	
	  #ifdef MR_WIN32
	    #ifndef PROT_NONE
	      #define PROT_NONE  0x0000
	    #endif
	    #ifndef PROT_READ
	      #define PROT_READ  0x0001
	    #endif
	    #ifndef PROT_WRITE
	      #define PROT_WRITE 0x0002
	    #endif
	  #endif

and that would be a bit safer.  Oh, wait, I see from the diff that is
exactly what you did.  Still, even then, there might be problems if
those macros were not defined at this point because the appropriate
system header file has not been included, but that system header file
was then included later on in the compilation unit.  If that were to
happen, you might get errors in the system header file because
PROT_NONE etc. were already defined.

> +++ runtime/mercury_conf_param.h	Tue Jun 27 14:15:52 2000
> @@ -345,12 +345,28 @@
>    #define MR_CAN_GET_PC_AT_SIGNAL
>  #endif
>  
> +        /* We can check for overflow using mprotect like functionality */
>  #if (defined(HAVE_MPROTECT) && defined(HAVE_SIGINFO)) || defined(MR_WIN32)
>    #define MR_CHECK_OVERFLOW_VIA_MPROTECT
>  #endif
>  
> +        /* The functionality of mprotect can be provided */
>  #if defined(HAVE_MPROTECT) || defined(MR_WIN32)
>    #define MR_PROTECTPAGE
> +#endif

The comments here are not as clear as they could be.
It's not entirely clear whether the comment is describing
the code that follows, or whether it is defining the meaning
of the macro.  It would be clearer if the documentation were
in the same format as that for e.g. MR_NEED_INITIALIZATION_AT_START.

> +#if defined(MR_WIN32)
> +  #define MR_WIN32_STRUCTURED_EXCEPTIONS
> +#endif

I think you should document the meaning of that configuration macro.

> +/*---------------------------------------------------------------------------*/
> +
> +/*
> +** Win32 API specific.
> +*/
> +#if defined(MR_WIN32)
> +  #define MR_WIN32_GETSYSTEMINFO	/* to determine page size */
> +  #define MR_WIN32_VIRTUAL_ALLOC	/* to provide mprotect functionality */
>  #endif

I think you should document the meaning of those configuration macros.
You've documented why they're used, but you haven't documented
what they mean.

> +++ runtime/mercury_memory_handlers.c	Tue Jun 27 14:08:08 2000
> @@ -554,7 +553,7 @@
>  
>  #endif /* not HAVE_SIGINFO_T && not HAVE_SIGCONTEXT_STRUCT */
>  
> -#ifdef MR_WIN32
> +#ifdef MR_WIN32_STRUCTURED_EXCEPTIONS
>  #include <tchar.h>

Hmm, here you're assuming that MR_WIN32_STRUCTURED_EXCEPTIONS
implies that we have <tchar.h>.  Is that necessary?
What are you using <tchar.h> for?  Is it just the `_T' macro?
What exactly does that do, and why do you need it?

If you really do need <tchar.h>, it would be a good idea to document
it in the documentation for MR_WIN32_STRUCTURED_EXCEPTIONS.

> @@ -600,61 +599,62 @@
>  ** Retrieve the name of a Win32 exception code as a string
>  */
>  static
> -LPTSTR MR_FindExceptionName(DWORD ExceptionCode)
> +LPTSTR MR_find_exception_name(DWORD exception_code)

The return type should be on the same line as `static'.

Is there any reason to use LPTSTR rather than `const char *' here?
Note that the code elsewhere assumes that LPTSTR can be printed
with `printf("...%s...", ...)'.

>  {
>  	int i;
> -	for (i = 0; i < sizeof(MR_ExceptionNames)
> -			/ sizeof(MR_ExceptionName); i++)
> -	{
> +	for (i = 0; i < sizeof(MR_exception_names)
> +			/ sizeof(MR_ExceptionName); i++) {

Since the `for' doesn't fit on one line, the `{' should go
on a line of its own.

> @@ -673,28 +672,25 @@
>  						(void *) zone->redzone,
>  						(void *) zone->top);
>  
> -				if ((zone->redzone <= lpAddress) &&
> -						(lpAddress <= zone->top))
> -				{
> +				if ((zone->redzone <= address) &&
> +						(address <= zone->top)) {

Likewise here.

> @@ -673,28 +672,25 @@
>  					fprintf(stderr,
>  						"\n***     Address is within"
>  						" redzone of "
>  						"%s#%d (!!zone overflowed!!)\n",
>  						zone->name, zone->id);
> -				}
> -				else if ((zone->bottom <= lpAddress) &&
> -						(lpAddress <= zone->top))
> -				{
> +				} else if ((zone->bottom <= address) &&
> +						(address <= zone->top)) {

And here.

> @@ -738,15 +734,13 @@
>  ** If TRUE, lpAddress is set to the accessed address and
>  ** lpAccessMode is set to the desired access (0 = read, 1 = write)
>  */
> -BOOL MR_ExceptionRecordIsAccessViolation(EXCEPTION_RECORD *Rec,
> -		void **lpAddress, int *lpAccessMode)
> +bool MR_exception_record_is_access_violation(EXCEPTION_RECORD *rec,
> +		void **address, int *access_mode)

It might be clearer to make that `address_ptr' and `access_mode_ptr' here.
(Sorry, I should have said that first time around.)
Also you need to fix the comment, which still refers to `lpAddress'
and `lpAccessMode'.
In function definitions, the return value (e.g. `bool' in this case)
should be on a line of its own.

> +++ runtime/mercury_memory_zones.h	Tue Jun 27 14:08:08 2000
> -int ProtectPage(void *Addr, size_t Size, int ProtFlags);
> +int MR_protect_pages(void *Addr, size_t Size, int ProtFlags);

s/Addr/addr/
s/Size/size/
s/ProtFlags/prot_flags/

> Index: mercury_conf_param.h
> @@ -547,6 +552,266 @@
>  }
>  
>  #endif /* not HAVE_SIGINFO_T && not HAVE_SIGCONTEXT_STRUCT */
> +
> +#ifdef MR_WIN32_STRUCTURED_EXCEPTIONS
> +#include <tchar.h>
> +
> +/*
> +** Exception code and their string representation
> +*/
> +#define DEFINE_EXCEPTION_NAME(a)   {a,_T(#a)}

I suggest s/_T(#a)/#a/

> +typedef struct
> +{
> +	DWORD exception_code;
> +	LPTSTR exception_name;

I suggest s/LPTSTR/const char */

> +} MR_ExceptionName;
> +
> +static
> +MR_ExceptionName MR_exception_names[] =

s/static/static const/

> +/*
> +** Retrieve the name of a Win32 exception code as a string
> +*/
> +static
> +LPTSTR MR_find_exception_name(DWORD exception_code)

s/LPTSTR/const char */

The return type should be on the same line as `static'.

> +	return _T("Unknown exception code");

I suggest s/_T(//
and s/)//

> +/*
> +** Explain an EXCEPTION_RECORD content into stderr.
> +*/
> +static
> +void MR_explain_exception_record(EXCEPTION_RECORD *rec)

The `void' should be on the same line as the `static'.

> +			/* Display AV address and acces mode */

s/acces/access/

> +			while(zone != NULL) {
> +				fprintf(stderr,
> +						"\n***    Checking zone %s#%d: "
> +						"0x%08x - 0x%08x - 0x%08x",
> +						zone->name, zone->id,
> +						(void *) zone->bottom,
> +						(void *) zone->redzone,
> +						(void *) zone->top);

Here you're printing `void *' using `%x' again.

> +/*
> +** Dump an EXCEPTION_RECORD content into stderr.
> +*/
> +static
> +void MR_dump_exception_record(EXCEPTION_RECORD *rec)

The `void' should be on the same line as `static'.

> +{
> +	int i;
> +	
> +	if (rec == NULL) {
> +		return;
> +	}
> +	
> +	fprintf(stderr, "\n***   Exception record at 0x%08lx:",
> +			(unsigned long) rec);
> +	fprintf(stderr, "\n***    Code        : 0x%08x (%s)",
> +			rec->ExceptionCode,
> +			MR_find_exception_name(rec->ExceptionCode));
> +	fprintf(stderr, "\n***    Flags       : 0x%08x", rec->ExceptionFlags);
> +	fprintf(stderr, "\n***    Address     : 0x%08x", rec->ExceptionAddress);
> +	for (i = 0; i < rec->NumberParameters; i++) {
> +		fprintf(stderr, "\n***    Parameter %d : 0x%08x", i,
> +				rec->ExceptionInformation[i]);
> +	}
> +	fprintf(stderr, "\n***    Next record : 0x%08x", rec->ExceptionRecord);

I think here again you may be using `%x' to print pointers.

> +bool MR_exception_record_is_access_violation(EXCEPTION_RECORD *rec,
> +		void **address, int *access_mode)
> +{
> +	if (rec->ExceptionCode == EXCEPTION_ACCESS_VIOLATION) {
> +		if (rec->NumberParameters >= 2) {
> +			(*access_mode) = (int)rec->ExceptionInformation[0];
> +			(*address) = (void *)rec->ExceptionInformation[1];

There should be a space after the casts, e.g. s/(int)/(int) /

> +int MR_filter_win32_exception(LPEXCEPTION_POINTERS exception_ptrs)

The return value should be on a line of its own.

> +	/*
> +	 * Pass exception back to upper handler. In most cases, this
> +	 * means activating UnhandledExceptionFilter, which will display
> +	 * a dialog box asking to user ro activate the Debugger or simply
> +	 * to kill the application
> +	 */
> +	return  EXCEPTION_CONTINUE_SEARCH;

That comment should be formatted according to our C coding guidelines.

> Index: mercury_memory_zones.c
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/runtime/mercury_memory_zones.c,v
> retrieving revision 1.10
> diff -u -r1.10 mercury_memory_zones.c
> --- mercury_memory_zones.c	2000/06/08 07:59:05	1.10
> +++ mercury_memory_zones.c	2000/06/27 12:16:25
> @@ -51,9 +51,84 @@
>    #include "mercury_thread.h"
>  #endif
>  
> +#ifdef MR_WIN32
> +  #include <windows.h>
> +#endif

Shouldn't that be `#ifdef MR_WIN32_VIRTUAL_ALLOC'?

> +/*---------------------------------------------------------------------------*/
> +
> +/*
> +** MR_PROTECTPAGE is now defined if we have some sort of mprotect like
> +** functionality, all checks for HAVE_MPROTECT should now use MR_PROTECTPAGE.
> +*/
> +#if defined(HAVE_MPROTECT)
> +int
> +MR_protect_pages(void *Addr, size_t Size, int ProtFlags)
> +{
> +	return mprotect((char *)Addr, Size, ProtFlags);
> +}
> +#elif defined(MR_WIN32_VIRTUAL_ALLOC)
...
> +#endif	/* HAVE_MPROTECT */

You broke the comment on that `#endif'; it should be
/* MR_WIN32_VIRTUAL_ALLOC */ rather than /* HAVE_MPROTECT */.

> +void *
> +memalign(size_t unit, size_t size)
> +{
> +	void *rc;
> +
> +	/*
> +	** We don't need to use the 'unit' value as the memory is always
> +	** aligned under Win32.
> +	*/
> +	rc = VirtualAlloc(NULL, size, MEM_COMMIT, PAGE_READWRITE);
> +	if (rc == NULL) {
> +		fprintf(stderr, "Error in VirtualAlloc(size=0x%08x): 0x%08x\n",
> +				size, GetLastError());

Here you use %x, but pass in a size_t.
I suggest using %lx and casting the size to unsigned long.

What does `rc' stand for?  It might help to choose a more meaningful
name for that variable.

> @@ -347,9 +423,8 @@
>  #ifdef	MR_CHECK_OVERFLOW_VIA_MPROTECT
>  	zone->redzone = zone->redzone_base;
>  
> -	if (mprotect((char *)zone->redzone,
> -		((char *)zone->top) - ((char *) zone->redzone), MY_PROT) < 0)
> -	{
> +	if (MR_protect_pages((char *)zone->redzone,
> +		((char *)zone->top) - ((char *) zone->redzone), MY_PROT) < 0) {

The `{' should be on a line of its own.

I'm not sure that I got all occurrences of return types on the
same line as the function name in function definitions.
I suggest you check through the diff.

I'd like to review another diff for this change
when you've addressed the issues mentioned above.

Cheers,
	Fergus.

-- 
Fergus Henderson <fjh at cs.mu.oz.au>  |  "I have always known that the pursuit
WWW: <http://www.cs.mu.oz.au/~fjh>  |  of excellence is a lethal habit"
PGP: finger fjh at 128.250.37.3        |     -- the last words of T. S. Garp.
--------------------------------------------------------------------------
mercury-developers mailing list
Post messages to:       mercury-developers at cs.mu.oz.au
Administrative Queries: owner-mercury-developers at cs.mu.oz.au
Subscriptions:          mercury-developers-request at cs.mu.oz.au
--------------------------------------------------------------------------