public inbox for archer@sourceware.org
 help / color / mirror / Atom feed
* Stop the Insanity! Linespec Rewrite
@ 2012-03-02  1:14 Keith Seitz
  2012-03-02 17:00 ` Tom Tromey
  2012-03-02 19:07 ` [Archer] " Joel Brobecker
  0 siblings, 2 replies; 20+ messages in thread
From: Keith Seitz @ 2012-03-02  1:14 UTC (permalink / raw)
  To: archer

Hi,

I think that most of us know the pain that is linespec.c. Well, Tom gave 
me the green light on this a while back, and I played with an initial 
design/implementation in September.

Earlier this month, I dug this out of mothballs, and it is now time to 
submit the work-in-progress for comment. This is *not* a finished design 
and/or implementation! It is simply a place to stop along the road and 
ask for advice, for a fresh set of eyes. Consider this akin to an RFC.

Let me recap what some of the more important requirements for this 
rewrite were:

- More robust parser. As several of us can attest to, adding just about 
anything to linespec.c is PAINFUL. Really painful. We need a more robust 
linespec-to-sal infrastructure.

- Consolidate similar functionality. There are several functions which 
pretty much do the same thing (e.g., decode_variable vs decode_compound 
vs the remaining bits of decode_internal).

- Rain on the quoting parade. This is actually pretty tightly coupled to 
the first requirement, but it is such a nightmare as it is now, that 
it's just worth calling out separately. I doubt I need say more.

- Enable a way for "explicit" linespecs (as I call them, for lack of a 
better term). I would like to add the ability to do, e.g., "break 
-sourcefile foo.c -function baz::doit(char*) -offset 3" and the like. 
Probably most useful to MI (and python), but not altogether useless to 
CLI users.

- Increase the maintainability and reduce the fragility of this code. If 
you haven't had the pleasure of hacking on linespecs, consider yourself 
lucky. You've never lost sleep over the prospect of digging into this code.

I'm sure there are some other requirements/wish-list items which I've 
forgotten. These are just basically mine.

If you'll pardon my rambling, here are some notes about what the 
design/implementation does/does not do:

o We now have a (trivial) parser and a lexer. The lexer "word" breaks 
the input on ':', but it does know about "::" as a scope operator for 
C++ (but nothing on ObjC). The whole thing attempts to integrate all 
languages into one design/implementation. You will not see any 
references to current_language anywhere, except one tiny place where 
canonicalization is done, and that's cut-n-paste from current sources.

o Speaking of canonicalization: that is now done in only one place in 
the parser.

o Quoting is greatly minimized. Got spaces in a filename? Doesn't matter 
anymore. The only thing that matters is ':'. Everything else is simply 
text. When a quote character (either " or ') is seen, everything is 
skipped to the next quote character. [No, you cannot mix quote 
characters in the same lexeme, e.g., "foo'bar'" will return the lexeme 
foo'bar'.] If your filename is "main::foo.cc", then you must quote that: 
"break 'main::foo.cc':3".

o As previously, Objective C is short-circuited. I have not changed that 
at all. Selector names would need quoting to avoid being lexed 
incorrectly. I haven't pursued this at all (and probably won't).

o All current functionality is maintained AFAICT. I did not add any new 
functionality that didn't just drop out from doing it this way. [One 
example: you can now do "break myclass::mymethod::a_label". It "just 
works."]

o decode_* are almost all gone. All symbol lookups are essentially done 
by one function (although there is one helper function for it). [Notable 
exceptions: decode_dollar and decode_objc.]

o Canonicalization (of the linespec) is now pretty trivial and isolated 
to one function. It is no longer scattered all over the place.

o Error reporting could be greatly expanded. The version I have 
committed simply maintains the status quo.

o Some linespecs don't work anymore. IMO, these existed because of the 
nightmare of the existing code. The only group of linespecs that won't 
work involve "goofy" quoting. For example, you cannot do: "break 
klass::'operator +'" anymore. Nor can you do "break 
'foo.c:static_function'". IMO a small price to pay for sanity.

o I have not written tests for this yet.

o I have not checked memory allocation or other such common problems. My 
goal was to get something that works.

o There is probably some dead code that can/could now be removed. 
[cp_validate_operator comes to mind] I have not even begun to look for this.

