This is the mail archive of the gdb-patches@sourceware.org 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] Reverse debugging, part 1/3: target interface


Daniel Jacobowitz wrote:

Need "set debug target 1" support for the new target ops.

Oh yeah. Thanks, will do.


> Also, it
might be nice to use the new style rather than using INHERIT (search
the vector at call time; see the "do not inherit" comments); that makes
the debugging output nicer, too.

OK, I think I know what you mean (the ->beneath business) -- but why is this better?


+ /* Set execution direction (forward/reverse). */
+ int (*to_set_execdir) (enum exec_direction_kind);
+ /* Get execution direction (forward/reverse). */
+ enum exec_direction_kind (*to_get_execdir) (void);
+

We've got these execdir methods...


+ /* Forward/reverse execution direction. + These will only be implemented by a target that supports reverse execution.
+ */

(comment formatting; wrap before the */ please?)
OK


+ #define target_get_execution_direction() \
+     (current_target.to_get_execdir ? \
+      (*current_target.to_get_execdir) () : EXEC_ERROR)


... but these _execution_direction methods.  Please choose one or the
other; it makes it simpler to find the uses and implementations at the
same time.

... are you referring to the choices of names?


I made the names of the macros more verbose because they're
more visible, and used in a context where any aid to understanding
is desireable.  For the struct members themselves, I used shorter
names because they're *not* all that visible, are used in a context
where it's not all that critical to understand precisely what they
do, and longer names are cumbersome.  For instance, they would make
the declarations in target.h really long.

Do you still think I should change it?

> You're below this bit:

/* The vCont packet doesn't need to specify threads via Hc. */ if (remote_vcont_resume (ptid, step, siggnal)) return;

So if the remote target supports vCont, you'll go forwards by accident
instead of reverse!  Talk about a traffic accident.

Oops, thanks for the catch. I'm not that familiar with vCont.


I think that's a pretty big FIXME.

Acknowledged, discussed in another sub-thread.


+   if (dir == EXEC_FORWARD || dir == EXEC_REVERSE)
+     return (remote_execdir = dir);


And please don't do that - no wonder Eli was confused by this
function.  If you want an assignment, put it on its own line :-)

Oh, alright... ;-)



+   remote_ops.to_get_execdir = remote_get_execdir;
+   remote_ops.to_set_execdir = remote_set_execdir;


What about the other remote ops vectors?  At least one isn't copied
from here.

Right, well, I figured get it accepted in one vector, then worry about extending it into others. Basically, as of this patch, it will work with "target remote" only (and I guess only in non-async mode).



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