Re: [ros-dev] [ros-diffs] [mjmartin] 40557: - Add volatile to variables in OutputDebugStringA. Fix #1 of ? for 3424.

[Timo Kreuzer]

Can you explain, why the variables need to be volatile? This looks like
a SEH problem.

[e-mail] schrieb:
> Author: mjmartin
> Date: Fri Apr 17 13:59:03 2009
> New Revision: 40557
>
> URL: http://svn.reactos.org/svn/reactos?rev=40557&view=rev
> Log:
> Add volatile to variables in OutputDebugStringA. Fix #1 of ? for 3424.
>
> Modified:
>     trunk/reactos/dll/win32/kernel32/debug/output.c
>
> Modified: trunk/reactos/dll/win32/kernel32/debug/output.c
> URL: http://svn.reactos.org/svn/reactos/trunk/reactos/dll/win32/kernel32/debug/ou...
> ==============================================================================
> --- trunk/reactos/dll/win32/kernel32/debug/output.c [iso-8859-1] (original)
> +++ trunk/reactos/dll/win32/kernel32/debug/output.c [iso-8859-1] Fri Apr 17 13:59:03 2009
> @@ -219,9 +219,9 @@
>    	static BOOL s_bDBMonMutexTriedOpen = FALSE;
> /* local copy of the mutex handle */
> HANDLE hDBMonMutex = s_hDBMonMutex;
> volatile HANDLE hDBMonMutex = s_hDBMonMutex;
> /* handle to the Section of the shared buffer */
> HANDLE hDBMonBuffer = NULL;
> volatile HANDLE hDBMonBuffer = NULL;
>
> /* pointer to the mapped view of the shared buffer. It consist of the current
>   process id followed by the message string */
>
> @@ -229,11 +229,11 @@
> /* event: signaled by the debug message monitor when OutputDebugString can write
>    to the shared buffer */
> HANDLE hDBMonBufferReady = NULL;
> volatile HANDLE hDBMonBufferReady = NULL;
>
> /* event: to be signaled by OutputDebugString when it's done writing to the
>   shared buffer */
> HANDLE hDBMonDataReady = NULL;
> volatile HANDLE hDBMonDataReady = NULL;
>
> /* mutex not opened, and no previous attempts to open/create it */
> if(hDBMonMutex == NULL && !s_bDBMonMutexTriedOpen)
>
> @@ -295,10 +295,10 @@
>    		_SEH2_TRY
>    		{
>    			/* size of the current output block */
> 		SIZE_T nRoundLen;
> 		volatile SIZE_T nRoundLen;
>
> 	/* size of the remainder of the string */
> 		SIZE_T nOutputStringLen;
> 		volatile SIZE_T nOutputStringLen;
>
> 	/* output the whole string */
> 	nOutputStringLen = strlen(_OutputString);
>
> @@ -339,11 +339,12 @@
>    				else
>    				{
>    					/* output in blocks of 512 characters */
> 				CHAR a_cBuffer[512];
> 				volatile PCHAR a_cBuffer;
> 				a_cBuffer = (CHAR*)HeapAlloc(GetProcessHeap(), 0, 512);
>
> 			/* write a maximum of 511 bytes */
> 				if(nOutputStringLen > (sizeof(a_cBuffer) - 2))
> 					nRoundLen = sizeof(a_cBuffer) - 2;
> 				if(nOutputStringLen > 510)
> 					nRoundLen = 510;
> 			else
> 				nRoundLen = nOutputStringLen;
>
> @@ -355,6 +356,8 @@
> 				/* send the current block to the kernel debugger */
> 				DbgPrint("%s", a_cBuffer);
> 				HeapFree(GetProcessHeap(), 0, a_cBuffer);
> 		}
>
> 		/* move to the next block */

[Michael Martin]

IIRC, at one point, someone had said something along the lines that under certain criteria variables used inside SEH2 blocks had to be declared with volatile.
Not sure if this had been fixed but either way it does looks like an SEH problem.

martin
> Date: Sat, 18 Apr 2009 12:36:31 +0200
> From: [e-mail]
> To: [e-mail]
> Subject: Re: [ros-dev] [ros-diffs] [mjmartin] 40557: - Add volatile to variables in OutputDebugStringA. Fix #1 of ? for 3424.
>
> Can you explain, why the variables need to be volatile? This looks like
> a SEH problem.
>
> [e-mail] schrieb:
> > Author: mjmartin
> > Date: Fri Apr 17 13:59:03 2009
> > New Revision: 40557
> >
> > URL: http://svn.reactos.org/svn/reactos?rev=40557&view=rev
> > Log:
> > Add volatile to variables in OutputDebugStringA. Fix #1 of ? for 3424.
> >
> > Modified:
> >     trunk/reactos/dll/win32/kernel32/debug/output.c
> >
> > Modified: trunk/reactos/dll/win32/kernel32/debug/output.c
> > URL: http://svn.reactos.org/svn/reactos/trunk/reactos/dll/win32/kernel32/debug/ou...
> > ==============================================================================
> > --- trunk/reactos/dll/win32/kernel32/debug/output.c [iso-8859-1] (original)
> > +++ trunk/reactos/dll/win32/kernel32/debug/output.c [iso-8859-1] Fri Apr 17 13:59:03 2009
> > @@ -219,9 +219,9 @@
> >    	static BOOL s_bDBMonMutexTriedOpen = FALSE;
> > /* local copy of the mutex handle */
> > HANDLE hDBMonMutex = s_hDBMonMutex;
> > volatile HANDLE hDBMonMutex = s_hDBMonMutex;
> > /* handle to the Section of the shared buffer */
> > HANDLE hDBMonBuffer = NULL;
> > volatile HANDLE hDBMonBuffer = NULL;
> >
> > /* pointer to the mapped view of the shared buffer. It consist of the current
> >   process id followed by the message string */
> >
> > @@ -229,11 +229,11 @@
> > /* event: signaled by the debug message monitor when OutputDebugString can write
> >    to the shared buffer */
> > HANDLE hDBMonBufferReady = NULL;
> > volatile HANDLE hDBMonBufferReady = NULL;
> >
> > /* event: to be signaled by OutputDebugString when it's done writing to the
> >   shared buffer */
> > HANDLE hDBMonDataReady = NULL;
> > volatile HANDLE hDBMonDataReady = NULL;
> >
> > /* mutex not opened, and no previous attempts to open/create it */
> > if(hDBMonMutex == NULL && !s_bDBMonMutexTriedOpen)
> >
> > @@ -295,10 +295,10 @@
> >    		_SEH2_TRY
> >    		{
> >    			/* size of the current output block */
> > 		SIZE_T nRoundLen;
> > 		volatile SIZE_T nRoundLen;
> >
> > 	/* size of the remainder of the string */
> > 		SIZE_T nOutputStringLen;
> > 		volatile SIZE_T nOutputStringLen;
> >
> > 	/* output the whole string */
> > 	nOutputStringLen = strlen(_OutputString);
> >
> > @@ -339,11 +339,12 @@
> >    				else
> >    				{
> >    					/* output in blocks of 512 characters */
> > 				CHAR a_cBuffer[512];
> > 				volatile PCHAR a_cBuffer;
> > 				a_cBuffer = (CHAR*)HeapAlloc(GetProcessHeap(), 0, 512);
> >
> > 			/* write a maximum of 511 bytes */
> > 				if(nOutputStringLen > (sizeof(a_cBuffer) - 2))
> > 					nRoundLen = sizeof(a_cBuffer) - 2;
> > 				if(nOutputStringLen > 510)
> > 					nRoundLen = 510;
> > 			else
> > 				nRoundLen = nOutputStringLen;
> >
> > @@ -355,6 +356,8 @@
> > 				/* send the current block to the kernel debugger */
> > 				DbgPrint("%s", a_cBuffer);
> > 				HeapFree(GetProcessHeap(), 0, a_cBuffer);
> > 		}
> >
> > 		/* move to the next block */
>
> Ros-dev mailing list
> [e-mail]
> http://www.reactos.org/mailman/listinfo/ros-dev

_________________________________________________________________
Rediscover Hotmail®: Get e-mail storage that grows with you. 
http://windowslive.com/RediscoverHotmail?ocid=TXT_TAGLM_WL_HM_Rediscover_Sto...

[Aleksey Bragin]

This is a PSEH specific "feature". GCC may think that the variable is  
not used when it's declared or used(?) and optimize it away. To  
prevent this a volatile is added.

At least that's how I remember KJK explained that, and iirc there was  
a ML thread here some time ago about same thing, when Dmitry  
Gorbachev added same thing to the ntoskrnl.

WBR,
Aleksey Bragin.

On Apr 21, 2009, at 9:24 PM, Michael Martin wrote:
> IIRC, at one point, someone had said something along the lines that  
> under certain criteria variables used inside SEH2 blocks had to be  
> declared with volatile.
> Not sure if this had been fixed but either way it does looks like  
> an SEH problem.
>
> martin
> > Date: Sat, 18 Apr 2009 12:36:31 +0200
> > From: [e-mail]
> > To: [e-mail]
> > Subject: Re: [ros-dev] [ros-diffs] [mjmartin] 40557: - Add
>
> volatile to variables in OutputDebugStringA. Fix #1 of ? for 3424.
> > Can you explain, why the variables need to be volatile? This
>
> looks like
> > a SEH problem.
> >
> > [e-mail] schrieb:
> > > Author: mjmartin
> > > Date: Fri Apr 17 13:59:03 2009
> > > New Revision: 40557
> > >
> > > URL: http://svn.reactos.org/svn/reactos?rev=40557&view=rev
> > > Log:
> > > Add volatile to variables in OutputDebugStringA. Fix #1 of ?
>
> for 3424.
> > > Modified:
> > > trunk/reactos/dll/win32/kernel32/debug/output.c
> > >
> > > Modified: trunk/reactos/dll/win32/kernel32/debug/output.c
> > > URL: http://svn.reactos.org/svn/reactos/trunk/reactos/dll/win32/
>
> kernel32/debug/output.c?rev=40557&r1=40556&r2=40557&view=diff
> > >
> ======================================================================
> > > --- trunk/reactos/dll/win32/kernel32/debug/output.c
>
> [iso-8859-1] (original)
> > > +++ trunk/reactos/dll/win32/kernel32/debug/output.c
>
> [iso-8859-1] Fri Apr 17 13:59:03 2009
> > > @@ -219,9 +219,9 @@
> > > static BOOL s_bDBMonMutexTriedOpen = FALSE;
> > >
> > > /* local copy of the mutex handle */
> > > HANDLE hDBMonMutex = s_hDBMonMutex;
> > > volatile HANDLE hDBMonMutex = s_hDBMonMutex;
> > >
> > > /* handle to the Section of the shared buffer */
> > > HANDLE hDBMonBuffer = NULL;
> > > volatile HANDLE hDBMonBuffer = NULL;
> > >
> > > /* pointer to the mapped view of the shared buffer. It consist
>
> of the current
> > > process id followed by the message string */
> > > @@ -229,11 +229,11 @@
> > >
> > > /* event: signaled by the debug message monitor when
>
> OutputDebugString can write
> > > to the shared buffer */
> > > HANDLE hDBMonBufferReady = NULL;
> > > volatile HANDLE hDBMonBufferReady = NULL;
> > >
> > > /* event: to be signaled by OutputDebugString when it's done
>
> writing to the
> > > shared buffer */
> > > HANDLE hDBMonDataReady = NULL;
> > > volatile HANDLE hDBMonDataReady = NULL;
> > >
> > > /* mutex not opened, and no previous attempts to open/create it */
> > > if(hDBMonMutex == NULL && !s_bDBMonMutexTriedOpen)
> > > @@ -295,10 +295,10 @@
> > > _SEH2_TRY
> > > {
> > > /* size of the current output block */
> > > SIZE_T nRoundLen;
> > > volatile SIZE_T nRoundLen;
> > >
> > > /* size of the remainder of the string */
> > > SIZE_T nOutputStringLen;
> > > volatile SIZE_T nOutputStringLen;
> > >
> > > /* output the whole string */
> > > nOutputStringLen = strlen(_OutputString);
> > > @@ -339,11 +339,12 @@
> > > else
> > > {
> > > /* output in blocks of 512 characters */
> > > CHAR a_cBuffer[512];
> > > volatile PCHAR a_cBuffer;
> > > a_cBuffer = (CHAR*)HeapAlloc(GetProcessHeap(), 0, 512);
> > >
> > > /* write a maximum of 511 bytes */
> > > if(nOutputStringLen > (sizeof(a_cBuffer) - 2))
> > > nRoundLen = sizeof(a_cBuffer) - 2;
> > > if(nOutputStringLen > 510)
> > > nRoundLen = 510;
> > >
> > > else
> > > nRoundLen = nOutputStringLen;
> > >
> > > @@ -355,6 +356,8 @@
> > >
> > > /* send the current block to the kernel debugger */
> > > DbgPrint("%s", a_cBuffer);
> > > HeapFree(GetProcessHeap(), 0, a_cBuffer);
> > >
> > > }
> > >
> > > /* move to the next block */
> >
> > Ros-dev mailing list
> > [e-mail]
> > http://www.reactos.org/mailman/listinfo/ros-dev
>
> Rediscover Hotmail®: Get e-mail storage that grows with you. Check  
> it out.
> _______________________________________________
> Ros-dev mailing list
> [e-mail]
> http://www.reactos.org/mailman/listinfo/ros-dev

[Dmitry Gorbachev]

Aleksey Bragin wrote:
> At least that's how I remember KJK explained that, and iirc there was
> a ML thread here some time ago about same thing, when Dmitry
> Gorbachev added same thing to the ntoskrnl.

IIRC, that was KJK::Hyperion. I just proposed to use something like
_SEH2_VOLATILE, instead of plain "volatile".