o Probably lots of cleanup to do. Probably goofed/thinko'd a few things, 
too, especially in the area of ambiguous linespecs. This is part of the 
reason for exposing this less-than-beta quality code to other developers 
and maintainers. I still have comments to myself all over the place 
(comments with "!!" and #if WHATS_THIS_FOR). Who needs a stinkin' 
notebook!? I take notes in the code.

Ok, so enough of that. You probably are all anxious to see what I've 
actually done...

If you will permit me a few more paragraphs of your time, please allow 
me very briefly describe the underlying idea. Basically, I've split 
linespec decoding into several main areas: symtabs, symbols, labels, 
line offsets, SALs, and canonicalization The first four are pretty 
obvious. We have to find those things. The last two are a rather large 
departure from the current code. [I won't go so far as to call it a design.]

Previously we constructed SALs as we found symbols. At the same time, we 
also constructed canonical linespec representations. We now take in a 
list of parameters and convert them to SALs in one swoop. Then we 
compute any necessary canonicalizations.

The main routine dealing with SALs is convert_linespec_to_sals. This 
converts a structure into a list of SALs, which is then returned to the 
caller. Canonicalization is done by canonicalize_linespec.

The structure which must be "filled-in" for convert_linespec_to_sals is 
"typedef struct linespec *linespec_t". Start with that. Right now, the 
only way to fill-in this structure is via the parser, but another 
function for "explicit" linespecs could fill it in, too, by using some 
of the new functions that I've introduced.

Ok, so enough chatter. You want to see the actual code.

Here you go: Archer branch "archer-keiths-linespec-rewrite" [duh!]

Please share your comments, concerns, and suggestions. Please help me 
stop the [linespec] insanity!

Keith

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: Stop the Insanity! Linespec Rewrite
  2012-03-02  1:14 Stop the Insanity! Linespec Rewrite Keith Seitz
@ 2012-03-02 17:00 ` Tom Tromey
  2012-03-02 22:41   ` Keith Seitz
  2012-03-02 19:07 ` [Archer] " Joel Brobecker
  1 sibling, 1 reply; 20+ messages in thread
From: Tom Tromey @ 2012-03-02 17:00 UTC (permalink / raw)
  To: Keith Seitz; +Cc: archer

Keith> Earlier this month, I dug this out of mothballs, and it is now time to
Keith> submit the work-in-progress for comment. This is *not* a finished
Keith> design and/or implementation! It is simply a place to stop along the
Keith> road and ask for advice, for a fresh set of eyes. Consider this akin
Keith> to an RFC.

I like the direction this is headed.
It is ready for the cleanup-and-submit phase, IMO.

Keith> I'm sure there are some other requirements/wish-list items which I've
Keith> forgotten. These are just basically mine.

Eventually --

* Allow fine-grained breakpoint re-setting by allowing a parsed linespec
  to be evaluated in a particular context.

* Reimplement linespec completion.

Keith> o We now have a (trivial) parser and a lexer. The lexer "word" breaks
Keith> the input on ':', but it does know about "::" as a scope operator for
Keith> C++ (but nothing on ObjC).

From reading the code I had a feeling that a leading ":" wasn't handled
properly.  So I tried it, and `b ::foo' causes an internal error.

A couple other notes -

linespec_parse_line_offset only allows decimal but the lexer allows hex.
But why bother with hex?

I noticed the code is missing 'const' in a number of places.

Keith> For example, you cannot do: "break
Keith> klass::'operator +'" anymore.

What is the issue with this one?

Keith> o Probably lots of cleanup to do. Probably goofed/thinko'd a few
Keith> things, too, especially in the area of ambiguous linespecs. This is
Keith> part of the reason for exposing this less-than-beta quality code to
Keith> other developers and maintainers. I still have comments to myself all
Keith> over the place (comments with "!!" and #if WHATS_THIS_FOR). Who needs
Keith> a stinkin' notebook!? I take notes in the code.

Hah, me too sometimes.

I saw the WHATS_THIS_FOR.  I'm not sure what it is for, but on some
platforms a symbol can start with "$", so maybe it is handling that
case.  You can maybe construct one with gcc -fdollars-in-identifiers.

Tom

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Archer] Stop the Insanity! Linespec Rewrite
  2012-03-02  1:14 Stop the Insanity! Linespec Rewrite Keith Seitz
  2012-03-02 17:00 ` Tom Tromey
@ 2012-03-02 19:07 ` Joel Brobecker
  2012-03-02 22:49   ` Keith Seitz
  1 sibling, 1 reply; 20+ messages in thread
From: Joel Brobecker @ 2012-03-02 19:07 UTC (permalink / raw)
  To: Keith Seitz; +Cc: archer

> Earlier this month, I dug this out of mothballs, and it is now time
> to submit the work-in-progress for comment. This is *not* a finished
> design and/or implementation! It is simply a place to stop along the
> road and ask for advice, for a fresh set of eyes. Consider this akin
> to an RFC.

I really like the fact that you're now splitting the parsing from the
evaluation (the transformation into SALs).

Looking at the parsing code, I had a feeling that it would not work
with Ada operators, and in particular with:

        (gdb) break "*"
        Argument required (expression to compute).

I then tried other operators that might be problematic, eg:

        (gdb) break "+"
        Function "+" not defined.
        Make breakpoint pending on future shared library load

But also simpler-to-handle operators such as "/" do not seem to work:

        (gdb) braek "/"
        Function "/" not defined.
        Make breakpoint pending on future shared library load

I reviewed the gdb.ada testsuite, and it appears that I failed to
add a testcase for those. I will put that on my list.

I can help with the implementation side of the above feature, but
do you think your parser could accomodate that? As discussed with
Tom, it is fine if we need to compromise a bit an accept "break *"
(the operator name without quotes) or "break '*'" (operator name
bracketed by single-quotes, which is illegal Ada, but who cares).

I have noticed a few other things:

  . Very minor: We now accept the "task" keyword in any casing (?).
    So now "break foo TASK 3" is accepted, whereas it wasn't in the past.

  . Your new linespec passes the entire gdb.ada testsuite. Congrats,
    this is no small feat, if you ask me!

  . Our testsuite spotted a couple of crashes. They might be related
    to the crash that Tom mentioned, although I kind of doubt it.
    Nevertheless, I'll investigate them after you've looked at Tom's
    report, just in case they end up having the same cause.

The code itself looks pretty good to me, although I mostly skimmed
through it. I agree with Tom that you can think of cleaning things up
and do an official submission. I'd like to resolve the issues mentioned
above before the patch actually goes in, but I have spare cycles next
week for that - let's enjoy those spares cycles while they last. They
have been pretty rare lately...

-- 
Joel

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: Stop the Insanity! Linespec Rewrite
  2012-03-02 17:00 ` Tom Tromey
@ 2012-03-02 22:41   ` Keith Seitz
  2012-03-03  2:36     ` Tom Tromey
  0 siblings, 1 reply; 20+ messages in thread
From: Keith Seitz @ 2012-03-02 22:41 UTC (permalink / raw)
  To: Tom Tromey; +Cc: archer

On 03/02/2012 08:59 AM, Tom Tromey wrote:
> Keith>  o We now have a (trivial) parser and a lexer. The lexer "word" breaks
> Keith>  the input on ':', but it does know about "::" as a scope operator for
> Keith>  C++ (but nothing on ObjC).
>
>  From reading the code I had a feeling that a leading ":" wasn't handled
> properly.  So I tried it, and `b ::foo' causes an internal error.

Doh! I can't believe there is no existing test for that! In any case,
I've got a patch for that.

> A couple other notes -
>
> linespec_parse_line_offset only allows decimal but the lexer allows hex.
> But why bother with hex?

That's a cleanup. I implemented that a loooong time ago, well, just 
because I could. I'll remove that during the cleanup phase (along with 
several other unnecessary things).

> I noticed the code is missing 'const' in a number of places.

I'll fix those.

> Keith>  For example, you cannot do: "break
> Keith>  klass::'operator +'" anymore.
>
> What is the issue with this one?

Not much, really. I could probably implement something to do it, but I 
really view this whole quoting issue as a workaround for bugs in the old 
linespec.c We don't/shouldn't really need that from here on out.

Let me know if you want me to add that back in.

> I saw the WHATS_THIS_FOR.  I'm not sure what it is for, but on some
> platforms a symbol can start with "$", so maybe it is handling that
> case.  You can maybe construct one with gcc -fdollars-in-identifiers.

I'll play with that and see if I can trigger this.

Thank you for taking a look!

Keith

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Archer] Stop the Insanity! Linespec Rewrite
  2012-03-02 19:07 ` [Archer] " Joel Brobecker
@ 2012-03-02 22:49   ` Keith Seitz
  2012-03-06 17:40     ` [Archer] " Joel Brobecker
  0 siblings, 1 reply; 20+ messages in thread
From: Keith Seitz @ 2012-03-02 22:49 UTC (permalink / raw)
  To: Joel Brobecker; +Cc: archer

On 03/02/2012 11:06 AM, Joel Brobecker wrote:
> I reviewed the gdb.ada testsuite, and it appears that I failed to
> add a testcase for those. I will put that on my list.

I have a patch to fix this. Thank you for bringing this to my attention 
(and adding tests for it). Your new operator_bps.exp passes 100% on 
archer-keiths-linespec-rewrite. [I haven't pushed the patch yet.]

> I have noticed a few other things:
>
>    . Very minor: We now accept the "task" keyword in any casing (?).
>      So now "break foo TASK 3" is accepted, whereas it wasn't in the past.

I need to audit that. Part of the cleanup phase. Lots to do yet.

>    . Our testsuite spotted a couple of crashes. They might be related
>      to the crash that Tom mentioned, although I kind of doubt it.
>      Nevertheless, I'll investigate them after you've looked at Tom's
>      report, just in case they end up having the same cause.

If you can tell me how to reproduce, I will take a look. There are 
probably lots of gotchas all over the place. I relied heavily (too 
much?) on the test suite. I wonder what bugs Go, D, and other languages 
might expose.

> I'd like to resolve the issues mentioned above before the patch
> actually goes in, [...]

Absolutely. I would not even think of submitting this for official 
review if it had any (real) regressions.

Thank you very much for your help.

Keith

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: Stop the Insanity! Linespec Rewrite
  2012-03-02 22:41   ` Keith Seitz
@ 2012-03-03  2:36     ` Tom Tromey
  0 siblings, 0 replies; 20+ messages in thread
From: Tom Tromey @ 2012-03-03  2:36 UTC (permalink / raw)
  To: Keith Seitz; +Cc: archer

Tom> From reading the code I had a feeling that a leading ":" wasn't handled
Tom> properly.  So I tried it, and `b ::foo' causes an internal error.

Keith> Doh! I can't believe there is no existing test for that!

It's always funny what is in there and what is not.
The test suite caught a lot of stuff for me during my linespec hacking.
But then sometimes you trip across a funny hole like this...

Keith> For example, you cannot do: "break
Keith> klass::'operator +'" anymore.

Tom> What is the issue with this one?

Keith> Not much, really. I could probably implement something to do it, but I
Keith> really view this whole quoting issue as a workaround for bugs in the
Keith> old linespec.c We don't/shouldn't really need that from here on out.

I remembered now -- this one has quotes in the middle of a token.
I agree, we don't want to deal with that unless there is a lot of
push-back.

Tom

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Archer] Re: [Archer] Stop the Insanity! Linespec Rewrite
  2012-03-02 22:49   ` Keith Seitz
@ 2012-03-06 17:40     ` Joel Brobecker
  2012-03-06 19:08       ` Keith Seitz
  0 siblings, 1 reply; 20+ messages in thread
From: Joel Brobecker @ 2012-03-06 17:40 UTC (permalink / raw)
  To: Keith Seitz; +Cc: archer

Hi Keith,

Sorry for the delay in answering this...

> I have a patch to fix this. Thank you for bringing this to my
> attention (and adding tests for it). Your new operator_bps.exp
> passes 100% on archer-keiths-linespec-rewrite. [I haven't pushed the
> patch yet.]

Awesome, thanks!

> >   . Our testsuite spotted a couple of crashes. They might be related
> >     to the crash that Tom mentioned, although I kind of doubt it.
> >     Nevertheless, I'll investigate them after you've looked at Tom's
> >     report, just in case they end up having the same cause.
> 
> If you can tell me how to reproduce, I will take a look. There are
> probably lots of gotchas all over the place. I relied heavily (too
> much?) on the test suite. I wonder what bugs Go, D, and other
> languages might expose.

OK. I have added two new testcases:

    * gdb.ada/bp_enum_homonym.exp: The testcase inserts a breakpoint
      using a function name ("archive"), but that name is also used
      as the name of one of the enumerals in an enumerated type.

    * gdb.ada/bp_on_var.exp: The testcase attempts to insert some
      breakpoints using variable names. The breakpoints should be
      rejected...

If you'd like me to take a look and tell you what's going on, I'd be
more than happy to...

-- 
Joel

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Archer] Re: [Archer] Stop the Insanity! Linespec Rewrite
  2012-03-06 17:40     ` [Archer] " Joel Brobecker
@ 2012-03-06 19:08       ` Keith Seitz
  2012-03-06 19:36         ` Keith Seitz
  0 siblings, 1 reply; 20+ messages in thread
From: Keith Seitz @ 2012-03-06 19:08 UTC (permalink / raw)
  To: Joel Brobecker; +Cc: archer

On 03/06/2012 09:39 AM, Joel Brobecker wrote:
>> If you can tell me how to reproduce, I will take a look. There are
>> probably lots of gotchas all over the place. I relied heavily (too
>> much?) on the test suite. I wonder what bugs Go, D, and other
>> languages might expose.
>
> OK. I have added two new testcases:
>
>      * gdb.ada/bp_enum_homonym.exp: The testcase inserts a breakpoint
>        using a function name ("archive"), but that name is also used
>        as the name of one of the enumerals in an enumerated type.
>
>      * gdb.ada/bp_on_var.exp: The testcase attempts to insert some
>        breakpoints using variable names. The breakpoints should be
>        rejected...
>
> If you'd like me to take a look and tell you what's going on, I'd be
> more than happy to...

I will take a look at this immediately. Thanks!

Keith

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Archer] Re: [Archer] Stop the Insanity! Linespec Rewrite
  2012-03-06 19:08       ` Keith Seitz
@ 2012-03-06 19:36         ` Keith Seitz
  2012-03-06 21:50           ` [Archer] " Joel Brobecker
  0 siblings, 1 reply; 20+ messages in thread
From: Keith Seitz @ 2012-03-06 19:36 UTC (permalink / raw)
  To: Joel Brobecker; +Cc: archer

On 03/06/2012 09:42 AM, Keith Seitz wrote:
>> If you'd like me to take a look and tell you what's going on, I'd be
>> more than happy to...
>
> I will take a look at this immediately. Thanks!

This turned out to be pretty simple. collect_symbols was unconditionally 
adding all matching symbols to the results, which is appropriate only 
with DECODE_LINE_LIST_MODE. [I admit, I didn't actually know one could 
do that. "list a_global_variable" Learn something new everyday!]

I pushed a patch for this. It now passes all of your new tests.

Any others?

Keith

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Archer] Re: [Archer] Re: [Archer] Stop the Insanity! Linespec Rewrite
  2012-03-06 19:36         ` Keith Seitz
@ 2012-03-06 21:50           ` Joel Brobecker
  2012-03-07  0:11             ` [Archer] " Joel Brobecker
  0 siblings, 1 reply; 20+ messages in thread
From: Joel Brobecker @ 2012-03-06 21:50 UTC (permalink / raw)
  To: Keith Seitz; +Cc: archer

> This turned out to be pretty simple. collect_symbols was
> unconditionally adding all matching symbols to the results, which is
> appropriate only with DECODE_LINE_LIST_MODE. [I admit, I didn't
> actually know one could do that. "list a_global_variable" Learn
> something new everyday!]

Ah, yes, I came across the very same problem when I was helping Tom
with the Ada regressions with his linespec patche series...

> I pushed a patch for this. It now passes all of your new tests.
> 
> Any others?

I do not think so. That was fast! I will make sure to update my
sources and test again ASAP, hopefully this evening or tomorrow.

Great responsiveness, Keith. And also, the fact that you were able
to fix the problems easily seems to confirm the inherent quality
of your new code. It's great.

Cheers,
-- 
Joel

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Archer] Re: [Archer] Re: [Archer] Re: [Archer] Stop the Insanity! Linespec Rewrite
  2012-03-06 21:50           ` [Archer] " Joel Brobecker
@ 2012-03-07  0:11             ` Joel Brobecker
  2012-03-07  1:08               ` Keith Seitz
  0 siblings, 1 reply; 20+ messages in thread
From: Joel Brobecker @ 2012-03-07  0:11 UTC (permalink / raw)
  To: Keith Seitz; +Cc: archer

> I do not think so. That was fast! I will make sure to update my
> sources and test again ASAP, hopefully this evening or tomorrow.

And I can now confirm that all issues detected by our testsuite
have been taken care of without introducing new regressions.

So, the only change of behavior is the fact that the breakpoint
command now accepts the "TASK" keyword in addition to "task".
Personally, I don't think it's a problem. So, it's just a matter
of deciding whether we want to start supporting it or not. Food
for thoughts: It could seem to be friendlier to accept it, but
I don't see that as a real advantage in practice. Perhaps it's easier
in terms of the code. But on the other, once we start supporting it,
we could be stuck with it...

-- 
Joel

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Archer] Stop the Insanity! Linespec Rewrite
  2012-03-07  0:11             ` [Archer] " Joel Brobecker
@ 2012-03-07  1:08               ` Keith Seitz
  2012-03-07  1:26                 ` [Archer] " Joel Brobecker
  0 siblings, 1 reply; 20+ messages in thread
From: Keith Seitz @ 2012-03-07  1:08 UTC (permalink / raw)
  To: Joel Brobecker; +Cc: archer

On 03/06/2012 04:11 PM, Joel Brobecker wrote:
>> I do not think so. That was fast! I will make sure to update my
>> sources and test again ASAP, hopefully this evening or tomorrow.
>
> And I can now confirm that all issues detected by our testsuite
> have been taken care of without introducing new regressions.

Hey, that's great news. Thank you for all your help with testing!

> So, the only change of behavior is the fact that the breakpoint
> command now accepts the "TASK" keyword in addition to "task".

I'm afraid I don't understand. As far as I can tell, CVS HEAD and 
archer-keiths-linespec-rewrite behave identically. The output is exactly 
the same:

$ gdb -nx -q gdb

Reading symbols from 
/home/keiths/sources/gdb/git/virgin/linux/gdb/gdb...done.
(gdb) b main task 3
Cannot inspect Ada tasks when program is not running
(gdb) b main TASK 3
Function "main TASK 3" not defined.
Make breakpoint pending on future shared library load? (y or [n]) n
(gdb) b main
Breakpoint 1 at 0x487143: file ../../gdb/gdb/gdb.c, line 29.
(gdb) r
Starting program: /home/keiths/sources/gdb/git/virgin/linux/gdb/gdb
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib64/libthread_db.so.1".

Breakpoint 1, main (argc=1, argv=0x7fffffffe0a8) at ../../gdb/gdb/gdb.c:29
29	  memset (&args, 0, sizeof args);
(gdb) b main task 3
Unknown task 3.
(gdb) b main TASK 3
Function "main TASK 3" not defined.
Make breakpoint pending on future shared library load? (y or [n]) n
(gdb)

Am I using this incorrectly?

Keith

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Archer] Re: [Archer] Stop the Insanity! Linespec Rewrite
  2012-03-07  1:08               ` Keith Seitz
