[RFA] parse and eval breakpoint conditions with correct language

[RFA] parse and eval breakpoint conditions with correct language

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 582 bytes --]

This RFA is a followup on the thread started on gdb@:

    http://sources.redhat.com/ml/gdb/2003-09/msg00082.html

I incorporated Jim's comments (set expout->language_defn, handle
NULL expression_context_block). There was also another gotcha
that the testsuite detected which is to handle the cases when
the context block language is not null, but the unknown language.

2003-09-09  J. Brobecker  <brobecker@gnat.com>

        * parse.c (parse_exp_1): Use the language associated to
        the context block when parsing an expression.

Tested on x86-linux.

Ok to apply?
-- 
Joel

[-- Attachment #2: parse.c.diff --]
[-- Type: text/plain, Size: 1670 bytes --]

Index: parse.c
===================================================================
RCS file: /cvs/src/src/gdb/parse.c,v
retrieving revision 1.36
diff -u -p -r1.36 parse.c
--- parse.c	9 Sep 2003 08:05:42 -0000	1.36
+++ parse.c	10 Sep 2003 01:51:40 -0000
@@ -1107,6 +1107,7 @@ struct expression *
 parse_exp_1 (char **stringptr, struct block *block, int comma)
 {
   struct cleanup *old_chain;
+  const struct language_defn *lang = NULL;
 
   lexptr = *stringptr;
   prev_lexptr = NULL;
@@ -1130,16 +1131,29 @@ parse_exp_1 (char **stringptr, struct bl
   else
     expression_context_block = get_selected_block (&expression_context_pc);
 
+  if (language_mode == language_mode_auto && expression_context_block != NULL)
+    {
+      /* Find the language associated to the context block.
+         Default to the current language if it can not be determined.  */
+      const struct symbol *func = block_function (expression_context_block);
+      if (func != NULL)
+        lang = language_def (SYMBOL_LANGUAGE (func));
+      if (lang == NULL || lang->la_language == language_unknown)
+        lang = current_language;
+    }
+  else
+    lang = current_language;
+
   namecopy = (char *) alloca (strlen (lexptr) + 1);
   expout_size = 10;
   expout_ptr = 0;
   expout = (struct expression *)
     xmalloc (sizeof (struct expression) + EXP_ELEM_TO_BYTES (expout_size));
-  expout->language_defn = current_language;
+  expout->language_defn = lang;
   make_cleanup (free_current_contents, &expout);
 
-  if (current_language->la_parser ())
-    current_language->la_error (NULL);
+  if (lang->la_parser ())
+    lang->la_error (NULL);
 
   discard_cleanups (old_chain);
 

Re: [RFA] parse and eval breakpoint conditions with correct language

[Jim Blandy]

Joel Brobecker <brobecker@gnat.com> writes:

> This RFA is a followup on the thread started on gdb@:
> 
>     http://sources.redhat.com/ml/gdb/2003-09/msg00082.html
> 
> I incorporated Jim's comments (set expout->language_defn, handle
> NULL expression_context_block). There was also another gotcha
> that the testsuite detected which is to handle the cases when
> the context block language is not null, but the unknown language.
> 
> 2003-09-09  J. Brobecker  <brobecker@gnat.com>
> 
>         * parse.c (parse_exp_1): Use the language associated to
>         the context block when parsing an expression.
> 
> Tested on x86-linux.
> 
> Ok to apply?

Looks good to me, definitely a step forward.

Re: [RFA] parse and eval breakpoint conditions with correct language

[Joel Brobecker]

> > 2003-09-09  J. Brobecker  <brobecker@gnat.com>
> > 
> >         * parse.c (parse_exp_1): Use the language associated to
> >         the context block when parsing an expression.
> > 
> > Tested on x86-linux.
> > 
> > Ok to apply?
> 
> Looks good to me, definitely a step forward.

Groumf! Our nightly regression test showed a small regression
which does not appear on my machine. We have an all-Ada program which
defines a function named Func1, and here is what the test does:

        (gdb) break *Func1'Address
        (gdb) run

(The first command says place a breakpoint at the first instruction
of Func1. What's important is that we use the "break *address" syntax).

The first break command correctly uses the ada language to parse
the expression (Func1'Address). The fun starts after "run":

        (gdb) run
        Starting program: /[...]/main 
        Error in re-setting breakpoint 1:
        No symbol "Func1" in current context.
        
        Program exited normally.
        Current language:  auto; currently c

What happens is that the inferior is stopped twice during the startup
phase after notifications about new shared libraries being loaded.
GDB then re-parses all breakpoint expressions and re-inserts them.
During this phase, the block given to parse_exp_1 is NULL, so GDB
uses the current block (ie the block corresponding to the current
frame) to determine the language. Since the current frame has nothing
to do with the breakpoint, we are sometimes unlucky enough to use
the wrong language to reparse the breakpoint. That's what happens
to the test above, and is dependent on how the target system is
setup.

Looking at the breakpoint_re_set() loop inside breakpoint_re_set_one(),
we can see the code that re-parses the breakpoint expression:

      set_language (b->language);
      [...]
      sals = decode_line_1 (&s, 1, (struct symtab *) NULL, 0, (char ***) NULL);

So we can see that the current language is adjusted before each
breakpoint expression is parsed, as a mean to tell the decoder to use
that language parser.

So far, I see two possible ways to prevent this from occuring:

  1. Add an extra parameter to parse_exp_1() explicitely telling
     it to use the context block language to do the parsing when
     set. Otherwise, the current language would be used. This
     parameter would always be unset except when parsing breakpoint
     conditions.

  2. The other approach that I used was more approximative: Do not
     use the current frame at all; safer to use the current language
     rather than using the current frame language. So we do the language
     detection only when the language mode is auto and parse_exp_1()
     is given a specific context block (ie BLOCK != NULL). It works
     well on all tests that I have run, including our own Ada testsuite
     which has been run yesterday night on all our build machines.

Approach 1 seems sharper to me, but will require a bit more surgery.
Approach 2 sounds right too, but is conceptually more complex and
difficult to ``prove''. In particular, proving that we do the language
detection in all the right cases and only the right cases is difficult.

Approach 2 is achieved with the following amendment to my previous
patch:
--- parse.c.orig	Wed Sep 10 21:16:37 2003
+++ parse.c	Thu Sep 11 14:04:53 2003
@@ -1131,11 +1131,22 @@ parse_exp_1 (char **stringptr, struct bl
   else
     expression_context_block = get_selected_block (&expression_context_pc);
 
-  if (language_mode == language_mode_auto && expression_context_block != NULL)
+  if (language_mode == language_mode_auto && block != NULL)
     {
-      /* Find the language associated to the context block.
-         Default to the current language if it can not be determined.  */
-      const struct symbol *func = block_function (expression_context_block);
+      /* Find the language associated to the given context block.
+         Default to the current language if it can not be determined.
+         
+         Note that we do this language detection only if the context
+         block has been specifically provided. This is because using
+         the language corresponding to the current frame can sometimes
+         give unexpected results.  For instance, this routine is often
+         called several times during the inferior startup phase to re-parse
+         breakpoint expressions after a new shared library has been loaded.
+         The language associated to the current frame at this moment is not
+         relevant for the breakpoint. Using it would therefore be silly, so
+         it seems better to rely on the current language rather than relying
+         on the current frame language to parse the expression.  */
+      const struct symbol *func = block_function (block);
       if (func != NULL)
         lang = language_def (SYMBOL_LANGUAGE (func));
       if (lang == NULL || lang->la_language == language_unknown)

I went with 2 because I'm a lazy bum, but must admit I also like 1...
Opinions? Revised patch attached:

2003-09-11  J. Brobecker  <brobecker@gnat.com>

        * parse.c (parse_exp_1): Use the language associated to
        the given block when parsing an expression.

Tested on x86-linux, and using our in-house Ada testsuite on all our
supported platforms. Ok to apply?

-- 
Joel

Re: [RFA] parse and eval breakpoint conditions with correct language

[Paul Koning]

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

 Joel> Groumf! Our nightly regression test showed a small regression
 Joel> which does not appear on my machine. We have an all-Ada program
 Joel> which defines a function named Func1, and here is what the test
 Joel> does:

 Joel> (gdb) break *Func1'Address (gdb) run

 Joel> (The first command says place a breakpoint at the first
 Joel> instruction of Func1. What's important is that we use the
 Joel> "break *address" syntax).

 Joel> The first break command correctly uses the ada language to
 Joel> parse the expression (Func1'Address). The fun starts after
 Joel> "run":

 Joel> (gdb) run Starting program: /[...]/main Error in re-setting
 Joel> breakpoint 1: No symbol "Func1" in current context.
        
 Joel> Program exited normally.  Current language: auto; currently c

 Joel> What happens is that the inferior is stopped twice during the
 Joel> startup phase after notifications about new shared libraries
 Joel> being loaded.  GDB then re-parses all breakpoint expressions
 Joel> and re-inserts them.  During this phase, the block given to
 Joel> parse_exp_1 is NULL, so GDB uses the current block (ie the
 Joel> block corresponding to the current frame) to determine the
 Joel> language. Since the current frame has nothing to do with the
 Joel> breakpoint, we are sometimes unlucky enough to use the wrong
 Joel> language to reparse the breakpoint. That's what happens to the
 Joel> test above, and is dependent on how the target system is setup.

Warning, the following is not based on a really deep understanding of
how gdb internals work...

Could the same sort of trouble occur when evaluating watchpoint
expressions?  The parsing should be done according to the language in
effect when the watchpoint is defined (either explicit, or from the
current block language given this patch) -- not the one that happens
to be in effect when gdb is checking for watch hits during execution. 

Is that a non-issue or could you run into the same sort of problem as
what Joel reported?

     paul

Re: [RFA] parse and eval breakpoint conditions with correct language

[Andrew Cagney]

> Groumf! Our nightly regression test showed a small regression
> which does not appear on my machine. We have an all-Ada program which
> defines a function named Func1, and here is what the test does:
> 
>         (gdb) break *Func1'Address
>         (gdb) run

I've been wondering if this language stuff was going to interact badly 
with breakpoints.  Anyway, is it possible to create an equivalent gcj 
test case since gcj is likely more accessable to developers?

Andrew

Re: [RFA] parse and eval breakpoint conditions with correct language

[Joel Brobecker]

> Could the same sort of trouble occur when evaluating watchpoint
> expressions?  The parsing should be done according to the language in
> effect when the watchpoint is defined (either explicit, or from the
> current block language given this patch) -- not the one that happens
> to be in effect when gdb is checking for watch hits during execution. 

That would be an issue that's a bit outside of the context of this
patch, but I had a quick look. And we don't seem to consider the
language when resetting watchpoints:

breakpoint_re_set_one(): 

    case bp_watchpoint:
    case bp_hardware_watchpoint:
    case bp_read_watchpoint:
    case bp_access_watchpoint:
      innermost_block = NULL;
      /* The issue arises of what context to evaluate this in.  The
         same one as when it was set, but what does that mean when
         symbols have been re-read?  We could save the filename and
         functionname, but if the context is more local than that, the
         best we could do would be something like how many levels deep
         and which index at that particular level, but that's going to
         be less stable than filenames or function names.  */

      /* So for now, just use a global context.  */
      if (b->exp)
        xfree (b->exp);
      b->exp = parse_expression (b->exp_string);

I made a small experiment with an Ada program, and it did not go
too well :-(. I had to use GDB 5.3 as I don't have under my immediate
reach an Ada-aware version of GDB based on the current head sources.
But I think the same problem applies there too.

        % gdb foo
        (gdb) watch light
        Hardware watchpoint 1: pck.light
        (gdb) set lang c
        (gdb) run
        Starting program: /[...]/foo 
        Error in re-setting breakpoint 1:
        No symbol "light" in current context.
        zsh: 24784 segmentation fault (core dumped)  gdb foo

The following patch seems to be fixing it, but I would need more
time to really think about it more thouroughly. 

--- breakpoint.c        5 Sep 2003 21:51:24 -0000       1.4
+++ breakpoint.c        11 Sep 2003 19:23:08 -0000
@@ -7089,6 +7089,7 @@ breakpoint_re_set_one (PTR bint)
       /* So for now, just use a global context.  */
       if (b->exp)
        xfree (b->exp);
+      set_language (b->language);
       b->exp = parse_expression (b->exp_string);
       b->exp_valid_block = innermost_block;
       mark = value_mark ();

-- 
Joel

Re: [RFA] parse and eval breakpoint conditions with correct language

[Joel Brobecker]

> I've been wondering if this language stuff was going to interact badly 
> with breakpoints.  Anyway, is it possible to create an equivalent gcj 
> test case since gcj is likely more accessable to developers?

It's probably possible, but I don't have gcj and I've never programed in
Java. So I'd like to leave this part to the contributors that do use
Java.

-- 
Joel

Re: [RFA] parse and eval breakpoint conditions with correct language

[Daniel Jacobowitz]

On Thu, Sep 11, 2003 at 03:20:44PM -0400, Andrew Cagney wrote:
> 
> >Groumf! Our nightly regression test showed a small regression
> >which does not appear on my machine. We have an all-Ada program which
> >defines a function named Func1, and here is what the test does:
> >
> >        (gdb) break *Func1'Address
> >        (gdb) run
> 
> I've been wondering if this language stuff was going to interact badly 
> with breakpoints.  Anyway, is it possible to create an equivalent gcj 
> test case since gcj is likely more accessable to developers?

I think this is going to become an increasingly prominent problem. 
You can create testcases for this in C++, if your system starts C++
programs up in startup code which is auto(asm) instead of c++.  I think
it's as simple as:
  (gdb) break classname::func

And we won't correctly load the class.  This happens on Solaris, not
sure about GNU/Linux.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [RFA] parse and eval breakpoint conditions with correct language

[Jim Blandy]

I think the correct behavior is to always re-parse the breakpoint in
the same language it was parsed with the first time.  If the user
liked how the breakpoint was interpreted the first time, then that's
what they meant.

We try to record that language in b->language, and
breakpoint_re_set_one tries to make sure that's the language used, by
calling set_language before re-parsing the breakpoint's line spec.
But the bug, it seems to me, is that breakpoint_re_set_one should also
set language_mode to language_mode_manual for that call to
decode_line_1, since it doesn't want any language_mode_auto-ish
language inference to mess things up.  That's what's happening now, if
I understand the misbehavior you've described.

Of course, the mode must be language_mode_auto when we re-parse any
breakpoint condition expressions or watchpoint expressions.  Both of
those expressions have blocks to use for context, and we want to use
their languages.

Re: [RFA] parse and eval breakpoint conditions with correct language

[Andrew Cagney]

> The following patch seems to be fixing it, but I would need more
> time to really think about it more thouroughly. 
> 
> --- breakpoint.c        5 Sep 2003 21:51:24 -0000       1.4
> +++ breakpoint.c        11 Sep 2003 19:23:08 -0000
> @@ -7089,6 +7089,7 @@ breakpoint_re_set_one (PTR bint)
>        /* So for now, just use a global context.  */
>        if (b->exp)
>         xfree (b->exp);
> +      set_language (b->language);
>        b->exp = parse_expression (b->exp_string);
>        b->exp_valid_block = innermost_block;
>        mark = value_mark ();

BTW, this setting of the global current_language all also worries me :-) 
  Should parse_expression take an explict language?

Andrew

Re: [RFA] parse and eval breakpoint conditions with correct language

[Joel Brobecker]

> BTW, this setting of the global current_language all also worries me :-) 
>  Should parse_expression take an explict language?

Me too, actually. I think passing the langauge to parse_expression et al
is a good idea. It's more work, but seems more manageable than using a
couple of global variables.

Jim, what do you think?

-- 
Joel

Re: [RFA] parse and eval breakpoint conditions with correct language

[Jim Blandy]

Joel Brobecker <brobecker@gnat.com> writes:

> > BTW, this setting of the global current_language all also worries me :-) 
> >  Should parse_expression take an explict language?
> 
> Me too, actually. I think passing the langauge to parse_expression et al
> is a good idea. It's more work, but seems more manageable than using a
> couple of global variables.
> 
> Jim, what do you think?

Definitely.  Mucking around with globals just to keep something from
throwing away information you've got right at hand is pretty
indicative.

What gives me pause, though, is that the breakpoint_re_set_one isn't
calling parse_exp_1 directly --- it's calling decode_line_1.  Are you
going to propagate the language all the way through there?  How do you
see approaching this?

Re: [RFA] parse and eval breakpoint conditions with correct language

[Joel Brobecker]

> What gives me pause, though, is that the breakpoint_re_set_one isn't
> calling parse_exp_1 directly --- it's calling decode_line_1.  Are you
> going to propagate the language all the way through there?

Yes, it's the only approach I see.

> How do you see approaching this?

I think the safest way to approach this is by working from the bottom to
the top, in tiny steps that should be backward compatible in terms of
functionality.

Let's define the objectives:

  1. We want parse_exp_1 to take a language argument, so that it can
     use it to do the expression parsing.

  2. We want to take advantage of it when parsing a breakpoint condition
     For that, we need to be able to determine the language associated
     to a given block (or a given PC, whatever is more convenient)

  3. We want to take advantage of 1 to make sure we use the same language
     everytime we reparse the condition.

  4. We want decode_line_1 to take a language argument too, so that when
     we reset the breakpoints, we can use the same language as the
     language that was used when the breakpoint was first created.

  5. We want to fix the watchpoint problem that we also noticed.

I think these 5 steps can be done separately. Here is the plan:

  1. Add a language parameter to parse_exp_1. If NULL, then use
     current_language. Fix all the calls the parse_exp_1 to use
     NULL. As is, this change should be a nop.

  2. Pass the appropriate language to parse_exp_1 in the instances
     where we parse a condition. That includes modifying
     condition_command(), break_command_1(), do_captured_breakpoints().
     These changes are self-contained, as these procedures can determine
     on their own the appropriate language. This should fix the original
     problem I reported.

  3. Fix the call to parse_exp_1 in breakpoint_re_set_one to use the
     same language to reparse the condition. This is also
     self-contained.

  4. a. Add a language argument to decode_line_1() using the same
        nop approach as for 1.
     b. Fix the call to decode_line_1 in breakpoint_re_set_one so
        that the appropriate language is used to parse the given
        expression. That will allow us to remove the call to
        set_language, and will make sure that the second problem
        that I reported will not re-appear.
  
  5. Fix the problem of the wrong language used to reinsert watchpoints.
     For that we need to do:
     
     a. Add a language paramter to parse_expression(). Same as 1.
     b. Fix the call to parse_expression in breakpoint_re_set_one()
        to use the same language as before when reparsing the watchpoint
        expression.
  
The downside is that we will not end up reviewing the entire calling
tree to parse_exp_1, so we may leave some problems behind. But whatever
problems they are, we will be no worse than before. And the upside is
that a lot of the changes should be mechanical, and most of the patches
should be reasonably small and easy to review.
     
Shall I do that?
-- 
Joel