|
From: Chris J. <ch...@at...> - 2004-08-31 18:41:47
|
Now that 2.2.0 is out is there any chance of getting the first of my debugging support patches into CVS? Chris -- http://www.atomice.com |
|
From: Chris J. <ch...@at...> - 2004-09-03 07:54:00
|
> Now that 2.2.0 is out is there any chance of getting the > first of my debugging support patches into CVS? Should I treat the silence as an implicit 'no'? Chris |
|
From: Tom H. <th...@cy...> - 2004-09-03 08:00:17
|
In message <013a01c4918b$ed40dff0$0207a8c0@avocado>
Chris January <ch...@at...> wrote:
>> Now that 2.2.0 is out is there any chance of getting the
>> first of my debugging support patches into CVS?
>
> Should I treat the silence as an implicit 'no'?
I don't have a problem with it.
Tom
--
Tom Hughes (th...@cy...)
Software Engineer, Cyberscience Corporation
http://www.cyberscience.com/
|
|
From: Julian S. <js...@ac...> - 2004-09-03 08:28:54
|
> > Now that 2.2.0 is out is there any chance of getting the > > first of my debugging support patches into CVS? > > Should I treat the silence as an implicit 'no'? Not necessarily. What you should take it as is there are a bunch of other large-scale structural changes to the software which we're dealing with right now, so thinking about your patch in detail is going to be delayed by that. Your best bet is to shout again in a month or so, and see what happens then. J |
|
From: Chris J. <ch...@at...> - 2004-09-04 10:51:38
|
> > > Now that 2.2.0 is out is there any chance of getting the > first of my > > > debugging support patches into CVS? > > > > Should I treat the silence as an implicit 'no'? > > Not necessarily. What you should take it as is there are a > bunch of other large-scale structural changes to the software > which we're dealing with right now, so thinking about your > patch in detail is going to be delayed by that. Your best > bet is to shout again in a month or so, and see what happens then. My apologies. I did not wish to sound impatient. Different projects have different policies for patches, etc. It's hard to know what the correct procedure is. Chris January |
|
From: Nicholas N. <nj...@ca...> - 2004-09-03 08:40:58
|
On Fri, 3 Sep 2004, Chris January wrote:
>> Now that 2.2.0 is out is there any chance of getting the
>> first of my debugging support patches into CVS?
>
> Should I treat the silence as an implicit 'no'?
No. More likely that no-one has any strong opinions, or are busy with
other things at the moment.
I looked at your patch, but haven't run it. I'm confused by a few
things.
- The single_step field you added to ThreadState -- it doesn't seem ever
be set.
- You've changed a few test conditions by one, and changed the 'jz' in the
dispatch loop to 'jle', but have not explained why this is necessary.
Some comments in the code would be helpful.
- You added 'stopped' to ThreadState, but I also can't see how it is set.
- 'stopped' is used like this:
+ if (VG_(threads)[tid_next].status == VgTs_Runnable &&
+ !VG_(threads)[tid_next].stopped)
should 'stopped' rather be folded into the 'status' type somehow? Eg.
have a VgTs_RunnableButStopped state, or similar?
- You said:
> These should be the only changes to existing parts of
> the core that are required to support live debugging. Obviously this
> patch doesn't add full debugging support in and of itself.
What more is required to get full debugging support? How much code will
it be?
- This code is unnecessarily repetitive:
+ if (trc == VG_TRC_EBP_JMP_BREAKPOINT) {
+ /* A software breakpoint. */
+ vg_tid_currently_in_baseBlock = vg_tid_last_in_baseBlock;
+ VG_(kkill) (0, VKI_SIGTRAP);
+ vg_tid_currently_in_baseBlock = 0;
+ break;
+ }
+
+ if (trc == VG_TRC_INNER_COUNTERZERO &&
+ VG_(threads)[tid].single_step) {
+ vg_tid_currently_in_baseBlock = vg_tid_last_in_baseBlock;
+ VG_(kkill) (0, VKI_SIGTRAP);
+ vg_tid_currently_in_baseBlock = 0;
+ break;
+ }
Finally, I still don't understand what the end result is intended to be
with these proposed changes -- do you start GDB, then load your program
under a Valgrind tool, then run it? Or something else?
N
|
|
From: Chris J. <ch...@at...> - 2004-09-04 10:50:11
|
> On Fri, 3 Sep 2004, Chris January wrote: >=20 > >> Now that 2.2.0 is out is there any chance of getting the=20 > first of my=20 > >> debugging support patches into CVS? > > > > Should I treat the silence as an implicit 'no'? >=20 > No. More likely that no-one has any strong opinions, or are=20 > busy with=20 > other things at the moment. >=20 >=20 > I looked at your patch, but haven't run it. I'm confused by a few=20 > things. >=20 >=20 > - The single_step field you added to ThreadState -- it=20 > doesn't seem ever=20 > be set. No - neither single_step or stopped are set anywhere at present. This = patch provides the infrastructure for adding debugging support but since there = is no debugger or debugger stub in Valgrind yet the two variables will = never get set. >=20 >=20 > - You've changed a few test conditions by one, and changed=20 > the 'jz' in the=20 > dispatch loop to 'jle', but have not explained why this is necessary.=20 > Some comments in the code would be helpful. I changed the tests for VG_(dispatch_ctr) so that setting it to 1 will execute only a single basic block. Previously setting it to 1 would = execute no basic blocks at all. >=20 >=20 > - You added 'stopped' to ThreadState, but I also can't see=20 > how it is set. As above the variable isn't set anywhere yet since no debugging stub = exists. >=20 >=20 > - 'stopped' is used like this: >=20 > + if (VG_(threads)[tid_next].status =3D=3D VgTs_Runnable = && > + !VG_(threads)[tid_next].stopped) >=20 > should 'stopped' rather be folded into the 'status' type=20 > somehow? Eg.=20 > have a VgTs_RunnableButStopped state, or similar? See my earlier reply to Tom Hughes. "The stopped flag (like the single_step flag) would be set by the = debugger. The reason it's a seperate flag and not a new VgTs_Stopped state is that when the debugger stops a thread it may be in any state (not just VgTs_Runnable). When the debugger resumes the thread it needs to restore = the state to its previous value. The easiest way to do this is not to modify = the state at all and have separate flag." >=20 >=20 > - You said: >=20 > > These should be the only changes to existing parts of > > the core that are required to support live debugging. Obviously this > > patch doesn't add full debugging support in and of itself. >=20 > What more is required to get full debugging support? How=20 > much code will=20 > it be? This depends on how it is implemented. If debugging support is = implemented using the method used here: http://www.atomice.com/gdb-valgrind.html = then the very little extra code is required. However that patch requires modifications to GDB. An (IMHO better) alternative is to implement a GDB server/stub in Valgrind which would require no or trivial changes to = GDB. Gdbserver could be used a starting point for this - it's about 20/25 = files. >=20 >=20 > - This code is unnecessarily repetitive: >=20 > + if (trc =3D=3D VG_TRC_EBP_JMP_BREAKPOINT) { > + /* A software breakpoint. */ > + vg_tid_currently_in_baseBlock =3D = vg_tid_last_in_baseBlock; > + VG_(kkill) (0, VKI_SIGTRAP); > + vg_tid_currently_in_baseBlock =3D 0; > + break; > + } > + > + if (trc =3D=3D VG_TRC_INNER_COUNTERZERO && > + VG_(threads)[tid].single_step) { > + vg_tid_currently_in_baseBlock =3D = vg_tid_last_in_baseBlock; > + VG_(kkill) (0, VKI_SIGTRAP); > + vg_tid_currently_in_baseBlock =3D 0; > + break; > + } Granted - the original patch had different code in the if blocks. >=20 >=20 > Finally, I still don't understand what the end result is=20 > intended to be=20 > with these proposed changes -- do you start GDB, then load=20 > your program=20 > under a Valgrind tool, then run it? Or something else? The end result of these changes is to make it easy to add debugging = support at a later stage because support for breakpoints and single stepping is already in the Valgrind core. No further changes would (hopefully) be required. Chris January |
|
From: Nicholas N. <nj...@ca...> - 2004-09-04 15:38:17
|
On Sat, 4 Sep 2004, Chris January wrote: > The end result of these changes is to make it easy to add debugging support > at a later stage because support for breakpoints and single stepping is > already in the Valgrind core. No further changes would (hopefully) be > required. I'd rather see the whole proposed change working before committing pieces of it. N |
|
From: Chris J. <ch...@at...> - 2004-09-05 16:48:46
|
> On Sat, 4 Sep 2004, Chris January wrote: >=20 > > The end result of these changes is to make it easy to add debugging=20 > > support at a later stage because support for breakpoints and single=20 > > stepping is already in the Valgrind core. No further changes would=20 > > (hopefully) be required. >=20 > I'd rather see the whole proposed change working before=20 > committing pieces=20 > of it. I understand. I thought it might be easier for people to check smaller patches. I'm reluctant to work on one large single patch that never gets accepted because of peoples' objections - submitting smaller patches = would help me know I'm on the right track. You can look at the patch I pointed = you to on my website to see how this smaller patch might be used by a = debugging stub/server. Chris January |
|
From: Nicholas N. <nj...@ca...> - 2004-09-05 19:55:10
|
On Sun, 5 Sep 2004, Chris January wrote: >> I'd rather see the whole proposed change working before committing >> pieces of it. > > I understand. I thought it might be easier for people to check smaller > patches. I'm reluctant to work on one large single patch that never gets > accepted because of peoples' objections - submitting smaller patches would > help me know I'm on the right track. Yeah, that's understandable. I think committing changes in pieces is fine so long as the end goal is clear, which is why seeing the whole thing helps. N |