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

[Joel Brobecker <brobecker at gnat dot com>]

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