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

[Fergus Henderson]

On 23-Jun-2000, Peter Ross <petdr at cs.mu.OZ.AU> wrote:
> 
> Use Win32 exceptions to report problems, when we are in a Win32
> environment.

With regard to portability, it is generally a bad idea to think about
things in terms of "being in a <foo> environment".  For example, people
used to distinguish between BSD environments versus System V environments,
but those distinctions have since become redundant.
It is generally a better idea to distinguish things based
on whether a certain API is available.

For example, is Cygwin a Win32 environement?  What about WINE?
Both of those environments push the boundaries of what it
means to be "in a Win32 environment", so the answer is not clear.
But if you ask "is the Win32 API available", then it's much
clearer that in both cases the answer should be yes.

Still, it is better to be as fine grained as possible; rather than
asking whether the whole Win32 API is available, it's better to ask
about individual features, or small groups of closely related
features, because some environments may support only a subset of the
whole Win32 API.  So rather than having `#ifdef MR_WIN32'
spread throughout the code, it is better to have

	#ifdef MR_WIN32
		#define MR_WIN32_THIS
		#define MR_WIN32_THAT
		#define MR_WIN32_THE_OTHER
	#endif

in some central spot (e.g. runtime/mercury_conf_param.h),
and then have each individual part of the code which depends on
some Win32 feature use `#ifdef MR_WIN32_SUCH_AND_SUCH'.
That way if environments such as Cygwin, Wine, and WinCE
only support some subset of these features, you can easily
add support for such environments by just adding some
new definitions to that central spot, e.g.

	#ifdef FOO
		#define MR_WIN32_THIS
		#undef MR_WIN32_THAT
		#define MR_WIN32_THE_OTHER
		...
	#endif
		
> runtime/mercury_conf_param.h:
>     If we are compiling in Win32 environment, we can check for overflow
>     via a mprotect like mechanism, so define
>     MR_CHECK_OVERFLOW_VIA_MPROTECT.
>     If HAVE_MPROTECT or MR_WIN32 is defined, define MR_PROTECTPAGE.
>     MR_PROTECTPAGE means that we have access to mprotect like
>     functionality.

These conditional compilation macros should be documented
in that file.

> runtime/mercury_memory_zones.c:
>     Define a function ProtectPage() which has the same interface as
>     mprotect.  It just calls mprotect when it exists or equivalent code
>     to emulate it under Win32.
>     Use MR_PROTECTPAGE in place of HAVE_MPROTECT, and replace all calls
>     to mprotect() with ProtectPage().

The name should follow our naming conventions: MR_protect_page()
rather than ProtectPage().

> runtime/mercury_memory_handlers.h:
>     Export MR_FilterWin32Exception for use in mercury_wrapper.c

Likewise that should be MR_filter_win32_exception().

> +++ mercury_conf_param.h	2000/06/23 11:57:59
> @@ -345,8 +345,12 @@
>    #define MR_CAN_GET_PC_AT_SIGNAL
>  #endif
>  
> -#if defined(HAVE_MPROTECT) && defined(HAVE_SIGINFO)
> +#if (defined(HAVE_MPROTECT) && defined(HAVE_SIGINFO)) || defined(MR_WIN32)
>    #define MR_CHECK_OVERFLOW_VIA_MPROTECT
> +#endif

Where does MR_WIN32 get defined?

