* Re: Proposal: convert function definitions to prototyped form @ 2000-06-02 8:40 David Taylor 2000-06-02 12:10 ` Kevin Buettner 0 siblings, 1 reply; 57+ messages in thread From: David Taylor @ 2000-06-02 8:40 UTC (permalink / raw) To: Kevin Buettner; +Cc: gdb Date: Fri, 2 Jun 2000 00:50:19 -0700 From: Kevin Buettner <kevinb@cygnus.com> [...] 2) ``protoize'' fails to convert functions disabled by ifdefs for the given platform. OTOH, on some other platform(s), these functions might not be disabled and would be converted. E.g, in breakpoint.c, on my GNU/Linux/x86 machine, protoize failed to convert create_longjmp_breakpoint() which is protected by an "#ifdef GET_LONGJMP_TARGET". With any solution, either you're going to have to check the results or you're going to have to hand convert some of the stuff. It's a tradeoff. From what you say below, your script also requires some later hand conversions. Does your script produce substantially less that needs to be hand converted? 3) ``protoize'' screws up on PTR types. E.g, in breakpoint.c, it converted static int cover_target_enable_exception_callback (arg) PTR arg; to static int cover_target_enable_exception_callback (__builtin_va_list arg) I consider this to be a more serious problem than the other complaints you have. 4) ``protoize'' does not reformat long argument lists. The lists end up entirely contained on one line. So... adopt the same solution for it that you chose to adopt for your script -- run indent on the output! For more information on protoize, see the "Running protoize" page: http://gcc.gnu.org/onlinedocs/gcc_2.html#SEC48 and the "Caveats of using protoize" page: http://gcc.gnu.org/onlinedocs/gcc_7.html#SEC135 I've used protoize before with good results. It was a fairly substantial project, though not as big as gdb. Two of my goals in creating the scripts for the PARAMS purging activities was that the scripts should be 1) easy to invoke and 2) require no hand editing of the output when done. I.e, you shouldn't have to edit any of the files that these scripts touch in order to fix I trust that "3)" is: "be conservative; not convert something if it can't be sure of getting it right". I'd much rather hand convert more of the code than have it make a subtle but incorrect change. errors so that you can build again. OTOH, the script may leave certain portions of the file alone that could possibly have be converted had the script been a little smarter. The things that the script fails to convert *will* have to be fixed later on (in order to complete the cleanup activity), either by another script, or by hand. For the PARAMS purging activity, I have spent a fair amount of time examining the diffs to make sure that this is indeed the case. (And I intend to do the same for the activity in this proposal.) Good. The reason that it is so important to avoid any hand edits is that we want people who have local repositories or checked out source trees to be able to run these conversion scripts against them so that merges will be less painful. (Even easy.) Agreed. With that said, keeping in mind the problems I noted above, I conclude that ``protoize'' is not the appropriate tool for us to use to convert function definitions in the gdb sources to their prototyped form. I only consider 3) -- screwing up on PTR types -- to be serious; the others seem minor enough. [...] Finally, I should note that I've done a test build on GNU/Linux/x86 and had no build problems, nor did I see any regressions when I ran the testsuite. The fix-decls script is below. I'd be interested in finding out if anyone else has a favorite script for doing this sort of thing. Other comments welcome too. I'd be tempted to do a build before running your script; stash away the object files; run the script; do another build; compare the object files... I consider the lack of (prototyped) function declarations to be more serious "problem" than the use of old style function definitions in old code. I'd like to see: . declarations near the top of every file (i.e., before the first function definition) for every static function in the file. . a declaration in an included header file for every non static function definition and for every non static function used. . changes to the default configuration to "encourage" developers to include the above declarations. ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: Proposal: convert function definitions to prototyped form 2000-06-02 8:40 Proposal: convert function definitions to prototyped form David Taylor @ 2000-06-02 12:10 ` Kevin Buettner 2000-06-03 3:58 ` Eli Zaretskii 0 siblings, 1 reply; 57+ messages in thread From: Kevin Buettner @ 2000-06-02 12:10 UTC (permalink / raw) To: David Taylor; +Cc: gdb On Jun 2, 11:39am, David Taylor wrote: > With any solution, either you're going to have to check the results or > you're going to have to hand convert some of the stuff. It's a > tradeoff. From what you say below, your script also requires some > later hand conversions. Yes. But, it's conservative in that it won't break your builds. (Or at least that's the intent.) > Does your script produce substantially less that needs to be hand > converted? I only ran protoize on breakpoint.c to see what the problems were. This was the file that required the most conversion, however, so hopefully it's a good representative sample. For this file, protoize converted 126 function declarations and fix-decls converted 138. In addition, not counting reformatting, the protoized version of breakpoint.c would require 5 hand edits in order to fix problems introduced in the conversion. The fix-decls version requires 0 hand edits. After conversion, the protoized version of breakpoint.c still had 15 functions which would need to be converted by hand (or some other tool) whereas the fix-decls version had only 3. Here's a table to summarizing the above: breakpoint.c conversion | protoize | fix-decls | ----------------------------------------------+----------+-----------+ Functions converted | 126 | 138 | ----------------------------------------------+----------+-----------+ Hand edits needed afterwards to fix problems | | | in the conversion process | 5 | 0 | ----------------------------------------------+----------+-----------+ Functions remaining to be converted | 15 | 3 | ----------------------------------------------+----------+-----------+ Total number of hand edits required | 20 | 3 | ----------------------------------------------+----------+-----------+ Recall that over all of the gdb sources, fix-decls converted slightly over 5200 declarations. I believe that there are only around 140 left to consider. This is somewhat higher than I would like and I may be able to reduce it somewhat by making the script a little bit smarter. But I also want to be certain that no errors are introduced in the process. I should note that there are many settings where protoize is a completely acceptable (and probably better) tool. With gdb, however, we have a lot of developers scattered across the globe who need to be able to deterministically apply the same transformations to their sources in order to make merges and checkins easier. With protoize, I think it's going be be difficult to guarantee determinism since the results will vary depending upon which platform you use to do the transformation. > 3) ``protoize'' screws up on PTR types. E.g, in breakpoint.c, it > converted > > static int > cover_target_enable_exception_callback (arg) > PTR arg; > > to > > static int > cover_target_enable_exception_callback (__builtin_va_list arg) > > I consider this to be a more serious problem than the other complaints > you have. I think the setup issue is important too. (I don't know if it's more serious though.) As I mentioned in my first point (which is no longer quoted), you have to do a configure in the source directory above gdb in order to to properly run protoize or else it complains that it can't find certain header files. Also, you need to make sure that the bfd header files are generated before you start. None of these problems are insurmountable, but to do things safely with protoize, and in order to avoid polluting your sources with files that belong in a build tree, it would be necessary for a script using protoize to make a complete copy of the source tree somewhere else in order to do the work. > 4) ``protoize'' does not reformat long argument lists. The lists > end up entirely contained on one line. > > So... adopt the same solution for it that you chose to adopt for your > script -- run indent on the output! Granted. A suitably imaginative script could identify just the lines that protoize touched and reformat those. I don't think we want to run indent on entire files however, at least not for this activity. > For more information on protoize, see the "Running protoize" page: > > http://gcc.gnu.org/onlinedocs/gcc_2.html#SEC48 > > and the "Caveats of using protoize" page: > > http://gcc.gnu.org/onlinedocs/gcc_7.html#SEC135 > > I've used protoize before with good results. It was a fairly > substantial project, though not as big as gdb. Okay. Out of curiousity, did the project in question have a large number of active developers? (This doesn't necessarily matter, but I think it's probably easier to manage this kind of thing when only a handful of developers are affected.) > Two of my goals in creating the scripts for the PARAMS purging > activities was that the scripts should be 1) easy to invoke and 2) > require no hand editing of the output when done. I.e, you shouldn't > have to edit any of the files that these scripts touch in order to fix > > I trust that "3)" is: > > "be conservative; not convert something if it can't be sure > of getting it right". You're right. I should have mentioned this. > I'd much rather hand convert more of the code than have it make a > subtle but incorrect change. I agree completely. [...] > Finally, I should note that I've done a test build on GNU/Linux/x86 > and had no build problems, nor did I see any regressions when I ran > the testsuite. > > The fix-decls script is below. I'd be interested in finding out if > anyone else has a favorite script for doing this sort of thing. Other > comments welcome too. > > I'd be tempted to do a build before running your script; stash away the > object files; run the script; do another build; compare the object > files... Good idea. I'll have to see what gcc does to compare object files. (I don't think a simple cmp works for ELF files.) > I consider the lack of (prototyped) function declarations to be more > serious "problem" than the use of old style function definitions in > old code. I'd like to see: > > . declarations near the top of every file (i.e., before the first > function definition) for every static function in the file. > > . a declaration in an included header file for every non static > function definition and for every non static function used. > > . changes to the default configuration to "encourage" developers to > include the above declarations. I don't disagree, but I think that adding prototypes for everything should be a separate activity. The question is whether it should occur before the activity covered by my proposal. And if it should occur before, once again, we'll need to find a way to help automate the process because if we attempt to do it incrementally by hand over a period of time, it is likely that it'll never get done. Kevin ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: Proposal: convert function definitions to prototyped form 2000-06-02 12:10 ` Kevin Buettner @ 2000-06-03 3:58 ` Eli Zaretskii [not found] ` <eliz@delorie.com> 0 siblings, 1 reply; 57+ messages in thread From: Eli Zaretskii @ 2000-06-03 3:58 UTC (permalink / raw) To: kevinb; +Cc: taylor, gdb > Date: Fri, 2 Jun 2000 12:10:42 -0700 > From: Kevin Buettner <kevinb@cygnus.com> > > > I've used protoize before with good results. It was a fairly > > substantial project, though not as big as gdb. > > Okay. Out of curiousity, did the project in question have a large > number of active developers? I think the real problem is not the number of developers, but the number of different configurations, and also different data types and functions concealed behind macros. `protoize' needs everything to be explicit, so it is not easy to run it on multi-platform project that uses macros to hide system dependencies (since you want the same macros back in the reformatted sources). This is a disadvantage of `protoize'. Its significant advantage is that its output is *always* correct, because it takes the info ``from the horse's mouth'': the compiler itself. In contrast, a script is simply a text-processing tool: it really doesn't understand the semantics of the source. In fact, it doesn't really understand the syntax very well. So with a script, we will always need a verification tool that can be trusted to find any potential bugs introduced by reformatting. > > I'd be tempted to do a build before running your script; stash away the > > object files; run the script; do another build; compare the object > > files... > > Good idea. I'll have to see what gcc does to compare object files. > (I don't think a simple cmp works for ELF files.) Comparing object files generally doesn't work. COFF (at least the variety used by DJGPP) is another case: if I compile the same source twice in a row, I get different object files (IIRC, the time stamp is recorded inside). One method I can suggest is to compile the source without optimizations, then run "objdump --disassemble" and compare the output of `objdump' for the two source files (original and reformatted). (I suggest to disable optimizations because it's possible that ANSI source allows the compiler to produce more optimal code that the K&R source.) Note that, since you need to compile the source to make sure reformatting didn't screw up anything, you essentially get the same problems you had with `protoize', albeit through the back door: you need to build for all supported configurations to make sure nothing's broken. ^ permalink raw reply [flat|nested] 57+ messages in thread
[parent not found: <eliz@delorie.com>]
* Re: Proposal: convert function definitions to prototyped form [not found] ` <eliz@delorie.com> @ 2000-06-03 10:50 ` Kevin Buettner 2000-06-03 11:37 ` Eli Zaretskii 2000-06-03 18:18 ` Andrew Cagney 2000-06-03 15:42 ` Kevin Buettner 2001-02-16 0:45 ` [RFC] Unified watchpoints for x86 platforms Kevin Buettner 2 siblings, 2 replies; 57+ messages in thread From: Kevin Buettner @ 2000-06-03 10:50 UTC (permalink / raw) To: Eli Zaretskii; +Cc: gdb On Jun 3, 6:58am, Eli Zaretskii wrote: > So with a script, we will always need a verification tool that can be > trusted to find any potential bugs introduced by reformatting. Right. That's why I wrote check-decls (see http://sourceware.cygnus.com/ml/gdb/2000-06/msg00028.html ) which takes the result of comparing (via diff -u) the original sources with the protoized sources and produces a C source file in which the portions from the protoized sources are used to construct prototypes and the portions from the original sources are used to construct (potentially?) corresponding function definitions. We can then invoke the C compiler (gcc -c -Wall) on the result and see what kinds of warnings and errors are produced. E.g, in an earlier (than the one I posted) version of fix-decls, I hadn't yet handled comma separated parameter lists and so the following: foo (a, b, c) int a; char *b, *c; was getting transformed into foo (int a, int b, char *b, *c) This type of mistake would've been quickly caught by check-decls + gcc. (As it was, I caught it myself because I was looking for it.) Also, since my fix-decls script merely looks for patterns which appear to be function definitions, it was finding if (overload_debug) { in find_overload_match() in valops.c and turning this into if (int overload_debug) { (Note that the ``if'' is at the beginning of the line in this function.) I found this one when I did a test build, but check-decls + the C compiler would have caught this one too. (fix-decls was quickly changed so that it no longer gets tripped up by this code.) Also, note that check-decls would've caught this mistake even if the the construct in question had appeared in some #if 0'd code whereas doing a build wouldn't have. I think it could still happen that something might slip by that won't work in the real code, but now that I've written check-decls, I think it is much, much less likely. Kevin ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: Proposal: convert function definitions to prototyped form 2000-06-03 10:50 ` Kevin Buettner @ 2000-06-03 11:37 ` Eli Zaretskii 2000-06-03 18:18 ` Andrew Cagney 1 sibling, 0 replies; 57+ messages in thread From: Eli Zaretskii @ 2000-06-03 11:37 UTC (permalink / raw) To: kevinb; +Cc: gdb > Date: Sat, 3 Jun 2000 10:50:39 -0700 > From: Kevin Buettner <kevinb@cygnus.com> > > That's why I wrote check-decls (see > http://sourceware.cygnus.com/ml/gdb/2000-06/msg00028.html > ) which takes the result of comparing (via diff -u) the original > sources with the protoized sources and produces a C source file in > which the portions from the protoized sources are used to construct > prototypes and the portions from the original sources are used to > construct (potentially?) corresponding function definitions. We can > then invoke the C compiler (gcc -c -Wall) on the result and see what > kinds of warnings and errors are produced. I saw that script, but I don't like the idea to depend on a human to judge what GCC warnings are okay to ignore and what aren't. I'd prefer an automated tool that would give a definite yes/no answer, if that's possible. ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: Proposal: convert function definitions to prototyped form 2000-06-03 10:50 ` Kevin Buettner 2000-06-03 11:37 ` Eli Zaretskii @ 2000-06-03 18:18 ` Andrew Cagney 1 sibling, 0 replies; 57+ messages in thread From: Andrew Cagney @ 2000-06-03 18:18 UTC (permalink / raw) To: Kevin Buettner; +Cc: Eli Zaretskii, gdb Kevin Buettner wrote: > > On Jun 3, 6:58am, Eli Zaretskii wrote: > > > So with a script, we will always need a verification tool that can be > > trusted to find any potential bugs introduced by reformatting. > > Right. > > That's why I wrote check-decls (see > http://sourceware.cygnus.com/ml/gdb/2000-06/msg00028.html > ) which takes the result of comparing (via diff -u) the original > sources with the protoized sources and produces a C source file in > which the portions from the protoized sources are used to construct > prototypes and the portions from the original sources are used to > construct (potentially?) corresponding function definitions. We can > then invoke the C compiler (gcc -c -Wall) on the result and see what > kinds of warnings and errors are produced. Rather than -Wall (badly missnamed) I'd suggest several more carefully selected flags such as: -Wimplict -Wreturn-type -Wstrict-prototypes -Wmissing-declarations. No need to make life difficult for your self :-) Andrew ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: Proposal: convert function definitions to prototyped form [not found] ` <eliz@delorie.com> 2000-06-03 10:50 ` Kevin Buettner @ 2000-06-03 15:42 ` Kevin Buettner 2001-02-16 0:45 ` [RFC] Unified watchpoints for x86 platforms Kevin Buettner 2 siblings, 0 replies; 57+ messages in thread From: Kevin Buettner @ 2000-06-03 15:42 UTC (permalink / raw) To: Eli Zaretskii; +Cc: gdb On Jun 3, 2:37pm, Eli Zaretskii wrote: > > Date: Sat, 3 Jun 2000 10:50:39 -0700 > > From: Kevin Buettner <kevinb@cygnus.com> > > > > That's why I wrote check-decls (see > > http://sourceware.cygnus.com/ml/gdb/2000-06/msg00028.html > > ) which takes the result of comparing (via diff -u) the original > > sources with the protoized sources and produces a C source file in > > which the portions from the protoized sources are used to construct > > prototypes and the portions from the original sources are used to > > construct (potentially?) corresponding function definitions. We can > > then invoke the C compiler (gcc -c -Wall) on the result and see what > > kinds of warnings and errors are produced. > > I saw that script, but I don't like the idea to depend on a human to > judge what GCC warnings are okay to ignore and what aren't. I'd > prefer an automated tool that would give a definite yes/no answer, if > that's possible. I see your point. If I do as Andrew suggests and prevent fix-decls from looking at the *-share directories, that will prevent the arg_type conflict which is responsible for the errors. That leaves only the exit() warning from standalone.c to worry about. I can probably devise a solution which will prevent that warning from occurring as well. Anyway... I will see if I can arrange it so that any error or warning indicates a problem and no errors or warnings indicates that all's well. Kevin ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC] Unified watchpoints for x86 platforms [not found] ` <eliz@delorie.com> 2000-06-03 10:50 ` Kevin Buettner 2000-06-03 15:42 ` Kevin Buettner @ 2001-02-16 0:45 ` Kevin Buettner 2 siblings, 0 replies; 57+ messages in thread From: Kevin Buettner @ 2001-02-16 0:45 UTC (permalink / raw) To: Eli Zaretskii; +Cc: gdb On Feb 16, 2:29am, Eli Zaretskii wrote: > > We're going to need to pass a PID, or perhaps some new representation > > of a execution context, to a lot of code functions that don't allready > > have such an argument. > > Sorry, I'm not sure I'm following: why do you envision we'll need to > pass the PID to functions that don't receive it today? What > function(s) did you have in mind? The idea (I think) is to make most uses of ``inferior_pid'' go away. In its place will be occurrences of PIDGET (ptid) (or something along these lines) where ptid is passed from somewhere else. As a result, it will very likely become necessary to pass the execution context (or perhaps an identifier representing the execution context) as a parameter or perhaps as a member of some larger structure. See http://sources.redhat.com/ml/gdb-patches/2000-10/msg00014.html This patch doesn't address the elimination of inferior_pid, but it does take a first step towards providing a more robust execution context identifier in the sense that it is now possible to encode a process id, thread id, and lightweight process id in one of these identifiers. At the moment, we use a single int to encode the following: - a process id (alone) - a process id and a thread id - a process id and an lwp id IIRC, there are also times where the int in question ends up containing just the thread id or just the lwp id. Kevin ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: gdb doesn't work very well with dynamic linked binaries @ 2000-09-07 1:55 James Cownie 2000-09-07 3:09 ` Mark Kettenis 0 siblings, 1 reply; 57+ messages in thread From: James Cownie @ 2000-09-07 1:55 UTC (permalink / raw) To: gdb Mark Kettenis wrote :- I'm not sure whether the debug registers are per-thread or per-VM-space in Linux. I'll probably need to look into the kernel source. To save you the time, they are per-thread, just like all the other process' registers. They are conceptually saved and restored on process scheduling events (which for linuxthreads is the same thing as thread scheduling events, since linuxthreads _are_ processes as far as the scheduler is concerned). The bug I mentioned previously is exactly that they're getting cleared by the kernel and then not getting restored on return to user space, leaving them wrong until the next reschedule :-( -- -- Jim James Cownie jcownie@etnus.com Etnus, Inc. Phone +44 117 9071438 ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: gdb doesn't work very well with dynamic linked binaries 2000-09-07 1:55 gdb doesn't work very well with dynamic linked binaries James Cownie @ 2000-09-07 3:09 ` Mark Kettenis 2000-09-07 8:02 ` Eli Zaretskii 2000-09-09 14:39 ` gdb doesn't work very well with dynamic linked binaries Peter.Schauer 0 siblings, 2 replies; 57+ messages in thread From: Mark Kettenis @ 2000-09-07 3:09 UTC (permalink / raw) To: jcownie; +Cc: gdb, eliz Date: Thu, 7 Sep 2000 04:55:38 -0400 From: James Cownie <jcownie@etnus.com> Mark Kettenis wrote :- I'm not sure whether the debug registers are per-thread or per-VM-space in Linux. I'll probably need to look into the kernel source. To save you the time, they are per-thread, just like all the other process' registers. Thanks, that was what I suspected from looking at the code. They are conceptually saved and restored on process scheduling events (which for linuxthreads is the same thing as thread scheduling events, since linuxthreads _are_ processes as far as the scheduler is concerned). Yep. This means that getting HW watchpoints working for multi-threaded processes is a bit difficult, since GDB expects them to be process-wide. So any HW watchpoints will have to be inserted in *all* threads, not just one as we do now. Eli, this probably means that adding the debugging registers to GDB's register cache isn't a good idea. Something more specialized is needed, i.e. a native-dependent interface that updates the address and control register in all threads. This might implicate that keeping the actual HW watchpoint support a native-only thing is a good idea. The bug I mentioned previously is exactly that they're getting cleared by the kernel and then not getting restored on return to user space, leaving them wrong until the next reschedule :-( I think I understand the problems now. It basically means that one cannot reliably watch area's that are somehow used in system calls. I suspect that Linux isn't the only kernel with this bug. AFAICS FreeBSD also simply disables any (user-space) watchpoints triggered from within the kernel. I don't know what the various x86 System V's (Solaris, SCO Open Server 5) do, but I wouldn't be surprised if it is broken there too. Mark ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: gdb doesn't work very well with dynamic linked binaries 2000-09-07 3:09 ` Mark Kettenis @ 2000-09-07 8:02 ` Eli Zaretskii 2000-09-08 8:30 ` Mark Kettenis 2000-09-09 14:39 ` gdb doesn't work very well with dynamic linked binaries Peter.Schauer 1 sibling, 1 reply; 57+ messages in thread From: Eli Zaretskii @ 2000-09-07 8:02 UTC (permalink / raw) To: kettenis; +Cc: jcownie, gdb > Date: Thu, 7 Sep 2000 12:09:50 +0200 (MET DST) > From: Mark Kettenis <kettenis@wins.uva.nl> > > Yep. This means that getting HW watchpoints working for > multi-threaded processes is a bit difficult, since GDB expects them to > be process-wide. So any HW watchpoints will have to be inserted in > *all* threads, not just one as we do now. > > Eli, this probably means that adding the debugging registers to GDB's > register cache isn't a good idea. Something more specialized is > needed, i.e. a native-dependent interface that updates the address and > control register in all threads. This might implicate that keeping > the actual HW watchpoint support a native-only thing is a good idea. Why ``native-dependent'' and not ``target-dependent''? Won't the same problem affect any multithreaded ia32 target? Or am I missing something? In any case, would a special array of debug registers be an okay solution? The elements of that array will be set by ia32_insert_watchpoint and ia32_remove_watchpoint (to be written), and target-dependent subroutines which resume the inferior and get control when the inferior is stopped will access that array to pass the registers to the debuggee. ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: gdb doesn't work very well with dynamic linked binaries 2000-09-07 8:02 ` Eli Zaretskii @ 2000-09-08 8:30 ` Mark Kettenis 2001-02-10 7:34 ` [RFC] Unified watchpoints for x86 platforms Eli Zaretskii 0 siblings, 1 reply; 57+ messages in thread From: Mark Kettenis @ 2000-09-08 8:30 UTC (permalink / raw) To: eliz; +Cc: gdb Date: Thu, 7 Sep 2000 11:00:32 -0400 (EDT) From: Eli Zaretskii <eliz@delorie.com> CC: jcownie@etnus.com, gdb@sources.redhat.com Content-Type: text Content-Length: 1217 > Date: Thu, 7 Sep 2000 12:09:50 +0200 (MET DST) > From: Mark Kettenis <kettenis@wins.uva.nl> > > Yep. This means that getting HW watchpoints working for > multi-threaded processes is a bit difficult, since GDB expects them to > be process-wide. So any HW watchpoints will have to be inserted in > *all* threads, not just one as we do now. > > Eli, this probably means that adding the debugging registers to GDB's > register cache isn't a good idea. Something more specialized is > needed, i.e. a native-dependent interface that updates the address and > control register in all threads. This might implicate that keeping > the actual HW watchpoint support a native-only thing is a good idea. Why ``native-dependent'' and not ``target-dependent''? Won't the same problem affect any multithreaded ia32 target? Or am I missing something? Well, we already have a special packet in the remote protocol for setting watchpoints, so in principle it isn't necessary to be ``target-dependant''. The problems are likely to be different for the other multi-threaded systems. Linux is kinda weird in that respect. In any case, would a special array of debug registers be an okay solution? The elements of that array will be set by ia32_insert_watchpoint and ia32_remove_watchpoint (to be written), and target-dependent subroutines which resume the inferior and get control when the inferior is stopped will access that array to pass the registers to the debuggee. I think it is important to realize that the address and control registers should be per-process, whereas the status register should be per-thread. On Linux, the code to actually pass the registers to the debuggee should probably live in the (currently native-only) threads-module. Oh, and please use an i386_ prefix for the function names instead of ia32_ for consistency :-). Mark ^ permalink raw reply [flat|nested] 57+ messages in thread
* [RFC] Unified watchpoints for x86 platforms 2000-09-08 8:30 ` Mark Kettenis @ 2001-02-10 7:34 ` Eli Zaretskii 2001-02-10 10:19 ` H . J . Lu 2001-02-15 3:48 ` Eli Zaretskii 0 siblings, 2 replies; 57+ messages in thread From: Eli Zaretskii @ 2001-02-10 7:34 UTC (permalink / raw) To: gdb [Yes, I know: it's been a looong time since I promised to make this happen. Still, it's better late than never ;-)] I started working on the unified support for hardware-assisted breakpoints and watchpoints on x86 platforms (see TODO). Since I don't feel I know enough about all the aspects of this on any platform but DJGPP, I thought I'd better get the framework agreed to before I start coding. Here's the API I suggest for use by higher-level GDB code: (Note: I'm not good at inventing names, so please suggest better ones if you want.) int i386_hwbp_insert (int pid, CORE_ADDR addr, int len, int kind); This function inserts a breakpoint or watchpoint to watch memory region starting at address ADDR whose length is LEN bytes. The watchpoint will watch said memory region for accesses whose type is defined by KIND: HW_READ break if the region is accessed for reading[1] HW_WRITE break if the region is accessed for writing HW_ACCESS break if the region is accessed for either reading or writing HW_IO_ACCESS same as HW_ACCESS type, but for I/O read/write access[2] HW_EXECUTE instruction execution breakpoint The function returns 0 upon success, else -1. Notes: [1] Since x86 doesn't support read data watchpoints, HW_READ will actually be implemented as a read/write watchpoint, and relies on higher-level GDB code to distinguish between reads and writes. The infrastructure to support this is already in place in breakpoint.c, since GDB 5.0. [2] I/O watchpoints are not currently supported (AFAIK) by GDB on any x86 platform. I can provide the code to handle it, but do people think we should define a command to access this feature? If so, should we provide separate read, write, and access types of watchpoints, or a single access type (the only one supported by x86's debug registers) is enough? Note that I/O watchpoints require that the DE (debug extensions) flag in the CR4 register be set. I don't know what platforms set it and under what circumstances. int i386_hwbp_remove (int pid, CORE_ADDR addr, int len, int kind); This function removes a breakpoint of watchpoint at address ADDR which watches a memory region of LEN bytes and whose type is given by KIND. It returns 0 upon success, else -1. int i386_hwbp_region_ok (CORE_ADDR addr, int len); This function tests whether a memory region of LEN bytes starting at ADDR can be watched with debug registers. It returns 1 if the region can be watched, 0 otherwise. int i386_hwbp_stopped_by_watchpoint (int pid); This function returns the address of a breakpoint or watchpoint which could have stopped the debuggee. If no watchpoint triggered, it returns 0. To actually insert and remove breakpoints and watchpoints, I need low-level system-dependent functions. Here's the API I suggest for this low-levwl layer. (These are macros so that targets could define them on their nm-*.h files. On a typical Unix or GNU/Linux system, each of these macros will call `ptrace' with suitable arguments.) void HWBP_SET_ADDR (int pid, int dr_num, CORE_ADDR addr); This macro sets the debug register DR_NUM in the inferior to watch the address ADDR. DR_NUM can be in the range [0..3]. void HWBP_SET_CONTROL (int pid, unsigned int val); This macro sets the DR7 debug control register in the inferior to the value VAL. unsigned int HWBP_GET_STATUS (int pid); This macro returns the value of the DR6 debug status register from the inferior. In the discussion we had back in September, Mark said that the status register should be per thread. Does that mean that we need an additional argument (int tid?) to pass to HWBP_GET_STATUS? If so, how will this argument get into the i386_hwbp_* functions which will call these macros? Or maybe the target end can figure out the thread id by itself with some TIDGET(pid) magic? Comments? Suggestions? Flames? ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC] Unified watchpoints for x86 platforms 2001-02-10 7:34 ` [RFC] Unified watchpoints for x86 platforms Eli Zaretskii @ 2001-02-10 10:19 ` H . J . Lu 2001-02-10 11:28 ` Eli Zaretskii 2001-02-15 3:48 ` Eli Zaretskii 1 sibling, 1 reply; 57+ messages in thread From: H . J . Lu @ 2001-02-10 10:19 UTC (permalink / raw) To: Eli Zaretskii; +Cc: gdb On Sat, Feb 10, 2001 at 10:33:52AM -0500, Eli Zaretskii wrote: > unsigned int HWBP_GET_STATUS (int pid); > > This macro returns the value of the DR6 debug status register from > the inferior. > > In the discussion we had back in September, Mark said that the > status register should be per thread. Does that mean that we need > an additional argument (int tid?) to pass to HWBP_GET_STATUS? If > so, how will this argument get into the i386_hwbp_* functions which > will call these macros? What is the difference between pid and tid in this case? Can we derive tid from pid? > > Or maybe the target end can figure out the thread id by itself with > some TIDGET(pid) magic? > > > Comments? Suggestions? Flames? I haven't looked at it in detail. I guess Linux can live with the one everyone agrees up on. If it turns out it is not true, I hope we can still change it :-(. Unfortunately, I don't have the time to try it out now. However, I will give it a try when the OS independent part is checked in and noone is working on Linux. Thanks. -- H.J. Lu (hjl@valinux.com) ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC] Unified watchpoints for x86 platforms 2001-02-10 10:19 ` H . J . Lu @ 2001-02-10 11:28 ` Eli Zaretskii 0 siblings, 0 replies; 57+ messages in thread From: Eli Zaretskii @ 2001-02-10 11:28 UTC (permalink / raw) To: hjl; +Cc: gdb > Date: Sat, 10 Feb 2001 10:18:37 -0800 > From: "H . J . Lu" <hjl@valinux.com> > > On Sat, Feb 10, 2001 at 10:33:52AM -0500, Eli Zaretskii wrote: > > unsigned int HWBP_GET_STATUS (int pid); > > > > This macro returns the value of the DR6 debug status register from > > the inferior. > > > > In the discussion we had back in September, Mark said that the > > status register should be per thread. Does that mean that we need > > an additional argument (int tid?) to pass to HWBP_GET_STATUS? If > > so, how will this argument get into the i386_hwbp_* functions which > > will call these macros? > > What is the difference between pid and tid in this case? Can we derive > tid from pid? That's what I'd like to know as well. Anyone? ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC] Unified watchpoints for x86 platforms 2001-02-10 7:34 ` [RFC] Unified watchpoints for x86 platforms Eli Zaretskii 2001-02-10 10:19 ` H . J . Lu @ 2001-02-15 3:48 ` Eli Zaretskii 2001-02-15 8:17 ` Mark Kettenis 2001-02-15 9:08 ` H . J . Lu 1 sibling, 2 replies; 57+ messages in thread From: Eli Zaretskii @ 2001-02-15 3:48 UTC (permalink / raw) To: gdb Ping! No one posted any approvals or disprovals of this design. Do I take the silence as a sign of agreement and start coding? > I started working on the unified support for hardware-assisted > breakpoints and watchpoints on x86 platforms (see TODO). Since I > don't feel I know enough about all the aspects of this on any platform > but DJGPP, I thought I'd better get the framework agreed to before I > start coding. > > Here's the API I suggest for use by higher-level GDB code: > > (Note: I'm not good at inventing names, so please suggest better > ones if you want.) > > int i386_hwbp_insert (int pid, CORE_ADDR addr, int len, int kind); > > This function inserts a breakpoint or watchpoint to watch memory > region starting at address ADDR whose length is LEN bytes. The > watchpoint will watch said memory region for accesses whose type > is defined by KIND: > > HW_READ break if the region is accessed for reading[1] > HW_WRITE break if the region is accessed for writing > HW_ACCESS break if the region is accessed for either > reading or writing > HW_IO_ACCESS same as HW_ACCESS type, but for I/O read/write > access[2] > HW_EXECUTE instruction execution breakpoint > > The function returns 0 upon success, else -1. > > Notes: > [1] Since x86 doesn't support read data watchpoints, HW_READ will > actually be implemented as a read/write watchpoint, and relies > on higher-level GDB code to distinguish between reads and > writes. The infrastructure to support this is already in place > in breakpoint.c, since GDB 5.0. > > [2] I/O watchpoints are not currently supported (AFAIK) by GDB on > any x86 platform. I can provide the code to handle it, but do > people think we should define a command to access this feature? > If so, should we provide separate read, write, and access types > of watchpoints, or a single access type (the only one supported > by x86's debug registers) is enough? > > Note that I/O watchpoints require that the DE (debug extensions) > flag in the CR4 register be set. I don't know what platforms > set it and under what circumstances. > > int i386_hwbp_remove (int pid, CORE_ADDR addr, int len, int kind); > > This function removes a breakpoint of watchpoint at address ADDR > which watches a memory region of LEN bytes and whose type is given > by KIND. It returns 0 upon success, else -1. > > int i386_hwbp_region_ok (CORE_ADDR addr, int len); > > This function tests whether a memory region of LEN bytes starting at > ADDR can be watched with debug registers. It returns 1 if the > region can be watched, 0 otherwise. > > int i386_hwbp_stopped_by_watchpoint (int pid); > > This function returns the address of a breakpoint or watchpoint > which could have stopped the debuggee. If no watchpoint triggered, > it returns 0. > > To actually insert and remove breakpoints and watchpoints, I need > low-level system-dependent functions. Here's the API I suggest for > this low-levwl layer. (These are macros so that targets could define > them on their nm-*.h files. On a typical Unix or GNU/Linux system, > each of these macros will call `ptrace' with suitable arguments.) > > void HWBP_SET_ADDR (int pid, int dr_num, CORE_ADDR addr); > > This macro sets the debug register DR_NUM in the inferior to watch > the address ADDR. DR_NUM can be in the range [0..3]. > > void HWBP_SET_CONTROL (int pid, unsigned int val); > > This macro sets the DR7 debug control register in the inferior to > the value VAL. > > unsigned int HWBP_GET_STATUS (int pid); > > This macro returns the value of the DR6 debug status register from > the inferior. > > In the discussion we had back in September, Mark said that the > status register should be per thread. Does that mean that we need > an additional argument (int tid?) to pass to HWBP_GET_STATUS? If > so, how will this argument get into the i386_hwbp_* functions which > will call these macros? > > Or maybe the target end can figure out the thread id by itself with > some TIDGET(pid) magic? > > > Comments? Suggestions? Flames? > ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC] Unified watchpoints for x86 platforms 2001-02-15 3:48 ` Eli Zaretskii @ 2001-02-15 8:17 ` Mark Kettenis 2001-02-15 9:32 ` Eli Zaretskii 2001-02-15 10:41 ` Kevin Buettner 2001-02-15 9:08 ` H . J . Lu 1 sibling, 2 replies; 57+ messages in thread From: Mark Kettenis @ 2001-02-15 8:17 UTC (permalink / raw) To: Eli Zaretskii; +Cc: gdb Eli Zaretskii <eliz@is.elta.co.il> writes: > Ping! > > No one posted any approvals or disprovals of this design. Do I take > the silence as a sign of agreement and start coding? Sorry for not responding earlier. In general, your proposal looks fine, but I think it is best to export functions similar to GDB's target_* functions: int i386_remove_watchpoint (CORE_ADDR addr, int len, enum target_hw_bp_type type); int i386_insert_watchpoint (CORE_ADDR addr, int len, enum target_hw_bp_type type); int i386_insert_hw_breakpoint (CORE_ADDR addr, void *shadow); int i386_remove_hw_breakpoint (CORE_ADDR addr, void *shadow); etc. Of course you can implement those on top of the functions mentioned below. > > I started working on the unified support for hardware-assisted > > breakpoints and watchpoints on x86 platforms (see TODO). Since I > > don't feel I know enough about all the aspects of this on any platform > > but DJGPP, I thought I'd better get the framework agreed to before I > > start coding. > > > > Here's the API I suggest for use by higher-level GDB code: > > > > (Note: I'm not good at inventing names, so please suggest better > > ones if you want.) > > > > int i386_hwbp_insert (int pid, CORE_ADDR addr, int len, int kind); Is there any particular reason why you need the PID argument? AFAICS it will always be equal to INFERIOR_PID, so I think we can do without it. This is also true for the other i386_hwbp_* functions you're proposing. > > > > This function inserts a breakpoint or watchpoint to watch memory > > region starting at address ADDR whose length is LEN bytes. The > > watchpoint will watch said memory region for accesses whose type > > is defined by KIND: > > > > HW_READ break if the region is accessed for reading[1] > > HW_WRITE break if the region is accessed for writing > > HW_ACCESS break if the region is accessed for either > > reading or writing > > HW_IO_ACCESS same as HW_ACCESS type, but for I/O read/write > > access[2] > > HW_EXECUTE instruction execution breakpoint Please consider using an enum instead of an int. It looks as if GDB's convention is to use lower-case names for enum values. > > The function returns 0 upon success, else -1. > > > > Notes: > > [1] Since x86 doesn't support read data watchpoints, HW_READ will > > actually be implemented as a read/write watchpoint, and relies > > on higher-level GDB code to distinguish between reads and > > writes. The infrastructure to support this is already in place > > in breakpoint.c, since GDB 5.0. Sounds OK. > > [2] I/O watchpoints are not currently supported (AFAIK) by GDB on > > any x86 platform. I can provide the code to handle it, but do > > people think we should define a command to access this feature? > > If so, should we provide separate read, write, and access types > > of watchpoints, or a single access type (the only one supported > > by x86's debug registers) is enough? I think this can be added later if people want it. > > Note that I/O watchpoints require that the DE (debug extensions) > > flag in the CR4 register be set. I don't know what platforms > > set it and under what circumstances. I don't think you can do this in any of the x86 Unixoid systems. > > > > int i386_hwbp_remove (int pid, CORE_ADDR addr, int len, int kind); > > > > This function removes a breakpoint of watchpoint at address ADDR > > which watches a memory region of LEN bytes and whose type is given > > by KIND. It returns 0 upon success, else -1. > > > > int i386_hwbp_region_ok (CORE_ADDR addr, int len); > > > > This function tests whether a memory region of LEN bytes starting at > > ADDR can be watched with debug registers. It returns 1 if the > > region can be watched, 0 otherwise. > > > > int i386_hwbp_stopped_by_watchpoint (int pid); > > > > This function returns the address of a breakpoint or watchpoint > > which could have stopped the debuggee. If no watchpoint triggered, > > it returns 0. > > > > To actually insert and remove breakpoints and watchpoints, I need > > low-level system-dependent functions. Here's the API I suggest for > > this low-levwl layer. (These are macros so that targets could define > > them on their nm-*.h files. On a typical Unix or GNU/Linux system, > > each of these macros will call `ptrace' with suitable arguments.) > > > > void HWBP_SET_ADDR (int pid, int dr_num, CORE_ADDR addr); > > > > This macro sets the debug register DR_NUM in the inferior to watch > > the address ADDR. DR_NUM can be in the range [0..3]. > > > > void HWBP_SET_CONTROL (int pid, unsigned int val); > > > > This macro sets the DR7 debug control register in the inferior to > > the value VAL. > > > > unsigned int HWBP_GET_STATUS (int pid); > > > > This macro returns the value of the DR6 debug status register from > > the inferior. Why not have simply long I386_GET_DR(int regnum) and I386_SET_DR(int regnum, long value) and let the system-dependent decide how to map the debug register number ([0..7], as used in the Intel documentation) into whatever is needed? > > In the discussion we had back in September, Mark said that the > > status register should be per thread. Does that mean that we need > > an additional argument (int tid?) to pass to HWBP_GET_STATUS? If > > so, how will this argument get into the i386_hwbp_* functions which > > will call these macros? I don't think an additional argument is needed. When calling HWBP_GET_STATUS, it is the current thread that has encountered a trap, and INFERIOR_PID should be set appropriately. > > Or maybe the target end can figure out the thread id by itself with > > some TIDGET(pid) magic? Indeed. Mark ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC] Unified watchpoints for x86 platforms 2001-02-15 8:17 ` Mark Kettenis @ 2001-02-15 9:32 ` Eli Zaretskii 2001-02-15 10:33 ` Mark Kettenis 2001-02-15 10:41 ` Kevin Buettner 1 sibling, 1 reply; 57+ messages in thread From: Eli Zaretskii @ 2001-02-15 9:32 UTC (permalink / raw) To: kettenis; +Cc: gdb > From: Mark Kettenis <kettenis@wins.uva.nl> > Date: 15 Feb 2001 17:17:17 +0100 > > In general, your proposal looks > fine, but I think it is best to export functions similar to GDB's > target_* functions: > > int i386_remove_watchpoint (CORE_ADDR addr, int len, > enum target_hw_bp_type type); > int i386_insert_watchpoint (CORE_ADDR addr, int len, > enum target_hw_bp_type type); > > int i386_insert_hw_breakpoint (CORE_ADDR addr, void *shadow); > int i386_remove_hw_breakpoint (CORE_ADDR addr, void *shadow); I didn't want to use those names because they are taken by i386v-nat.c. If we use these names, we will have to modify i386v-nat.c, which is used by many x86 platforms. I didn't want such a profound effect; some platform maintainers might not want the new interface, and will be less than happy to rewrite their code for no good reason. > > > int i386_hwbp_insert (int pid, CORE_ADDR addr, int len, int kind); > > Is there any particular reason why you need the PID argument? Just the existing interfaces. DJGPP obviously doesn't use that argument. > > > HW_READ break if the region is accessed for reading[1] > > > HW_WRITE break if the region is accessed for writing > > > HW_ACCESS break if the region is accessed for either > > > reading or writing > > > HW_IO_ACCESS same as HW_ACCESS type, but for I/O read/write > > > access[2] > > > HW_EXECUTE instruction execution breakpoint > > Please consider using an enum instead of an int. It looks as if GDB's > convention is to use lower-case names for enum values. I intended to use an enum. I believe the upper-case names were just a coincidence. > > > void HWBP_SET_ADDR (int pid, int dr_num, CORE_ADDR addr); > > > > > > This macro sets the debug register DR_NUM in the inferior to watch > > > the address ADDR. DR_NUM can be in the range [0..3]. > > > > > > void HWBP_SET_CONTROL (int pid, unsigned int val); > > > > > > This macro sets the DR7 debug control register in the inferior to > > > the value VAL. > > > > > > unsigned int HWBP_GET_STATUS (int pid); > > > > > > This macro returns the value of the DR6 debug status register from > > > the inferior. > > Why not have simply long I386_GET_DR(int regnum) and I386_SET_DR(int > regnum, long value) and let the system-dependent decide how to map the > debug register number ([0..7], as used in the Intel documentation) > into whatever is needed? Why bother the target end to do that? They will all do the same, and the code I'll write knows exactly when it needs what register. In addition, I386_GET_DR might mislead someone into thinking that it actually gets the value of any DRi from the inferior. That was not my intention: I don't need to fetch any debug register except DR6. Thanks for the feedback. I think I will start coding this weekend, so if someone has more input, please hurry ;-) ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC] Unified watchpoints for x86 platforms 2001-02-15 9:32 ` Eli Zaretskii @ 2001-02-15 10:33 ` Mark Kettenis 2001-02-15 13:26 ` Eli Zaretskii 0 siblings, 1 reply; 57+ messages in thread From: Mark Kettenis @ 2001-02-15 10:33 UTC (permalink / raw) To: eliz; +Cc: gdb Date: Thu, 15 Feb 2001 12:32:03 -0500 (EST) From: Eli Zaretskii <eliz@delorie.com> > From: Mark Kettenis <kettenis@wins.uva.nl> > Date: 15 Feb 2001 17:17:17 +0100 > > In general, your proposal looks > fine, but I think it is best to export functions similar to GDB's > target_* functions: > > int i386_remove_watchpoint (CORE_ADDR addr, int len, > enum target_hw_bp_type type); > int i386_insert_watchpoint (CORE_ADDR addr, int len, > enum target_hw_bp_type type); > > int i386_insert_hw_breakpoint (CORE_ADDR addr, void *shadow); > int i386_remove_hw_breakpoint (CORE_ADDR addr, void *shadow); I didn't want to use those names because they are taken by i386v-nat.c. If we use these names, we will have to modify i386v-nat.c, which is used by many x86 platforms. I didn't want such a profound effect; some platform maintainers might not want the new interface, and will be less than happy to rewrite their code for no good reason. Ah, but that isn't an issue here. Targets using the old code from i386v-nat.c won't use the generic code and vice versa. Moreover, I think I'll re-implement the stuff in i386v-nat.c with the help of the generic code. And as a last resort we can always rename the i386_* functions in i386v-nat.c into i386v_*. And really the only platforms that actually use that code Linux and SCO Open Server 5. So please use the names I suggest above. I think they're much clearer. > > > int i386_hwbp_insert (int pid, CORE_ADDR addr, int len, int kind); > > Is there any particular reason why you need the PID argument? Just the existing interfaces. DJGPP obviously doesn't use that argument. Linux and SCO Open Server don't need it either. So please drop it. > > > HW_READ break if the region is accessed for reading[1] > > > HW_WRITE break if the region is accessed for writing > > > HW_ACCESS break if the region is accessed for either > > > reading or writing > > > HW_IO_ACCESS same as HW_ACCESS type, but for I/O read/write > > > access[2] > > > HW_EXECUTE instruction execution breakpoint > > Please consider using an enum instead of an int. It looks as if GDB's > convention is to use lower-case names for enum values. I intended to use an enum. I believe the upper-case names were just a coincidence. See Andrews reply on that. > > > void HWBP_SET_ADDR (int pid, int dr_num, CORE_ADDR addr); > > > > > > This macro sets the debug register DR_NUM in the inferior to watch > > > the address ADDR. DR_NUM can be in the range [0..3]. > > > > > > void HWBP_SET_CONTROL (int pid, unsigned int val); > > > > > > This macro sets the DR7 debug control register in the inferior to > > > the value VAL. > > > > > > unsigned int HWBP_GET_STATUS (int pid); > > > > > > This macro returns the value of the DR6 debug status register from > > > the inferior. > > Why not have simply long I386_GET_DR(int regnum) and I386_SET_DR(int > regnum, long value) and let the system-dependent decide how to map the > debug register number ([0..7], as used in the Intel documentation) > into whatever is needed? Why bother the target end to do that? They will all do the same, and the code I'll write knows exactly when it needs what register. It would make the implementation on Linux easier :-). But the difference isn't really that big. In addition, I386_GET_DR might mislead someone into thinking that it actually gets the value of any DRi from the inferior. That was not my intention: I don't need to fetch any debug register except DR6. Good point. Mark ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC] Unified watchpoints for x86 platforms 2001-02-15 10:33 ` Mark Kettenis @ 2001-02-15 13:26 ` Eli Zaretskii 0 siblings, 0 replies; 57+ messages in thread From: Eli Zaretskii @ 2001-02-15 13:26 UTC (permalink / raw) To: kettenis; +Cc: gdb > Date: Thu, 15 Feb 2001 19:33:40 +0100 > From: Mark Kettenis <kettenis@wins.uva.nl> > > So please use the names I suggest above. Will do. > I think they're much clearer. Yes, I prefer them as well. > > Why not have simply long I386_GET_DR(int regnum) and I386_SET_DR(int > > regnum, long value) and let the system-dependent decide how to map the > > debug register number ([0..7], as used in the Intel documentation) > > into whatever is needed? > > Why bother the target end to do that? They will all do the same, and > the code I'll write knows exactly when it needs what register. > > It would make the implementation on Linux easier :-). Could you please explain in a few words why? Even if I don't change the interface, I think it is important for me to know about such subtle aspects. What I see in i386v-nat.c is that each of these macros maps directly into a ptrace call with appropriate arguments. For example, here's how DR7 (the debug control register) is set: ptrace (6, pid, offsetof (struct user, u_debugreg[DR_CONTROL]), debug_control_mirror); and here's how DR6, the debug status register, is fetched: status = ptrace (3, pid, offsetof (struct user, u_debugreg[DR_STATUS]), 0); Even ia64-linux-nat.c does something very similar, even though it is not one of the potential customers of what I'm about to write. ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC] Unified watchpoints for x86 platforms 2001-02-15 8:17 ` Mark Kettenis 2001-02-15 9:32 ` Eli Zaretskii @ 2001-02-15 10:41 ` Kevin Buettner 2001-02-15 13:26 ` Eli Zaretskii 1 sibling, 1 reply; 57+ messages in thread From: Kevin Buettner @ 2001-02-15 10:41 UTC (permalink / raw) To: Mark Kettenis, Eli Zaretskii; +Cc: gdb On Feb 15, 5:17pm, Mark Kettenis wrote: > > > I started working on the unified support for hardware-assisted > > > breakpoints and watchpoints on x86 platforms (see TODO). Since I > > > don't feel I know enough about all the aspects of this on any platform > > > but DJGPP, I thought I'd better get the framework agreed to before I > > > start coding. > > > > > > Here's the API I suggest for use by higher-level GDB code: > > > > > > (Note: I'm not good at inventing names, so please suggest better > > > ones if you want.) > > > > > > int i386_hwbp_insert (int pid, CORE_ADDR addr, int len, int kind); > > Is there any particular reason why you need the PID argument? AFAICS > it will always be equal to INFERIOR_PID, so I think we can do without > it. This is also true for the other i386_hwbp_* functions you're > proposing. I think it'd be better to not rely on ``inferior_pid''. I would rather see the explicitly passed. There will come a day when GDB is able to debug more than one process at a time and to perpetuate reliance on inferior pid would be short sighted. > > > In the discussion we had back in September, Mark said that the > > > status register should be per thread. Does that mean that we need > > > an additional argument (int tid?) to pass to HWBP_GET_STATUS? If > > > so, how will this argument get into the i386_hwbp_* functions which > > > will call these macros? > > I don't think an additional argument is needed. When calling > HWBP_GET_STATUS, it is the current thread that has encountered a trap, > and INFERIOR_PID should be set appropriately. > > > > Or maybe the target end can figure out the thread id by itself with > > > some TIDGET(pid) magic? Hopefully I'll find time to merge my pid/tid/lwp patch sometime soon. When this occurs, you'll be able to extract the thread id from what is now the pid argument. I have read the rest of Eli's proposal as well as Mark's comments and I agree with the rest of Mark's remarks. Kevin ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC] Unified watchpoints for x86 platforms 2001-02-15 10:41 ` Kevin Buettner @ 2001-02-15 13:26 ` Eli Zaretskii 2001-02-15 14:46 ` J.T. Conklin 0 siblings, 1 reply; 57+ messages in thread From: Eli Zaretskii @ 2001-02-15 13:26 UTC (permalink / raw) To: kevinb; +Cc: kettenis, gdb > Date: Thu, 15 Feb 2001 11:41:35 -0700 > From: Kevin Buettner <kevinb@cygnus.com> > > > > Is there any particular reason why you need the PID argument? AFAICS > > it will always be equal to INFERIOR_PID, so I think we can do without > > it. This is also true for the other i386_hwbp_* functions you're > > proposing. > > I think it'd be better to not rely on ``inferior_pid''. I would > rather see the explicitly passed. There will come a day when GDB > is able to debug more than one process at a time and to perpetuate > reliance on inferior pid would be short sighted. I have two opposite opinions here. We need to resolve this somehow. > I have read the rest of Eli's proposal as well as Mark's comments and > I agree with the rest of Mark's remarks. Thanks for the feedback. ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC] Unified watchpoints for x86 platforms 2001-02-15 13:26 ` Eli Zaretskii @ 2001-02-15 14:46 ` J.T. Conklin 2001-02-15 16:11 ` Kevin Buettner ` (2 more replies) 0 siblings, 3 replies; 57+ messages in thread From: J.T. Conklin @ 2001-02-15 14:46 UTC (permalink / raw) To: Eli Zaretskii; +Cc: kevinb, kettenis, gdb >>>>> "Eli" == Eli Zaretskii <eliz@delorie.com> writes: >> > Is there any particular reason why you need the PID argument? AFAICS >> > it will always be equal to INFERIOR_PID, so I think we can do without >> > it. This is also true for the other i386_hwbp_* functions you're >> > proposing. >> >> I think it'd be better to not rely on ``inferior_pid''. I would >> rather see the explicitly passed. There will come a day when GDB >> is able to debug more than one process at a time and to perpetuate >> reliance on inferior pid would be short sighted. Eli> I have two opposite opinions here. We need to resolve this somehow. We're going to need to pass a PID, or perhaps some new representation of a execution context, to a lot of code functions that don't allready have such an argument. It is not clear to me that adding such an argument "because it will be needed" is correct, considering that the design has not yet started. The truth is we don't know "what" will be needed, so we'll have to revisit this function (among many others) down the line anyway. --jtc -- J.T. Conklin RedBack Networks ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC] Unified watchpoints for x86 platforms 2001-02-15 14:46 ` J.T. Conklin @ 2001-02-15 16:11 ` Kevin Buettner 2001-02-15 23:29 ` Eli Zaretskii 2001-02-24 10:14 ` Eli Zaretskii 2001-02-15 23:30 ` [RFC] " Eli Zaretskii 2001-02-16 0:00 ` Mark Kettenis 2 siblings, 2 replies; 57+ messages in thread From: Kevin Buettner @ 2001-02-15 16:11 UTC (permalink / raw) To: jtc, Eli Zaretskii; +Cc: kevinb, kettenis, gdb On Feb 15, 2:45pm, J.T. Conklin wrote: > Subject: Re: [RFC] Unified watchpoints for x86 platforms > >>>>> "Eli" == Eli Zaretskii <eliz@delorie.com> writes: > >> > Is there any particular reason why you need the PID argument? AFAICS > >> > it will always be equal to INFERIOR_PID, so I think we can do without > >> > it. This is also true for the other i386_hwbp_* functions you're > >> > proposing. > >> > >> I think it'd be better to not rely on ``inferior_pid''. I would > >> rather see the explicitly passed. There will come a day when GDB > >> is able to debug more than one process at a time and to perpetuate > >> reliance on inferior pid would be short sighted. > > Eli> I have two opposite opinions here. We need to resolve this somehow. > > We're going to need to pass a PID, or perhaps some new representation > of a execution context, to a lot of code functions that don't allready > have such an argument. It is not clear to me that adding such an > argument "because it will be needed" is correct, considering that the > design has not yet started. The truth is we don't know "what" will be > needed, so we'll have to revisit this function (among many others) > down the line anyway. Eli, I think the answer is to use your best judgement. Regardless of whether you pass the pid in or simply use inferior_pid, your new watchpoint code is going to have to eventually be changed to use a different representation of the execution context. I happen to think that it might actually change less if you pass a parameter, but after thinking about it a bit more, I can see why someone else might hold the opposite opinion. Kevin ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC] Unified watchpoints for x86 platforms 2001-02-15 16:11 ` Kevin Buettner @ 2001-02-15 23:29 ` Eli Zaretskii 2001-02-24 10:14 ` Eli Zaretskii 1 sibling, 0 replies; 57+ messages in thread From: Eli Zaretskii @ 2001-02-15 23:29 UTC (permalink / raw) To: kevinb; +Cc: jtc, kettenis, gdb > Date: Thu, 15 Feb 2001 17:09:53 -0700 > From: Kevin Buettner <kevinb@cygnus.com> > > I think the answer is to use your best judgement. Regardless of > whether you pass the pid in or simply use inferior_pid, your new > watchpoint code is going to have to eventually be changed to use > a different representation of the execution context. I happen to > think that it might actually change less if you pass a parameter, but > after thinking about it a bit more, I can see why someone else might > hold the opposite opinion. Okay, thanks for the feedback. I'll sleep on this and then decide. ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC] Unified watchpoints for x86 platforms 2001-02-15 16:11 ` Kevin Buettner 2001-02-15 23:29 ` Eli Zaretskii @ 2001-02-24 10:14 ` Eli Zaretskii 2001-02-27 3:28 ` Mark Kettenis 1 sibling, 1 reply; 57+ messages in thread From: Eli Zaretskii @ 2001-02-24 10:14 UTC (permalink / raw) To: gdb While writing the code for the x86 watchpoint support, a question popped up regarding the Global Enable and Local Enable flags in the DR7 (Debug Control) register. For those who might not remember what those flags are: Each hardware-assisted breakpoint or watchpoint should be enabled by setting bits in the DR7 register. A watchpoint can be enabled locally or globally, depending on which bit in DR7 is set for that watchpoint. A watchpoint that is enabled locally will only break if it is hit in the current task. A watchpoint that is enabled globally will break in any task. Now, I understand that, by and large, x86 platforms enable watchpoints locally, to avoid spurious traps triggered by other processes. If that is true, should the code I write use the Local Enable flag unconditionally? Or perhaps it would be useful to define an API that enables a target to control whether the global or local enable flag is used? Opinions? ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC] Unified watchpoints for x86 platforms 2001-02-24 10:14 ` Eli Zaretskii @ 2001-02-27 3:28 ` Mark Kettenis 2001-02-27 10:57 ` Eli Zaretskii 2001-03-21 15:59 ` [RFA] " Eli Zaretskii 0 siblings, 2 replies; 57+ messages in thread From: Mark Kettenis @ 2001-02-27 3:28 UTC (permalink / raw) To: Eli Zaretskii; +Cc: gdb Eli Zaretskii <eliz@delorie.com> writes: > While writing the code for the x86 watchpoint support, a question > popped up regarding the Global Enable and Local Enable flags in the > DR7 (Debug Control) register. > > For those who might not remember what those flags are: Each > hardware-assisted breakpoint or watchpoint should be enabled by > setting bits in the DR7 register. A watchpoint can be enabled locally > or globally, depending on which bit in DR7 is set for that watchpoint. > A watchpoint that is enabled locally will only break if it is hit in > the current task. A watchpoint that is enabled globally will break in > any task. Makes me wonder if global watchpoints would really work if hardware task switching isn't used. Anyway, I guess most x86 OS'es wouldn't allow any real global watchpoints. For Linux it doesn't matter whether you set the local or the global bit. > Now, I understand that, by and large, x86 platforms enable watchpoints > locally, to avoid spurious traps triggered by other processes. If > that is true, should the code I write use the Local Enable flag > unconditionally? Or perhaps it would be useful to define an API that > enables a target to control whether the global or local enable flag is > used? In principle a GDB command to control whether to enable watchpoints locally or globally wouldn't be a bad idea. But I don't know any targets where this would be useful. So if it isn't useful for DJGPP either, I wouldn't bother. A comment about the issue at the right spot would be nice though. Mark ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC] Unified watchpoints for x86 platforms 2001-02-27 3:28 ` Mark Kettenis @ 2001-02-27 10:57 ` Eli Zaretskii 2001-03-21 15:59 ` [RFA] " Eli Zaretskii 1 sibling, 0 replies; 57+ messages in thread From: Eli Zaretskii @ 2001-02-27 10:57 UTC (permalink / raw) To: kettenis; +Cc: gdb > From: Mark Kettenis <kettenis@wins.uva.nl> > Date: 27 Feb 2001 12:28:43 +0100 > > > > For those who might not remember what those flags are: Each > > hardware-assisted breakpoint or watchpoint should be enabled by > > setting bits in the DR7 register. A watchpoint can be enabled locally > > or globally, depending on which bit in DR7 is set for that watchpoint. > > A watchpoint that is enabled locally will only break if it is hit in > > the current task. A watchpoint that is enabled globally will break in > > any task. > > Makes me wonder if global watchpoints would really work if hardware > task switching isn't used. I guess the answer depends on how exactly is the software task switching implemented. Namely, does it reset the GE and LE bits in DR7 when the task switch happens. > In principle a GDB command to control whether to enable watchpoints > locally or globally wouldn't be a bad idea. But I don't know any > targets where this would be useful. So if it isn't useful for DJGPP > either, I wouldn't bother. A comment about the issue at the right > spot would be nice though. That was my tendency as well. Thanks for the feedback. ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFA] Unified watchpoints for x86 platforms 2001-02-27 3:28 ` Mark Kettenis 2001-02-27 10:57 ` Eli Zaretskii @ 2001-03-21 15:59 ` Eli Zaretskii 1 sibling, 0 replies; 57+ messages in thread From: Eli Zaretskii @ 2001-03-21 15:59 UTC (permalink / raw) To: gdb This is the first cut of the code to support hardware breakpoints and watchpoints on all x86 platforms. It works for me in the DJGPP port. Please critique as you see fit. I'm still working on the doco changes for gdbint.texinfo, but I thought there's no reason not to start the review process right away. Note that the names of the functions and macros have changed a little since the draft API I posted here. Also, since Fernando said that there are remote targets which might want to use hardware watchpoints, I decided to put the code on i386-tdep.c. I'd like to thank everybody who participated in discussions and returned feedback. 2001-03-03 Eli Zaretskii <eliz@is.elta.co.il> Unified support for hardware breakpoints and watchpoints on x86 targets: * config/i386/tm-i386.h (TARGET_HAS_HARDWARE_WATCHPOINTS): Define. (i386_cleanup_dregs, i386_insert_watchpoint) (i386_remove_watchpoint, i386_region_ok_for_watchpoint) (i386_stopped_by_hwbp, i386_stopped_data_address) (i386_insert_hw_breakpoint, i386_remove_hw_breakpoint): Declare prototypes. (TARGET_CAN_USE_HARDWARE_WATCHPOINT) (TARGET_REGION_OK_FOR_HW_WATCHPOINT, HAVE_CONTINUABLE_WATCHPOINT) (STOPPED_BY_WATCHPOINT, target_stopped_data_address) (target_insert_watchpoint, target_remove_watchpoint) (target_insert_hw_breakpoint, target_remove_hw_breakpoint): Define to call the appropriate i386_* functions. * i386-tdep.c (I386_DR_CONTROL_MASK, I386_DR_LOCAL_ENABLE) (I386_DR_GLOBAL_ENABLE, I386_DR_DISABLE, I386_DR_SET_RW_LEN) (I386_DR_GET_RW_LEN, I386_DR_WATCH_HIT): New macros. (dr_mirror, dr_status_mirror, dr_control_mirror, dr_ref_count) (maint_show_dr): New variables. (i386_cleanup_dregs, i386_show_dr, i386_length_and_rw_bits) (i386_insert_aligned_watchpoint, i386_remove_aligned_watchpoint) (i386_handle_nonaligned_watchpoint, i386_insert_watchpoint) (i386_remove_watchpoint, i386_region_ok_for_watchpoint) (i386_stopped_data_address, i386_stopped_by_hwbp) (i386_insert_hw_breakpoint, i386_remove_hw_breakpoint): New functions. (_initialize_i386_tdep): Add new maint command `show-debug-regs', sets maint_show_dr to non-zero value and activates debugging print-outs in functions which insert, remove, and test watchpoints and hardware breakpoints. --- gdb/i386-tdep.c~0 Thu Dec 21 22:52:58 2000 +++ gdb/i386-tdep.c Sat Mar 3 23:18:12 2001 @@ -1,5 +1,5 @@ /* Intel 386 target-dependent stuff. - Copyright (C) 1988, 1989, 1991, 1994, 1995, 1996, 1998 + Copyright (C) 1988, 1989, 1991, 1994, 1995, 1996, 1998, 2001 Free Software Foundation, Inc. This file is part of GDB. @@ -858,6 +858,659 @@ i386_register_convert_to_raw (struct typ memcpy (to, from, FPU_REG_RAW_SIZE); } + +/* Support for hardware watchpoints and breakpoints using the x86 + debug registers. + + This provides several functions for inserting and removing + hardware-assisted breakpoints and watchpoints, testing if + one or more of the watchpoints triggerd and at what address, + checking whether a given region can be watched, etc. See + the section "Exported API functions" below. + + A target which wants to use these functions should define + several macros, such as `target_insert_watchpoint' and + `target_stopped_data_address', listed in target.h, to call + the appropriate functions below. + + Each target should also provide several low-level macros + that will be called to insert watchpoints and hardware + breakpoints into the inferior, remove them, and check their + status. These macros are: + + I386_DR_LOW_SET_CONTROL -- set the debug control (DR7) + register to a given value + + I386_DR_LOW_SET_ADDR -- put an address into one debug + register + + I386_DR_LOW_RESET_ADDR -- reset the address stored in + one debug register + + I386_DR_LOW_GET_STATUS -- return the value of the debug + status (DR6) register. + + The functions below implement debug registers sharing by + reference counts, and allow to watch regions up to 16 bytes + long. */ + +#ifdef TARGET_HAS_HARDWARE_WATCHPOINTS + +#ifdef HAVE_PTRACE_H +#include <ptrace.h> +#else +#ifdef HAVE_SYS_PTRACE_H +#include <sys/ptrace.h> +#endif +#endif + +#ifdef HAVE_SYS_USER +#include <sys/user.h> +#endif + +/* FIXME: The following should be just "#include <sys/debugreg.h>", + but the the Linux 2.1.x kernel and glibc 2.0.x are not in sync; + including <sys/debugreg.h> will result in an error. With luck, + these losers will get their act together and we can trash this hack + in the near future. + + --jsm 1998-10-21; modified by eliz 2001-02-24. */ + +#ifdef HAVE_ASM_DEBUGREG_H +#include <asm/debugreg.h> +#else /* !HAVE_ASM_DEBUGREG_H */ + +#ifdef HAVE_SYS_DEBUGREG_H +#include <sys/debugreg.h> +#else /* !HAVE_SYS_DEBUGREG_H */ + +/* Provide definitions for platforms which don't have debugreg.h, such + as DJGPP. As a side effect, explain what each macro means or does. */ + +/* Debug registers' indices. */ +#define DR_FIRSTADDR 0 /* index of first debug address register */ +#define DR_LASTADDR 3 /* index of last debug address register */ +#define DR_STATUS 6 /* index of debug status register (DR6) */ +#define DR_CONTROL 7 /* index of debug control register (DR7) */ + +/* DR7 Debug Control register fields. */ + +/* How many bits to skip in DR7 to get to R/W and LEN fields. */ +#define DR_CONTROL_SHIFT 16 +/* How many bits in DR7 per R/W and LEN field for each watchpoint. */ +#define DR_CONTROL_SIZE 4 + +/* Watchpoint/breakpoint read/write fields in DR7. */ +#define DR_RW_EXECUTE (0x0) /* break on instruction execution */ +#define DR_RW_WRITE (0x1) /* break on data writes */ +#define DR_RW_READ (0x3) /* break on data reads or writes */ + +/* Watchpoint/breakpoint length fields in DR7. The 2-bit left shift + is so we could OR this with the read/write field defined above. */ +#define DR_LEN_1 (0x0 << 2) /* 1-byte region watch or breakpt */ +#define DR_LEN_2 (0x1 << 2) /* 2-byte region watch */ +#define DR_LEN_4 (0x3 << 2) /* 4-byte region watch */ + +/* Local and Global Enable flags in DR7. + + When the Local Enable flag is set, the breakpoint/watchpoint is + enabled only for the current task; the processor automatically + clears this flag on every task switch. When the Global Enable + flag is set, the breakpoint/watchpoint is enabled for all tasks; + the processor never clears this flag. + + Currently, all watchpoint are locally enabled. If you need to + enable them globally, read the comment which pertains to this in + i386_insert_aligned_watchpoint below. */ +#define DR_LOCAL_ENABLE_SHIFT 0 /* extra shift to the local enable bit */ +#define DR_GLOBAL_ENABLE_SHIFT 1 /* extra shift to the global enable bit */ +#define DR_ENABLE_SIZE 2 /* 2 enable bits per debug register */ + +/* Local and global exact breakpoint enable flags (a.k.a. slowdown + flags). These are only required on i386, to allow detection of the + exact instruction which caused a watchpoint to break; i486 and + later processors do that automatically. We set these flags for + back compatibility. */ +#define DR_LOCAL_SLOWDOWN (0x100) +#define DR_GLOBAL_SLOWDOWN (0x200) + +/* Fields reserved by Intel. This includes the GD (General Detect + Enable) flag, which causes a debug exception to be generated when a + MOV instruction accesses one of the debug registers. + + FIXME: My Intel manual says we should use 0xF800, not 0xFC00. */ +#define DR_CONTROL_RESERVED (0xFC00) + +#endif /* !HAVE_SYS_DEBUGREG_H */ + +#endif /* !HAVE_ASM_DEBUGREG_H */ + +/* This is here for completeness. No platform supports this + functionality yet (as of Feb-2001). Note that the DE flag in the + CR4 register needs to be set to support this. */ +#ifndef DR_RW_IORW +#define DR_RW_IORW (0x2) /* break on I/O reads or writes */ +#endif + +/* Auxiliary helper macros. */ + +/* A value that masks all fields in DR7 that are reserved by Intel. */ +#define I386_DR_CONTROL_MASK (~DR_CONTROL_RESERVED) + +/* The I'th debug register is vacant if its Local and Global Enable + bits are reset in the Debug Control register. */ +#define I386_DR_VACANT(i) \ + ((dr_control_mirror & (3 << (DR_ENABLE_SIZE * (i)))) == 0) + +/* Locally enable the break/watchpoint in the I'th debug register. */ +#define I386_DR_LOCAL_ENABLE(i) \ + dr_control_mirror |= (1 << (DR_LOCAL_ENABLE_SHIFT + DR_ENABLE_SIZE * (i))) + +/* Globally enable the break/watchpoint in the I'th debug register. */ +#define I386_DR_GLOBAL_ENABLE(i) \ + dr_control_mirror |= (1 << (DR_GLOBAL_ENABLE_SHIFT + DR_ENABLE_SIZE * (i))) + +/* Disable the break/watchpoint in the I'th debug register. */ +#define I386_DR_DISABLE(i) \ + dr_control_mirror &= ~(3 << (DR_ENABLE_SIZE * (i))) + +/* Set in DR7 the RW and LEN fields for the I'th debug register. */ +#define I386_DR_SET_RW_LEN(i,rwlen) \ + do { \ + dr_control_mirror &= ~(0x0f << (DR_CONTROL_SHIFT+DR_CONTROL_SIZE*(i))); \ + dr_control_mirror |= ((rwlen) << (DR_CONTROL_SHIFT+DR_CONTROL_SIZE*(i))); \ + } while (0) + +/* Get from DR7 the RW and LEN fields for the I'th debug register. */ +#define I386_DR_GET_RW_LEN(i) \ + ((dr_control_mirror >> (DR_CONTROL_SHIFT + DR_CONTROL_SIZE * (i))) & 0x0f) + +/* Did the watchpoint whose address is in the I'th register break? */ +#define I386_DR_WATCH_HIT(i) (dr_status_mirror & (1 << (i))) + +/* A macro to loop over all debug registers. */ +#define ALL_DEBUG_REGISTERS(i) for (i = 0; i <= DR_LASTADDR-DR_FIRSTADDR; i++) + +/* This is in i386v-nat.c, so let's have it here, just in case. */ +#if !defined (offsetof) +#define offsetof(TYPE, MEMBER) ((unsigned long) &((TYPE *)0)->MEMBER) +#endif + +/* Mirror the inferior's DRi registers. We keep the status and + control registers separated because they don't hold addresses. */ +static CORE_ADDR dr_mirror[DR_LASTADDR - DR_FIRSTADDR + 1]; +static unsigned dr_status_mirror, dr_control_mirror; + +/* Reference counts for each debug register. */ +static int dr_ref_count[DR_LASTADDR - DR_FIRSTADDR + 1]; + +/* Whether or not to print the mirrored debug registers. */ +static int maint_show_dr; + +/* Types of operations supported by i386_handle_nonaligned_watchpoint. */ +typedef enum { WP_INSERT, WP_REMOVE, WP_COUNT } i386_wp_op_t; + +/* Exported API functions. */ + +/* Clear the reference counts and forget everything we knew about DRi. */ +void i386_cleanup_dregs (void); + +/* Insert a watchpoint to watch a memory region which starts at + address ADDR and whose length is LEN bytes. Watch memory accesses + of the type TYPE. Return 0 on success, -1 on failure. */ +int i386_insert_watchpoint (CORE_ADDR addr, int len, int type); + +/* Remove a watchpoint that watched the memory region which starts at + address ADDR, whose length is LEN bytes, and for accesses of the + type TYPE. Return 0 on success, -1 on failure. */ +int i386_remove_watchpoint (CORE_ADDR addr, int len, int type); + +/* Return non-zero if we can watch a memory region that starts at + address ADDR and whose length is LEN bytes. */ +int i386_region_ok_for_watchpoint (CORE_ADDR addr, int len); + +/* Return non-zero if the inferior has some break/watchpoint that + triggered. */ +int i386_stopped_by_hwbp (void); + +/* If the inferior has some break/watchpoint that triggered, return + the address associated with that break/watchpoint. Otherwise, + return zero. */ +CORE_ADDR i386_stopped_data_address (void); + +/* Insert a hardware-assisted breakpoint at address ADDR. SHADOW is + unused. Return 0 on success, EBUSY on failure. */ +int i386_insert_hw_breakpoint (CORE_ADDR addr, void *shadow); + +/* Remove a hardware-assisted breakpoint at address ADDR. SHADOW is + unused. Return 0 on success, -1 on failure. */ +int i386_remove_hw_breakpoint (CORE_ADDR addr, void *shadow); + +/* Internal functions. */ + +/* Return the value of a 4-bit field for DR7 suitable for watching a + region of LEN bytes for accesses of type TYPE. LEN is assumed + to have the value of 1, 2, or 4. */ +static unsigned i386_length_and_rw_bits (int len, enum target_hw_bp_type type); + +/* Insert a watchpoint at address ADDR, which is assumed to be aligned + according to the length of the region to watch. LEN_RW_BITS is the + value of the bit-field from DR7 which describes the length and + access type of the region to be watched by this watchpoint. Return + 0 on success, -1 on failure. */ +static int i386_insert_aligned_watchpoint (CORE_ADDR addr, + unsigned len_rw_bits); + +/* Remove a watchpoint at address ADDR, which is assumed to be aligned + according to the length of the region to watch. LEN_RW_BITS is the + value of the bits from DR7 which describes the length and access + type of the region watched by this watchpoint. Return 0 on + success, -1 on failure. */ +static int i386_remove_aligned_watchpoint (CORE_ADDR addr, + unsigned len_rw_bits); + +/* Insert or remove a (possibly non-aligned) watchpoint, or count the + number of debug registers required to watch a region at address + ADDR whose length is LEN for accesses of type TYPE. Return 0 on + successful insertion or removal, a positive number when queried + about the number of registers, or -1 on failure. If WHAT is not + a valid value, returns EINVAL. */ +static int i386_handle_nonaligned_watchpoint (i386_wp_op_t what, + CORE_ADDR addr, int len, + enum target_hw_bp_type type); + +/* Implementation. */ + +/* Clear the reference counts and forget everything we knew about + the debug registers. */ +void +i386_cleanup_dregs (void) +{ + int i; + + ALL_DEBUG_REGISTERS(i) + { + dr_mirror[i] = 0; + dr_ref_count[i] = 0; + } + dr_control_mirror = 0; + dr_status_mirror = 0; +} + +/* Print the values of the mirrored debug registers. + This is called when maint_show_dr is non-zero. To set that + up, type "maint show-debug-regs" at GDB's prompt. */ +static void +i386_show_dr (const char *func, CORE_ADDR addr, + int len, enum target_hw_bp_type type) +{ + int i; + + puts_unfiltered (func); + if (addr || len) + printf_unfiltered (" (addr=%lx, len=%d, type=%s)", + /* This code is for ia32, so casting CORE_ADDR + to unsigned long should be okay. */ + (unsigned long)addr, len, + type == hw_write ? "data-write" + : (type == hw_read ? "data-read" + : (type == hw_access ? "data-read/write" + : (type == hw_execute ? "instruction-execute" + /* FIXME: if/when I/O read/write + watchpoints are supported, add them + here. */ + : "??unknown??")))); + puts_unfiltered (":\n"); + printf_unfiltered ("CONTROL (DR7): %08x STATUS (DR6): %08x\n", + dr_control_mirror, dr_status_mirror); + ALL_DEBUG_REGISTERS(i) + { + printf_unfiltered ("DR%d: addr=%08lx, ref.count=%d DR%d: addr=%08lx, ref.count=%d\n", + i, dr_mirror[i], dr_ref_count[i], + i+1, dr_mirror[i+1], dr_ref_count[i+1]); + i++; + } +} + +/* Return the value of a 4-bit field for DR7 suitable for watching a + region of LEN bytes for accesses of type TYPE. LEN is assumed + to have the value of 1, 2, or 4. */ +static unsigned +i386_length_and_rw_bits (int len, enum target_hw_bp_type type) +{ + unsigned rw; + + switch (type) + { + case hw_execute: + rw = DR_RW_EXECUTE; + break; + case hw_write: + rw = DR_RW_WRITE; + break; + case hw_read: /* x86 doesn't support data-read watchpoints */ + case hw_access: + rw = DR_RW_READ; + break; +#if 0 + case hw_io_access: /* not yet supported */ + rw = DR_RW_IORW; + break; +#endif + default: + internal_error ("\ +Invalid hw breakpoint type %d in i386_length_and_rw_bits.\n", (int)type); + } + + switch (len) + { + case 4: + return (DR_LEN_4 | rw); + case 2: + return (DR_LEN_2 | rw); + case 1: + return (DR_LEN_1 | rw); + default: + internal_error ("\ +Invalid hw breakpoint length %d in i386_length_and_rw_bits.\n", len); + } +} + +/* Insert a watchpoint at address ADDR, which is assumed to be aligned + according to the length of the region to watch. LEN_RW_BITS is the + value of the bits from DR7 which describes the length and access + type of the region to be watched by this watchpoint. Return 0 on + success, -1 on failure. */ +static int +i386_insert_aligned_watchpoint (CORE_ADDR addr, unsigned len_rw_bits) +{ + int i; + + /* First, look for an occupied debug register with the same address + and the same RW and LEN definitions. If we find one, we can + reuse it for this watchpoint as well (and save a register). */ + ALL_DEBUG_REGISTERS(i) + { + if (!I386_DR_VACANT (i) + && dr_mirror[i] == addr + && I386_DR_GET_RW_LEN (i) == len_rw_bits) + { + dr_ref_count[i]++; + return 0; + } + } + + /* Next, look for a vacant debug register. */ + ALL_DEBUG_REGISTERS(i) + { + if (I386_DR_VACANT (i)) + break; + } + + /* No more debug registers! */ + if (i > DR_LASTADDR - DR_FIRSTADDR) + return -1; + + /* Now set up the register I to watch our region. */ + + /* Record the info in our local mirrored array. */ + dr_mirror[i] = addr; + dr_ref_count[i] = 1; + I386_DR_SET_RW_LEN (i, len_rw_bits); + /* Note: we only enable the watchpoint locally, i.e. in the current + task. Currently, no x86 target allows or supports global + watchpoints; however, if any target would want that in the + future, GDB should probably provide a command to control whether + to enable watchpoints globally or locally, and the code below + should use global or local enable and slow-down flags as + appropriate. */ + I386_DR_LOCAL_ENABLE (i); + dr_control_mirror |= DR_LOCAL_SLOWDOWN; + dr_control_mirror &= I386_DR_CONTROL_MASK; + + /* Finally, actually pass the info to the inferior. */ + I386_DR_LOW_SET_CONTROL (dr_control_mirror); + I386_DR_LOW_SET_ADDR (i + DR_FIRSTADDR, addr); + + return 0; +} + +/* Remove a watchpoint at address ADDR, which is assumed to be aligned + according to the length of the region to watch. LEN_RW_BITS is the + value of the bits from DR7 which describes the length and access + type of the region watched by this watchpoint. Return 0 on + success, -1 on failure. */ +static int +i386_remove_aligned_watchpoint (CORE_ADDR addr, unsigned len_rw_bits) +{ + int i, retval = -1; + + ALL_DEBUG_REGISTERS(i) + { + if (!I386_DR_VACANT (i) + && dr_mirror[i] == addr + && I386_DR_GET_RW_LEN (i) == len_rw_bits) + { + if (--dr_ref_count[i] == 0) /* no longer in use? */ + { + /* Reset our mirror. */ + dr_mirror[i] = 0; + I386_DR_DISABLE (i); + /* Reset it in the inferior. */ + I386_DR_LOW_RESET_ADDR (i + DR_FIRSTADDR); + I386_DR_LOW_SET_CONTROL (dr_control_mirror); + } + retval = 0; + } + } + + return retval; +} + +/* Insert or remove a (possibly non-aligned) watchpoint, or count the + number of debug registers required to watch a region at address + ADDR whose length is LEN for accesses of type TYPE. Return 0 on + successful insertion or removal, a positive number when queried + about the number of registers, or -1 on failure. If WHAT is not + a valid value, returns EINVAL. */ +static int +i386_handle_nonaligned_watchpoint (i386_wp_op_t what, CORE_ADDR addr, int len, + enum target_hw_bp_type type) +{ + int align; + int size; + int rv = 0, status = 0; + + static int size_try_array[4][4] = + { + { 1, 1, 1, 1 }, /* trying size one */ + { 2, 1, 2, 1 }, /* trying size two */ + { 2, 1, 2, 1 }, /* trying size three */ + { 4, 1, 2, 1 } /* trying size four */ + }; + + while (len > 0) + { + align = addr % 4; + /* Four is the maximum length an x86 debug register can watch. */ + size = size_try_array[len > 4 ? 3 : len - 1][align]; + if (what == WP_COUNT) + /* size_try_array[] is defined so that each iteration through + the loop is guaranteed to produce an address and a size + that can be watched with a single debug register. Thus, + for counting the registers required to watch a region, we + simply need to increment the count on each iteration. */ + rv++; + else + { + unsigned len_rw = i386_length_and_rw_bits (size, type); + + if (what == WP_INSERT) + status = i386_insert_aligned_watchpoint (addr, len_rw); + else if (what == WP_REMOVE) + status = i386_remove_aligned_watchpoint (addr, len_rw); + else + status = EINVAL; + /* We keep the loop going even after a failure, because some + of the other aligned watchpoints might still succeed + (e.g. if they watch addresses that are already watched, + in which case we just increment the reference counts of + occupied debug registers). If we break out of the loop + too early, we could cause those addresses watched by + other watchpoints to be disabled when breakpoint.c reacts + to our failure to insert this watchpoint and tries to + remove it. */ + if (status) + rv = status; + } + addr += size; + len -= size; + } + return rv; +} + +/* Insert a watchpoint to watch a memory region which starts at + address ADDR and whose length is LEN bytes. Watch memory accesses + of the type TYPE. Return 0 on success, -1 on failure. */ +int +i386_insert_watchpoint (CORE_ADDR addr, int len, int type) +{ + int retval; + + if (len == 3 || len > 4 || addr % len != 0) + retval = i386_handle_nonaligned_watchpoint (WP_INSERT, addr, len, type); + else + { + unsigned len_rw = i386_length_and_rw_bits (len, type); + + retval = i386_insert_aligned_watchpoint (addr, len_rw); + } + + if (maint_show_dr) + i386_show_dr ("insert_watchpoint", addr, len, type); + + return retval; +} + +/* Remove a watchpoint that watched the memory region which starts at + address ADDR, whose length is LEN bytes, and for accesses of the + type TYPE. Return 0 on success, -1 on failure. */ +int +i386_remove_watchpoint (CORE_ADDR addr, int len, int type) +{ + int retval; + + if (len == 3 || len > 4 || addr % len != 0) + retval = i386_handle_nonaligned_watchpoint (WP_REMOVE, addr, len, type); + else + { + unsigned len_rw = i386_length_and_rw_bits (len, type); + + retval = i386_remove_aligned_watchpoint (addr, len_rw); + } + + if (maint_show_dr) + i386_show_dr ("remove_watchpoint", addr, len, type); + + return retval; +} + +/* Return non-zero if we can watch a memory region that starts at + address ADDR and whose length is LEN bytes. */ +int +i386_region_ok_for_watchpoint (CORE_ADDR addr, int len) +{ + /* Compute how many aligned watchpoints we would need to cover this + region. */ + int nregs = i386_handle_nonaligned_watchpoint (WP_COUNT, addr, len, + hw_write); + + return nregs <= 4 ? 1 : 0; +} + +/* If the inferior has some watchpoint that triggered, return the + address associated with that watchpoint. Otherwise, return + zero. */ +CORE_ADDR +i386_stopped_data_address (void) +{ + int i; + CORE_ADDR ret = 0; + + dr_status_mirror = I386_DR_LOW_GET_STATUS (); + + ALL_DEBUG_REGISTERS(i) + { + if (I386_DR_WATCH_HIT (i) + /* This second condition makes sure DRi is set up for a data + watchpoint, not a hardware breakpoint. The reason is + that GDB doesn't call the target_stopped_data_address + method except for data watchpoints. In fact, if this + function returns non-zero for a hardware breakpoint, GDB + will become confused. You _have_ been warned! */ + && I386_DR_GET_RW_LEN (i) != 0) + { + ret = dr_mirror[i]; + if (maint_show_dr) + i386_show_dr ("watchpoint_hit", ret, -1, hw_write); + } + } + if (maint_show_dr && ret == 0) + i386_show_dr ("stopped_data_addr", 0, 0, hw_write); + + return ret; +} + +/* Return non-zero if the inferior has some break/watchpoint that + triggered. */ +int +i386_stopped_by_hwbp (void) +{ + int i; + + dr_status_mirror = I386_DR_LOW_GET_STATUS (); + if (maint_show_dr) + i386_show_dr ("stopped_by_hwbp", 0, 0, hw_execute); + + ALL_DEBUG_REGISTERS(i) + { + if (I386_DR_WATCH_HIT (i)) + return 1; + } + + return 0; +} + +/* Insert a hardware-assisted breakpoint at address ADDR. SHADOW is + unused. Return 0 on success, EBUSY on failure. */ +int +i386_insert_hw_breakpoint (CORE_ADDR addr, void *shadow) +{ + unsigned len_rw = i386_length_and_rw_bits (1, hw_execute); + int retval = i386_insert_aligned_watchpoint (addr, len_rw) ? EBUSY : 0; + + if (maint_show_dr) + i386_show_dr ("insert_hwbp", addr, 1, hw_execute); + + return retval; +} + +/* Remove a hardware-assisted breakpoint at address ADDR. SHADOW is + unused. Return 0 on success, -1 on failure. */ +int +i386_remove_hw_breakpoint (CORE_ADDR addr, void *shadow) +{ + unsigned len_rw = i386_length_and_rw_bits (1, hw_execute); + int retval = i386_remove_aligned_watchpoint (addr, len_rw); + + if (maint_show_dr) + i386_show_dr ("remove_hwbp", addr, 1, hw_execute); + + return retval; +} + +#endif /* TARGET_HAS_HARDWARE_WATCHPOINTS */ + #ifdef I386V4_SIGTRAMP_SAVED_PC /* Get saved user PC for sigtramp from the pushed ucontext on the stack @@ -1014,4 +1667,15 @@ and the default value is \"att\".", in the disassembly_flavor variable */ set_disassembly_flavor (); + + /* A maintenance command to enable printing the internal DRi mirror + variables. */ + add_set_cmd ("show-debug-regs", class_maintenance, + var_boolean, (char *) &maint_show_dr, + "\ +Set whether to show variables that mirror the x86 debug registers.\n\ +Use \"on\" to enable, \"off\" to disable.\n\ +If enabled, the debug registers values are shown when GDB inserts\n\ +or removes a hardware breakpoint or watchpoint, and when the inferior\n\ +triggers a breakpoint or watchpoint.", &maintenancelist); } --- gdb/config/i386/tm-i386.h~0 Thu Jan 4 17:46:20 2001 +++ gdb/config/i386/tm-i386.h Sat Mar 3 17:50:52 2001 @@ -1,5 +1,5 @@ /* Macro definitions for GDB on an Intel i[345]86. - Copyright (C) 1995, 1996, 2000 Free Software Foundation, Inc. + Copyright (C) 1995, 1996, 2000, 2001 Free Software Foundation, Inc. This file is part of GDB. @@ -418,4 +418,97 @@ extern void print_387_status_word (unsig #define SP_ARG0 (1 * 4) + + +/* Hardware-assisted breakpoints and watchpoints. */ + +#ifdef TARGET_HAS_HARDWARE_WATCHPOINTS + +/* Clear the reference counts and forget everything we knew about DRi. */ +extern void i386_cleanup_dregs (void); + +/* Insert a watchpoint to watch a memory region which starts at + address ADDR and whose length is LEN bytes. Watch memory accesses + of the type TYPE. Return 0 on success, -1 on failure. */ +extern int i386_insert_watchpoint (CORE_ADDR addr, int len, int type); + +/* Remove a watchpoint that watched the memory region which starts at + address ADDR, whose length is LEN bytes, and for accesses of the + type TYPE. Return 0 on success, -1 on failure. */ +extern int i386_remove_watchpoint (CORE_ADDR addr, int len, int type); + +/* Return non-zero if we can watch a memory region that starts at + address ADDR and whose length is LEN bytes. */ +extern int i386_region_ok_for_watchpoint (CORE_ADDR addr, int len); + +/* Return non-zero if the inferior has some break/watchpoint that + triggered. */ +extern int i386_stopped_by_hwbp (void); + +/* If the inferior has some break/watchpoint that triggered, return + the address associated with that break/watchpoint. Otherwise, + return zero. */ +extern CORE_ADDR i386_stopped_data_address (void); + +/* Insert a hardware-assisted breakpoint at address ADDR. SHADOW is + unused. Return 0 on success, EBUSY on failure. */ +extern int i386_insert_hw_breakpoint (CORE_ADDR addr, void *shadow); + +/* Remove a hardware-assisted breakpoint at address ADDR. SHADOW is + unused. Return 0 on success, -1 on failure. */ +extern int i386_remove_hw_breakpoint (CORE_ADDR addr, void *shadow); + +/* Returns the number of hardware watchpoints of type TYPE that we can + set. Value is positive if we can set CNT watchpoints, zero if + setting watchpoints of type TYPE is not supported, and negative if + CNT is more than the maximum number of watchpoints of type TYPE + that we can support. TYPE is one of bp_hardware_watchpoint, + bp_read_watchpoint, bp_write_watchpoint, or bp_hardware_breakpoint. + CNT is the number of such watchpoints used so far (including this + one). OTHERTYPE is non-zero if other types of watchpoints are + currently enabled. + + We always return 1 here because we don't have enough information + about possible overlap of addresses that they want to watch. As + an extreme example, consider the case where all the watchpoints + watch the same address and the same region length: then we can + handle a virtually unlimited number of watchpoints, due to debug + register sharing implemented via reference counts in i386-tdep.c. */ + +#define TARGET_CAN_USE_HARDWARE_WATCHPOINT(type, cnt, ot) 1 + +/* Returns non-zero if we can use hardware watchpoints to watch a region + whose address is ADDR and whose length is LEN. */ + +#define TARGET_REGION_OK_FOR_HW_WATCHPOINT(addr,len) \ + i386_region_ok_for_watchpoint(addr,len) + +/* After a watchpoint trap, the PC points to the instruction after the + one that caused the trap. Therefore we don't need to step over it. + But we do need to reset the status register to avoid another trap. */ + +#define HAVE_CONTINUABLE_WATCHPOINT + +#define STOPPED_BY_WATCHPOINT(W) (i386_stopped_data_address () != 0) + +#define target_stopped_data_address() i386_stopped_data_address () + +/* Use these macros for watchpoint insertion/removal. */ + +#define target_insert_watchpoint(addr, len, type) \ + i386_insert_watchpoint (addr, len, type) + +#define target_remove_watchpoint(addr, len, type) \ + i386_remove_watchpoint (addr, len, type) + +#define target_insert_hw_breakpoint(addr, shadow) \ + i386_insert_hw_breakpoint(addr, shadow) + +#define target_remove_hw_breakpoint(addr, shadow) \ + i386_remove_hw_breakpoint(addr, shadow) + +#define DECR_PC_AFTER_HW_BREAK 0 + +#endif /* TARGET_HAS_HARDWARE_WATCHPOINTS */ + #endif /* ifndef TM_I386_H */ ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC] Unified watchpoints for x86 platforms 2001-02-15 14:46 ` J.T. Conklin 2001-02-15 16:11 ` Kevin Buettner @ 2001-02-15 23:30 ` Eli Zaretskii 2001-02-16 10:52 ` J.T. Conklin 2001-02-16 0:00 ` Mark Kettenis 2 siblings, 1 reply; 57+ messages in thread From: Eli Zaretskii @ 2001-02-15 23:30 UTC (permalink / raw) To: jtc; +Cc: kevinb, kettenis, gdb > From: jtc@redback.com (J.T. Conklin) > Date: 15 Feb 2001 14:45:16 -0800 > > We're going to need to pass a PID, or perhaps some new representation > of a execution context, to a lot of code functions that don't allready > have such an argument. Sorry, I'm not sure I'm following: why do you envision we'll need to pass the PID to functions that don't receive it today? What function(s) did you have in mind? The current watchpoint implementation on i386v-nat.c, for example, does accept PID as an argument. ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC] Unified watchpoints for x86 platforms 2001-02-15 23:30 ` [RFC] " Eli Zaretskii @ 2001-02-16 10:52 ` J.T. Conklin 0 siblings, 0 replies; 57+ messages in thread From: J.T. Conklin @ 2001-02-16 10:52 UTC (permalink / raw) To: Eli Zaretskii; +Cc: kevinb, kettenis, gdb >>>>> "Eli" == Eli Zaretskii <eliz@delorie.com> writes: >> From: jtc@redback.com (J.T. Conklin) >> Date: 15 Feb 2001 14:45:16 -0800 >> >> We're going to need to pass a PID, or perhaps some new representation >> of a execution context, to a lot of code functions that don't allready >> have such an argument. Eli> Sorry, I'm not sure I'm following: why do you envision we'll need to Eli> pass the PID to functions that don't receive it today? What Eli> function(s) did you have in mind? I was speaking in generalities. But if GDB is ever going to be able to debug multiple independent processes (perhaps with multiple threads) as has been a stated long term goal, we are going to have to entirely revamp how a "execution context" is represented, and how target functions know what context they are operating on. IMO, an inferior_pid global, and passing pids to various functions is not enough. --jtc -- J.T. Conklin RedBack Networks ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC] Unified watchpoints for x86 platforms 2001-02-15 14:46 ` J.T. Conklin 2001-02-15 16:11 ` Kevin Buettner 2001-02-15 23:30 ` [RFC] " Eli Zaretskii @ 2001-02-16 0:00 ` Mark Kettenis 2 siblings, 0 replies; 57+ messages in thread From: Mark Kettenis @ 2001-02-16 0:00 UTC (permalink / raw) To: jtc; +Cc: eliz, kevinb, gdb From: jtc@redback.com (J.T. Conklin) Date: 15 Feb 2001 14:45:16 -0800 >>>>> "Eli" == Eli Zaretskii <eliz@delorie.com> writes: >> > Is there any particular reason why you need the PID argument? AFAICS >> > it will always be equal to INFERIOR_PID, so I think we can do without >> > it. This is also true for the other i386_hwbp_* functions you're >> > proposing. >> >> I think it'd be better to not rely on ``inferior_pid''. I would >> rather see the explicitly passed. There will come a day when GDB >> is able to debug more than one process at a time and to perpetuate >> reliance on inferior pid would be short sighted. Eli> I have two opposite opinions here. We need to resolve this somehow. We're going to need to pass a PID, or perhaps some new representation of a execution context, to a lot of code functions that don't allready have such an argument. It is not clear to me that adding such an argument "because it will be needed" is correct, considering that the design has not yet started. The truth is we don't know "what" will be needed, so we'll have to revisit this function (among many others) down the line anyway. I agree with J.T. here. Mark ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC] Unified watchpoints for x86 platforms 2001-02-15 3:48 ` Eli Zaretskii 2001-02-15 8:17 ` Mark Kettenis @ 2001-02-15 9:08 ` H . J . Lu 1 sibling, 0 replies; 57+ messages in thread From: H . J . Lu @ 2001-02-15 9:08 UTC (permalink / raw) To: Eli Zaretskii; +Cc: gdb On Thu, Feb 15, 2001 at 01:46:27PM +0200, Eli Zaretskii wrote: > Ping! > > No one posted any approvals or disprovals of this design. Do I take > the silence as a sign of agreement and start coding? > Why not? H.J. ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: gdb doesn't work very well with dynamic linked binaries 2000-09-07 3:09 ` Mark Kettenis 2000-09-07 8:02 ` Eli Zaretskii @ 2000-09-09 14:39 ` Peter.Schauer 1 sibling, 0 replies; 57+ messages in thread From: Peter.Schauer @ 2000-09-09 14:39 UTC (permalink / raw) To: Mark Kettenis; +Cc: gdb, eliz > The bug I mentioned previously is exactly that they're getting > cleared by the kernel and then not getting restored on return > to user space, leaving them wrong until the next reschedule :-( > > I think I understand the problems now. It basically means that one > cannot reliably watch area's that are somehow used in system calls. > > I suspect that Linux isn't the only kernel with this bug. AFAICS > FreeBSD also simply disables any (user-space) watchpoints triggered > from within the kernel. I don't know what the various x86 System V's > (Solaris, SCO Open Server 5) do, but I wouldn't be surprised if it is > broken there too. AFAIK Solaris never provided access to the x86 debug registers. Starting with Solaris 2.6, support for hardware watchpoints is provided via procfs (protect pages which contain watchpoint areas, examine address on pagefault, cause breakpoint if address is within watchpoint area). Perhaps this scheme (which allows for an arbitrary number of watchpoints) could be adopted by the Linux kernel. Debug register access might be valuable for embedded targets, but the generic x86 target should not assume availability of debug register access. -- Peter Schauer pes@regent.e-technik.tu-muenchen.de ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: Proposal: convert function definitions to prototyped form @ 2000-06-02 13:11 David Taylor 0 siblings, 0 replies; 57+ messages in thread From: David Taylor @ 2000-06-02 13:11 UTC (permalink / raw) To: Kevin Buettner; +Cc: gdb Date: Fri, 2 Jun 2000 12:10:42 -0700 From: Kevin Buettner <kevinb@cygnus.com> On Jun 2, 11:39am, David Taylor wrote: > 3) ``protoize'' screws up on PTR types. E.g, in breakpoint.c, it > converted > > static int > cover_target_enable_exception_callback (arg) > PTR arg; > > to > > static int > cover_target_enable_exception_callback (__builtin_va_list arg) > > I consider this to be a more serious problem than the other complaints > you have. I think the setup issue is important too. (I don't know if it's more serious though.) As I mentioned in my first point (which is no longer quoted), you have to do a configure in the source directory above gdb Sorry, I didn't consider it relevant to what I was replying to. in order to to properly run protoize or else it complains that it can't find certain header files. Also, you need to make sure that the bfd header files are generated before you start. None of these problems are insurmountable, but to do things safely with protoize, and in order to avoid polluting your sources with files that belong in a build tree, it would be necessary for a script using protoize to make a complete copy of the source tree somewhere else in order to do the work. I don't consider the setup issue to be a big issue. It requires some time, but it doesn't require much human time. Configure in the source tree, then build. Setup done. During the commit phase, you don't commit anything that is new -- you only commit files that existed prior to doing the configure. No separate tree needed. > 4) ``protoize'' does not reformat long argument lists. The lists > end up entirely contained on one line. > > So... adopt the same solution for it that you chose to adopt for your > script -- run indent on the output! Granted. A suitably imaginative script could identify just the lines that protoize touched and reformat those. I don't think we want to run indent on entire files however, at least not for this activity. Since Stan previously ran indent on all of the files, re-running it *should* produce no change for stuff that hasn't changed. Should produce no change. I'm not saying it won't. If it changes its own output when presented with it as input, I would consider that a bug. > I've used protoize before with good results. It was a fairly > substantial project, though not as big as gdb. Okay. Out of curiousity, did the project in question have a large number of active developers? (This doesn't necessarily matter, but I think it's probably easier to manage this kind of thing when only a handful of developers are affected.) It was at a former company. There were a small number of active developers all within the company. The total project was around 200-250K lines of code. There was a great deal less use of #if within the sources than there is in GDB... [...] I don't disagree, but I think that adding prototypes for everything should be a separate activity. The question is whether it should occur before the activity covered by my proposal. And if it should occur before, once again, we'll need to find a way to help automate the process because if we attempt to do it incrementally by hand over a period of time, it is likely that it'll never get done. I agree that it should be a separate activity. I think that your script is a good thing. And due to the amount of hand editing that appears to be necessary with protoize vs the amount that appears to be necessary with your script, I think we should use your script. Ultimately, though, since you're doing the work, you get to select the tool. So long as it is fairly reliable and reasonably quick (quick in terms of *your time*, not in terms of *elapsed time*), it doesn't ultimately matter to me what tool you use. (Which is not to say that I am not interested in knowing what tool you use -- I am interested.) Like I said, I think your script is a good thing. And I look forward to you using it to protoize the bulk of the GDB sources. Kevin David ^ permalink raw reply [flat|nested] 57+ messages in thread
* Proposal: convert function definitions to prototyped form @ 2000-06-02 0:50 Kevin Buettner 2000-06-02 2:42 ` Andrew Cagney ` (3 more replies) 0 siblings, 4 replies; 57+ messages in thread From: Kevin Buettner @ 2000-06-02 0:50 UTC (permalink / raw) To: gdb As many of you know, I'm in the midst of purging the use of ``PARAMS'' in prototyped function declarations from the gdb sources. After this activity is concluded, I'd like to begin converting function definitions whose parameters are declared using the traditional C style to the ISO C prototyped form. I.e, I'd like to convert functions of the form int foo (a, b, c) int a; char *b; int *c; { ... } to int foo (int a, char *b, int *c) { ... } To this end, I've been working on a script to automate most of this conversion. I've appended a work-in-progress version to the end of this message and will discuss it in some detail further below. But first... it's been pointed out to me that the ``protoize'' program is supposed to do the same thing. I gave ``protoize'' a whirl and noticed the following problems: 1) A substantial amount of initial setup is involved. Only in the simplest cases could you just feed a list of files to work on to protoize and expect it to work okay. In particular, for gdb, I found it necessary to do a configure *in the source directory* so that xm.h, tm.h, and nm.h are set up. I also found it necessary to build in bfd so that certain header files are created. The reason all of this is necessary is because protoize invokes gcc to learn about the program. As a consequence, it is also important to know the set of -I switches that one normally feeds to gcc to do a build. 2) ``protoize'' fails to convert functions disabled by ifdefs for the given platform. OTOH, on some other platform(s), these functions might not be disabled and would be converted. E.g, in breakpoint.c, on my GNU/Linux/x86 machine, protoize failed to convert create_longjmp_breakpoint() which is protected by an "#ifdef GET_LONGJMP_TARGET". 3) ``protoize'' screws up on PTR types. E.g, in breakpoint.c, it converted static int cover_target_enable_exception_callback (arg) PTR arg; to static int cover_target_enable_exception_callback (__builtin_va_list arg) 4) ``protoize'' does not reformat long argument lists. The lists end up entirely contained on one line. For more information on protoize, see the "Running protoize" page: http://gcc.gnu.org/onlinedocs/gcc_2.html#SEC48 and the "Caveats of using protoize" page: http://gcc.gnu.org/onlinedocs/gcc_7.html#SEC135 Two of my goals in creating the scripts for the PARAMS purging activities was that the scripts should be 1) easy to invoke and 2) require no hand editing of the output when done. I.e, you shouldn't have to edit any of the files that these scripts touch in order to fix errors so that you can build again. OTOH, the script may leave certain portions of the file alone that could possibly have be converted had the script been a little smarter. The things that the script fails to convert *will* have to be fixed later on (in order to complete the cleanup activity), either by another script, or by hand. For the PARAMS purging activity, I have spent a fair amount of time examining the diffs to make sure that this is indeed the case. (And I intend to do the same for the activity in this proposal.) The reason that it is so important to avoid any hand edits is that we want people who have local repositories or checked out source trees to be able to run these conversion scripts against them so that merges will be less painful. (Even easy.) With that said, keeping in mind the problems I noted above, I conclude that ``protoize'' is not the appropriate tool for us to use to convert function definitions in the gdb sources to their prototyped form. Now, I'll tell you about the fix-decls script (see below). The fix-decls script is run via "fix-decls <dirname>" where <dirname> is the name of the directory to (recursively) fix. It only touches the .c files, with the exception of gnu-regex.c and the testsuite files. After it converts a traditional C style declaration to a prototyped one, it invokes GNU indent (on the function declaration only) to make sure that our indentation standards are still adhered to. There is no gratuitous reformatting of any other code. Here are a list of things that it won't handle (at the moment, anyway): - declarations with comments in them - function pointers - array declarations - malformed declarations (e.g, look at m88k_skip_prologue in m88k-tdep.c) Functions with traditional C declarations with any of the above in them will be left alone. (And need to be converted by hand at some point.) But this isn't so bad. When I ran the script over the gdb sources, it converted 5,204 function definitions. After throwing out gnu-regex.c (which needs to track glibc), all of the stuff in the testsuite, and everything in tui (which has ifdefs around each and every function declaration), there are only 143 cases left to examine by hand. I've sampled these and many of them are unhandled due to having comments following a parameter's type declaration. If I could figure out what to do about comments, we'd have even fewer cases to fix up by hand. Here are some of the more interesting cases that it does handle: Missing int in declaration (i960-tdep.c): CORE_ADDR -frame_args_address (fi, must_be_correct) - struct frame_info *fi; +frame_args_address (struct frame_info *fi, int must_be_correct) { Comma separated parameter lists (remote-mm.c): static void -init_target_mm (tstart, tend, dstart, dend, entry, ms_size, rs_size, arg_start) - ADDR32 tstart, tend, dstart, dend, entry; - INT32 ms_size, rs_size; - ADDR32 arg_start; +init_target_mm (ADDR32 tstart, ADDR32 tend, ADDR32 dstart, ADDR32 dend, + ADDR32 entry, INT32 ms_size, INT32 rs_size, ADDR32 arg_start) { Comma separated list with differing number of stars on the parameter names (sparc-tdep.c): static branch_type -isbranch (instruction, addr, target) - long instruction; - CORE_ADDR addr, *target; +isbranch (long instruction, CORE_ADDR addr, CORE_ADDR * target) { And another one (command.c): struct cmd_list_element * -lookup_cmd_1 (text, clist, result_list, ignore_help_classes) - char **text; - struct cmd_list_element *clist, **result_list; - int ignore_help_classes; +lookup_cmd_1 (char **text, struct cmd_list_element *clist, + struct cmd_list_element **result_list, int ignore_help_classes) { Out of order comma separated parameter list - note positions of ``from'' and ``to'' (gdbserver/remote-utils.c): void -decode_M_packet (from, mem_addr_ptr, len_ptr, to) - char *from, *to; - CORE_ADDR *mem_addr_ptr; - unsigned int *len_ptr; +decode_M_packet (char *from, CORE_ADDR * mem_addr_ptr, unsigned int *len_ptr, + char *to) { Finally, I should note that I've done a test build on GNU/Linux/x86 and had no build problems, nor did I see any regressions when I ran the testsuite. The fix-decls script is below. I'd be interested in finding out if anyone else has a favorite script for doing this sort of thing. Other comments welcome too. --- fix-decls --- #!/usr/bin/perl -w use File::Find; use FileHandle; use IPC::Open3; use English; my ($root) = @ARGV; if (!defined($root)) { die "Usage: $0 root\n"; } @ARGV = (); find( sub { if ($_ eq 'testsuite') { $File::Find::prune = 1; } elsif (-f && -T && /\.c$/ && $_ ne "gnu-regex.c") { push @ARGV, $File::Find::name; } }, $root ); #$INPLACE_EDIT = '.bak'; $INPLACE_EDIT = ''; undef $/; # slurp entire files while (<>) { s/ ^ # line start ( \w+ # function name ) ( \s* # spaces \( # left paren \s* # spaces ) ( (?:\w+\s*,\s*)* # 1 thru N-1 parameter names \w+ # last parameter name ) ( \s* # spaces \) # right paren ) ( (?: [^;{}()]+; # trad C parameter decl )* ) ( \s* # spaces ) (?=^{) # lookahead to make sure we see a # right curly brace at the beginning # of the line / fix_decl($1, $2, $3, $4, $5, $6); /smgex; s/ ^ # line start ( \w+ # function name ) \s* # spaces \( # left paren \s* # spaces \) # right paren (?=\s*^{) # lookahead to make sure we see a # right curly brace at the beginning # of the line /$1 (void)/smgx; print; } sub fix_decl { my ($funcname, $lparen, $params, $rparen, $decls, $spaces) = @_; my %h = (); my ($param, $decl); # Define $bailstr to be the original function declaration. We # return it when we see something which doesn't make sense. my $bailstr = $funcname . $lparen . $params . $rparen . $decls . $spaces; if ($funcname =~ /^(do|while|if)$/) { # 'if', 'do', and 'while' are not function names # find_overload_match() in valops.c contains an if statement # which is confused as a function if we don't have this test. return $bailstr; } foreach $param (split /\s*,\s*/, $params) { if (defined $h{$param} || $param eq 'void') { # Bail; either param has already been encountered or # it's void in which case the decl in question is already # ISO C. return $bailstr; } $h{$param} = "int $param"; # Default } $decls =~ s/\s*;\Z//; # remove final semicolon foreach $decl (split /\s*;\s*/, $decls) { my ($type, $dparams) = $decl =~ /^ # beginning of string (.*?) # type ( # dparams... (?: \** # stars \s* # spaces \w+ # identifier \s* # spaces , # comma \s* # spaces )* # any number of the above \** # stars \s* # spaces \w+ # identifier ) $ # end of string /sx; return $bailstr if !defined $type || !defined $dparams; $type =~ s/\A\s+//; # nuke leading spaces $type =~ s/\s+\Z//; # nuke trailing spaces return $bailstr if $type eq ''; # Bail if no type foreach $param (split /\s*,\s*/, $dparams) { my ($stars, $stripped_param) = $param =~ /(\**)\s*(\w+)/; if (!defined($stripped_param) || !defined $h{$stripped_param}) { # Either we couldn't find the parameter or else # the parameter wasn't found in the parameter list return $bailstr; } $h{$stripped_param} = "$type $param"; } } my $newparams = join(', ', map { $h{$_} } split(/\s*,\s*/, $params)); my $newdecl = reindent("$funcname ($newparams)\n{\n}\n"); $newdecl =~ s/{\n}//; return $newdecl; } sub reindent { my ($decl, $line_length) = @_; $line_length = 80 unless defined $line_length; my ($rfh, $wfh, $efh) = (FileHandle->new, FileHandle->new, FileHandle->new); my $pid = open3($wfh, $rfh, $efh, "indent -l$line_length"); $rfh->input_record_separator(undef); $efh->input_record_separator(undef); $wfh->print($decl); $wfh->close(); my $replacement = <$rfh>; $rfh->close(); my $errstr = <$efh>; $efh->close(); waitpid $pid, 0; $replacement =~ s#\n$##; if ($errstr ne "") { print STDERR "Check $ARGV...\n$errstr\nInput:$decl\nOutput:$replacement\n\n" } $replacement; } --- end fix-decls --- ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: Proposal: convert function definitions to prototyped form 2000-06-02 0:50 Kevin Buettner @ 2000-06-02 2:42 ` Andrew Cagney 2000-06-03 0:13 ` Kevin Buettner 2000-06-03 0:21 ` Kevin Buettner 2000-06-02 5:26 ` Mark Kettenis ` (2 subsequent siblings) 3 siblings, 2 replies; 57+ messages in thread From: Andrew Cagney @ 2000-06-02 2:42 UTC (permalink / raw) To: Kevin Buettner; +Cc: gdb Kevin Buettner wrote: > > As many of you know, I'm in the midst of purging the use of ``PARAMS'' > in prototyped function declarations from the gdb sources. After this > activity is concluded, I'd like to begin converting function > definitions whose parameters are declared using the traditional C > style to the ISO C prototyped form. I.e, I'd like to convert > functions of the form > > int > foo (a, b, c) > int a; > char *b; > int *c; > { > ... > } > > to > > int foo (int a, char *b, int *c) > { > ... > } (you mean int foo (int a, char *b, int *c) :-) The obvious thing to note is that by moving to pure ISO-C we eliminate the type promotion problems. For instance: void foo (enum e e); void foo (e) enum e e; { } is not well defined and many compilers reject it. Plenty of similar examples (involving char, short, and the like exist). It is one less issue that people need to worry about. The only concern I have is, given the slightly more complex nature of the script (compared to PARAMS) there is a possibility that the conversion re-orders or re-types the argument list. With that in mind, should a pre-cursor to this be to least have prototypes for all (global?) functions (-Wmissing-prototypes?) or only do the conversion when there is a prototype visible? Andrew PS: You may want to add gdb/*-share to the list of directories to avoid. ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: Proposal: convert function definitions to prototyped form 2000-06-02 2:42 ` Andrew Cagney @ 2000-06-03 0:13 ` Kevin Buettner 2000-06-03 0:21 ` Kevin Buettner 1 sibling, 0 replies; 57+ messages in thread From: Kevin Buettner @ 2000-06-03 0:13 UTC (permalink / raw) To: Andrew Cagney; +Cc: gdb On Jun 2, 7:42pm, Andrew Cagney wrote: > The only concern I have is, given the slightly more complex nature of > the script (compared to PARAMS) there is a possibility that the > conversion re-orders or re-types the argument list. With that in mind, > should a pre-cursor to this be to least have prototypes for all > (global?) functions (-Wmissing-prototypes?) or only do the conversion > when there is a prototype visible? I've given this matter a lot of thought. I agree that it would be desirable to have prototypes for all functions. Unfortunately, while it is easy to generate prototypes, it's not so easy to know where to stick them. Also, even if we had prototypes in place, there's no guarantee that we'd catch the errors after a few builds because I think there's some code in gdb (though I don't know how much) that never gets built! (Due to ifdefs and near obsolete ports.) What we really need is a method for vetting all of the changes immediately after running the script. I.e, we need to make sure that the conversion does no reordering or retyping of any argument list. Also, we need to make sure that the rewritten function declaration is syntactically correct. While examining the diffs (made with the -u switch) an idea occurred to me. Consider the following example: diff -ur ../../orig/gdb-sourceware/wrapper.c ./wrapper.c --- ../../orig/gdb-sourceware/wrapper.c Sat May 27 17:10:27 2000 +++ ./wrapper.c Thu Jun 1 23:33:16 2000 @@ -57,11 +57,8 @@ static int wrap_parse_and_eval_type (char *); int -gdb_parse_exp_1 (stringptr, block, comma, expression) - char **stringptr; - struct block *block; - int comma; - struct expression **expression; +gdb_parse_exp_1 (char **stringptr, struct block *block, int comma, + struct expression **expression) { struct gdb_wrapper_arguments args; args.args[0].pointer = stringptr; In the above diff, the lines prepended with `-' represent the original K&R definition. And the lines prepended with `+' represent the transformed code. Moreover, the diff is extremely regular in this respect. So... If you take the lines which begin with `+', prepend the type on the line before the `-' lines and tack a semicolon onto the end, you end up with a prototype declaration. And, if you take the lines beginning with `-', again tack the type onto the front and put a function body underneath it, you have a K&R style (traditional) function definition. Now if you put these into a file with the prototype first and the K&R definition later on, you can run "gcc -Wall" on it to see if any warnings are produced. Obviously, if we get warnings, we need to look closer to see if something went wrong with the fix-decls conversion. Of course, there are other details to consider, like making sure that all of the types, structs, unions, and enums are declared. Also, in a source tree as big as gdb, we'll likely wind up with a number of functions with the same name, so some method of disambiguating these will be necessary. And then of course, there's the matter of no declared return type and other oddments. I've written a script called ``check-decls'' which performs these transformations on the diff output. When I run it on the above diff, it produces the following output (indented by four spaces by me for readability) struct block { int f0; }; struct expression { int f1; }; #define INLINE #define private #define CONST const #define NORETURN void init___ (void *); int gdb_parse_exp_1 (char **stringptr, struct block *block, int comma, struct expression **expression); int gdb_parse_exp_1 (stringptr, block, comma, expression) char **stringptr; struct block *block; int comma; struct expression **expression; { int ret; init___ (&ret); return ret; } void use___ (void) { } The use___ () function isn't interesting in this example, but it would be if there had been a static declaration. Here's what happens when I run it on *all* of the diffs: ocotillo:ptests$ ./check-decls <declsdiff >prog.c ocotillo:ptests$ wc prog.c 50235 112228 960827 prog.c ocotillo:ptests$ gcc -c -Wall prog.c prog.c: In function `exit': prog.c:39303: warning: function declared `noreturn' has a `return' statement prog.c: At top level: prog.c:45903: parse error before `arg_type' prog.c: In function `value_primitive_field': prog.c:45907: declaration for parameter `arg_type' but no such parameter prog.c:45906: declaration for parameter `fieldno' but no such parameter prog.c:45905: declaration for parameter `offset' but no such parameter prog.c:45904: declaration for parameter `arg1' but no such parameter prog.c:45908: argument `arg_type' doesn't match prototype prog.c:5886: prototype declaration prog.c:45908: argument `arg1' doesn't match prototype prog.c:5886: prototype declaration The `exit' warning is due to the fact that there's a declaration and definition of exit() from standalone.c. It is of no concern. The error following this warning looks more serious. Here's the declaration and the definition of the function involved: value_ptr value_primitive_field (register value_ptr arg1, int offset, register int fieldno, register struct type *arg_type); value_ptr value_primitive_field (arg1, offset, fieldno, arg_type) register value_ptr arg1; int offset; register int fieldno; register struct type *arg_type; { value_ptr ret; init___ (&ret); return ret; } I looked at this for a long, long time and didn't see anything wrong. Finally, I realized that arg_type was a type from a different file. (Which is one of the problems with throwing everything into one big pot.) Anyway, here's the type that the script declared: typedef struct t44 { int f44; } arg_type; And here's the (transformed) definition which caused it to be defined: bool_t xdr_arg_type(xdrs, objp) XDR *xdrs; arg_type *objp; { bool_t ret; init___ (&ret); return ret; } So it turns out that it's nothing to worry about. And that's it. There are no other warnings or errors. Which means that the transformation was successful and didn't mess up any of the parameter types. The check-decls script is below. One might argue that it is about as complex as the fix-decls script. This is true, but the code which actually extracts the `-' and `+' lines is fairly simple. Also, after being extracted, there are no transformations made to these lines aside from appending ___<num> to the function name if the script detects that the function name has already been seen. Most importantly, the parameter lists are not rewritten in any way. Most of the complexity is in the analysis and generation of the type, struct, enum, and union declarations. But uniqueness of these is easy to verify. Plus, if something is screwed up, the compiler complains. --- check-decls --- #!/usr/bin/perl -w # Feed this script a unidiff after running fix-decls and it generates # (on stdout) a program which may be used to test the validity of the # conversion. Just run the result through gcc -Wall and if it # generates any warnings, there's a problem... undef $/; # slurp mode my $diff = <>; # read entire diff in $diff; my $decls = ''; my $defns = ''; my %userstructs = (); my %userenums = (); my %usertypes = (); my %funcnames = (); my $funcname_gensym = 0; # for names that clash my @needuse; while ($diff =~ / ( ^ # beginning of line [^\n]+ # everything til the end of line ) \n # newline ( (?: ^ # beginning of line - # minus sign (?: \n # either just a newline | # -- or -- [^-\n] # any character but minus and newline [^\n]* # the rest of the line \n # including the newline ) )+ # one or more of the above ) ( (?: ^ # beginning of line \+ # plus sign [^+] # any character but plus [^\n]* # the rest of the line \n # including the newline )+ # one or more of the above ) /mgx) { my ($rettype, $traddecl, $isodecl) = ($1, $2, $3); # Remove leading diff character from the lines extracted foreach ($rettype, $traddecl, $isodecl) { s/^.//mg; } # Find type names in parameter list my $parmdecls = $traddecl; $parmdecls =~ s/^\w+\s*\([^)]*\)//; foreach my $parm (split /\s*;\s*/, $parmdecls) { $parm =~ s/\s*\**\w+(,|$).*$//; analyze_type($parm); } # Resolve collisions between function name (either due to statics # or due to the names being in different branches of an ifdef) my ($funcname) = $traddecl =~ /^(\w+)/; if (defined $funcnames{$funcname}) { foreach ($traddecl, $isodecl) { s/\b$funcname\b/${funcname}___$funcname_gensym/; } $funcname .= "___$funcname_gensym"; $funcname_gensym++; } $funcnames{$funcname} = $funcname; # Nuke comments in the return type $rettype =~ s#/\*.*?\*/##g; # Nuke partial comment in return type $rettype =~ s#^.*?\*/##; # Eliminate ``CALLBACK'' from return type $rettype =~ s/\bCALLBACK\b//; # Eliminate ``extern'' from return type $rettype =~ s/\bextern\b//; # Eliminate leading and trailing spaces from return type $rettype =~ s/^\s*//; $rettype =~ s/\s*$//; if (($rettype =~ /^#/) || ($rettype eq '')) { # preprocessor line or empty string $rettype = 'int'; } elsif ($rettype eq "static") { $rettype = 'static int'; } elsif ($rettype eq "private") { $rettype = 'static int'; } else { analyze_type($rettype); } $isodecl =~ s/\n\Z/;\n/; $decls .= "$rettype $isodecl"; if ($rettype =~ /\bvoid$/) { $defns .= "$rettype\n$traddecl\{\n}\n\n"; } else { $defns .= "$rettype\n$traddecl\{\n $rettype ret;\n" . " init___ (&ret);\n return ret;\n}\n\n"; } if ($rettype =~/\bstatic\b/) { push @needuse, $funcname; } } my $typeidx = 0; foreach $key (sort keys %usertypes) { print "typedef struct t$typeidx { int f$typeidx; } $key;\n"; $typeidx++; } foreach $key (sort keys %userstructs) { print "$key { int f$typeidx; };\n"; $typeidx++; } foreach $key (sort keys %userenums) { print "$key { e$typeidx };\n"; $typeidx++; } print "#define INLINE\n"; print "#define private\n"; print "#define CONST const\n"; print "#define NORETURN\n"; print "void init___ (void *);\n"; print $decls; print "\n"; print $defns; print "void\nuse___ (void)\n{\n"; foreach (@needuse) { print " init___ ($_);\n"; } print "}\n"; sub analyze_type { my ($parm) = @_; $parm =~ s/\s*\**\s*$//; my $type; if ($parm =~ /\b(struct|union)\b/) { $parm =~ s/\A.*\b(struct|union)\b/$1/s; $parm =~ s/\s*\**\s*\Z//s; $userstructs{$parm} = $parm; } elsif ($parm =~ /\b(enum)\b/) { $parm =~ s/\A.*\b(enum)\b/$1/s; $parm =~ s/\s*\**\s*\Z//s; $userenums{$parm} = $parm; } elsif ((($type) = $parm =~ /(\w+)$/) && ($type !~ /^(int|char|long|short|unsigned|double |register|void|const|static)$/x)) { $usertypes{$type} = $type; } } --- end check-decls --- ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: Proposal: convert function definitions to prototyped form 2000-06-02 2:42 ` Andrew Cagney 2000-06-03 0:13 ` Kevin Buettner @ 2000-06-03 0:21 ` Kevin Buettner 2000-06-03 4:52 ` Andrew Cagney 1 sibling, 1 reply; 57+ messages in thread From: Kevin Buettner @ 2000-06-03 0:21 UTC (permalink / raw) To: Andrew Cagney; +Cc: gdb On Jun 2, 7:42pm, Andrew Cagney wrote: > PS: You may want to add gdb/*-share to the list of directories to avoid. I can certainly do this, but I'd like to know why they shouldn't be converted... ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: Proposal: convert function definitions to prototyped form 2000-06-03 0:21 ` Kevin Buettner @ 2000-06-03 4:52 ` Andrew Cagney 0 siblings, 0 replies; 57+ messages in thread From: Andrew Cagney @ 2000-06-03 4:52 UTC (permalink / raw) To: Kevin Buettner; +Cc: gdb Kevin Buettner wrote: > > On Jun 2, 7:42pm, Andrew Cagney wrote: > > > PS: You may want to add gdb/*-share to the list of directories to avoid. > > I can certainly do this, but I'd like to know why they shouldn't be > converted... Some of the *-share files are based on code from third parties. We should probably try to avoid munging that code - it will make it harder for us merge back patches. rdi-share comes to mind. As you note, the TUI which will be cleaned up, can be handed separatly. Andrew ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: Proposal: convert function definitions to prototyped form 2000-06-02 0:50 Kevin Buettner 2000-06-02 2:42 ` Andrew Cagney @ 2000-06-02 5:26 ` Mark Kettenis 2000-06-02 8:16 ` Kevin Buettner 2000-06-03 4:48 ` Andrew Cagney 2000-06-12 18:10 ` Andrew Cagney 3 siblings, 1 reply; 57+ messages in thread From: Mark Kettenis @ 2000-06-02 5:26 UTC (permalink / raw) To: kevinb; +Cc: gdb Date: Fri, 2 Jun 2000 00:50:19 -0700 From: Kevin Buettner <kevinb@cygnus.com> Comma separated list with differing number of stars on the parameter names (sparc-tdep.c): static branch_type -isbranch (instruction, addr, target) - long instruction; - CORE_ADDR addr, *target; +isbranch (long instruction, CORE_ADDR addr, CORE_ADDR * target) { I guess you should tweak it some more such that it outputs CORE_ADDR *target instead of CORE_ADDR * target (note the spurious space between * and target). Mark ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: Proposal: convert function definitions to prototyped form 2000-06-02 5:26 ` Mark Kettenis @ 2000-06-02 8:16 ` Kevin Buettner 2000-06-02 10:44 ` J.T. Conklin 0 siblings, 1 reply; 57+ messages in thread From: Kevin Buettner @ 2000-06-02 8:16 UTC (permalink / raw) To: Mark Kettenis; +Cc: gdb On Jun 2, 2:26pm, Mark Kettenis wrote: > Date: Fri, 2 Jun 2000 00:50:19 -0700 > From: Kevin Buettner <kevinb@cygnus.com> > > Comma separated list with differing number of stars on the parameter > names (sparc-tdep.c): > > static branch_type > -isbranch (instruction, addr, target) > - long instruction; > - CORE_ADDR addr, *target; > +isbranch (long instruction, CORE_ADDR addr, CORE_ADDR * target) > { > > I guess you should tweak it some more such that it outputs > > CORE_ADDR *target > > instead of > > CORE_ADDR * target > > (note the spurious space between * and target). I noticed that. The space was put there by ``indent''. I would very much like to get rid of that space and it would be easy to make the perl script postprocess the ``indent'' output. But in doing so, we (obviously) generate different output than that of ``indent''. I suppose the other solution is to fix indent. :-) FYI, I'm using GNU indent 2.2.5. Kevin ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: Proposal: convert function definitions to prototyped form 2000-06-02 8:16 ` Kevin Buettner @ 2000-06-02 10:44 ` J.T. Conklin 2000-06-03 5:17 ` Andrew Cagney 2000-06-12 16:30 ` Eric Bachalo 0 siblings, 2 replies; 57+ messages in thread From: J.T. Conklin @ 2000-06-02 10:44 UTC (permalink / raw) To: Kevin Buettner; +Cc: Mark Kettenis, gdb >>>>> "Kevin" == Kevin Buettner <kevinb@cygnus.com> writes: Kevin> I noticed that. The space was put there by ``indent''. I Kevin> would very much like to get rid of that space and it would be Kevin> easy to make the perl script postprocess the ``indent'' output. Kevin> But in doing so, we (obviously) generate different output than Kevin> that of ``indent''. Kevin> Kevin> I suppose the other solution is to fix indent. :-) You can tell indent about all the types defined by typedef with -T option, and then it won't add the extra space. It shouldn't be too difficult to identify all the types. It might be useful for us to maintain an indent.pro file that has these definitions so that additional runs of indent don't add back the space. --jtc -- J.T. Conklin RedBack Networks ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: Proposal: convert function definitions to prototyped form 2000-06-02 10:44 ` J.T. Conklin @ 2000-06-03 5:17 ` Andrew Cagney 2000-06-05 11:05 ` J.T. Conklin 2000-06-12 16:30 ` Eric Bachalo 1 sibling, 1 reply; 57+ messages in thread From: Andrew Cagney @ 2000-06-03 5:17 UTC (permalink / raw) To: jtc; +Cc: Kevin Buettner, Mark Kettenis, gdb "J.T. Conklin" wrote: > > >>>>> "Kevin" == Kevin Buettner <kevinb@cygnus.com> writes: > Kevin> I noticed that. The space was put there by ``indent''. I > Kevin> would very much like to get rid of that space and it would be > Kevin> easy to make the perl script postprocess the ``indent'' output. > Kevin> But in doing so, we (obviously) generate different output than > Kevin> that of ``indent''. > Kevin> > Kevin> I suppose the other solution is to fix indent. :-) > > You can tell indent about all the types defined by typedef with -T > option, and then it won't add the extra space. It shouldn't be too > difficult to identify all the types. > > It might be useful for us to maintain an indent.pro file that has > these definitions so that additional runs of indent don't add back > the space. Given that people are currently editing headers to remove spaces by hand this sounds like a good idea. One thing, how does one pass ``indent.pro'' rather than have it pick up ``.indent.pro''? I suspect it can be maintained largely by hand (please not another perl script to maintain it - we can't make perl a requirement on developers :-) enjoy, Andrew ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: Proposal: convert function definitions to prototyped form 2000-06-03 5:17 ` Andrew Cagney @ 2000-06-05 11:05 ` J.T. Conklin 0 siblings, 0 replies; 57+ messages in thread From: J.T. Conklin @ 2000-06-05 11:05 UTC (permalink / raw) To: Andrew Cagney; +Cc: Kevin Buettner, Mark Kettenis, gdb >>>>> "Andrew" == Andrew Cagney <ac131313@cygnus.com> writes: Andrew> Given that people are currently editing headers to remove Andrew> spaces by hand this sounds like a good idea. One thing, how Andrew> does one pass ``indent.pro'' rather than have it pick up Andrew> ``.indent.pro''? I suspect it can be maintained largely by Andrew> hand (please not another perl script to maintain it - we can't Andrew> make perl a requirement on developers :-) For what we want to do, indent needs an option to disable processing the .indent.pro files located in the current or home directories, and another to process an arbitrary file. I examined the latest revision of the indent sources, 2.2.5, and I can not find an option to process a file containing options. It appears that indent is being actively maintained. Perhaps someone with a bit of time on their hands can work with the indent maintainer to get such an option in a future revesion. --jtc -- J.T. Conklin RedBack Networks ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: Proposal: convert function definitions to prototyped form 2000-06-02 10:44 ` J.T. Conklin 2000-06-03 5:17 ` Andrew Cagney @ 2000-06-12 16:30 ` Eric Bachalo 2000-06-12 17:29 ` Andrew Cagney ` (2 more replies) 1 sibling, 3 replies; 57+ messages in thread From: Eric Bachalo @ 2000-06-12 16:30 UTC (permalink / raw) To: jtc; +Cc: Kevin Buettner, Mark Kettenis, gdb "J.T. Conklin" wrote: > > >>>>> "Kevin" == Kevin Buettner <kevinb@cygnus.com> writes: > Kevin> I noticed that. The space was put there by ``indent''. I > Kevin> would very much like to get rid of that space and it would be > Kevin> easy to make the perl script postprocess the ``indent'' output. > Kevin> But in doing so, we (obviously) generate different output than > Kevin> that of ``indent''. > Kevin> > Kevin> I suppose the other solution is to fix indent. :-) > > You can tell indent about all the types defined by typedef with -T > option, and then it won't add the extra space. It shouldn't be too > difficult to identify all the types. > > It might be useful for us to maintain an indent.pro file that has > these definitions so that additional runs of indent don't add back > the space. I believe etags does a fairly good job a finding typedefs. I would be a fairly simple script to parse the TAGS file to make a list of all typedefs. I have a python script for this. Not sure how ported etags and python are for all the hosts that compile GDB though. - Eric Bachalo ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: Proposal: convert function definitions to prototyped form 2000-06-12 16:30 ` Eric Bachalo @ 2000-06-12 17:29 ` Andrew Cagney 2000-06-12 18:03 ` Daniel Berlin 2000-06-12 18:55 ` Kevin Buettner 2000-06-13 3:34 ` Eli Zaretskii 2 siblings, 1 reply; 57+ messages in thread From: Andrew Cagney @ 2000-06-12 17:29 UTC (permalink / raw) To: Eric Bachalo; +Cc: gdb Eric Bachalo wrote: > > "J.T. Conklin" wrote: > > > > >>>>> "Kevin" == Kevin Buettner <kevinb@cygnus.com> writes: > > Kevin> I noticed that. The space was put there by ``indent''. I > > Kevin> would very much like to get rid of that space and it would be > > Kevin> easy to make the perl script postprocess the ``indent'' output. > > Kevin> But in doing so, we (obviously) generate different output than > > Kevin> that of ``indent''. > > Kevin> > > Kevin> I suppose the other solution is to fix indent. :-) > > > > You can tell indent about all the types defined by typedef with -T > > option, and then it won't add the extra space. It shouldn't be too > > difficult to identify all the types. > > > > It might be useful for us to maintain an indent.pro file that has > > these definitions so that additional runs of indent don't add back > > the space. > > I believe etags does a fairly good job a finding typedefs. I would be a > fairly simple script to parse the TAGS file to make a list of all typedefs. I > have a python script for this. Not sure how ported etags and python are for > all the hosts that compile GDB though. As a guide, check the list in: http://www.gnu.ai.mit.edu/prep/standards_45.html Andrew ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: Proposal: convert function definitions to prototyped form 2000-06-12 17:29 ` Andrew Cagney @ 2000-06-12 18:03 ` Daniel Berlin 2000-06-12 18:15 ` Stan Shebs 0 siblings, 1 reply; 57+ messages in thread From: Daniel Berlin @ 2000-06-12 18:03 UTC (permalink / raw) To: Andrew Cagney; +Cc: Eric Bachalo, gdb > > I believe etags does a fairly good job a finding typedefs. I would be a > > fairly simple script to parse the TAGS file to make a list of all typedefs. I > > have a python script for this. Not sure how ported etags and python are for > > all the hosts that compile GDB though. > > As a guide, check the list in: > > http://www.gnu.ai.mit.edu/prep/standards_45.html > > Andrew > Funny you guys bring up the number of types in GDB. According to the debug info, there are >82,000. How exactly are we counting them, again? ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: Proposal: convert function definitions to prototyped form 2000-06-12 18:03 ` Daniel Berlin @ 2000-06-12 18:15 ` Stan Shebs 2000-06-12 18:23 ` Daniel Berlin 0 siblings, 1 reply; 57+ messages in thread From: Stan Shebs @ 2000-06-12 18:15 UTC (permalink / raw) To: Daniel Berlin; +Cc: Andrew Cagney, Eric Bachalo, gdb Daniel Berlin wrote: > > Funny you guys bring up the number of types in GDB. > According to the debug info, there are >82,000. > How exactly are we counting them, again? Since GDB sources are around 400K lines or so, it's highly unlikely that there is a type definition for every five lines in the C sources. You're seeing duplicates in the debug info, presumably. Stan ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: Proposal: convert function definitions to prototyped form 2000-06-12 18:15 ` Stan Shebs @ 2000-06-12 18:23 ` Daniel Berlin 2000-06-12 19:16 ` Anatoly Vorobey 0 siblings, 1 reply; 57+ messages in thread From: Daniel Berlin @ 2000-06-12 18:23 UTC (permalink / raw) To: Stan Shebs; +Cc: Andrew Cagney, Eric Bachalo, gdb Before my patches to eliminate duplicate dwarf2 info, it said there were 120k. So something isn't adding up here. I find it hard to believe we duplicate that much info. It's pretty much absurd. --Dan On Mon, 12 Jun 2000, Stan Shebs wrote: > Daniel Berlin wrote: > > > > Funny you guys bring up the number of types in GDB. > > According to the debug info, there are >82,000. > > How exactly are we counting them, again? > > Since GDB sources are around 400K lines or so, it's highly unlikely > that there is a type definition for every five lines in the > C sources. You're seeing duplicates in the debug info, presumably. > > Stan > ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: Proposal: convert function definitions to prototyped form 2000-06-12 18:23 ` Daniel Berlin @ 2000-06-12 19:16 ` Anatoly Vorobey 2000-06-12 19:42 ` Daniel Berlin 0 siblings, 1 reply; 57+ messages in thread From: Anatoly Vorobey @ 2000-06-12 19:16 UTC (permalink / raw) To: Daniel Berlin; +Cc: gdb On Mon, Jun 12, 2000 at 06:23:27PM -0700, Daniel Berlin wrote: > Before my patches to eliminate duplicate dwarf2 info, it said there were > 120k. > So something isn't adding up here. How do you eliminate them? The vast majority of types in debug info are duplicate header file types coming from different source files. Reducing this should cut the number much more drastically than from 120k to 80k. I'd say by factor of 8 at the very least. My code which reads stabs info says that there are 52276 types (not trying to remove duplicates) defined in gdb (not including libbfd, libiberty and libopcodes; including them gives another 13000 or so) and of them 886 unique type *names*. This gives a lower bound, obviously, since names may be reused or may denote different types even coming from the same header due to different defines -- but it's also more useful than the raw number since it doesn't count the multitude of unnamed automatically generated types which are pointers, aliases, forward references, etc. That would mean around one type name per 400 lines of code. Since almost all type declarations are in header files which are about 1/10 of all sources in size, it looks reasonable. -- Anatoly Vorobey, mellon@pobox.com http://pobox.com/~mellon/ "Angels can fly because they take themselves lightly" - G.K.Chesterton ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: Proposal: convert function definitions to prototyped form 2000-06-12 19:16 ` Anatoly Vorobey @ 2000-06-12 19:42 ` Daniel Berlin 0 siblings, 0 replies; 57+ messages in thread From: Daniel Berlin @ 2000-06-12 19:42 UTC (permalink / raw) To: Anatoly Vorobey; +Cc: gdb On Tue, 13 Jun 2000, Anatoly Vorobey wrote: > On Mon, Jun 12, 2000 at 06:23:27PM -0700, Daniel Berlin wrote: > > Before my patches to eliminate duplicate dwarf2 info, it said there were > > 120k. > > So something isn't adding up here. > > How do you eliminate them? The vast majority of types in debug info > are duplicate header file types coming from different source files. > Reducing this should cut the number much more drastically than > from 120k to 80k. I'd say by factor of 8 at the very least. Easy. I have a simple hash table, 4096 entries right now. before we read a type die, we see if the name of the type is in the hash table (C++ ODR lets me, nobody with a C program has complained they have the same type in multiple translation units that isn't really the same type. I suspect it would break other stuff anyway), if it is, we use the type in the hash table. Otherwise, we enter it into the hash table. If it's in the hash table, but the name doesn't match, i have some code (not in gdb 5.0, because it's noisy and only for profiling, obviously) to tell me it's a collision. No collisions found. Theoretically, this should remove all duplicate type info being read in if there aren't any collisions. So something else is making them, most likely lookup_pointer_type or thereabouts. I guess part of the problem is that we can't free types. But i still think we generate way too many types. > > That would mean around one type name per 400 lines of code. Since almost all > type declarations are in header files which are about 1/10 of all sources > in size, it looks reasonable. > > -- > Anatoly Vorobey, > mellon@pobox.com http://pobox.com/~mellon/ > "Angels can fly because they take themselves lightly" - G.K.Chesterton > ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: Proposal: convert function definitions to prototyped form 2000-06-12 16:30 ` Eric Bachalo 2000-06-12 17:29 ` Andrew Cagney @ 2000-06-12 18:55 ` Kevin Buettner 2000-06-13 3:34 ` Eli Zaretskii 2 siblings, 0 replies; 57+ messages in thread From: Kevin Buettner @ 2000-06-12 18:55 UTC (permalink / raw) To: Eric Bachalo; +Cc: gdb On Jun 12, 4:30pm, Eric Bachalo wrote: > > You can tell indent about all the types defined by typedef with -T > > option, and then it won't add the extra space. It shouldn't be too > > difficult to identify all the types. > > > > It might be useful for us to maintain an indent.pro file that has > > these definitions so that additional runs of indent don't add back > > the space. > > I believe etags does a fairly good job a finding typedefs. I would be a > fairly simple script to parse the TAGS file to make a list of all typedefs. I > have a python script for this. Not sure how ported etags and python are for > all the hosts that compile GDB though. What you suggest would work so long as you also run etags over the system header files. For the task at hand, however, the check-decls script has already identified the necessary types. (Recall that check-decls is run on the "diff -u" output after doing a fix-decls. It constructs a program which is then compiled with "gcc -Wall" to check the validity of the protoization performed by fix-decls. In the process of constructing the validating program, it must first identify all of the types requiring typedefs.) Here is the list of types found by check-decls: ADDR32, B_TYPE, COMMON_ENTRY_PTR, CORE_ADDR, CPUSpace, DCACHE, DIE_REF, DOUBLEST, EXTR, EventRecord, FDR, FILE, HWND, INSN_WORD, INT32, LONG, LONGEST, LPARAM, LRESULT, PDR, PTR, PTRACE_ARG3_TYPE, PXDB_header_ptr, Point, Ptrace_return, RDB_EVENT, REGISTER_TYPE, RgnHandle, Rptrace, SAVED_BF_PTR, SAVED_F77_COMMON_PTR, SAVED_FUNCTION, SYMR, TTRACE_ARG_TYPE, UINT, ULONGEST, WAITTYPE, WPARAM, WindowPtr, XDR, YYSTYPE, alpha_extra_func_info_t, arg_array, arg_one, arg_type, arg_value, argsin, asection, attach_continue_t, bfd, bfd_arch_info_type, bfd_byte, bfd_signed_vma, bfd_vma, bool_t, boolean, boolean_t, bpstat, branch_type, catch_errors_ftype, catch_fork_kind, cma__t_int_tcb, disassemble_info, dld_cache_t, dnttpointer, dst_rec_ptr_t, dst_sec, dst_sect_ref_t, dst_type_t, file_ptr, fltset_t, fpregset_t, func_call, gdb_client_data, gdb_fpregset_t, gdb_gregset_t, gdb_thread_t, gdb_threadref, gregset_t, host_callback, insertion_state_t, insn_type, kern_return_t, lwpid_t, mach_msg_header_t, mach_msg_id_t, mach_msg_type_name_t, mach_port_mscount_t, mach_port_t, memory_page_t, memxferfunc, mips_extra_func_info_t, namespace_enum, off_t, pid_t, port_chain_t, process_state_t, procinfo, quick_file_entry, quick_module_entry, quick_procedure_entry, return_mask, rmt_thread_action, sec_ptr, serial_t, serial_ttystate, sigset_t, size_t, sltpointer, stepping_mode_t, sysset_t, t_inst, task_t, td_err_e, td_thr_state_e, td_thr_type_e, td_thragent_t, td_thrhandle_t, thread_array_t, thread_info, thread_t, threadinfo, threadref, time_t, tree, ttevents_t, ttreq_t, ttstate_t, ttwopt_t, u_long, ui_file_delete_ftype, ui_file_flush_ftype, ui_file_fputs_ftype, ui_file_isatty_ftype, ui_file_put_ftype, ui_file_rewind_ftype, va_list, value_ptr, and xdrproc_t Note that there are likely typedefs for types in the gdb sources missing from the above list, but these were not necessary for doing the fix-decls conversion. Also, there are some in the above list which are defined in system header files. But that's okay because indent still needs to know about them in order to do a good job of formatting. ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: Proposal: convert function definitions to prototyped form 2000-06-12 16:30 ` Eric Bachalo 2000-06-12 17:29 ` Andrew Cagney 2000-06-12 18:55 ` Kevin Buettner @ 2000-06-13 3:34 ` Eli Zaretskii 2 siblings, 0 replies; 57+ messages in thread From: Eli Zaretskii @ 2000-06-13 3:34 UTC (permalink / raw) To: ebachalo; +Cc: jtc, kevinb, kettenis, gdb > Date: Mon, 12 Jun 2000 16:30:44 -0700 > From: Eric Bachalo <ebachalo@redhat.com> > > Not sure how ported etags and python are for all the hosts that > compile GDB though. What platforms cause your concern? ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: Proposal: convert function definitions to prototyped form 2000-06-02 0:50 Kevin Buettner 2000-06-02 2:42 ` Andrew Cagney 2000-06-02 5:26 ` Mark Kettenis @ 2000-06-03 4:48 ` Andrew Cagney 2000-06-12 18:10 ` Andrew Cagney 3 siblings, 0 replies; 57+ messages in thread From: Andrew Cagney @ 2000-06-03 4:48 UTC (permalink / raw) To: Kevin Buettner; +Cc: gdb Kevin Buettner wrote: > > As many of you know, I'm in the midst of purging the use of ``PARAMS'' > in prototyped function declarations from the gdb sources. After this > activity is concluded, I'd like to begin converting function > definitions whose parameters are declared using the traditional C > style to the ISO C prototyped form. I.e, I'd like to convert > functions of the form Something to consider with the timing. There are Pascal, ObjectiveC.* and random other Apple files pending. It might be good to wait until the buil of the work is in the repository so that we reduce the number of contributors / maintainers that get hit for six by this one :-) Unlike params, this one will really hurt people with private diffs. Andrew ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: Proposal: convert function definitions to prototyped form 2000-06-02 0:50 Kevin Buettner ` (2 preceding siblings ...) 2000-06-03 4:48 ` Andrew Cagney @ 2000-06-12 18:10 ` Andrew Cagney 2000-06-12 19:48 ` Kevin Buettner 3 siblings, 1 reply; 57+ messages in thread From: Andrew Cagney @ 2000-06-12 18:10 UTC (permalink / raw) To: Kevin Buettner; +Cc: gdb Kevin Buettner wrote: > > As many of you know, I'm in the midst of purging the use of ``PARAMS'' > in prototyped function declarations from the gdb sources. After this > activity is concluded, I'd like to begin converting function > definitions whose parameters are declared using the traditional C > style to the ISO C prototyped form. I.e, I'd like to convert > functions of the form Kevin, Would a fair summary of this be that you've encountered the following road blocks: o indent.pro So that indent gives better results o a drain of some of the backlog of patches (pascal I believe is now going in). Apple and HP? No one has objected to the principal (well not on gdb-patches :-) and the tool would be based on perl rather than the the gcc thing. Once those blockages are cleared, it can be scheduled and can go through? Andrew ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: Proposal: convert function definitions to prototyped form 2000-06-12 18:10 ` Andrew Cagney @ 2000-06-12 19:48 ` Kevin Buettner 0 siblings, 0 replies; 57+ messages in thread From: Kevin Buettner @ 2000-06-12 19:48 UTC (permalink / raw) To: Andrew Cagney; +Cc: gdb On Jun 13, 11:09am, Andrew Cagney wrote: > Kevin Buettner wrote: > > > > As many of you know, I'm in the midst of purging the use of ``PARAMS'' > > in prototyped function declarations from the gdb sources. After this > > activity is concluded, I'd like to begin converting function > > definitions whose parameters are declared using the traditional C > > style to the ISO C prototyped form. I.e, I'd like to convert > > functions of the form > > Kevin, > > Would a fair summary of this be that you've encountered the following > road blocks: > > o indent.pro > > So that indent gives better results I don't think this is a serious road block. While it would be nice to be able to specify an indent.pro file (e.g., "indent --options-file gdbtypes.pro"), it's not absolutely necessary for the task at hand. As I noted in my reply to Eric, I already know the complete list of types which indent has to know about in order to complete the protoization task. It is easy to put this list in the script which does the conversion. I.e, @typelist = qw(ADDR32 B_TYPE ... value_ptr xdrproc_t); $indentoptions = '-T ' . join(' -T ', @typelist); > o a drain of some of the backlog of > patches (pascal I believe is now > going in). > > Apple and HP? I think the patch backlog is the real road block. However, even for this road block, the only reason for delaying the protoization activity is to make it easier on the people doing the patch integration. Perhaps setting a date for the protoization activity would help motivate the patch integrators to clear some of the backlog? > No one has objected to the principal (well not on gdb-patches :-) and > the tool would be based on perl rather than the the gcc thing. > > Once those blockages are cleared, it can be scheduled and can go > through? I still have a little over a week to go on the PARAMS elimination activity, so any time after then is good for me. How does midnight GMT of Sunday July 9 sound? That's about four weeks away. Is that enough time for the patch integrators to clear the patch backlog? ^ permalink raw reply [flat|nested] 57+ messages in thread
end of thread, other threads:[~2001-03-21 15:59 UTC | newest] Thread overview: 57+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2000-06-02 8:40 Proposal: convert function definitions to prototyped form David Taylor 2000-06-02 12:10 ` Kevin Buettner 2000-06-03 3:58 ` Eli Zaretskii [not found] ` <eliz@delorie.com> 2000-06-03 10:50 ` Kevin Buettner 2000-06-03 11:37 ` Eli Zaretskii 2000-06-03 18:18 ` Andrew Cagney 2000-06-03 15:42 ` Kevin Buettner 2001-02-16 0:45 ` [RFC] Unified watchpoints for x86 platforms Kevin Buettner -- strict thread matches above, loose matches on Subject: below -- 2000-09-07 1:55 gdb doesn't work very well with dynamic linked binaries James Cownie 2000-09-07 3:09 ` Mark Kettenis 2000-09-07 8:02 ` Eli Zaretskii 2000-09-08 8:30 ` Mark Kettenis 2001-02-10 7:34 ` [RFC] Unified watchpoints for x86 platforms Eli Zaretskii 2001-02-10 10:19 ` H . J . Lu 2001-02-10 11:28 ` Eli Zaretskii 2001-02-15 3:48 ` Eli Zaretskii 2001-02-15 8:17 ` Mark Kettenis 2001-02-15 9:32 ` Eli Zaretskii 2001-02-15 10:33 ` Mark Kettenis 2001-02-15 13:26 ` Eli Zaretskii 2001-02-15 10:41 ` Kevin Buettner 2001-02-15 13:26 ` Eli Zaretskii 2001-02-15 14:46 ` J.T. Conklin 2001-02-15 16:11 ` Kevin Buettner 2001-02-15 23:29 ` Eli Zaretskii 2001-02-24 10:14 ` Eli Zaretskii 2001-02-27 3:28 ` Mark Kettenis 2001-02-27 10:57 ` Eli Zaretskii 2001-03-21 15:59 ` [RFA] " Eli Zaretskii 2001-02-15 23:30 ` [RFC] " Eli Zaretskii 2001-02-16 10:52 ` J.T. Conklin 2001-02-16 0:00 ` Mark Kettenis 2001-02-15 9:08 ` H . J . Lu 2000-09-09 14:39 ` gdb doesn't work very well with dynamic linked binaries Peter.Schauer 2000-06-02 13:11 Proposal: convert function definitions to prototyped form David Taylor 2000-06-02 0:50 Kevin Buettner 2000-06-02 2:42 ` Andrew Cagney 2000-06-03 0:13 ` Kevin Buettner 2000-06-03 0:21 ` Kevin Buettner 2000-06-03 4:52 ` Andrew Cagney 2000-06-02 5:26 ` Mark Kettenis 2000-06-02 8:16 ` Kevin Buettner 2000-06-02 10:44 ` J.T. Conklin 2000-06-03 5:17 ` Andrew Cagney 2000-06-05 11:05 ` J.T. Conklin 2000-06-12 16:30 ` Eric Bachalo 2000-06-12 17:29 ` Andrew Cagney 2000-06-12 18:03 ` Daniel Berlin 2000-06-12 18:15 ` Stan Shebs 2000-06-12 18:23 ` Daniel Berlin 2000-06-12 19:16 ` Anatoly Vorobey 2000-06-12 19:42 ` Daniel Berlin 2000-06-12 18:55 ` Kevin Buettner 2000-06-13 3:34 ` Eli Zaretskii 2000-06-03 4:48 ` Andrew Cagney 2000-06-12 18:10 ` Andrew Cagney 2000-06-12 19:48 ` Kevin Buettner
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).