public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* Question for Ada people, debugging gdb.ada/ptype_tagged_param.exp
@ 2019-12-05  5:04 Simon Marchi
  2019-12-08  0:51 ` Joel Brobecker
  0 siblings, 1 reply; 7+ messages in thread
From: Simon Marchi @ 2019-12-05  5:04 UTC (permalink / raw)
  To: GDB patches

Hi,

I've decided to try to tackle the first test that fails locally for me when I run
"make check", and that is gdb.ada/ptype_tagged_param.exp.  Here's what I found, I
would need help from Ada experts to know what to do next.

The problem is that we should be seeing this:

(gdb) ptype s
type = new pck.shape with record
    r: integer;
end record

but we see:

(gdb) ptype s
type = <ref> tagged record
    x: integer;
    y: integer;
end record

After debugging GDB, and reading the comment here [1], I understood that it was
because we failed to dynamically resolve the object's type.  Resolving the
object's type is done by reading the object's tag.  The tag points to some "type
specific data", abbreviated "tsd".  To interpret this type specific data, we need
to know its layout, so we lookup its type, "ada__tags__type_specific_data", in
ada_get_tsd_type.  The problem is that this type is not found.

I found that this symbol is provided by the GNAT runtime (in my mind, the equivalent
of glibc but for Ada), but only if it has been compiled with debug info.  It isn't
the case with my distro's package apparently, and I suppose it's the same for pretty
much all distros.

I also found that when building an Ada program using gnatmake, it's possible to tell
it to rebuild all files, including the files of the runtime.  It's then possible to
rebuild them with debug info.

So when I build the code of the test case manually, with

  $ gnatmake foo pck -a -f -g3 -O0

And loading the program in GDB, I do get the expected output when I do "ptype s".

The question is, what do I do with this?  Should test cases requiring access to debug
info from the runtime rebuild the runtime with debug info?  It takes a bit longer, but
it's not too bad (~10s).  Should we identify that the runtime is missing debug info
and XFAIL the test?  Something else?

Thanks,

Simon

[1] https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gdb/ada-lang.c;h=3289a8e5c8eef918370afe44298808210f9b864e;hb=refs/heads/master#l10621

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Question for Ada people, debugging gdb.ada/ptype_tagged_param.exp
  2019-12-05  5:04 Question for Ada people, debugging gdb.ada/ptype_tagged_param.exp Simon Marchi
@ 2019-12-08  0:51 ` Joel Brobecker
  2019-12-08  1:55   ` Simon Marchi
  0 siblings, 1 reply; 7+ messages in thread
From: Joel Brobecker @ 2019-12-08  0:51 UTC (permalink / raw)
  To: Simon Marchi; +Cc: GDB patches

Hi Simon,


> After debugging GDB, and reading the comment here [1], I understood
> that it was because we failed to dynamically resolve the object's
> type.  Resolving the object's type is done by reading the object's
> tag.  The tag points to some "type specific data", abbreviated "tsd".
> To interpret this type s upecific data, we need to know its layout, so
> we lookup its type, "ada__tags__type_specific_data", in
> ada_get_tsd_type.  The problem is that this type is not found.
> 
> I found that this symbol is provided by the GNAT runtime (in my mind,
> the equivalent of glibc but for Ada), but only if it has been compiled
> with debug info.  It isn't the case with my distro's package
> apparently, and I suppose it's the same for pretty much all distros.

Nice detective work!

What you are seeing is unfortunately a "classic". We know that many
distros (maybe all), have decided that they would build everything
with debugging info, and then split the debugging info into separate
packages which are not installed by default. As you can see, this
breaks some of the Ada debugging -- anything that relies on debugging
information from the runtime. I can't remember what else might be
broken by the stripping; I think Ada tasking support, maybe other
things.

I think the only reasonable answer to this problem is to tell people
that, to debug Ada, they need to make sure the runtime hasn't been
entirely stripped. It doesn't need every single unit in the runtime
to have debugging info, only a few of them, and the Makefiles in
GCC know which ones. When that information is missing, it breaks
the debugger.

> The question is, what do I do with this?  Should test cases requiring
> access to debug info from the runtime rebuild the runtime with debug
> info?  It takes a bit longer, but it's not too bad (~10s).

I do not think this would be right. It would prevent developers
who are using a compiler which hasn't been stripped to run the testsuite
and verify that everything is working as expected -- or said
differently, that we're not accidently starting to depend on debug
info from a unit that is normally not built with debug info.

> Should we identify that the runtime is missing debug info and XFAIL
> the test?

We could do that, if you find it worth the effort.

> Something else?

Cannot thing of anything else.

-- 
Joel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Question for Ada people, debugging gdb.ada/ptype_tagged_param.exp
  2019-12-08  0:51 ` Joel Brobecker
