[RFA/ada] Improve is_known_support_routine

[RFA/ada] Improve is_known_support_routine

[Joel Brobecker]

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

Hello,

A customer just reported us an issue that we never noticed before.
It can be reduced to this:

    procedure Foo is
    begin
       raise Constraint_Error;
    end Foo;

We're setting an exception catchpoint. Then we debug from a location
that's not where the sources are ("cd subdir"):

    % gnatmake -g foo
    % gdb foo
    (gdb) cd subdir
    (gdb) catch exception
    (gdb) run
    Catchpoint 1, CONSTRAINT_ERROR at <__gnat_raise_nodefer_with_msg>
       (e=0x805844c) at a-except.adb:818
    818     a-except.adb: No such file or directory.
            in a-except.adb
    (gdb) f
    #0  <__gnat_raise_nodefer_with_msg> (e=0x805844c) at a-except.adb:818
    818     in a-except.adb

As you see, the debugger did not go up the call stack to foo.adb.
Here is the the expected output:

    (gdb) run
    Catchpoint 1, CONSTRAINT_ERROR at 0x080497fe in foo () at foo.adb:3
    3          raise Constraint_Error;
    (gdb) f
    #4  0x080497fe in foo () at foo.adb:3
    3          raise Constraint_Error;

The is_known_support_routine identifies any frame for which we cannot
find the source file. The check we currently have in place is a bit too
simplistic, so I replaced it with the function we use to locate source
files:

-  if (stat (sal.symtab->filename, &st))
+  if (symtab_to_fullname (sal.symtab) == NULL)

Unfortunately, I cannot reproduce this within our testsuite, because
we use GNAT project files to build the executable. One of the side effects
of using a project file is that the compiler is called with the absolute
filename rather than a relative filename. As a result, symtab->filename
contains the absolute path, and hence the stat call always succeeds.

Because we offer the option of building GDB out of tree, apart from
creating on the fly the setup in a temporary directory, followed by
compiling and then testing there, I am not sure how to create a testcase
for this issue... We were able to add a testcase in our testsuite,
however, as it uses another type of technology that allows us to build
our example programs without project files.

For now, I propose that we commit this patch without adding a testcase.
I think that the symtab_to_fullname routine is already fairly
extensively tested as it just a wrapper around a fairly central routine
in source location.

2007-01-06  Joel Brobecker  <brobecker@adacore.com>

        * ada-lang.c: Add include of source.h.
        (is_known_support_routine): Improve the check verifying that the file
        associated to this frame exists.
        * Makefile.in (ada-lang.o): Add dependency on source.h.

Tested on x86-linux. Ok to apply?

Thank you,
-- 
Joel

PS: I'll start working on the other, more cosmetic, improvements for
    that routine tomrorrow.


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

Index: ada-lang.c
===================================================================
RCS file: /cvs/src/src/gdb/ada-lang.c,v
retrieving revision 1.88
diff -u -p -r1.88 ada-lang.c
--- ada-lang.c	4 Jan 2007 06:31:51 -0000	1.88
+++ ada-lang.c	6 Jan 2007 17:28:59 -0000
@@ -55,6 +55,7 @@ Boston, MA 02110-1301, USA.  */
 #include "exceptions.h"
 #include "annotate.h"
 #include "valprint.h"
+#include "source.h"
 
 #ifndef ADA_RETAIN_DOTS
 #define ADA_RETAIN_DOTS 0
