[RFA] re-read symbols before "start"-ing...

[RFA] re-read symbols before "start"-ing...

[Joel Brobecker]

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

Humpf,

I thought I checked that this scenario was working:

        - build first (Ada program, main program name is "first")
        - build second (Ada program, main program name is "second")
        - mv first common
        - gdb
          (gdb) file common
          (gdb) start
          --> lands in "first"
        - mv common first
        - mv second common
        - (gdb) start
          --> lands in "second"

I thought I checked that this was working because I added an
executable-changed notification just for that. In reality, it
doesn't work. When I tried a new testcase that I am currently
writing, here is what I actually get:

    (gdb) start^M
    Breakpoint 2 at 0x804980b: file /home/no-backup/brobecke/testing/gdb-public/gdb/testsuite/gdb.ada/exec_changed/first.adb, line 4.^M
    `/home/no-backup/brobecke/testing/gdb-public/gdb/testsuite/gdb.ada/exec_changed/common' has changed; re-reading symbols.^M
    Error in re-setting breakpoint 2:^M
    Function "_ada_first" not defined.^M
    Error in re-setting breakpoint 2:^M
    Function "_ada_first" not defined.^M
    Error in re-setting breakpoint 2:^M
    Function "_ada_first" not defined.^M
    Error in re-setting breakpoint 2:^M
    Function "_ada_first" not defined.^M
    ^M
    Program exited normally.^M
    (gdb) FAIL: gdb.ada/exec_changed.exp: start second

What happens here is that we insert the breakpoint *before* the
notification is received. So we end up using the cached value
for the main procedure name, which is out of date. The answer
is to make sure the symbols are re-read if the executable has
changed.

Bad point for me, sorry... Fortunatly, only Ada is affected for now.
Attached is a patch.

2005-03-07  Joel Brobecker  <brobecker@adacore.com>

        * infcmd.c (start_command): Make sure the symbols are up to date
        before setting the temporary breakpoint.

Tested on x86-linux.  This fixes the FAIL above. Otherwise, the results
are identical.

OK to commit?

Thanks,
-- 
Joel

[-- Attachment #2: start-reread.diff --]
[-- Type: text/plain, Size: 853 bytes --]

Index: infcmd.c
===================================================================
RCS file: /cvs/src/src/gdb/infcmd.c,v
retrieving revision 1.134
diff -p -r1.134 infcmd.c
*** infcmd.c	21 Feb 2005 03:25:56 -0000	1.134
--- infcmd.c	8 Mar 2005 06:43:05 -0000
*************** start_command (char *args, int from_tty)
*** 512,517 ****
--- 512,523 ----
       the user changes its mind.  */
    kill_if_already_running (from_tty);
  
+   /* Make sure that the symbols are up to date.  Otherwise, we might
+      end up inserting the temporary breakpoint before we receive the
+      executable-changed notification, potentially causing us to use
+      the wrong function name as the main procedure.  */
+   reread_symbols ();
+ 
    /* Insert the temporary breakpoint, and run...  */
    tbreak_command (main_name (), 0);
    run_command (args, from_tty);

Re: [RFA] re-read symbols before "start"-ing...

[Daniel Jacobowitz]

On Mon, Mar 07, 2005 at 10:45:29PM -0800, Joel Brobecker wrote:
> What happens here is that we insert the breakpoint *before* the
> notification is received. So we end up using the cached value
> for the main procedure name, which is out of date. The answer
> is to make sure the symbols are re-read if the executable has
> changed.
> 
> Bad point for me, sorry... Fortunatly, only Ada is affected for now.
> Attached is a patch.
> 
> 2005-03-07  Joel Brobecker  <brobecker@adacore.com>
> 
>         * infcmd.c (start_command): Make sure the symbols are up to date
>         before setting the temporary breakpoint.
> 
> Tested on x86-linux.  This fixes the FAIL above. Otherwise, the results
> are identical.
> 
> OK to commit?

Just my two cents, but I think you may want to add an argument to
run_command, instead, and create the breakpoint there.  For instance
there's probably a case where you'll need reopen_exec_file ().  And
reread_symbols is passably expensive - lots of stat().

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [RFA] re-read symbols before "start"-ing...

[Joel Brobecker]

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

> > 2005-03-07  Joel Brobecker  <brobecker@adacore.com>
> > 
> >         * infcmd.c (start_command): Make sure the symbols are up to date
> >         before setting the temporary breakpoint.
> 
> Just my two cents, but I think you may want to add an argument to
> run_command, instead, and create the breakpoint there.  For instance
> there's probably a case where you'll need reopen_exec_file ().  And
> reread_symbols is passably expensive - lots of stat().

I wasn't too concerned about the expensiveness of reread_symbols,
because I think the "start" command is used only once in a while,
so the performance hit should be only occasional. I was also hoping,
but this is only guessing game on my part, that the system would cache
theses operations so that the next stat operations are made from memory,
and hence be very fast.

However, I see your point with reopen_exec_file(), though.

How about the following patch? Because run_command is used as a callback
for the "run" command, I couldn't change its profile, so instead I
extracted its body into run_command_1, and added the extra parameter
there. On my first try, I was a bit silly, because I made this parameter
a char * containing the location of the temporary breakpoint. I then
called this procedure like this (in start_command()):

        run_command_1 (args, from_tty, main_name());

Which doesn't work, since we still end up calling main_main() before
we have verified whether symbols should be reread or not. So instead,
I settled for a flag. And we get to simplify start_command() a bit,
by removing some code that is no longer needed (the call to
kill_if_already_running()).

2005-03-08  Joel Brobecker  <brobecker@adacore.com>

        * infcmd.c (run_command_1): New function, extracted from
        run_command.
        (run_command): Replace implementation by call to run_command_1.
        (start_command): Use run_command_1 to insert the temporary
        breakpoint and run the program. Remove code that's no longer
        needed, as already done at the proper time by run_command_1.

Tested on x86-linux, no regression. I have a testcase that I will submit
shortly that used to fail before the change, and passes after.

OK to apply?

Thanks,
-- 
Joel

[-- Attachment #2: restart.diff --]
[-- Type: text/plain, Size: 1976 bytes --]

Index: infcmd.c
===================================================================
RCS file: /cvs/src/src/gdb/infcmd.c,v
retrieving revision 1.134
diff -u -p -r1.134 infcmd.c
--- infcmd.c	21 Feb 2005 03:25:56 -0000	1.134
+++ infcmd.c	8 Mar 2005 21:13:23 -0000
@@ -400,8 +400,12 @@ Start it from the beginning? "))
     }
 }
 
+/* Implement the "run" command. If TBREAK_AT_MAIN is set, then insert
+   a temporary breakpoint at the begining of the main program before
+   running the program.  */
+
 static void
-run_command (char *args, int from_tty)
+run_command_1 (char *args, int from_tty, int tbreak_at_main)
 {
   char *exec_file;
 
@@ -425,6 +429,10 @@ run_command (char *args, int from_tty)
   reopen_exec_file ();
   reread_symbols ();
 
+  /* Insert the temporary breakpoint if a location was specified.  */
+  if (tbreak_at_main)
+    tbreak_command (main_name (), 0);
+
   exec_file = (char *) get_exec_file (0);
 
   /* We keep symbols from add-symbol-file, on the grounds that the
@@ -487,6 +495,12 @@ run_command (char *args, int from_tty)
 
 
 static void
+run_command (char *args, int from_tty)
+{
+  run_command_1 (args, from_tty, 0);
+}
+
+static void
 run_no_args_command (char *args, int from_tty)
 {
   char *old_args = set_inferior_args (xstrdup (""));
@@ -506,15 +520,8 @@ start_command (char *args, int from_tty)
   if (!have_minimal_symbols ())
     error (_("No symbol table loaded.  Use the \"file\" command."));
 
-  /* If the inferior is already running, we want to ask the user if we
-     should restart it or not before we insert the temporary breakpoint.
-     This makes sure that this command doesn't have any side effect if
-     the user changes its mind.  */
-  kill_if_already_running (from_tty);
-
-  /* Insert the temporary breakpoint, and run...  */
-  tbreak_command (main_name (), 0);
-  run_command (args, from_tty);
+  /* Run the program until reaching the main procedure...  */
+  run_command_1 (args, from_tty, 1);
 } 
 
 void

Re: [RFA] re-read symbols before "start"-ing...

[Daniel Jacobowitz]

On Tue, Mar 08, 2005 at 01:28:16PM -0800, Joel Brobecker wrote:
> 2005-03-08  Joel Brobecker  <brobecker@adacore.com>
> 
>         * infcmd.c (run_command_1): New function, extracted from
>         run_command.
>         (run_command): Replace implementation by call to run_command_1.
>         (start_command): Use run_command_1 to insert the temporary
>         breakpoint and run the program. Remove code that's no longer
>         needed, as already done at the proper time by run_command_1.
> 
> Tested on x86-linux, no regression. I have a testcase that I will submit
> shortly that used to fail before the change, and passes after.
> 
> OK to apply?

Thanks, that's just what I had in mind.  This is OK.


-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [RFA] re-read symbols before "start"-ing...

[Joel Brobecker]

> > 2005-03-08  Joel Brobecker  <brobecker@adacore.com>
> > 
> >         * infcmd.c (run_command_1): New function, extracted from
> >         run_command.
> >         (run_command): Replace implementation by call to run_command_1.
> >         (start_command): Use run_command_1 to insert the temporary
> >         breakpoint and run the program. Remove code that's no longer
> >         needed, as already done at the proper time by run_command_1.
>
> Thanks, that's just what I had in mind.  This is OK.

Great! Thank you, I checked the changes in.

Just for the record, I also just submitted the associated testcase.
See http://sources.redhat.com/ml/gdb-patches/2005-03/msg00130.html

-- 
Joel