@ 2019-12-08  1:55   ` Simon Marchi
  2019-12-12 22:48     ` Joel Brobecker
  0 siblings, 1 reply; 7+ messages in thread
From: Simon Marchi @ 2019-12-08  1:55 UTC (permalink / raw)
  To: Joel Brobecker; +Cc: GDB patches

On 2019-12-07 7:51 p.m., Joel Brobecker wrote:
> Hi Simon,
> 
> 
>> After debugging GDB, and reading the comment here [1], I understood
>> that it was because we failed to dynamically resolve the object's
>> type.  Resolving the object's type is done by reading the object's
>> tag.  The tag points to some "type specific data", abbreviated "tsd".
>> To interpret this type s upecific data, we need to know its layout, so
>> we lookup its type, "ada__tags__type_specific_data", in
>> ada_get_tsd_type.  The problem is that this type is not found.
>>
>> I found that this symbol is provided by the GNAT runtime (in my mind,
>> the equivalent of glibc but for Ada), but only if it has been compiled
>> with debug info.  It isn't the case with my distro's package
>> apparently, and I suppose it's the same for pretty much all distros.
> 
> Nice detective work!
> 
> What you are seeing is unfortunately a "classic". We know that many
> distros (maybe all), have decided that they would build everything
> with debugging info, and then split the debugging info into separate
> packages which are not installed by default. As you can see, this
> breaks some of the Ada debugging -- anything that relies on debugging
> information from the runtime. I can't remember what else might be
> broken by the stripping; I think Ada tasking support, maybe other
> things.
> 
> I think the only reasonable answer to this problem is to tell people
> that, to debug Ada, they need to make sure the runtime hasn't been
> entirely stripped. It doesn't need every single unit in the runtime
> to have debugging info, only a few of them, and the Makefiles in
> GCC know which ones. When that information is missing, it breaks
> the debugger.
> 
>> The question is, what do I do with this?  Should test cases requiring
>> access to debug info from the runtime rebuild the runtime with debug
>> info?  It takes a bit longer, but it's not too bad (~10s).
> 
> I do not think this would be right. It would prevent developers
> who are using a compiler which hasn't been stripped to run the testsuite
> and verify that everything is working as expected -- or said
> differently, that we're not accidently starting to depend on debug
> info from a unit that is normally not built with debug info.
> 
>> Should we identify that the runtime is missing debug info and XFAIL
>> the test?
> 
> We could do that, if you find it worth the effort.

In the spirit of reducing FAILs (a test should either PASS, XFAIL or KFAIL), I think it's
worth the effort, I'll give it a try.  I spent quite a bit of time on it, so if we make it
properly XFAIL, we will hopefully spare others some time.

I think we could write a "gnat_has_debug_info" proc that compiles an hello world program and
checks for the presence of a particular symbol that will only be present if the runtime has
debug info.  Do you have a suggestion of a good symbol to use here, that would allow to
distinguish the presence and absence of debug info?