> +#if defined(HAVE_MPROTECT) || defined(MR_WIN32)
> +  #define MR_PROTECTPAGE
>  #endif
>  
>  /*---------------------------------------------------------------------------*/
> Index: mercury_memory.c
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/runtime/mercury_memory.c,v
> retrieving revision 1.18
> diff -u -r1.18 mercury_memory.c
> --- mercury_memory.c	2000/06/08 07:59:04	1.18
> +++ mercury_memory.c	2000/06/23 11:58:00
> @@ -96,7 +96,19 @@
>  #if defined(HAVE_SYSCONF) && defined(_SC_PAGESIZE)
>    #define	getpagesize()	sysconf(_SC_PAGESIZE)
>  #elif !defined(HAVE_GETPAGESIZE)
> -  #define	getpagesize()	8192
> +  #if defined(MR_WIN32)
> +    #include <windows.h>
> +
> +    static
> +    size_t getpagesize(void)
> +    {
> +	SYSTEM_INFO SysInfo;
> +	GetSystemInfo(&SysInfo);
> +	return (size_t) SysInfo.dwPageSize;
> +    }
> +  #else
> +    #define	getpagesize()	8192
> +  #endif
>  #endif
>  
>  /*---------------------------------------------------------------------------*/
> Index: mercury_memory_handlers.c
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/runtime/mercury_memory_handlers.c,v
> retrieving revision 1.11
> diff -u -r1.11 mercury_memory_handlers.c
> --- mercury_memory_handlers.c	2000/06/08 07:59:04	1.11
> +++ mercury_memory_handlers.c	2000/06/23 11:58:01
> @@ -132,7 +132,7 @@
>  static bool 
>  try_munprotect(void *addr, void *context)
>  {
> -#ifndef HAVE_SIGINFO
> +#if !(defined(HAVE_SIGINFO) || defined(MR_WIN32))
>  	return FALSE;
>  #else
>  	Word *    fault_addr;
> @@ -221,7 +221,7 @@
>  	    zone->name, zone->id, (void *) zone->redzone, (void *) new_zone,
>  	    (int)zone_size);
>  	}
> -	if (mprotect((char *)zone->redzone, zone_size,
> +	if (ProtectPage((char *)zone->redzone, zone_size,
>  	    PROT_READ|PROT_WRITE) < 0)
>  	{
>  	    char buf[2560];
> @@ -262,12 +262,18 @@
>  void
>  setup_signals(void)
>  {
> -#ifdef SIGBUS
> +/*
> +** When using Win32 don't set any signal handler.
> +** See mercury_wrapper.c for the reason why.
> +*/
> +#ifndef MR_WIN32
> +  #ifdef SIGBUS
>  	MR_setup_signal(SIGBUS, (Code *) bus_handler, TRUE,
>  		"Mercury runtime: cannot set SIGBUS handler");
> -#endif
> +  #endif
>  	MR_setup_signal(SIGSEGV, (Code *) segv_handler, TRUE,
>  		"Mercury runtime: cannot set SIGSEGV handler");
> +#endif
>  }
>  
>  static char *
> @@ -547,6 +553,272 @@
>  }
>  
>  #endif /* not HAVE_SIGINFO_T && not HAVE_SIGCONTEXT_STRUCT */
> +
> +#ifdef MR_WIN32
> +#include <tchar.h>
> +
> +/*
> +** Exception code and their string representation
> +*/
> +#define DEFINE_EXCEPTION_NAME(a)   {a,_T(#a)}
> +
> +typedef struct
> +{
> +	DWORD dwExceptionCode;
> +	LPTSTR pszExceptionName;
> +} MR_ExceptionName;
>
> +static
> +MR_ExceptionName MR_ExceptionNames[] =

Please use the Mercury C coding conventions
rather than the MS ones:
	s/dwExceptionCode/exception_code/
	s/pszExceptionName/exception_name/
	s/MR_ExceptionNames/MR_exception_names/

> +{
> +	DEFINE_EXCEPTION_NAME(EXCEPTION_ACCESS_VIOLATION),
> +	DEFINE_EXCEPTION_NAME(EXCEPTION_DATATYPE_MISALIGNMENT),
> +	DEFINE_EXCEPTION_NAME(EXCEPTION_BREAKPOINT),
> +	DEFINE_EXCEPTION_NAME(EXCEPTION_SINGLE_STEP),
> +	DEFINE_EXCEPTION_NAME(EXCEPTION_ARRAY_BOUNDS_EXCEEDED),
> +	DEFINE_EXCEPTION_NAME(EXCEPTION_FLT_DENORMAL_OPERAND),
> +	DEFINE_EXCEPTION_NAME(EXCEPTION_FLT_DIVIDE_BY_ZERO),
> +	DEFINE_EXCEPTION_NAME(EXCEPTION_FLT_INEXACT_RESULT),
> +	DEFINE_EXCEPTION_NAME(EXCEPTION_FLT_INVALID_OPERATION),
> +	DEFINE_EXCEPTION_NAME(EXCEPTION_FLT_OVERFLOW),
> +	DEFINE_EXCEPTION_NAME(EXCEPTION_FLT_STACK_CHECK),
> +	DEFINE_EXCEPTION_NAME(EXCEPTION_FLT_UNDERFLOW),
> +	DEFINE_EXCEPTION_NAME(EXCEPTION_INT_DIVIDE_BY_ZERO),
> +	DEFINE_EXCEPTION_NAME(EXCEPTION_INT_OVERFLOW),
> +	DEFINE_EXCEPTION_NAME(EXCEPTION_PRIV_INSTRUCTION),
> +	DEFINE_EXCEPTION_NAME(EXCEPTION_IN_PAGE_ERROR),
> +	DEFINE_EXCEPTION_NAME(EXCEPTION_ILLEGAL_INSTRUCTION),
> +	DEFINE_EXCEPTION_NAME(EXCEPTION_NONCONTINUABLE_EXCEPTION),
> +	DEFINE_EXCEPTION_NAME(EXCEPTION_STACK_OVERFLOW),
> +	DEFINE_EXCEPTION_NAME(EXCEPTION_INVALID_DISPOSITION),
> +	DEFINE_EXCEPTION_NAME(EXCEPTION_GUARD_PAGE),
> +	DEFINE_EXCEPTION_NAME(EXCEPTION_INVALID_HANDLE)
> +};
> +
> +
> +/*
> +** Retrieve the name of a Win32 exception code as a string
> +*/
> +static
> +LPTSTR MR_FindExceptionName(DWORD ExceptionCode)
> +{
> +	int i;
> +	for (i = 0; i < sizeof(MR_ExceptionNames)
> +			/ sizeof(MR_ExceptionName); i++)
> +	{
> +		if (MR_ExceptionNames[i].dwExceptionCode == ExceptionCode) {
> +			return MR_ExceptionNames[i].pszExceptionName;
> +		}
> +	}
> +	return _T("Unknown exception code");
> +}

Likewise here:
	s/ExceptionCode/exception_code/g

> +/*
> +** Explain an EXCEPTION_RECORD content into stderr.
> +*/
> +static
> +void MR_ExplainExceptionRecord(EXCEPTION_RECORD *Rec)

s/Rec/rec/

> +{
> +	fprintf(stderr, "\n");
> +	fprintf(stderr, "\n*** Explanation of the exception record");
> +	if (Rec == NULL)
> +	{

The `{' should be on the same line as the `if'.

> +	{
> +		void *lpAddress;
> +		int AccessMode;

s/lpAddress/address/
s/AccessMode/access_mode/

> +		/* If the exception is an access violation */
> +		if (MR_ExceptionRecordIsAccessViolation(Rec,
> +					&lpAddress, &AccessMode))
> +		{
> +			MemoryZone *zone;
> +
> +			/* Display AV address and acces mode */
> +			fprintf(stderr, "\n***   An access violation occured"
> +					" at address 0x%08x, while attempting"
> +					" to ", (void *)lpAddress);

Use `%08lx' rather than `%08x', and
you need to cast to `unsigned long' rather than to `void *'.

> +			if (AccessMode == 0) {
> +				fprintf(stderr, "\n***   read "
> +						"inaccessible data");
> +			}
> +			else if (AccessMode == 1) {
> +				fprintf(stderr, "\n***   write to an "
> +						"inaccessible (or protected)"
> +						" address");
> +			}
> +			else
> +						"mode %d (strange...)]",
> +						AccessMode);

The `}' should be on the same line as the `else'.
It would be a good idea to define named constants rather than
using 0 and 1 here.
The final else statement here seems to be syntactically
invalid, and it should go inside curlies.

> +			fprintf(stderr, "\n***   Trying to see if this "
> +					"stands within a mercury zone...");
> +			/*
> +			** Browse the mercury memory zones to see if the
> +			** AV address references one of them.
> +			*/
> +			zone = get_used_memory_zones();
> +			while(zone != NULL)
> +			{

The `{' should be on the same line as the `while'.

> +				/*
> +				** Dont need to call handler, because it
> +				** has much less information than us

s/Dont/Don't/

> +static
> +void MR_DumpExceptionRecord(EXCEPTION_RECORD *Rec)

s/Rec/rec/

> +{
> +	int i;
> +	
> +	if (Rec == NULL) {
> +		return;
> +	}
> +	
> +	fprintf(stderr, "\n***   Exception record at 0x%08x:", Rec);

Here you're printing a pointer using `%08x'.
You need to either
	(a) cast the pointer to `unsigned' first
	(b) use `%08lx' and cast the pointer to `unsigned long' first
	(c) use `%p' instead

Of these, I recommend (b).

> +	fprintf(stderr, "\n***    Code        : 0x%08x (%s)",
> +			Rec->ExceptionCode,
> +			MR_FindExceptionName(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++)
> +	{

The `{' should be on the same line as the `for'.

> +		fprintf(stderr, "\n***    Parameter %d : 0x%08x", i,
> +				Rec->ExceptionInformation[i]);
> +	}
> +	fprintf(stderr, "\n***    Next Record : 0x%08x", Rec->ExceptionRecord);
> +	
> +	/* Try to explain the exception more "gracefully" */
> +	MR_ExplainExceptionRecord(Rec);
> +	MR_DumpExceptionRecord(Rec->ExceptionRecord);
> +}
> +
> +
> +/*
> +** Return TRUE iff ExecpPtrs indicates an Access Violation
> +** 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)

s/BOOL/bool/
s/MR_ExceptionRecordIsAccessViolation/MR_Exception_record_is_access_violation/
s/Rec/rec/
s/lpAddress/address/
s/lpAccessMode/access_mode/

> +{
> +	if (Rec->ExceptionCode == EXCEPTION_ACCESS_VIOLATION)
> +	{
> +		if (Rec->NumberParameters >= 2)
> +		{

The `{'s should be on the same lines as the `if's.

> +int MR_FilterWin32Exception(LPEXCEPTION_POINTERS ExcepPtrs)

s/ExcepPtrs/exception_ptrs/

> +{
> +	void *lpAddress;
> +	int AccessMode;

Likewise.

> +
> +	/* If the exception is an access violation */
> +	if (MR_ExceptionRecordIsAccessViolation(ExcepPtrs->ExceptionRecord,
> +				&lpAddress, &AccessMode))
> +	{
> +		/* If we can unprotect to memory zone */
> +		if (try_munprotect(lpAddress, ExcepPtrs))
> +		{
> +			if (MR_memdebug)
> +			{

The `{'s should be on the same lines as the `if's.

> +++ mercury_memory_handlers.h	2000/06/23 11:58:01
> @@ -37,4 +37,31 @@
>  
>  void	setup_signals(void);
>  
> +#ifdef MR_WIN32
...
> +#include <windows.h>
> +#include <winnt.h>
> +
> +int MR_FilterWin32Exception(LPEXCEPTION_POINTERS ExcepPtrs);
> +#endif

Why do you include <winnt.h>?
Will that cause problems on Windows 95?

s/ExcepPtrs/exception_ptrs/

>  #endif /* not MERCURY_MEMORY_HANDLERS_H */
> 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/23 11:58:01
> @@ -51,9 +51,86 @@
>    #include "mercury_thread.h"
>  #endif
>  
> +#ifdef MR_WIN32
> +  #include <windows.h>
> +#endif
> +
>  /*---------------------------------------------------------------------------*/
>  
> -#ifdef	CONSERVATIVE_GC
> +/*
> +** 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
> +ProtectPage(void *Addr, size_t Size, int ProtFlags)
> +{
> +	return mprotect((char *)Addr, Size, ProtFlags);
> +}
> +#elif defined(MR_WIN32)
> +/*
> +** Emulate mprotect under Win32.
> +** Return -1 on failure
> +*/
> +int
> +ProtectPage(void *Addr, size_t Size, int ProtFlags)
> +{
> +	int rc;
> +	DWORD Flags, OldFlags;
> +
> +	if (ProtFlags & PROT_WRITE) {
> +		Flags = PAGE_READWRITE;
> +	}
> +	else if (ProtFlags & PROT_READ) {
> +		Flags = PAGE_READONLY;
> +	}
> +	else {
> +		Flags = PAGE_NOACCESS;
> +	}

The `}'s should be on the same line as the next `else'.

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

This function might protect more than one page, right?
So I think ProtectPage or MR_protect_page is probably not a good name
for it.  Instead it should be `MR_protect_pages' (note plural).

> +	if (rc < 0) {
> +		fprintf(stderr,
> +			"Error in VirtualProtect(Addr=0x%08x, Size=0x%08x):"
> +			" 0x%08x\n", Addr, Size, GetLastError());
> +	}
> +
> +	return rc;
> +}
> +#endif	/* HAVE_MPROTECT */
> +
> +
> +/*---------------------------------------------------------------------------*/
> +
> +
> +#if defined(MR_WIN32)
> +/*
> +** Under Win32, we use VirtualAlloc instead of the standard malloc,
> +** since we will have to call VirtualProtect later on the pages
> +** allocated here.
> +*/
> +void *
> +memalign(size_t Unit, size_t Size)
> +{

s/Unit/unit/
s/Size/size/

> +	void *rc;
> +
> +	/*RPA: We don't need to specify 'unit' of anything else, because
> +	       the memory will be anyway aligned under Win32 */
> +	rc = VirtualAlloc(NULL,
> +					  Size,
> +					  MEM_COMMIT,
> +					  PAGE_READWRITE);

It would be clearer to put that all on one line rather than wrapping it
over four lines.

The comment should be formatted

	/*
	** ...
	** ...
	*/

Part of the comment ("specify 'unit' of anything else") does not make sense
and should be rewritten.

> +	if (rc == NULL)
> +		fprintf(stderr, "Error in VirtualAlloc(Size=0x%08x): 0x%08x\n",
> +				Size, GetLastError());

The body of `if' statements like this should be enclosed in curly braces.

>  /*
> +** Rather then using mprotect directly, we call ProtectPage which
> +** is OS independent.
> +*/
> +#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.

> +int ProtectPage(void *Addr, size_t Size, int ProtFlags);

The names there all need to be modified to meet the Mercury C coding
guidelines.

> --- mercury_wrapper.c	2000/06/08 07:59:06	1.62
> +++ mercury_wrapper.c	2000/06/23 11:58:06
> @@ -32,6 +32,10 @@
>  #include	<stdio.h>
>  #include	<string.h>
>  
> +#ifdef MR_WIN32
> +#include <excpt.h>
> +#endif

s/#include/  #include/

> @@ -894,6 +898,30 @@
>  
>  	static	int	repcounter;
>  
> +#ifdef MR_WIN32
> +	/*
> +	** Under Win32 we use the following construction to handle exceptions.
> +	**   __try
> +	**   {
> +	**     <various stuff>
> +	**   }
> +	**   __except(MR_FilterWin32Exception(GetExceptionInformation())
> +	**   {
> +	**   }

You should use a different #define to test for this feature.
Here you are testing for whether the compiler supports the Win32
structured exception handling mechanism.  There will certainly
be environments which mostly support the Win32 API but for which the
compiler does not support these language extensions, for
example mingw32 (the Cygwin gcc with the `-mno-cygwin' option and the
mingw32 libraries).

----------

I'd like to see another diff when you've addressed the above
review comments.

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
--------------------------------------------------------------------------