@ 2012-03-07  1:26                 ` Joel Brobecker
  2012-03-07 14:39                   ` Tom Tromey
  0 siblings, 1 reply; 20+ messages in thread
From: Joel Brobecker @ 2012-03-07  1:26 UTC (permalink / raw)
  To: Keith Seitz; +Cc: archer

> I'm afraid I don't understand. As far as I can tell, CVS HEAD and
> archer-keiths-linespec-rewrite behave identically. The output is
> exactly the same:

The thing is that the task ID you are using needs to be valid.
So, basically, you need to use an Ada program that uses tasking,
and the task ID needs to be known at the time the breakpoint is
inserted.

My favorite little Ada program happens to be available from
gdb.ada/mi_task_arg. Using that program:

    gdb -q task_switch
    (gdb) b break_me
    Breakpoint 1 at 0x403a5a: file /[...]/task_switch.adb, line 57.
    (gdb) run
    Starting program: /[...]/task_switch 
    [Thread debugging using libthread_db enabled]
    Using host libthread_db library "/lib/libthread_db.so.1".
    [New Thread 0x7ffff7852910 (LWP 32425)]
    [New Thread 0x7ffff764e910 (LWP 32426)]
    [Switching to Thread 0x7ffff764e910 (LWP 32426)]

    Breakpoint 1, task_switch.break_me () at /[...]/task_switch.adb:57
    57            null;
    (gdb) info tasks
       ID       TID P-ID Pri State                  Name
        1    645010       48 Child Activation Wait  main_task
        2    645d80    1  48 Accept or Select Term  my_callee
    *   3    649490    1  48 Runnable               my_caller
    (gdb) b task_switch.adb:70 task 1 
    Breakpoint 2 at 0x40365a: file /[...]/task_switch.adb, line 70.