Simon

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Question for Ada people, debugging gdb.ada/ptype_tagged_param.exp
  2019-12-08  1:55   ` Simon Marchi
@ 2019-12-12 22:48     ` Joel Brobecker
  2019-12-15  4:09       ` Simon Marchi
  0 siblings, 1 reply; 7+ messages in thread
From: Joel Brobecker @ 2019-12-12 22:48 UTC (permalink / raw)
  To: Simon Marchi; +Cc: GDB patches

Hi Simon,

> >> Should we identify that the runtime is missing debug info and XFAIL
> >> the test?
> > 
> > We could do that, if you find it worth the effort.
> 
> In the spirit of reducing FAILs (a test should either PASS, XFAIL or
> KFAIL), I think it's worth the effort, I'll give it a try.  I spent
> quite a bit of time on it, so if we make it properly XFAIL, we will
> hopefully spare others some time.

OK, makes sense.

> I think we could write a "gnat_has_debug_info" proc that compiles an
> hello world program and checks for the presence of a particular symbol
> that will only be present if the runtime has debug info.  Do you have
> a suggestion of a good symbol to use here, that would allow to
> distinguish the presence and absence of debug info?

FWIW, this is really about the runtime, so I would mention it
in the name of the function.

The other thing you might want to be careful of is that, on some
platforms, the gnat runtime used for linking by default is the
shared libgnat. So, if you want to check for the presence of
a given symbol, you would have to start the program first.
Shouldn't be a big issue, I believe; you just have to run until
reaching a known subprogram in the inferior.

The approach of checking for a given symbol to infer whether
the runtime as a whole has the necessary debug info for a given
test should work in practice -- as I do not a situation where
only part of the units that need debugging info have debug info,
but not others, is worth worrying about.

Probably one good function to look for is the following:

        __gnat_debug_raise_exception

The thing with these internal functions is that they are subject
to change. We try not to, but this has happened as you can see
from the various declarations of struct exception_support_info
objects in ada-lang.c (search for default_exception_support_info).
Luckily, I think this one has been quite stable (unchanged since
March 2007 according to one of the comments0, and I am hoping
will remain the same for a veeeery long time.

Thanks for your help doing that, Simon!

-- 
Joel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Question for Ada people, debugging gdb.ada/ptype_tagged_param.exp
  2019-12-12 22:48     ` Joel Brobecker
@ 2019-12-15  4:09       ` Simon Marchi
  2019-12-23  7:29         ` Joel Brobecker
  0 siblings, 1 reply; 7+ messages in thread
From: Simon Marchi @ 2019-12-15  4:09 UTC (permalink / raw)
  To: Joel Brobecker; +Cc: GDB patches

On 2019-12-12 5:48 p.m., Joel Brobecker wrote:
> FWIW, this is really about the runtime, so I would mention it
> in the name of the function.

Noted.

> The other thing you might want to be careful of is that, on some
> platforms, the gnat runtime used for linking by default is the
> shared libgnat. So, if you want to check for the presence of
> a given symbol, you would have to start the program first.
> Shouldn't be a big issue, I believe; you just have to run until
> reaching a known subprogram in the inferior.

I did that, it doesn't seem to be a problem.

> The approach of checking for a given symbol to infer whether
> the runtime as a whole has the necessary debug info for a given
> test should work in practice -- as I do not a situation where
> only part of the units that need debugging info have debug info,
> but not others, is worth worrying about.
> 
> Probably one good function to look for is the following:
> 
>         __gnat_debug_raise_exception

Great, thanks.

> The thing with these internal functions is that they are subject
> to change. We try not to, but this has happened as you can see
> from the various declarations of struct exception_support_info
> objects in ada-lang.c (search for default_exception_support_info).
> Luckily, I think this one has been quite stable (unchanged since
> March 2007 according to one of the comments0, and I am hoping
> will remain the same for a veeeery long time.

And even if it changes, we can adapt the test easily.

Here's a patch.  It works on a runtime without debug info, but I'll need
your help to test it on a runtime with debug info, as I didn't find a
configuration that worked.  I tried to install the libgnat-7-dbg package
on Ubuntu 18.04, but it didn't help.


From 431f8d1d6ff7dd343a4c35ab67318a44690abd4b Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@polymtl.ca>
Date: Sat, 14 Dec 2019 22:16:05 -0500
Subject: [PATCH] Adjust test gdb.ada/ptype_tagged_param.exp for when GNAT
 runtime does not have debug info

This test verifies that GDB correctly identifies the run-time type of
"s" as being the type "Circle".  However, that can only be done
correctly if the GNAT runtime has been compiled and shipped with debug
information, so that GDB can poke in its internal data structures.
Currently the test fails when when running against a GNAT runtime
without debug info.  This is the case, for example, on Arch Linux using
the distribution package.

This patch adds a helper in lib/ada.exp to check whether the GNAT
runtime has debug info or not.  It then uses it in
gdb.ada/ptype_tagged_param.exp to expect a different result, depending
on whether we have debug info or not in the runtime.

At first, I made it so we would XFAIL the test, in the absence of debug
info, but then I thought that we might as well test for the output we
expect in the absence of debug info instead.

gdb/testsuite/ChangeLog:

	* lib/ada.exp (gnat_runtime_has_debug_info): New proc.
	* lib/gnat-debug-info-test.adb: New file.
	* gdb.ada/ptype_tagged_param.exp: Use
	gnat_runtime_has_debug_info, expect a different output if
	runtime does not have debug info.
---
 gdb/testsuite/gdb.ada/ptype_tagged_param.exp | 22 +++++++++---
 gdb/testsuite/lib/ada.exp                    | 36 ++++++++++++++++++++
 gdb/testsuite/lib/gnat-debug-info-test.adb   |  6 ++++
 3 files changed, 60 insertions(+), 4 deletions(-)
 create mode 100644 gdb/testsuite/lib/gnat-debug-info-test.adb

diff --git a/gdb/testsuite/gdb.ada/ptype_tagged_param.exp b/gdb/testsuite/gdb.ada/ptype_tagged_param.exp
index 567ef8251d9f..87a676c9ca24 100644
--- a/gdb/testsuite/gdb.ada/ptype_tagged_param.exp
+++ b/gdb/testsuite/gdb.ada/ptype_tagged_param.exp
@@ -21,14 +21,28 @@ if {[gdb_compile_ada "${srcfile}" "${binfile}" executable [list debug ]] != "" }
   return -1
 }

