| Message ID | 81337265-2508-4e7a-1acc-4bf09b664f42@polymtl.ca |
|---|---|
| State | New |
| Headers | show |
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.
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
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;