The above works. But if you start using a different casing on
the "task" keyword, as below, the current GDB rejects it:

    (gdb) b task_switch.adb:70 TASK 1
    Junk at end of arguments.
    (gdb) b task_switch.adb:70 Task 1
    Junk at end of arguments.

(note: the "info tasks" command is not necessary for things to work;
 GDB will automatically compute the list of tasks as needed)

-- 
Joel

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Archer] Re: [Archer] Stop the Insanity! Linespec Rewrite
  2012-03-07  1:26                 ` [Archer] " Joel Brobecker
@ 2012-03-07 14:39                   ` Tom Tromey
  2012-03-07 15:01                     ` Tom Tromey
  0 siblings, 1 reply; 20+ messages in thread
From: Tom Tromey @ 2012-03-07 14:39 UTC (permalink / raw)
  To: Joel Brobecker; +Cc: Keith Seitz, archer

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

>> I'm afraid I don't understand. As far as I can tell, CVS HEAD and
>> archer-keiths-linespec-rewrite behave identically. The output is
>> exactly the same:

Joel> The thing is that the task ID you are using needs to be valid.
Joel> So, basically, you need to use an Ada program that uses tasking,
Joel> and the task ID needs to be known at the time the breakpoint is
Joel> inserted.

Joel> The above works. But if you start using a different casing on
Joel> the "task" keyword, as below, the current GDB rejects it:

Joel>     (gdb) b task_switch.adb:70 TASK 1
Joel>     Junk at end of arguments.

I tried this with CVS HEAD gdb and it still fails:

(gdb) b task_switch.adb:70 TASK 1
Junk at end of arguments.

Do you have a local patch for this?

Tom

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Archer] Re: [Archer] Stop the Insanity! Linespec Rewrite
  2012-03-07 14:39                   ` Tom Tromey
