From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29785 invoked by alias); 11 Sep 2003 18:09:23 -0000 Mailing-List: contact gdb-patches-help@sources.redhat.com; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sources.redhat.com Received: (qmail 29777 invoked from network); 11 Sep 2003 18:09:21 -0000 Received: from unknown (HELO takamaka.act-europe.fr) (142.179.108.108) by sources.redhat.com with SMTP; 11 Sep 2003 18:09:21 -0000 Received: by takamaka.act-europe.fr (Postfix, from userid 507) id 017FBD2DAF; Thu, 11 Sep 2003 11:09:20 -0700 (PDT) Date: Thu, 11 Sep 2003 18:09:00 -0000 From: Joel Brobecker To: Jim Blandy Cc: gdb-patches@sources.redhat.com Subject: Re: [RFA] parse and eval breakpoint conditions with correct language Message-ID: <20030911180920.GD945@gnat.com> References: <20030910015400.GS423@gnat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.4i X-SW-Source: 2003-09/txt/msg00225.txt.bz2 > > 2003-09-09 J. Brobecker > > > > * 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 * 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