This is the mail archive of the gdb-patches@sources.redhat.com mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [RFA 2] Debug register support in win32-nat.c


At 13:39 13/01/02 -0500, Christopher Faylor wrote:
>On Tue, Jan 08, 2002 at 10:26:12AM +0100, Pierre Muller wrote:
>>At 10:20 08/01/2002 , vous avez ?crit:
>>>    This is a follow up of my first
>>>proposal for win32 debug register support
>>>(which enables hardware watchpoints,
>>>I never tested the hardware breakpoints, as they don't have
>>>much advantages over normal breakpoints on i386 processors).
>>
>>Sorry, I forgot to add the link
>>http://sources.redhat.com/ml/gdb-patches/2001-11/msg00537.html
>>and all the follow-ups.
>
>I applied this patch but it doesn't seem to build.  I get a:
>libgdb.a(win32-nat.o): In function `child_mourn_inferior':
>/cygnus/src/uberbaum/gdb/win32-nat.c:1398: undefined reference to
`_i386_cleanup_dregs'

Did I forget to incorporate the change in config/i386/cygwin.mh ?
No, I rechecked the reference patch and it is included.

Did you rerun configure in gdb build directory?
This is necessary as the nm.h link
is changed by my patch to cygwin.mh,
but might be missed by the Makefile itself.
Run ../../src/gdb/configure in build/gdb
if src and build dirs are at same dir level should suppress
that error.

>I assume that this is related to your other patch.
>
>In the meantime, I noticed a couple of things:
>
>- ChangeLog needs to be wrapped to 80 columns.
>
>- ChangeLog wording needs more verbs and more description.  For instance:
>        (debug_registers_changed): Non zero whenever the debug registers
where changed and
>        need to be written to inferior.
>   You need to mention that this is a new variable:
>        (debug_registers_changed): New variable.  Reflects when debug
registers are changed and
>        need to be written to inferior.
>
>- In do_initial_child_stuff, I'd prefer that you either use sizeof to
>  derive the size of the dr array for zeroing or use a defined constant,
>  rather than just a raw "7".

OK, so I should rewrite it to 
+  for (i = 0; i < sizeof (dr) / sizeof (dr[0]); i++)
+    dr[i] = 0;
   
Is that correct?
(Please excuse me to still ask so basic C questions,
but I never used C outside GDB code itself...)

>- I'm wondering if your implementation is thread safe?  You're storing
>  debug registers in a global array and copying them into a structure
>  as needed.  Couldn't they just be stored in the per-thread structure?
>  You could add a debug_registers_used value to the structure, if necessary.

The basic idea of my implementation is that 
watchpoints should be triggered by an expression that is modifed by
any thread, and thus the same value should be written to 
all threads. 
This means that:
 - we only need one copy of the debug registers.
 - we need to write their value to all threads
(including newly created ones, which is the major difference
between this patch respective to the first patch).
 - this of course assumes that the debuggee does not itself 
change its debug registers, in that case the patch
will not work correctly, but I think that this 
is a reasonable limitation.

I agree that the linux implementation 
does not set the debug registers for all threads but this
means that if a watched expression is modified by another
thread than the current thread at the time of setting
the watchpoint will not be caught and that is much
worse...

 If Christopher is able to build and test my patch
after a rerun of the configure scriptand if
he agrees with my remarks above, I will be happy 
to resubmit a third patch taking his remarks into account.
Thanks for reviewing my patch, Christopher.




Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]