@ 2012-03-07 15:01                     ` Tom Tromey
  2012-03-07 15:39                       ` [Archer] " Joel Brobecker
  0 siblings, 1 reply; 20+ messages in thread
From: Tom Tromey @ 2012-03-07 15:01 UTC (permalink / raw)
  To: Joel Brobecker; +Cc: Keith Seitz, archer

Tom> I tried this with CVS HEAD gdb and it still fails:
Tom> (gdb) b task_switch.adb:70 TASK 1
Tom> Junk at end of arguments.
Tom> Do you have a local patch for this?

Pedro pointed out that your original note actually said that you thought
"TASK" was accepted by Keith's branch, not CVS HEAD.

But, this isn't the case, the code uses strncmp.

So, I think there is no change in behavior here and everything is ok.

Tom

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Archer] Re: [Archer] Re: [Archer] Stop the Insanity! Linespec Rewrite
  2012-03-07 15:01                     ` Tom Tromey
@ 2012-03-07 15:39                       ` Joel Brobecker
  2012-03-07 16:06                         ` Tom Tromey
  2012-03-07 21:11                         ` [Archer] " Keith Seitz
  0 siblings, 2 replies; 20+ messages in thread
From: Joel Brobecker @ 2012-03-07 15:39 UTC (permalink / raw)
  To: Tom Tromey; +Cc: Keith Seitz, archer