+set has_runtime_debug_info [gnat_runtime_has_debug_info]
+
 clean_restart ${testfile}

 if ![runto "position_x" ] then {
   return -1
 }

-set eol "\[\r\n\]+"
-set sp "\[ \t\]*"
+if { $has_runtime_debug_info } {
+    gdb_test "ptype s" \
+	[multi_line \
+	    "type = <ref> new pck.shape with record" \
+	    "    r: integer;" \
+	    "end record"] \
+	"ptype s, with debug info"
+} else {
+    gdb_test "ptype s" \
+	[multi_line \
+	    "type = <ref> tagged record" \
+	    "    x: integer;" \
+	    "    y: integer;" \
+	    "end record" ] \
+	"ptype s, without debug info"
+}

-gdb_test "ptype s" \
-         "type = <ref> new pck.shape with record${eol}${sp}r: integer;${eol}end record"
diff --git a/gdb/testsuite/lib/ada.exp b/gdb/testsuite/lib/ada.exp
index 45c41806a648..378c0db610e4 100644
--- a/gdb/testsuite/lib/ada.exp
+++ b/gdb/testsuite/lib/ada.exp
@@ -149,3 +149,39 @@ proc gnatmake_version_at_least { major } {
     # Unknown, return 1
     return 1
 }
+
+# Return 1 if the GNAT runtime appears to have debug info.
+
+gdb_caching_proc gnat_runtime_has_debug_info {
+    global srcdir
+
+    set src "$srcdir/lib/gnat-debug-info-test.adb"
+    set dst [standard_output_file "gnat-debug-info-test"]
+
+    if { [gdb_compile_ada $src $dst executable {debug}] != "" } {
+	fail "failed to compile gnat-debug-info test binary"
+	return 0
+    }
+
+    clean_restart $dst
+
+    if { ! [runto "Hello"] } {
+	fail "failed to run to Hello"
+	return 0
+    }
+
+    set has_debug_info 0
+
+    gdb_test_multiple "whatis __gnat_debug_raise_exception" "" {
+	-re "type = <text variable, no debug info>" { }
+	-re "type = void" {
+	    set has_debug_info 1
+	}
+	default {
+	    # Some other unexpected output...
+	    fail $gdb_test_name
+	}
+    }
+
+    return 0
+}
diff --git a/gdb/testsuite/lib/gnat-debug-info-test.adb b/gdb/testsuite/lib/gnat-debug-info-test.adb
new file mode 100644
index 000000000000..f1831ad86991
--- /dev/null
+++ b/gdb/testsuite/lib/gnat-debug-info-test.adb
@@ -0,0 +1,6 @@
+with Ada.Text_IO;
+
+procedure Hello is
+begin
+   Ada.Text_IO.Put_Line("Hello, world!");
+end Hello;
-- 
2.24.1

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Question for Ada people, debugging gdb.ada/ptype_tagged_param.exp
  2019-12-15  4:09       ` Simon Marchi
@ 2019-12-23  7:29         ` Joel Brobecker
  2019-12-28  2:04           ` Simon Marchi
  0 siblings, 1 reply; 7+ messages in thread
From: Joel Brobecker @ 2019-12-23  7:29 UTC (permalink / raw)
  To: Simon Marchi; +Cc: GDB patches

Hi Simon,

Sorry for the delay in answering you. I've been meaning to answer
for the last couple of weekends, but I haven't managed to find
the time due to travels :-(.

> Here's a patch.  It works on a runtime without debug info, but I'll need
> your help to test it on a runtime with debug info, as I didn't find a
> configuration that worked.  I tried to install the libgnat-7-dbg package
> on Ubuntu 18.04, but it didn't help.

Thanks, Simon.

> >From 431f8d1d6ff7dd343a4c35ab67318a44690abd4b Mon Sep 17 00:00:00 2001
> From: Simon Marchi <simon.marchi@polymtl.ca>
> Date: Sat, 14 Dec 2019 22:16:05 -0500
> Subject: [PATCH] Adjust test gdb.ada/ptype_tagged_param.exp for when GNAT
>  runtime does not have debug info
> 
> This test verifies that GDB correctly identifies the run-time type of
> "s" as being the type "Circle".  However, that can only be done
> correctly if the GNAT runtime has been compiled and shipped with debug
> information, so that GDB can poke in its internal data structures.
> Currently the test fails when when running against a GNAT runtime
> without debug info.  This is the case, for example, on Arch Linux using
> the distribution package.
> 
> This patch adds a helper in lib/ada.exp to check whether the GNAT
> runtime has debug info or not.  It then uses it in
> gdb.ada/ptype_tagged_param.exp to expect a different result, depending
> on whether we have debug info or not in the runtime.
> 
> At first, I made it so we would XFAIL the test, in the absence of debug
> info, but then I thought that we might as well test for the output we
> expect in the absence of debug info instead.

FWIW, I don't mind checking for the result that we get when the runtime
is missing the debugging information. The only downside I see is that
the expected behavior is less predictable: It depends on both
the implementation chosen by the compiler, which can change over time,
and also across targets, but can also depend on GDB's implementation
choices too.

For my money, I think the most practical approach is to actually
say "this test requires debugging information from the runtime,
so if you do not have it, then the testcase cannot be run", in
which case we return UNSUPPORTED instead of returning an XFAIL
or changing the expected output.

I like your approach because it forces us to test GDB more thouroughly,
and in particular it verifies the behavior when in degraded mode.
We should just be aware that this would be at the cost of some
occasional testcase maintenance work, when anything affecting
the somewhat unspecified outcome changes.

> gdb/testsuite/ChangeLog:
> 
> 	* lib/ada.exp (gnat_runtime_has_debug_info): New proc.
> 	* lib/gnat-debug-info-test.adb: New file.
> 	* gdb.ada/ptype_tagged_param.exp: Use
> 	gnat_runtime_has_debug_info, expect a different output if
> 	runtime does not have debug info.

I tested this patch, and found a thinko-bug. See below.

> ---
>  gdb/testsuite/gdb.ada/ptype_tagged_param.exp | 22 +++++++++---
>  gdb/testsuite/lib/ada.exp                    | 36 ++++++++++++++++++++
>  gdb/testsuite/lib/gnat-debug-info-test.adb   |  6 ++++
>  3 files changed, 60 insertions(+), 4 deletions(-)
>  create mode 100644 gdb/testsuite/lib/gnat-debug-info-test.adb
> 
> diff --git a/gdb/testsuite/gdb.ada/ptype_tagged_param.exp b/gdb/testsuite/gdb.ada/ptype_tagged_param.exp
> index 567ef8251d9f..87a676c9ca24 100644
> --- a/gdb/testsuite/gdb.ada/ptype_tagged_param.exp
> +++ b/gdb/testsuite/gdb.ada/ptype_tagged_param.exp
> @@ -21,14 +21,28 @@ if {[gdb_compile_ada "${srcfile}" "${binfile}" executable [list debug ]] != "" }
>    return -1
>  }
> 
> +set has_runtime_debug_info [gnat_runtime_has_debug_info]
> +
>  clean_restart ${testfile}
> 
>  if ![runto "position_x" ] then {
>    return -1
>  }
> 
> -set eol "\[\r\n\]+"
> -set sp "\[ \t\]*"
> +if { $has_runtime_debug_info } {
> +    gdb_test "ptype s" \
> +	[multi_line \
> +	    "type = <ref> new pck.shape with record" \
> +	    "    r: integer;" \
> +	    "end record"] \
> +	"ptype s, with debug info"
> +} else {
> +    gdb_test "ptype s" \
> +	[multi_line \
> +	    "type = <ref> tagged record" \
> +	    "    x: integer;" \
> +	    "    y: integer;" \
> +	    "end record" ] \
> +	"ptype s, without debug info"
> +}

+1 for the use of multi_line here. Thanks Simon!

> 
> -gdb_test "ptype s" \
> -         "type = <ref> new pck.shape with record${eol}${sp}r: integer;${eol}end record"
> diff --git a/gdb/testsuite/lib/ada.exp b/gdb/testsuite/lib/ada.exp
> index 45c41806a648..378c0db610e4 100644
> --- a/gdb/testsuite/lib/ada.exp
> +++ b/gdb/testsuite/lib/ada.exp
> @@ -149,3 +149,39 @@ proc gnatmake_version_at_least { major } {
>      # Unknown, return 1
>      return 1
>  }
> +
> +# Return 1 if the GNAT runtime appears to have debug info.
> +
> +gdb_caching_proc gnat_runtime_has_debug_info {
> +    global srcdir
> +
> +    set src "$srcdir/lib/gnat-debug-info-test.adb"
> +    set dst [standard_output_file "gnat-debug-info-test"]
> +
> +    if { [gdb_compile_ada $src $dst executable {debug}] != "" } {
> +	fail "failed to compile gnat-debug-info test binary"
> +	return 0
> +    }
> +
> +    clean_restart $dst
> +
> +    if { ! [runto "Hello"] } {
> +	fail "failed to run to Hello"
> +	return 0
> +    }
> +
> +    set has_debug_info 0
> +
> +    gdb_test_multiple "whatis __gnat_debug_raise_exception" "" {
> +	-re "type = <text variable, no debug info>" { }
> +	-re "type = void" {
> +	    set has_debug_info 1
> +	}
> +	default {
> +	    # Some other unexpected output...
> +	    fail $gdb_test_name
> +	}
> +    }
> +
> +    return 0

I think you meant to return $has_debug_info here. Otherwise,
running ptype_tagged_param.exp, I get a failure indicating that
it tried to match the output thinking the runtime has no debug
info.

> diff --git a/gdb/testsuite/lib/gnat-debug-info-test.adb b/gdb/testsuite/lib/gnat-debug-info-test.adb
> new file mode 100644
> index 000000000000..f1831ad86991
> --- /dev/null
> +++ b/gdb/testsuite/lib/gnat-debug-info-test.adb
> @@ -0,0 +1,6 @@
> +with Ada.Text_IO;
> +
> +procedure Hello is

It's not absolutely crucial, but it is better if the name of
the file matches the name of the unit. I would rename the file
to something like gnat_debug_info_test.adb, and then change
the name of the subprogram to GNAT_Debug_Info_Test.

If the use of dashes was to avoid undercore characters, which are
traditionally more efforts to type when on US keyboards, we can
always drop those entirely...

Other than that, the patch looks good to me.

-- 
Joel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Question for Ada people, debugging gdb.ada/ptype_tagged_param.exp
  2019-12-23  7:29         ` Joel Brobecker