@@ -9072,7 +9073,7 @@ is_known_support_routine (struct frame_i
      symbols; in this case, the filename referenced by these symbols
      does not exists.  */
 
-  if (stat (sal.symtab->filename, &st))
+  if (symtab_to_fullname (sal.symtab) == NULL)
     return 1;
 
   for (i = 0; known_runtime_file_name_patterns[i] != NULL; i += 1)
Index: Makefile.in
===================================================================
RCS file: /cvs/src/src/gdb/Makefile.in,v
retrieving revision 1.870
diff -u -p -r1.870 Makefile.in
--- Makefile.in	4 Jan 2007 22:11:44 -0000	1.870
+++ Makefile.in	6 Jan 2007 17:29:08 -0000
@@ -1696,7 +1696,8 @@ ada-lang.o: ada-lang.c $(defs_h) $(gdb_s
 	$(inferior_h) $(symfile_h) $(objfiles_h) $(breakpoint_h) \
 	$(gdbcore_h) $(hashtab_h) $(gdb_obstack_h) $(ada_lang_h) \
 	$(completer_h) $(gdb_stat_h) $(ui_out_h) $(block_h) $(infcall_h) \
-	$(dictionary_h) $(exceptions_h) $(annotate_h) $(valprint_h)
+	$(dictionary_h) $(exceptions_h) $(annotate_h) $(valprint_h) \
+	$(source_h)
 ada-typeprint.o: ada-typeprint.c $(defs_h) $(gdb_obstack_h) $(bfd_h) \
 	$(symtab_h) $(gdbtypes_h) $(expression_h) $(value_h) $(gdbcore_h) \
 	$(target_h) $(command_h) $(gdbcmd_h) $(language_h) $(demangle_h) \

Re: [RFA/ada] Improve is_known_support_routine

[Joel Brobecker]

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

I noticed while cleaning up the same code that I forgot to delete
a local variable that became unused.  Here is a new patch.
Rested on x86-linux.

> A customer just reported us an issue that we never noticed before.
> It can be reduced to this:
> 
>     procedure Foo is
>     begin
>        raise Constraint_Error;
>     end Foo;
> 
> We're setting an exception catchpoint. Then we debug from a location
> that's not where the sources are ("cd subdir"):
> 
>     % gnatmake -g foo
>     % gdb foo
>     (gdb) cd subdir
>     (gdb) catch exception
>     (gdb) run
>     Catchpoint 1, CONSTRAINT_ERROR at <__gnat_raise_nodefer_with_msg>
>        (e=0x805844c) at a-except.adb:818
>     818     a-except.adb: No such file or directory.
>             in a-except.adb
>     (gdb) f
>     #0  <__gnat_raise_nodefer_with_msg> (e=0x805844c) at a-except.adb:818
>     818     in a-except.adb
> 
> As you see, the debugger did not go up the call stack to foo.adb.
> Here is the the expected output:
> 
>     (gdb) run
>     Catchpoint 1, CONSTRAINT_ERROR at 0x080497fe in foo () at foo.adb:3
>     3          raise Constraint_Error;
>     (gdb) f
>     #4  0x080497fe in foo () at foo.adb:3
>     3          raise Constraint_Error;
> 
> The is_known_support_routine identifies any frame for which we cannot
> find the source file. The check we currently have in place is a bit too
> simplistic, so I replaced it with the function we use to locate source
> files:
> 
> -  if (stat (sal.symtab->filename, &st))
> +  if (symtab_to_fullname (sal.symtab) == NULL)
> 
> Unfortunately, I cannot reproduce this within our testsuite, because
> we use GNAT project files to build the executable. One of the side effects
> of using a project file is that the compiler is called with the absolute
> filename rather than a relative filename. As a result, symtab->filename
> contains the absolute path, and hence the stat call always succeeds.
> 
> Because we offer the option of building GDB out of tree, apart from
> creating on the fly the setup in a temporary directory, followed by
> compiling and then testing there, I am not sure how to create a testcase
> for this issue... We were able to add a testcase in our testsuite,
> however, as it uses another type of technology that allows us to build
> our example programs without project files.
> 
> For now, I propose that we commit this patch without adding a testcase.
> I think that the symtab_to_fullname routine is already fairly
> extensively tested as it just a wrapper around a fairly central routine
> in source location.
> 
> 2007-01-06  Joel Brobecker  <brobecker@adacore.com>
> 
>         * ada-lang.c: Add include of source.h.
>         (is_known_support_routine): Improve the check verifying that the file
>         associated to this frame exists.
>         * Makefile.in (ada-lang.o): Add dependency on source.h.
> 
> Tested on x86-linux. Ok to apply?

Thank you,
-- 
Joel

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

Index: ada-lang.c
===================================================================
RCS file: /cvs/src/src/gdb/ada-lang.c,v
retrieving revision 1.88
diff -u -p -r1.88 ada-lang.c
--- ada-lang.c	4 Jan 2007 06:31:51 -0000	1.88
+++ ada-lang.c	7 Jan 2007 07:11:44 -0000
@@ -55,6 +55,7 @@ Boston, MA 02110-1301, USA.  */
 #include "exceptions.h"
 #include "annotate.h"
 #include "valprint.h"
+#include "source.h"
 
 #ifndef ADA_RETAIN_DOTS
 #define ADA_RETAIN_DOTS 0
@@ -9056,7 +9057,6 @@ is_known_support_routine (struct frame_i
     = find_pc_line (get_frame_pc (frame), pc_is_after_call);
   char *func_name;
   int i;
-  struct stat st;
 
   /* The heuristic:
      1. The symtab is null (indicating no debugging symbols)
@@ -9072,7 +9072,7 @@ is_known_support_routine (struct frame_i
      symbols; in this case, the filename referenced by these symbols
      does not exists.  */
 
-  if (stat (sal.symtab->filename, &st))
+  if (symtab_to_fullname (sal.symtab) == NULL)
     return 1;
 
   for (i = 0; known_runtime_file_name_patterns[i] != NULL; i += 1)
Index: Makefile.in
===================================================================
RCS file: /cvs/src/src/gdb/Makefile.in,v
retrieving revision 1.870
diff -u -p -r1.870 Makefile.in
--- Makefile.in	4 Jan 2007 22:11:44 -0000	1.870
+++ Makefile.in	7 Jan 2007 07:11:52 -0000
@@ -1696,7 +1696,8 @@ ada-lang.o: ada-lang.c $(defs_h) $(gdb_s
 	$(inferior_h) $(symfile_h) $(objfiles_h) $(breakpoint_h) \
 	$(gdbcore_h) $(hashtab_h) $(gdb_obstack_h) $(ada_lang_h) \
 	$(completer_h) $(gdb_stat_h) $(ui_out_h) $(block_h) $(infcall_h) \
-	$(dictionary_h) $(exceptions_h) $(annotate_h) $(valprint_h)
+	$(dictionary_h) $(exceptions_h) $(annotate_h) $(valprint_h) \
+	$(source_h)
 ada-typeprint.o: ada-typeprint.c $(defs_h) $(gdb_obstack_h) $(bfd_h) \
 	$(symtab_h) $(gdbtypes_h) $(expression_h) $(value_h) $(gdbcore_h) \
 	$(target_h) $(command_h) $(gdbcmd_h) $(language_h) $(demangle_h) \

Re: [RFA/ada] Improve is_known_support_routine

[Daniel Jacobowitz]

On Sat, Jan 06, 2007 at 10:26:52PM +0400, Joel Brobecker wrote:
> The is_known_support_routine identifies any frame for which we cannot
> find the source file. The check we currently have in place is a bit too
> simplistic, so I replaced it with the function we use to locate source
> files:
> 
> -  if (stat (sal.symtab->filename, &st))
> +  if (symtab_to_fullname (sal.symtab) == NULL)

Fine with me, although I wonder if "the source is missing" is really
the concept you want.  Why should whether source for the runtime
library is present determine what we show?  That would make the
feature more awkward to use for people who built their own libgnat and
still have the build tree.

Anyway, OK meanwhile.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA/ada] Improve is_known_support_routine

[Joel Brobecker]

> > The is_known_support_routine identifies any frame for which we cannot
> > find the source file. The check we currently have in place is a bit too
> > simplistic, so I replaced it with the function we use to locate source
> > files:
> > 
> > -  if (stat (sal.symtab->filename, &st))
> > +  if (symtab_to_fullname (sal.symtab) == NULL)
> 
> Fine with me, although I wonder if "the source is missing" is really
> the concept you want.  Why should whether source for the runtime
> library is present determine what we show?  That would make the
> feature more awkward to use for people who built their own libgnat and
> still have the build tree.

Actually, when I worked on cleaning up that routine, I found that
the attached comment was a bit incomplete. Here the new comment
explaining the main reason for discarding that frame:

   /* If there is a symtab, but the associated source file cannot be
      located, then assume this is not user code:  Selecting a frame
      for which we cannot display the code would not be very helpful
      for the user.  This should also take care of case such as VxWorks
      where the kernel has some debugging info provided for a few units.  */

   if (symtab_to_fullname (sal.symtab) == NULL)
     return 1;

In terms of the people building their own libgnat, it is true that
this might make it a bit more awkward for them. This is not necessarily
true that they would want in this case the debugger to select a frame
inside the runtime.  So we tried to find a compromise based on our own
usage and user feedback that was the most helpful in most cases.

-- 
Joel

Re: [RFA/ada] Improve is_known_support_routine

[Joel Brobecker]

> > The is_known_support_routine identifies any frame for which we cannot
> > find the source file. The check we currently have in place is a bit too
> > simplistic, so I replaced it with the function we use to locate source
> > files:
> > 
> > -  if (stat (sal.symtab->filename, &st))
> > +  if (symtab_to_fullname (sal.symtab) == NULL)
> 
> Anyway, OK meanwhile.

Thank you, checked in.

-- 
Joel