> Pedro pointed out that your original note actually said that you thought
> "TASK" was accepted by Keith's branch, not CVS HEAD.
> But, this isn't the case, the code uses strncmp.
> So, I think there is no change in behavior here and everything is ok.

Aha, indeed, it's not what I thought.  I think that the "TASK" keyword
and what is behind it appears to be ignored when the "*" syntax is
used. The following works as expected:

    (gdb) break *break_me'address task 2
    Breakpoint 2 at 0x403a52: file /[...]/task_switch.adb, line 55.
    (gdb) info break
    [...]
    2       breakpoint     keep y   0x0000000000403a52 in task_switch.break_me 
                                    at /[...]/task_switch.adb:55 task 2
                                                                 ^^^^^^

The following accepts the command:

    (gdb) break *break_me'address TASK 2
    Note: breakpoint 2 also set at pc 0x403a52.
    Breakpoint 3 at 0x403a52: file /[...]/task_switch.adb, line 55.

But inserts a breakpoint without the task constraint:

    (gdb) info break
    [...]
    3       breakpoint     keep y   0x0000000000403a52 in task_switch.break_me 
                                    at /[...]/task_switch.adb:55

It might be something to do with the Ada expression parser?

-- 
Joel

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Archer] Re: [Archer] Re: [Archer] Stop the Insanity! Linespec Rewrite
  2012-03-07 15:39                       ` [Archer] " Joel Brobecker
@ 2012-03-07 16:06                         ` Tom Tromey
  2012-03-07 21:19                           ` Joel Brobecker
  2012-03-07 21:11                         ` [Archer] " Keith Seitz
  1 sibling, 1 reply; 20+ messages in thread