@ 2019-12-28  2:04           ` Simon Marchi
  0 siblings, 0 replies; 7+ messages in thread
From: Simon Marchi @ 2019-12-28  2:04 UTC (permalink / raw)
  To: Joel Brobecker; +Cc: GDB patches

On 2019-12-23 2:29 a.m., Joel Brobecker wrote:
>> At first, I made it so we would XFAIL the test, in the absence of debug
>> info, but then I thought that we might as well test for the output we
>> expect in the absence of debug info instead.
> 
> FWIW, I don't mind checking for the result that we get when the runtime
> is missing the debugging information. The only downside I see is that
> the expected behavior is less predictable: It depends on both
> the implementation chosen by the compiler, which can change over time,
> and also across targets, but can also depend on GDB's implementation
> choices too.
> 
> For my money, I think the most practical approach is to actually
> say "this test requires debugging information from the runtime,
> so if you do not have it, then the testcase cannot be run", in
> which case we return UNSUPPORTED instead of returning an XFAIL
> or changing the expected output.
> 
> I like your approach because it forces us to test GDB more thouroughly,
> and in particular it verifies the behavior when in degraded mode.
> We should just be aware that this would be at the cost of some
> occasional testcase maintenance work, when anything affecting
> the somewhat unspecified outcome changes.

I have a slight preference for testing GDB's actual behavior instead of just saying
"unsupported", because if GDB's behavior changes by inadvertence, the test case will
notify us and we can decide whether that change makes sense (and if so update the
test case) or if it's actually a regression.

If as you say, the output changes based on the compiler and the target, it won't be
too hard to change the test to accept various outputs.  Or if it's really too much
trouble, I would maybe use something like:

  gdb_test "ptype s" ".*"

which would ensure at least that GDB doesn't crash when issuing this command
without debug info available.

>> diff --git a/gdb/testsuite/lib/ada.exp b/gdb/testsuite/lib/ada.exp
>> index 45c41806a648..378c0db610e4 100644
>> --- a/gdb/testsuite/lib/ada.exp
>> +++ b/gdb/testsuite/lib/ada.exp
>> @@ -149,3 +149,39 @@ proc gnatmake_version_at_least { major } {
>>      # Unknown, return 1
>>      return 1
>>  }
>> +
>> +# Return 1 if the GNAT runtime appears to have debug info.
>> +
>> +gdb_caching_proc gnat_runtime_has_debug_info {
>> +    global srcdir
>> +
>> +    set src "$srcdir/lib/gnat-debug-info-test.adb"
>> +    set dst [standard_output_file "gnat-debug-info-test"]
>> +
>> +    if { [gdb_compile_ada $src $dst executable {debug}] != "" } {
>> +	fail "failed to compile gnat-debug-info test binary"
>> +	return 0
>> +    }
>> +
>> +    clean_restart $dst
>> +
>> +    if { ! [runto "Hello"] } {
>> +	fail "failed to run to Hello"
>> +	return 0
>> +    }
>> +
>> +    set has_debug_info 0
>> +
>> +    gdb_test_multiple "whatis __gnat_debug_raise_exception" "" {
>> +	-re "type = <text variable, no debug info>" { }
>> +	-re "type = void" {
>> +	    set has_debug_info 1
>> +	}
>> +	default {
>> +	    # Some other unexpected output...
>> +	    fail $gdb_test_name
>> +	}
>> +    }
>> +
>> +    return 0
> 
> I think you meant to return $has_debug_info here. Otherwise,
> running ptype_tagged_param.exp, I get a failure indicating that
> it tried to match the output thinking the runtime has no debug
> info.

Eh, of course!

>> diff --git a/gdb/testsuite/lib/gnat-debug-info-test.adb b/gdb/testsuite/lib/gnat-debug-info-test.adb
>> new file mode 100644
>> index 000000000000..f1831ad86991
>> --- /dev/null
>> +++ b/gdb/testsuite/lib/gnat-debug-info-test.adb
>> @@ -0,0 +1,6 @@
>> +with Ada.Text_IO;
>> +
>> +procedure Hello is
> 
> It's not absolutely crucial, but it is better if the name of
> the file matches the name of the unit. I would rename the file
> to something like gnat_debug_info_test.adb, and then change
> the name of the subprogram to GNAT_Debug_Info_Test.
> 
> If the use of dashes was to avoid undercore characters, which are
> traditionally more efforts to type when on US keyboards, we can
> always drop those entirely...

Haha, no, my choices were due to my ignorance of the good practices when
using Ada.  I applied your suggestions.

> Other than that, the patch looks good to me.

Thanks for the review.  I ended up downloading GNAT from

  https://www.adacore.com/download

which lets me test the "with debug info" case, and confirm that it now works,
so I pushed the patch with the changes mentioned above.

Simon

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2019-12-28  2:04 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-05  5:04 Question for Ada people, debugging gdb.ada/ptype_tagged_param.exp Simon Marchi
2019-12-08  0:51 ` Joel Brobecker
2019-12-08  1:55   ` Simon Marchi
2019-12-12 22:48     ` Joel Brobecker
2019-12-15  4:09       ` Simon Marchi
2019-12-23  7:29         ` Joel Brobecker
2019-12-28  2:04           ` Simon Marchi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).