Re: [PATCH] Windows gdb: Fix resetting of the debug-registers bit in ContextFlags

On 2026-07-21 17:51, Tom Tromey wrote:
>>>>>> "Hannes" == Hannes Domani <[email protected]> writes:
> 
> Hannes>  Ping.
> 
> I'd prefer Pedro reply, but FWIW I think the patch looks reasonable.
> You can have my approval but please wait a bit to see if Pedro has some
> other comments.

I'm a little confused, since AFAICT, there has been no patch update to address the
comments I made earlier.

We've established that the patch isn't really "fixing" the resetting of registers, as the
missing context flag is ignored anyway.  So at the very least I was expecting that the subject and commit
log would be updated to match reality.

> Am Montag, 6. Juli 2026 um 17:02:35 MESZ hat Hannes Domani <[email protected]> Folgendes geschrieben:
> 
>> It's just great that all your mails are blocked by yahoo...

Sorry, but I don't know what I can do about that.  My hosting provider, including email is dreamhost,
which is quite popular and I believe used by others in the community too.  I don't have anything special
going on with my email AFAIK.

>>  
>>  
>> Am Mittwoch, 1. Juli 2026 um 21:03:09 MESZ hat Pedro Alves <pedro () palves ! net> Folgendes geschrieben:
>>  
>>> On 2026-06-27 15:27, Hannes Domani wrote:
>>>> The CONTEXT_DEBUG_REGISTERS also includes the arch-specific bit
>>>> (CONTEXT_i386 or CONTEXT_AMD64) which is included in all CONTEXT_*
>>>> defines.
>>>>
>>>> So this basically just checks if any CONTEXT_* define is set:
>>>>   if ((context->ContextFlags & CONTEXT_DEBUG_REGISTERS) != 0)
>>>>
>>>> And similarily, unsetting CONTEXT_DEBUG_REGISTERS removes the
>>>  
>>> similarily => similarly
>>  
>> Right.

Can you fix?

>>  
>>  
>>>> arch-specific bit as well.
>>>>
>>>> So this creates a CONTEXT_DEBUG_REG_FLAG define with just the
>>>> debug-registers bit, and uses it in these problematic locations.
>>>  
>>> How did you notice this?  Like, GDB was misbehaving and you found the
>>> issue, was it by inspection?  I'd be good to have that info in the commit log.
>>  
>> I noticed because I was doing some changes in that function, and
>> CONTEXT_DEBUG_REGISTERS stood out to me very quickly, because for WOW64 I
>> would expect WindowsContext<decltype(context)>::debug to be used instead.

OK.  Can you please add this to the commit log?  That does seem like something
we need to fix, and should be rationale for the change, right?

>>  
>>  
>>>> ---
>>>>   gdb/x86-windows-nat.c | 10 +++++++---
>>>>   1 file changed, 7 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/gdb/x86-windows-nat.c b/gdb/x86-windows-nat.c
>>>> index 27adeb1f154..3368814ed96 100644
>>>> --- a/gdb/x86-windows-nat.c
>>>> +++ b/gdb/x86-windows-nat.c
>>>> @@ -42,6 +42,10 @@ enum
>>>>  
>>>>   #define DR6_CLEAR_VALUE 0xffff0ff0
>>>  
>>>>  
>>>> +/* The CONTEXT_DEBUG_REGISTERS define without the arch-specific bit
>>>> +  (CONTEXT_i386 or CONTEXT_AMD64).  */
>>>> +#define CONTEXT_DEBUG_REG_FLAG 0x10
>>>> +
>>>  
>>> Did you consider avoiding harcoding numbers, like:
>>>  
>>> #ifdef __x86_64__
>>> # define CONTEXT_ARCH_BIT CONTEXT_AMD64
>>> #else
>>> # define CONTEXT_ARCH_BIT CONTEXT_i386
>>> #endif
>>>  
>>> #define CONTEXT_DEBUG_REG_FLAG (CONTEXT_DEBUG_REGISTERS & ~CONTEXT_ARCH_BIT)
>>  
>> I did consider this:
>>  
>> #define CONTEXT_DEBUG_REG_FLAG (CONTEXT_DEBUG_REGISTERS & ~CONTEXT_CONTROL)

OK, so why did you decide against it?


>> I did some experiments, and it looks like SetThreadContext doesn't care at
>> all about the arch bit, so it is working like your original intention.
>> I thought it would fail in the arch bit is missing, but I was wrong about that.
>>  

Seeing this, FYI, I didn't know if you planed on dropping the patch, or sending an
updated one with a commit log that reflects the finding.  But I didn't think the
current one as it was, was ready.

Pedro Alves