From: Keith Seitz <keiths@redhat.com>
To: Doug Evans <xdje42@gmail.com>
Cc: "gdb-patches@sourceware.org ml" <gdb-patches@sourceware.org>
Subject: Re: [RFA 2/9] Explicit locations v2 - Event locations API
Date: Tue, 27 Jan 2015 04:38:00 -0000 [thread overview]
Message-ID: <54C6A395.9080502@redhat.com> (raw)
In-Reply-To: <m3r3ydm0gv.fsf@sspiff.org>
Hi, Doug,
I apologize for the long hiatus on this, but it seems I needed a (long)
break to "rebase" myself!
On 10/12/2014 02:39 PM, Doug Evans wrote:
> This is one part I still don't fully grok.
> How come there's both location and copy_location here,
> given that we don't do anything with location except make a copy of it?
Okay, I have spent quite a bit of time trying to fix this, and while I
was very frustrated with this, I am now *very* grateful that you've
pushed me to readdress/revisit this. Thank you.
In the end, I've changed the way all of this works. This temporary copy
hack, advance pointer functions, and more is all gone and no longer
necessary.
I believe this revision addresses just about everything that you've raised.
You might ask, "How?" [*Might*? You will! :-)]
Really it all boils down to how linespecs are handled. The "usual"
mechanism that I am/have been proposing is:
char *arg; /* e.g., break_command_1 et al */
struct event_location *location;
struct cleanup *cleanup;
location = string_to_event_location (&arg, current_language);
cleanup = make_cleanup_delete_event_location (location);
sals = decode_line_full (location, ...);
/* ... */
do_cleanups (cleanup);
The thing that has been holding all of this back is that
string_to_event_location has never been able to deal with linespecs.
Given any other location type, string_to_event_location can advance the
argument over any processed input.
So the obvious solution to this is to teach it to do the same for
linespecs. I don't know why I didn't do this earlier. I even wrote a
proper lexer for linespecs a while ago. Using this,
string_to_event_location can now handle all location types the same.
[And it was *much* easier than I originally feared.]
As a result, there is no more need for all this "advance input pointer"
fooey that I was doing before. In fact, I've changed the linespec parser
and decode_line_* so that they don't even take a char** anymore. They
don't "advance" anything anymore at all. That is all done a priori by
string_to_event_location (or rather, the function that this function
calls to do it for linespecs).
> Also, event_locations are destructed differently, depending on how they're
> created, and IWBN to always just use the one destructor.
All gone.
> Also, I'd like to handle what event_location_advance_input_ptr
> and advance_event_location_ptr do differently, if only a little bit.
All gone, too.
> Which leads me to my last high level issue: maintaining intermediate
> parse state in event_location. I gather linespecs force some pragmatism
> on us (at least if we want to avoid yet more changes, which I'm totally
> happy to avoid for now). For example, this code is a bit confusing:
>
> breakpoint.c:location_to_sals:
>
> if (b->condition_not_parsed)
> {
> if (event_location_type (location) == LINESPEC_LOCATION)
> s = get_linespec_location (location);
> else
> s = b->extra_string;
>
This code is modified, but essentially still exists. The one big
difference now is that this is done for all location types. In other
words, regardless of the location type, b->extra_string is /always/
parsed for conditon/thread/task. [more on dprintf later]
This happens because string_to_event_location can advance the input
string past the actual location specification, leaving, eg., "arg" above
pointing at any conditional/thread/task. The code passes this as
extra_string to create_breakpoint (which is so vaguely documented so as
to allow me, without much consternation, to use it this way).
[This does not interfere with dprintfs, since conditions et al are not
supported on dprintf "inline" in this manner, e.g., 'dprintf
main,"hello" if argc == 1' is NOT supported. Although this patch series
makes it much easier to fix.]
> If we add two pointers, one to the beginning of the string and one
> to the current parse location, then get_linespec_location could
> always return a pointer to the beginning of the string, and then
> a new function could return a pointer to the current parse state
> (get_current_linespec_text or some such).
> Does that make sense?
> [It seems so, and it would involve minimal changes.]
Yup, and I actually did do this (at your suggestion). It's not so quite
so trivial, but it doesn't really matter anymore. It's all gone. :-)
>> As I mentioned the last time around, I've changed the event location
>> API to compute this string when it can (it caches a copy of it inside
>> the struct event_location -- I'm seeing this string computed many,
>> many times).
>
> Yeah, I like this.
>
>> There are two instances in which we save the string instead of computing it:
>
> This part not so much. :-)
It's not so bad. Either we put members into struct event_location for
all these corner conditions, or we just save the string, which is what
we really need/use.
>> 1) pending breakpoints: we save the entire input string into the
>> location (needed to preserve, e.g., conditions)
This is still the same. I did find a bug when a pending breakpoint is
resolved, i.e.,
(gdb) break -function main if argc == 1
Set pending? y
(gdb) file myfile
...
(gdb) save breakpoints bps
(gdb) shell cat bps
break -function main if argc == 1
cond $bpnum argc == 1
[FWIW, this occurs today with linespecs/address strings.]
I've fixed this (for linespecs, too!) in the code by clearing the string
representation when the breakpoint is resolved. The next time it is
needed, it will be (properly) computed.
>> 2) When converting/canonicalizing a linespec to explicit form, we must
>> save the linespec string representation (so that we can reproduce the
>> linespec the way the user originally specified it). The other option
>> here is to add a flag to event_location to note this. Then we can
>> compute the location. I chose to simply save the linespec string
>> representation.
>
> I like the consistency of event_location.as_string always being
> a cached copy of the string form of the location.
> Otherwise it's confusing.
> Does it make sense to add another string to event_location to
> handle (1) and (2) above?
We could, sure. Is it necessary, though? I am not so sure. Like
linespecs/address strings, these event locations represent the user's
view of a location in the inferior. We already manipulate this string
representation in the same places today. [The one exception is the fix
for the above-mentioned bug. That's the one new place we manipulate the
string representation.]
> I'm hoping these suggestions make sense and they won't involve
> too many changes.
Just a bit of bookkeeping to get back into it. It's a large patch
series; it just takes a while to do anything. [Perhaps my process needs
improving, too.]
Shall I submit this new series, or would you like to offer any further
comments on this or subsequent patches in the series?
Keith
PS. I will be leaving for FOSDEM tomorrow (Tues, 27 Jan) and returning
on Mon, 9 Feb). I will not have much access to anything other than email
at this time.
next prev parent reply other threads:[~2015-01-26 20:29 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-05-08 17:59 Keith Seitz
2014-05-12 22:59 ` Doug Evans
2014-05-30 18:16 ` Keith Seitz
2014-06-06 5:45 ` Doug Evans
2014-06-06 6:09 ` Doug Evans
2014-09-03 19:32 ` Keith Seitz
2014-10-12 21:40 ` Doug Evans
2015-01-27 4:38 ` Keith Seitz [this message]
2015-01-28 20:59 ` Doug Evans
2015-01-28 22:12 ` Doug Evans
2015-01-29 0:04 ` Doug Evans
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=54C6A395.9080502@redhat.com \
--to=keiths@redhat.com \
--cc=gdb-patches@sourceware.org \
--cc=xdje42@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).