[patch/cygwin] Remove dependency on __COPY_CONTEXT_SIZE

  Hi,

I'd like to apply the below patch.  It depends on the definition of a
macro in a Cygwin header which was originally introduced to allow 
building on Cygwin versions prior to introducing this feature.

However, this macro was introduced in 2006, so the time having to
rebuild on such old Cygwin versions is long gone.  Above that, the
definition of the macro depends on a datastructure wrongly named and
residing in the wrong header.  Also, __COPY_CONTEXT_SIZE is simply
equivalent to sizeof(CONTEXT) anyway.

Therefore I'd like to remove the dependency of GDB on this macro.

Patch below.


Thanks,
Corinna


	* windows-nat.c (do_windows_fetch_inferior_registers): Drop
	testing and using __COPY_CONTEXT_SIZE.  Just use sizeof (CONTEXT)
	directly instead.
	(handle_output_debug_string): Ditto.
  

On 03/30/2015 11:04 AM, Corinna Vinschen wrote:

> @@ -820,7 +819,7 @@ handle_output_debug_string (struct target_waitstatus *ourstatus)
>  #endif
>  	warning (("%s"), s);
>      }
> -#ifdef __COPY_CONTEXT_SIZE
> +#ifdef __CYGWIN__
>    else
>      {
>        /* Got a cygwin signal marker.  A cygwin signal is followed by
> @@ -847,8 +846,8 @@ handle_output_debug_string (struct target_waitstatus *ourstatus)
>  	  else if ((x = (LPCVOID) (uintptr_t) strtoull (p, NULL, 0))
>  		   && ReadProcessMemory (current_process_handle, x,
>  					 &saved_context,
> -					 __COPY_CONTEXT_SIZE, &n)
> -		   && n == __COPY_CONTEXT_SIZE)
> +					 sizeof (CONTEXT), &n)

Is that really wise?  AFAIK, the size of the CONTEXT structure can
grow as MSFT adds more registers to support newer machines.

It seems to me that the gdb and cygwin.dll builds can disagree
on the size of that structure.  Seems to me that this mechanism should
have a way to let GDB know the size of the context structure.  Or maybe
read just saved_context.ContextFlags first, and infer the size from
that, reading in the most we understand?


> +		   && n == sizeof (CONTEXT))
>  	    have_saved_context = 1;
>  	  current_event.dwThreadId = retval;
>  	}
> 

Thanks,
Pedro Alves
  

On Mar 31 13:34, Pedro Alves wrote:
> On 03/30/2015 11:04 AM, Corinna Vinschen wrote:
> 
> > @@ -820,7 +819,7 @@ handle_output_debug_string (struct target_waitstatus *ourstatus)
> >  #endif
> >  	warning (("%s"), s);
> >      }
> > -#ifdef __COPY_CONTEXT_SIZE
> > +#ifdef __CYGWIN__
> >    else
> >      {
> >        /* Got a cygwin signal marker.  A cygwin signal is followed by
> > @@ -847,8 +846,8 @@ handle_output_debug_string (struct target_waitstatus *ourstatus)
> >  	  else if ((x = (LPCVOID) (uintptr_t) strtoull (p, NULL, 0))
> >  		   && ReadProcessMemory (current_process_handle, x,
> >  					 &saved_context,
> > -					 __COPY_CONTEXT_SIZE, &n)
> > -		   && n == __COPY_CONTEXT_SIZE)
> > +					 sizeof (CONTEXT), &n)
> 
> Is that really wise?  AFAIK, the size of the CONTEXT structure can
> grow as MSFT adds more registers to support newer machines.

No, that's not possible.  The CONTEXT structure matches the platform.
It doesn't even contain a version number.  Consider that the structure
is available in user space.  If Microsoft changes the size on a given
platform, applications built for this platform might crash due to
overwritten memory.  They wouldn't do that.

> It seems to me that the gdb and cygwin.dll builds can disagree
> on the size of that structure.

No.  The size is constant for the platform.  If Cygwin and GDB disagree,
they hgaven't been built for the same platform, or one of them is using
a broken CONTEXT definition.  However, if Cygwin and GDB disagree,
wouldn't GDB and the Microsoft DLLs have the same problem?   That's really
not a problem.

> Seems to me that this mechanism should
> have a way to let GDB know the size of the context structure.  Or maybe
> read just saved_context.ContextFlags first, and infer the size from
> that, reading in the most we understand?

Again, the context structure is a user-space exported per-platform
structure.  The only party here which could change it is Microsoft,
and Microsoft would never do this because it would break the ABI
for existing application.


Corinna
  

On 03/31/2015 03:36 PM, Corinna Vinschen wrote:
> On Mar 31 13:34, Pedro Alves wrote:
>> On 03/30/2015 11:04 AM, Corinna Vinschen wrote:
>>
>>> @@ -820,7 +819,7 @@ handle_output_debug_string (struct target_waitstatus *ourstatus)
>>>  #endif
>>>  	warning (("%s"), s);
>>>      }
>>> -#ifdef __COPY_CONTEXT_SIZE
>>> +#ifdef __CYGWIN__
>>>    else
>>>      {
>>>        /* Got a cygwin signal marker.  A cygwin signal is followed by
>>> @@ -847,8 +846,8 @@ handle_output_debug_string (struct target_waitstatus *ourstatus)
>>>  	  else if ((x = (LPCVOID) (uintptr_t) strtoull (p, NULL, 0))
>>>  		   && ReadProcessMemory (current_process_handle, x,
>>>  					 &saved_context,
>>> -					 __COPY_CONTEXT_SIZE, &n)
>>> -		   && n == __COPY_CONTEXT_SIZE)
>>> +					 sizeof (CONTEXT), &n)
>>
>> Is that really wise?  AFAIK, the size of the CONTEXT structure can
>> grow as MSFT adds more registers to support newer machines.
> 
> No, that's not possible.  The CONTEXT structure matches the platform.
> It doesn't even contain a version number.  Consider that the structure
> is available in user space.  If Microsoft changes the size on a given
> platform, applications built for this platform might crash due to
> overwritten memory.  They wouldn't do that.

That's not true.   GetThreadContext takes a size parameter,
and only writes to the bits that the caller requests with
context.ContextFlags.  A size parameter is common in Windows API land
to permit later versions.  If the structure grows, evidently the new
fields will need to be requested with a new context.ContextFlags flag.  Old
applications will never request that extra flag, and will be passing
a smaller SIZE to GetThreadContext, so it won't ever overwrite memory.

See the description of InitializeContext's parameters:

 https://msdn.microsoft.com/en-us/library/windows/desktop/hh134237%28v=vs.85%29.aspx

And the remarks section:

"InitializeContext can be used to initialize a CONTEXT structure within a buffer
with the required size and alignment characteristics. This routine is required if
the CONTEXT_XSTATE ContextFlag is specified since the required context
size and alignment may change depending on which processor features are
enabled on the system.
...
Applications may subsequently remove, but must never add, bits from
the ContextFlags member of CONTEXT.
"

Thanks,
Pedro Alves
  

On Mar 31 15:58, Pedro Alves wrote:
> On 03/31/2015 03:36 PM, Corinna Vinschen wrote:
> > On Mar 31 13:34, Pedro Alves wrote:
> >> On 03/30/2015 11:04 AM, Corinna Vinschen wrote:
> >>
> >>> @@ -820,7 +819,7 @@ handle_output_debug_string (struct target_waitstatus *ourstatus)
> >>>  #endif
> >>>  	warning (("%s"), s);
> >>>      }
> >>> -#ifdef __COPY_CONTEXT_SIZE
> >>> +#ifdef __CYGWIN__
> >>>    else
> >>>      {
> >>>        /* Got a cygwin signal marker.  A cygwin signal is followed by
> >>> @@ -847,8 +846,8 @@ handle_output_debug_string (struct target_waitstatus *ourstatus)
> >>>  	  else if ((x = (LPCVOID) (uintptr_t) strtoull (p, NULL, 0))
> >>>  		   && ReadProcessMemory (current_process_handle, x,
> >>>  					 &saved_context,
> >>> -					 __COPY_CONTEXT_SIZE, &n)
> >>> -		   && n == __COPY_CONTEXT_SIZE)
> >>> +					 sizeof (CONTEXT), &n)
> >>
> >> Is that really wise?  AFAIK, the size of the CONTEXT structure can
> >> grow as MSFT adds more registers to support newer machines.
> > 
> > No, that's not possible.  The CONTEXT structure matches the platform.
> > It doesn't even contain a version number.  Consider that the structure
> > is available in user space.  If Microsoft changes the size on a given
> > platform, applications built for this platform might crash due to
> > overwritten memory.  They wouldn't do that.
> 
> That's not true.   GetThreadContext takes a size parameter,
> and only writes to the bits that the caller requests with
> context.ContextFlags.

The ContextFlags member is not a size parameter, it's a bit flag
parameter.  It tells the OS which values are required, but the flags are
responsible for different aspects of the CONTEXT structure and this does
not change the size of the CONTEXT datatype.  The code in question
always copies the exact size of CONTEXT, and that doesn't change per
platform.

> A size parameter is common in Windows API land
> to permit later versions.  If the structure grows, evidently the new
> fields will need to be requested with a new context.ContextFlags flag.  Old
> applications will never request that extra flag, and will be passing
> a smaller SIZE to GetThreadContext, so it won't ever overwrite memory.
> 
> See the description of InitializeContext's parameters:
> 
>  https://msdn.microsoft.com/en-us/library/windows/desktop/hh134237%28v=vs.85%29.aspx
> 
> And the remarks section:
> 
> "InitializeContext can be used to initialize a CONTEXT structure within a buffer
> with the required size and alignment characteristics. This routine is required if
> the CONTEXT_XSTATE ContextFlag is specified since the required context
> size and alignment may change depending on which processor features are
> enabled on the system.

Right, but this does not change the size of the CONTEXT datatype.  The
additional AVX values require more space than available in the CONTEXT
struct.  That's why using CONTEXT_XSTATE and the AVX functions require
to use InitializeContext; the size required to get these values is
larger than CONTEXT, thus the function returns ERROR_INSUFFICIENT_BUFFER
if ContextLength is == sizeof (CONTEXT) only.

And, we're not using this.  The local variable filled with the data is
of type CONTEXT and the data transmitted from Cygwin to GDB is of type
CONTEXT.  It's still the same size, independent of the availablity of
CONTEXT_XSTATE.


Corinna
  

On 03/31/2015 04:42 PM, Corinna Vinschen wrote:
> On Mar 31 15:58, Pedro Alves wrote:
>> On 03/31/2015 03:36 PM, Corinna Vinschen wrote:
>>> On Mar 31 13:34, Pedro Alves wrote:
>>>> On 03/30/2015 11:04 AM, Corinna Vinschen wrote:
>>>>
>>>>> @@ -820,7 +819,7 @@ handle_output_debug_string (struct target_waitstatus *ourstatus)
>>>>>  #endif
>>>>>  	warning (("%s"), s);
>>>>>      }
>>>>> -#ifdef __COPY_CONTEXT_SIZE
>>>>> +#ifdef __CYGWIN__
>>>>>    else
>>>>>      {
>>>>>        /* Got a cygwin signal marker.  A cygwin signal is followed by
>>>>> @@ -847,8 +846,8 @@ handle_output_debug_string (struct target_waitstatus *ourstatus)
>>>>>  	  else if ((x = (LPCVOID) (uintptr_t) strtoull (p, NULL, 0))
>>>>>  		   && ReadProcessMemory (current_process_handle, x,
>>>>>  					 &saved_context,
>>>>> -					 __COPY_CONTEXT_SIZE, &n)
>>>>> -		   && n == __COPY_CONTEXT_SIZE)
>>>>> +					 sizeof (CONTEXT), &n)
>>>>
>>>> Is that really wise?  AFAIK, the size of the CONTEXT structure can
>>>> grow as MSFT adds more registers to support newer machines.
>>>
>>> No, that's not possible.  The CONTEXT structure matches the platform.
>>> It doesn't even contain a version number.  Consider that the structure
>>> is available in user space.  If Microsoft changes the size on a given
>>> platform, applications built for this platform might crash due to
>>> overwritten memory.  They wouldn't do that.
>>
>> That's not true.   GetThreadContext takes a size parameter,
>> and only writes to the bits that the caller requests with
>> context.ContextFlags.
> 
> The ContextFlags member is not a size parameter,

I didn't say it was.  The GetThreadContext function takes
an IN+OUT size parameter in _addition to the ContextFlags flag.
Both can be used for versioning.

>> See the description of InitializeContext's parameters:
>>
>>  https://msdn.microsoft.com/en-us/library/windows/desktop/hh134237%28v=vs.85%29.aspx
>>
>> And the remarks section:
>>
>> "InitializeContext can be used to initialize a CONTEXT structure within a buffer
>> with the required size and alignment characteristics. This routine is required if
>> the CONTEXT_XSTATE ContextFlag is specified since the required context
>> size and alignment may change depending on which processor features are
>> enabled on the system.
> 
> Right, but this does not change the size of the CONTEXT datatype.  The
> additional AVX values require more space than available in the CONTEXT
> struct.  That's why using CONTEXT_XSTATE and the AVX functions require
> to use InitializeContext; the size required to get these values is
> larger than CONTEXT, thus the function returns ERROR_INSUFFICIENT_BUFFER
> if ContextLength is == sizeof (CONTEXT) only.

I'm almost sure in the old days, the CONTEXT structure didn't have
the ExtendedRegisters field at all.  I think it's bad to hard code
the size of the CONTEXT structure, but won't argue further.  Patch
is OK if you'd really like to apply it as is.


@@ -847,8 +846,8 @@ handle_output_debug_string (struct target_waitstatus *ourstatus)
 	  else if ((x = (LPCVOID) (uintptr_t) strtoull (p, NULL, 0))
 		   && ReadProcessMemory (current_process_handle, x,
 					 &saved_context,
-					 __COPY_CONTEXT_SIZE, &n)
-		   && n == __COPY_CONTEXT_SIZE)
+					 sizeof (CONTEXT), &n)
+		   && n == sizeof (CONTEXT))
 	    have_saved_context = 1;
 	  current_event.dwThreadId = retval;
 	}

> 
> And, we're not using this.  The local variable filled with the data is
> of type CONTEXT and the data transmitted from Cygwin to GDB is of type
> CONTEXT.  It's still the same size, independent of the availablity of
> CONTEXT_XSTATE.

Where can the debugger find the signal's xstate state then?

Thanks,
Pedro Alves
  

On Mar 31 17:30, Pedro Alves wrote:
> On 03/31/2015 04:42 PM, Corinna Vinschen wrote:
> > On Mar 31 15:58, Pedro Alves wrote:
> >> On 03/31/2015 03:36 PM, Corinna Vinschen wrote:
> >>> On Mar 31 13:34, Pedro Alves wrote:
> >>>> On 03/30/2015 11:04 AM, Corinna Vinschen wrote:
> >>>>
> >>>>> @@ -820,7 +819,7 @@ handle_output_debug_string (struct target_waitstatus *ourstatus)
> >>>>>  #endif
> >>>>>  	warning (("%s"), s);
> >>>>>      }
> >>>>> -#ifdef __COPY_CONTEXT_SIZE
> >>>>> +#ifdef __CYGWIN__
> >>>>>    else
> >>>>>      {
> >>>>>        /* Got a cygwin signal marker.  A cygwin signal is followed by
> >>>>> @@ -847,8 +846,8 @@ handle_output_debug_string (struct target_waitstatus *ourstatus)
> >>>>>  	  else if ((x = (LPCVOID) (uintptr_t) strtoull (p, NULL, 0))
> >>>>>  		   && ReadProcessMemory (current_process_handle, x,
> >>>>>  					 &saved_context,
> >>>>> -					 __COPY_CONTEXT_SIZE, &n)
> >>>>> -		   && n == __COPY_CONTEXT_SIZE)
> >>>>> +					 sizeof (CONTEXT), &n)
> >>>>
> >>>> Is that really wise?  AFAIK, the size of the CONTEXT structure can
> >>>> grow as MSFT adds more registers to support newer machines.
> >>>
> >>> No, that's not possible.  The CONTEXT structure matches the platform.
> >>> It doesn't even contain a version number.  Consider that the structure
> >>> is available in user space.  If Microsoft changes the size on a given
> >>> platform, applications built for this platform might crash due to
> >>> overwritten memory.  They wouldn't do that.
> >>
> >> That's not true.   GetThreadContext takes a size parameter,
> >> and only writes to the bits that the caller requests with
> >> context.ContextFlags.
> > 
> > The ContextFlags member is not a size parameter,
> 
> I didn't say it was.  The GetThreadContext function takes
> an IN+OUT size parameter in _addition to the ContextFlags flag.

Uhm...no, it doesn't.  The prototype is

  BOOL WINAPI GetThreadContext(
    _In_     HANDLE hThread,
    _Inout_  LPCONTEXT lpContext
  );

Only the ContextFlags member qualifies what's written to *lpContext.  See
https://msdn.microsoft.com/en-us/library/windows/desktop/ms679362%28v=vs.85%29.aspx

> > Right, but this does not change the size of the CONTEXT datatype.  The
> > additional AVX values require more space than available in the CONTEXT
> > struct.  That's why using CONTEXT_XSTATE and the AVX functions require
> > to use InitializeContext; the size required to get these values is
> > larger than CONTEXT, thus the function returns ERROR_INSUFFICIENT_BUFFER
> > if ContextLength is == sizeof (CONTEXT) only.
> 
> I'm almost sure in the old days, the CONTEXT structure didn't have
> the ExtendedRegisters field at all.

I don't know about that.  But the fact that CONTEXT is not a opaque
structure but exposed to user space speaks against that.  Every time
a Win32 datatype needs a change, MSFT kept the old datatype intact
and added an "Ex" or "2" type instead, just as with the functions.

> I think it's bad to hard code
> the size of the CONTEXT structure, but won't argue further.  Patch
> is OK if you'd really like to apply it as is.

Thanks.  But incidentally I retract the patch.  It seems we made a
mistake both, on 32 and 64 bit Cygwin as far as the definition of
__COPY_CONTEXT_SIZE is concerned.  Changing that to sizeof(CONTEXT) now
would potentially break backward compatibility with all Cygwin versions
up to today.  Oh well.

Sorry for the longish discussion for nothing :(


Corinna
  

On 03/31/2015 07:32 PM, Corinna Vinschen wrote:
> On Mar 31 17:30, Pedro Alves wrote:
>>>> That's not true.   GetThreadContext takes a size parameter,
>>>> and only writes to the bits that the caller requests with
>>>> context.ContextFlags.
>>>
>>> The ContextFlags member is not a size parameter,
>>
>> I didn't say it was.  The GetThreadContext function takes
>> an IN+OUT size parameter in _addition to the ContextFlags flag.
> 
> Uhm...no, it doesn't.  The prototype is
> 
>   BOOL WINAPI GetThreadContext(
>     _In_     HANDLE hThread,
>     _Inout_  LPCONTEXT lpContext
>   );

Eh, somehow I got confused.  Sorry about that.

>> I'm almost sure in the old days, the CONTEXT structure didn't have
>> the ExtendedRegisters field at all.
> 
> I don't know about that.  But the fact that CONTEXT is not a opaque
> structure but exposed to user space speaks against that.  Every time
> a Win32 datatype needs a change, MSFT kept the old datatype intact
> and added an "Ex" or "2" type instead, just as with the functions.

OK.

> 
>> I think it's bad to hard code
>> the size of the CONTEXT structure, but won't argue further.  Patch
>> is OK if you'd really like to apply it as is.
> 
> Thanks.  But incidentally I retract the patch.  It seems we made a
> mistake both, on 32 and 64 bit Cygwin as far as the definition of
> __COPY_CONTEXT_SIZE is concerned.  Changing that to sizeof(CONTEXT) now
> would potentially break backward compatibility with all Cygwin versions
> up to today.  Oh well.

Not sure I understand what you mean, but OK.

> Sorry for the longish discussion for nothing :(

Sounds like it wasn't for nothing then.

Thanks,
Pedro Alves
  

On Apr  1 11:44, Pedro Alves wrote:
> On 03/31/2015 07:32 PM, Corinna Vinschen wrote:
> > On Mar 31 17:30, Pedro Alves wrote:
> >> I think it's bad to hard code
> >> the size of the CONTEXT structure, but won't argue further.  Patch
> >> is OK if you'd really like to apply it as is.
> > 
> > Thanks.  But incidentally I retract the patch.  It seems we made a
> > mistake both, on 32 and 64 bit Cygwin as far as the definition of
> > __COPY_CONTEXT_SIZE is concerned.  Changing that to sizeof(CONTEXT) now
> > would potentially break backward compatibility with all Cygwin versions
> > up to today.  Oh well.
> 
> Not sure I understand what you mean, but OK.

I was a bit unclear, sorry.  What I was trying to say is this.

Only yesterday in a discussion on IRC it turned out that the definition
of __COPY_CONTEXT_SIZE was never identical to sizeof(CONTEXT).  The
definition of __COPY_CONTEXT_SIZE was based on an underlying datatype,
almost, but not quite identical to CONTEXT (i.e. a bug).

As a result, __COPY_CONTEXT_SIZE < sizeof(CONTEXT).  Worse, Cygwin
itself up to the current version 1.7.35 (fixed in the git repo) only
stored the leading __COPY_CONTEXT_SIZE bytes of the signal CONTEXT to 
internal storage for GDB's digestion.

Assuming we change GDB now to copy sizeof(CONTEXT), and assuming we're
running under a Cygwin <= 1.7.35.

GDB would copy random data following the leading __COPY_CONTEXT_SIZE
bytes of a CONTEXT to its own CONTEXT.  Thus, when later calling
SetThreadContext with this data, it would copy random data into the
OSes thread context.  Which sounds like a really, really bad idea to
me.

Therefore, for backward compat reasons we should keep this up for a
while, until we decide not to support Cygwin versions <= 1.7.35 in
GDB any longer.  Given that only the leading part of the signal
context (actually, basically only the content of Eip/Rip) is really
important here, nothing much is lost.

I hope I could clarify the situation.  If you have another idea
how we could handle this a bit..., well, more correct or something,
please do tell.

> > Sorry for the longish discussion for nothing :(
> 
> Sounds like it wasn't for nothing then.

Indeed :)


Thanks,
Corinna