From: Tom Tromey @ 2012-03-07 16:06 UTC (permalink / raw)
  To: Joel Brobecker; +Cc: Keith Seitz, archer

Joel>     (gdb) break *break_me'address TASK 2

Joel> It might be something to do with the Ada expression parser?

I'm a little surprised this works.
I don't have a theory that would explain why; I though this token should
be handled in breakpoint.c:find_condition_and_thread, which uses a
case-sensitive comparison.

Tom

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Archer] Stop the Insanity! Linespec Rewrite
  2012-03-07 15:39                       ` [Archer] " Joel Brobecker
  2012-03-07 16:06                         ` Tom Tromey
@ 2012-03-07 21:11                         ` Keith Seitz
  2012-03-08  3:02                           ` Joel Brobecker
  1 sibling, 1 reply; 20+ messages in thread
From: Keith Seitz @ 2012-03-07 21:11 UTC (permalink / raw)
  To: Joel Brobecker; +Cc: archer

On 03/07/2012 07:39 AM, Joel Brobecker wrote:
>
> It might be something to do with the Ada expression parser?

Actually it was a failure on the new parser to properly deal with 
parse_to_comma_and_eval (used in linespec_expression_to_pc), which 
retokenizes the input based on whitespace.

Thus, the parser thought the token was "break_me'address TASK 3" and 
forwarded the input stream past this whole thing. I've pushed several 
patches (mostly cleanups), including one which should fix this problem.

Could you verify that I didn't break anything else?

Keith

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: Stop the Insanity! Linespec Rewrite
  2012-03-07 16:06                         ` Tom Tromey
@ 2012-03-07 21:19                           ` Joel Brobecker
  0 siblings, 0 replies; 20+ messages in thread
From: Joel Brobecker @ 2012-03-07 21:19 UTC (permalink / raw)
  To: Tom Tromey; +Cc: Keith Seitz, archer

On Wed, Mar 07, 2012 at 09:05:37AM -0700, Tom Tromey wrote:
> Joel>     (gdb) break *break_me'address TASK 2
> 
> Joel> It might be something to do with the Ada expression parser?
> 
> I'm a little surprised this works.
> I don't have a theory that would explain why; I though this token should
> be handled in breakpoint.c:find_condition_and_thread, which uses a
> case-sensitive comparison.

This is from code inspection only, so to be taken with a grain of salt.
But I think I see the source of the problem.

In HEAD, when we see a '*', we call decode_indirect:

  if (**argptr == '*')
    {
      do_cleanups (cleanup);
      return decode_indirect (self, argptr);
    }

And decode_indirect more or less returns:

  pc = value_as_address (parse_to_comma_and_eval (argptr));

The important part, here, is that parse_to_comma_and_eval updates
argptr to point to the end of the expression it used, thus leaving
the "TASK 2" part unread, thus allowing find_condition_and_thread
to report the extraneous tokens.

On the other hand, it looks like the lexer is just swallowing everything,
and then passing that to the parser:

    static CORE_ADDR
    linespec_expression_to_pc (char *expr)
    {
      char **expr_ptr = &expr;
      [...]
      return value_as_address (parse_to_comma_and_eval (expr_ptr));
    }

But it doesn't looks like it's not really checking that expr_ptr
after the function is not checking that parse_to_comma_and_eval
is leaving nothing unread from expr...

-- 
Joel

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: Stop the Insanity! Linespec Rewrite
  2012-03-07 21:11                         ` [Archer] " Keith Seitz
@ 2012-03-08  3:02                           ` Joel Brobecker
  0 siblings, 0 replies; 20+ messages in thread
From: Joel Brobecker @ 2012-03-08  3:02 UTC (permalink / raw)
  To: Keith Seitz; +Cc: archer

> Actually it was a failure on the new parser to properly deal with
> parse_to_comma_and_eval (used in linespec_expression_to_pc), which
> retokenizes the input based on whitespace.

Our emails crossed paths :-)

> Could you verify that I didn't break anything else?

Of course. You've got to love git and the fact that you're making
fast-forward pushes (instead of rebasing + forced push), which
hugely simplifies my task.

Anyway, all this rambling to say that the testsuite runs clean.

I also forgot to say that your patch improves the diagnostic
in the following case:

    --- Regression: KB27-008__file_star_line_linespec:break foo.adb:*:3
    - Could not match y-or-n question
        Function "*:3" not defined in "foo.adb".
        Make breakpoint pending on future shared library load
    - Against input line
        Function "*" not defined in "foo.adb".
        Make breakpoint pending on future shared library load

The difference is that is says function "*" instead of "*:3" when
doing:

        (gdb) break foo.adb:*:3

For the record, "*" used to be a way in AdaCore's debugger to say
"break on all instances". It was a long long time ago, and Paul and
I killed that syntax since then, and we have a check to verify
that it returns an error.

I wonder if it would work with an operator, though... I think you'd
need the double-quotes. Aha!

    (gdb) b ops.adb:"*":35
    Breakpoint 1 at 0x40217a: file /[...]/ops.adb, line 35.
    (gdb) b ops.adb:*:35
    Function "*:35" not defined in "ops.adb".
    Make breakpoint pending on future shared library load? (y or [n]) n

And your debugger behaves beautifully as well:

    (gdb) b ops.adb:"*":35
    Breakpoint 1 at 0x40217a: file /[...]/ops.adb, line 35.
    (gdb) b ops.adb:*:35
    Function "*" not defined in "ops.adb".
    Make breakpoint pending on future shared library load? (y or [n]) n

(this is using the gdb.ada/operator_bp testcase).

I need to augment that testcase, now :-/.

-- 
Joel

^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2012-03-08  3:02 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-03-02  1:14 Stop the Insanity! Linespec Rewrite Keith Seitz
2012-03-02 17:00 ` Tom Tromey
2012-03-02 22:41   ` Keith Seitz
2012-03-03  2:36     ` Tom Tromey
2012-03-02 19:07 ` [Archer] " Joel Brobecker
2012-03-02 22:49   ` Keith Seitz
2012-03-06 17:40     ` [Archer] " Joel Brobecker
2012-03-06 19:08       ` Keith Seitz
2012-03-06 19:36         ` Keith Seitz
2012-03-06 21:50           ` [Archer] " Joel Brobecker
2012-03-07  0:11             ` [Archer] " Joel Brobecker
2012-03-07  1:08               ` Keith Seitz
2012-03-07  1:26                 ` [Archer] " Joel Brobecker
2012-03-07 14:39                   ` Tom Tromey
2012-03-07 15:01                     ` Tom Tromey
2012-03-07 15:39                       ` [Archer] " Joel Brobecker
2012-03-07 16:06                         ` Tom Tromey
2012-03-07 21:19                           ` Joel Brobecker
2012-03-07 21:11                         ` [Archer] " Keith Seitz
2012-03-08  3:02                           ` Joel Brobecker

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).