public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [PATCH] gdb/debuginfod: Prevent out_of_range exception
@ 2022-04-08 22:50 Aaron Merey
  2022-04-09 15:41 ` Simon Marchi
  2022-04-12 16:09 ` [PATCH] gdb/debuginfod: Prevent out_of_range exception Tom Tromey
  0 siblings, 2 replies; 12+ messages in thread
From: Aaron Merey @ 2022-04-08 22:50 UTC (permalink / raw)
  To: gdb-patches

Trailing whitespace in the string of debuginfod URLs causes an
out_of_range exception during the printing of URLs for the first
use notice.

To fix this, stop printing URLs when the substring to be printed
consists only of whitespace.
---
 gdb/debuginfod-support.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
index 6c2d3fb2951..668c50c405c 100644
--- a/gdb/debuginfod-support.c
+++ b/gdb/debuginfod-support.c
@@ -187,10 +187,13 @@ debuginfod_is_enabled ()
       gdb::string_view url_view (urls);
       while (true)
 	{
-	  url_view = url_view.substr (url_view.find_first_not_of (' '));
+	  size_t off = url_view.find_first_not_of (' ');
+	  if (off == gdb::string_view::npos)
+	    break;
+	  url_view = url_view.substr (off);
 	  if (url_view.empty ())
 	    break;
-	  size_t off = url_view.find_first_of (' ');
+	  off = url_view.find_first_of (' ');
 	  gdb_printf
 	    (_("  <%ps>\n"),
 	     styled_string (file_name_style.style (),
-- 
2.35.1


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

* Re: [PATCH] gdb/debuginfod: Prevent out_of_range exception
  2022-04-08 22:50 [PATCH] gdb/debuginfod: Prevent out_of_range exception Aaron Merey
@ 2022-04-09 15:41 ` Simon Marchi
  2022-04-14 18:56   ` Aaron Merey
  2022-04-12 16:09 ` [PATCH] gdb/debuginfod: Prevent out_of_range exception Tom Tromey
  1 sibling, 1 reply; 12+ messages in thread
From: Simon Marchi @ 2022-04-09 15:41 UTC (permalink / raw)
  To: Aaron Merey, gdb-patches



On 2022-04-08 18:50, Aaron Merey via Gdb-patches wrote:
> Trailing whitespace in the string of debuginfod URLs causes an
> out_of_range exception during the printing of URLs for the first
> use notice.
> 
> To fix this, stop printing URLs when the substring to be printed
> consists only of whitespace.
> ---
>  gdb/debuginfod-support.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
> index 6c2d3fb2951..668c50c405c 100644
> --- a/gdb/debuginfod-support.c
> +++ b/gdb/debuginfod-support.c
> @@ -187,10 +187,13 @@ debuginfod_is_enabled ()
>        gdb::string_view url_view (urls);
>        while (true)
>  	{
> -	  url_view = url_view.substr (url_view.find_first_not_of (' '));
> +	  size_t off = url_view.find_first_not_of (' ');
> +	  if (off == gdb::string_view::npos)
> +	    break;
> +	  url_view = url_view.substr (off);
>  	  if (url_view.empty ())
>  	    break;


The change LGTM.  A test would be nice though, if not too complicated.

Also, I wonder if that url_view.empty() check is useful though.  Can it
really happen?

Simon

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

* Re: [PATCH] gdb/debuginfod: Prevent out_of_range exception
  2022-04-08 22:50 [PATCH] gdb/debuginfod: Prevent out_of_range exception Aaron Merey
  2022-04-09 15:41 ` Simon Marchi
@ 2022-04-12 16:09 ` Tom Tromey
  1 sibling, 0 replies; 12+ messages in thread
From: Tom Tromey @ 2022-04-12 16:09 UTC (permalink / raw)
  To: Aaron Merey via Gdb-patches

>>>>> "Aaron" == Aaron Merey via Gdb-patches <gdb-patches@sourceware.org> writes:

Aaron> Trailing whitespace in the string of debuginfod URLs causes an
Aaron> out_of_range exception during the printing of URLs for the first
Aaron> use notice.

Aaron> To fix this, stop printing URLs when the substring to be printed
Aaron> consists only of whitespace.

Thank you for doing this.

Tom

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

* Re: [PATCH] gdb/debuginfod: Prevent out_of_range exception
  2022-04-09 15:41 ` Simon Marchi
@ 2022-04-14 18:56   ` Aaron Merey
  2022-04-18 14:13     ` Tom Tromey
  2022-04-18 16:25     ` Pedro Alves
  0 siblings, 2 replies; 12+ messages in thread
From: Aaron Merey @ 2022-04-14 18:56 UTC (permalink / raw)
  To: simon.marchi; +Cc: gdb-patches, Aaron Merey

Hi Simon,

On Sat, Apr 9, 2022 at 11:42 AM Simon Marchi <simon.marchi@polymtl.ca> wrote:
> On 2022-04-08 18:50, Aaron Merey via Gdb-patches wrote:
> > -       url_view = url_view.substr (url_view.find_first_not_of (' '));
> > +       size_t off = url_view.find_first_not_of (' ');
> > +       if (off == gdb::string_view::npos)
> > +         break;
> > +       url_view = url_view.substr (off);
> >         if (url_view.empty ())
> >           break;
>
>
> The change LGTM.  A test would be nice though, if not too complicated.

I added a few first-use notice testcases.  I used gdb_expect instead of
gdb_test because I couldn't get gdb_test to match any text preceeding
the y/n prompt in the first-use notices, including the list of URLs.
I didn't have this problem with gdb_expect.

> Also, I wonder if that url_view.empty() check is useful though.  Can it
> really happen?

I think you're right.  If we get to that check then there must be at least
one non-whitespace character, so the url_view won't be empty.  I've removed
the check.

Aaron

---
gdb/debuginfod: Prevent out_of_range exception

Trailing whitespace in the string of debuginfod URLs causes an
out_of_range exception during the printing of URLs for the first
use notice.

To fix this, stop printing URLs when the substring to be printed
consists only of whitespace.

Also add first-use notice testcases.
---
 gdb/debuginfod-support.c                      |  7 +-
 .../gdb.debuginfod/fetch_src_and_symbols.exp  | 67 +++++++++++++++++++
 2 files changed, 71 insertions(+), 3 deletions(-)

diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
index 6c2d3fb2951..4ce2e786b6a 100644
--- a/gdb/debuginfod-support.c
+++ b/gdb/debuginfod-support.c
@@ -187,10 +187,11 @@ debuginfod_is_enabled ()
       gdb::string_view url_view (urls);
       while (true)
 	{
-	  url_view = url_view.substr (url_view.find_first_not_of (' '));
-	  if (url_view.empty ())
+	  size_t off = url_view.find_first_not_of (' ');
+	  if (off == gdb::string_view::npos)
 	    break;
-	  size_t off = url_view.find_first_of (' ');
+	  url_view = url_view.substr (off);
+	  off = url_view.find_first_of (' ');
 	  gdb_printf
 	    (_("  <%ps>\n"),
 	     styled_string (file_name_style.style (),
diff --git a/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp b/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp
index f12ed7d486c..3eb27276538 100644
--- a/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp
+++ b/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp
@@ -279,6 +279,73 @@ proc local_url { } {
     gdb_test_no_output "set debuginfod enabled on"
     gdb_test "file $binfile" ".*Reading symbols from.*debuginfo.*" \
 	"file [file tail $binfile] cmd on"
+
+    global gdb_prompt
+
+    # Test that URLs are printed correctly for the first-use notice
+    set testname "Notice empty URL"
+    set url ""
+    setenv DEBUGINFOD_URLS $url
+    clean_restart
+    send_gdb "file $binfile\n"
+    gdb_expect {
+	-re ".*Enable debuginfod.*" { fail $testname }
+	-re ".*$gdb_prompt" { pass $testname }
+	timeout { fail $testname }
+    }
+
+    set testname "Notice whitespace URL"
+    set url "   "
+    setenv DEBUGINFOD_URLS $url
+    clean_restart
+    send_gdb "file $binfile\n"
+    gdb_expect {
+	-re ".*URLs:\r\nEnable debuginfod.*" { pass $testname }
+	-re ".*$gdb_prompt" { fail $testname }
+	timeout { fail $testname }
+    }
+
+    set testname "Notice 1 URL"
+    set url "http://127.0.0.1:$port"
+    setenv DEBUGINFOD_URLS $url
+    clean_restart
+    send_gdb "file $binfile\n"
+    gdb_expect {
+	-re ".*<$url>.*" { pass $testname }
+	-re "$gdb_prompt" { fail $testname }
+	timeout { fail $testname }
+    }
+
+    set testname "Notice 1 URL with whitespace"
+    setenv DEBUGINFOD_URLS "  $url  "
+    clean_restart
+    send_gdb "file $binfile\n"
+    gdb_expect {
+	-re ".*<$url>.*" { pass $testname }
+	-re "$gdb_prompt" { fail $testname }
+	timeout { fail $testname }
+    }
+
+    set testname "Notice 2 URLs"
+    set url2 "127.0.0.1:$port"
+    setenv DEBUGINFOD_URLS "$url $url2"
+    clean_restart
+    send_gdb "file $binfile\n"
+    gdb_expect {
+	-re ".*<$url>\r\n.*<$url2>.*" { pass $testname }
+	-re "$gdb_prompt" { fail $testname }
+	timeout { fail $testname }
+    }
+
+    set testname "Notice 2 URLs with whitespace"
+    setenv DEBUGINFOD_URLS "  $url $url2  "
+    clean_restart
+    send_gdb "file $binfile\n"
+    gdb_expect {
+	-re ".*<$url>\r\n.*<$url2>.*" { pass $testname }
+	-re "$gdb_prompt" { fail $testname }
+	timeout { fail $testname }
+    }
 }
 
 set envlist \
-- 
2.35.1


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

* Re: [PATCH] gdb/debuginfod: Prevent out_of_range exception
  2022-04-14 18:56   ` Aaron Merey
@ 2022-04-18 14:13     ` Tom Tromey
  2022-04-18 16:25     ` Pedro Alves
  1 sibling, 0 replies; 12+ messages in thread
From: Tom Tromey @ 2022-04-18 14:13 UTC (permalink / raw)
  To: Aaron Merey via Gdb-patches

>>>>> "Aaron" == Aaron Merey via Gdb-patches <gdb-patches@sourceware.org> writes:

Aaron> gdb/debuginfod: Prevent out_of_range exception

Aaron> Trailing whitespace in the string of debuginfod URLs causes an
Aaron> out_of_range exception during the printing of URLs for the first
Aaron> use notice.

Aaron> To fix this, stop printing URLs when the substring to be printed
Aaron> consists only of whitespace.

Aaron> Also add first-use notice testcases.

This looks good to me.  Thank you for fixing this.

Tom

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

* Re: [PATCH] gdb/debuginfod: Prevent out_of_range exception
  2022-04-14 18:56   ` Aaron Merey
  2022-04-18 14:13     ` Tom Tromey
@ 2022-04-18 16:25     ` Pedro Alves
  2022-04-19 20:38       ` Aaron Merey
  1 sibling, 1 reply; 12+ messages in thread
From: Pedro Alves @ 2022-04-18 16:25 UTC (permalink / raw)
  To: Aaron Merey, simon.marchi; +Cc: gdb-patches

On 2022-04-14 19:56, Aaron Merey via Gdb-patches wrote:
> +    # Test that URLs are printed correctly for the first-use notice
> +    set testname "Notice empty URL"
> +    set url ""
> +    setenv DEBUGINFOD_URLS $url

With remote-host testing, this will only change the environment on the build machine,
while GDB runs on the host machine.  Looking at the file, I see it is already
doing a bunch of things that won't work in remote-host scenario, so we can ignore it
for this patch, but really the testcase should have an early exit if [is_remote host].

> +    clean_restart
> +    send_gdb "file $binfile\n"
> +    gdb_expect {
> +	-re ".*Enable debuginfod.*" { fail $testname }
> +	-re ".*$gdb_prompt" { pass $testname }
> +	timeout { fail $testname }
> +    }
> +

We really shouldn't be adding plain gdb_expect uses for GDB matches.  E.g., why not gdb_test here?

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

* Re: [PATCH] gdb/debuginfod: Prevent out_of_range exception
  2022-04-18 16:25     ` Pedro Alves
@ 2022-04-19 20:38       ` Aaron Merey
  2022-04-19 20:44         ` Pedro Alves
  0 siblings, 1 reply; 12+ messages in thread
From: Aaron Merey @ 2022-04-19 20:38 UTC (permalink / raw)
  To: Pedro Alves; +Cc: Simon Marchi, gdb-patches

Hi Pedro,

On Mon, Apr 18, 2022 at 12:25 PM Pedro Alves <pedro@palves.net> wrote:
> > +    clean_restart
> > +    send_gdb "file $binfile\n"
> > +    gdb_expect {
> > +     -re ".*Enable debuginfod.*" { fail $testname }
> > +     -re ".*$gdb_prompt" { pass $testname }
> > +     timeout { fail $testname }
> > +    }
> > +
>
> We really shouldn't be adding plain gdb_expect uses for GDB matches.  E.g., why not gdb_test here?

I wasn't able to get gdb_test to match any text preceding the y/n prompt
in the debuginfod first use notice.  This text includes the URL output being
tested.  I didn't have this issue with gdb_expect.

FWIW the first URL test that you quoted could be done with gdb_test since
we're testing that there is no first use notice when $DEBUGINFOD_URLS
is empty.  I used gdb_expect here for consistency but this could be changed.

Aaron


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

* Re: [PATCH] gdb/debuginfod: Prevent out_of_range exception
  2022-04-19 20:38       ` Aaron Merey
@ 2022-04-19 20:44         ` Pedro Alves
  2022-04-19 21:11           ` Aaron Merey
  0 siblings, 1 reply; 12+ messages in thread
From: Pedro Alves @ 2022-04-19 20:44 UTC (permalink / raw)
  To: Aaron Merey; +Cc: Simon Marchi, gdb-patches

On 2022-04-19 21:38, Aaron Merey wrote:
> Hi Pedro,
> 
> On Mon, Apr 18, 2022 at 12:25 PM Pedro Alves <pedro@palves.net> wrote:
>>> +    clean_restart
>>> +    send_gdb "file $binfile\n"
>>> +    gdb_expect {
>>> +     -re ".*Enable debuginfod.*" { fail $testname }
>>> +     -re ".*$gdb_prompt" { pass $testname }
>>> +     timeout { fail $testname }
>>> +    }
>>> +
>>
>> We really shouldn't be adding plain gdb_expect uses for GDB matches.  E.g., why not gdb_test here?
> 
> I wasn't able to get gdb_test to match any text preceding the y/n prompt
> in the debuginfod first use notice.  This text includes the URL output being
> tested.  I didn't have this issue with gdb_expect.

It may be that we need to use gdb_test_multiple instead of gdb_test above then.
Note also that the "Enable debuginfod" match is incorrect for not matching the prompt.

Can you show what the output of a passing case, and a failing case for the above
test look like?  After seeing those, we should be able to suggest what the
gdb_test_multiple should look like.

> 
> FWIW the first URL test that you quoted could be done with gdb_test since
> we're testing that there is no first use notice when $DEBUGINFOD_URLS
> is empty.  I used gdb_expect here for consistency but this could be changed.
> 
> Aaron
> 


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

* Re: [PATCH] gdb/debuginfod: Prevent out_of_range exception
  2022-04-19 20:44         ` Pedro Alves
@ 2022-04-19 21:11           ` Aaron Merey
  2022-04-20 19:24             ` Pedro Alves
  0 siblings, 1 reply; 12+ messages in thread
From: Aaron Merey @ 2022-04-19 21:11 UTC (permalink / raw)
  To: Pedro Alves; +Cc: Simon Marchi, gdb-patches

On Tue, Apr 19, 2022 at 4:44 PM Pedro Alves <pedro@palves.net> wrote:
>
> On 2022-04-19 21:38, Aaron Merey wrote:
> > Hi Pedro,
> >
> > On Mon, Apr 18, 2022 at 12:25 PM Pedro Alves <pedro@palves.net> wrote:
> >>> +    clean_restart
> >>> +    send_gdb "file $binfile\n"
> >>> +    gdb_expect {
> >>> +     -re ".*Enable debuginfod.*" { fail $testname }
> >>> +     -re ".*$gdb_prompt" { pass $testname }
> >>> +     timeout { fail $testname }
> >>> +    }
> >>> +
> >>
> >> We really shouldn't be adding plain gdb_expect uses for GDB matches.  E.g., why not gdb_test here?
> >
> > I wasn't able to get gdb_test to match any text preceding the y/n prompt
> > in the debuginfod first use notice.  This text includes the URL output being
> > tested.  I didn't have this issue with gdb_expect.
>
> It may be that we need to use gdb_test_multiple instead of gdb_test above then.
> Note also that the "Enable debuginfod" match is incorrect for not matching the prompt.
>
> Can you show what the output of a passing case, and a failing case for the above
> test look like?  After seeing those, we should be able to suggest what the
> gdb_test_multiple should look like.

A failing test would contain the first use notice and prompt to enable
debuginfod since an empty $DEBUGINFOD_URLS is meant to disable debuginfod:

  (gdb) file <$binfile>
  Reading symbols from <$binfile>...
  This GDB supports auto-downloading debuginfo from the following URLs:
  Enable debuginfod for this session? (y or [n])

A passing test would contain no debuginfod prompt:

  (gdb) file <$binfile>
  Reading symbols from <$binfile>...
  (No debugging symbols found in <$binfile>)
  (gdb)

Aaron


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

* Re: [PATCH] gdb/debuginfod: Prevent out_of_range exception
  2022-04-19 21:11           ` Aaron Merey
@ 2022-04-20 19:24             ` Pedro Alves
  2022-04-20 22:09               ` Aaron Merey
  2022-04-21 12:49               ` [pushed] gdb.debuginfod/fetch_src_and_symbols.exp: Fix "notice empty URL" test Pedro Alves
  0 siblings, 2 replies; 12+ messages in thread
From: Pedro Alves @ 2022-04-20 19:24 UTC (permalink / raw)
  To: Aaron Merey; +Cc: Simon Marchi, gdb-patches

On 2022-04-19 22:11, Aaron Merey wrote:
> On Tue, Apr 19, 2022 at 4:44 PM Pedro Alves <pedro@palves.net> wrote:
>>
>> On 2022-04-19 21:38, Aaron Merey wrote:
>>> Hi Pedro,
>>>
>>> On Mon, Apr 18, 2022 at 12:25 PM Pedro Alves <pedro@palves.net> wrote:
>>>>> +    clean_restart
>>>>> +    send_gdb "file $binfile\n"
>>>>> +    gdb_expect {
>>>>> +     -re ".*Enable debuginfod.*" { fail $testname }
>>>>> +     -re ".*$gdb_prompt" { pass $testname }
>>>>> +     timeout { fail $testname }
>>>>> +    }
>>>>> +
>>>>
>>>> We really shouldn't be adding plain gdb_expect uses for GDB matches.  E.g., why not gdb_test here?
>>>
>>> I wasn't able to get gdb_test to match any text preceding the y/n prompt
>>> in the debuginfod first use notice.  This text includes the URL output being
>>> tested.  I didn't have this issue with gdb_expect.
>>
>> It may be that we need to use gdb_test_multiple instead of gdb_test above then.
>> Note also that the "Enable debuginfod" match is incorrect for not matching the prompt.
>>
>> Can you show what the output of a passing case, and a failing case for the above
>> test look like?  After seeing those, we should be able to suggest what the
>> gdb_test_multiple should look like.
> 
> A failing test would contain the first use notice and prompt to enable
> debuginfod since an empty $DEBUGINFOD_URLS is meant to disable debuginfod:
> 
>   (gdb) file <$binfile>
>   Reading symbols from <$binfile>...
>   This GDB supports auto-downloading debuginfo from the following URLs:
>   Enable debuginfod for this session? (y or [n])
> 
> A passing test would contain no debuginfod prompt:
> 
>   (gdb) file <$binfile>
>   Reading symbols from <$binfile>...
>   (No debugging symbols found in <$binfile>)
>   (gdb)
> 

Thanks.  I ended up (finally) installing Debuginfod, and playing with the testcase locally.

Here's a patch that applies on top of yours, showing what I think this should look like.

I added a helper procedure to avoid repetition, and also tightened all the matches, such that
on the tests that expect 1 URL, we really only pass if one URL is printed.  Before, due to
use of .*, the test would still pass if more than one URL was printed.  Same for expecting 2,
printing >2, etc.

I also lowercased the test messages, and added missing periods, and added a couple commments.


I notice one odd thing -- so empty DEBUGINFOD_URLS disables Debuginfod, while DEBUGINFOD_URLS
with only whitespaces does not.  I found that surprising, I would expect the "only whitespaces"
case to behave the same as empty string, since effectively there are no URLs.  Maybe it's my
expectations that are off.

Feel free to squash this into your patch.

From 9b2498424b9f192a7d3aaa7fa21ccfd4dd02b176 Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Wed, 20 Apr 2022 19:37:39 +0100
Subject: [PATCH] Don't use send_gdb/gdb_expect

Change-Id: Id1dcd434c1d8e750aea663bde51fc35a0c2f8306
---
 .../gdb.debuginfod/fetch_src_and_symbols.exp  | 107 +++++++++---------
 1 file changed, 51 insertions(+), 56 deletions(-)

diff --git a/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp b/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp
index 3eb27276538..6da9a86faa8 100644
--- a/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp
+++ b/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp
@@ -186,6 +186,28 @@ proc no_url { } {
     gdb_test "core $::corefile" ".*in ?? ().*" "file [file tail $::corefile]"
 }
 
+# Test that GDB prints the debuginfod URLs when loading files.  URLS
+# is the string set in the DEBUGINFOD_URLS environment variable.
+# PATTERN_RE is the URLs pattern we expect to see out of GDB.  TEST is
+# the test name.
+
+proc test_urls {urls pattern_re test} {
+    setenv DEBUGINFOD_URLS $urls
+    clean_restart
+
+    if {$pattern_re != ""} {
+	set urls_re " +${pattern_re}\r\n"
+    } else {
+	set urls_re ""
+    }
+
+    # Use "with confirm off" to avoid having to deal with the
+    # "Enable debuginfod for this session? (y or [n])" question.
+    gdb_test "with confirm off -- file $::binfile" \
+	"following URLs:\r\n${urls_re}Debuginfod .*" \
+	$test
+}
+
 proc local_url { } {
     global binfile outputdir db debugdir
 
@@ -280,72 +302,45 @@ proc local_url { } {
     gdb_test "file $binfile" ".*Reading symbols from.*debuginfo.*" \
 	"file [file tail $binfile] cmd on"
 
-    global gdb_prompt
+    # Test that URLs are printed correctly for the first-use notice.
 
-    # Test that URLs are printed correctly for the first-use notice
-    set testname "Notice empty URL"
-    set url ""
-    setenv DEBUGINFOD_URLS $url
+    # Empty URLS disables Debuginfod.
+    setenv DEBUGINFOD_URLS ""
     clean_restart
-    send_gdb "file $binfile\n"
-    gdb_expect {
-	-re ".*Enable debuginfod.*" { fail $testname }
-	-re ".*$gdb_prompt" { pass $testname }
-	timeout { fail $testname }
+    # Disable confirmation to avoid having to deal with a query.  See
+    # test_urls.
+    gdb_test_multiple "with confirm off -- file $binfile" "notice empty URL" {
+	-re ".*Enable debuginfod.*" {
+	    fail $gdb_test_name
+	}
+	-re -wrap "" {
+	    pass $gdb_test_name
+	}
     }
 
-    set testname "Notice whitespace URL"
-    set url "   "
-    setenv DEBUGINFOD_URLS $url
-    clean_restart
-    send_gdb "file $binfile\n"
-    gdb_expect {
-	-re ".*URLs:\r\nEnable debuginfod.*" { pass $testname }
-	-re ".*$gdb_prompt" { fail $testname }
-	timeout { fail $testname }
-    }
+    test_urls "   " \
+        "" \
+        "notice whitespace URL"
 
-    set testname "Notice 1 URL"
     set url "http://127.0.0.1:$port"
-    setenv DEBUGINFOD_URLS $url
-    clean_restart
-    send_gdb "file $binfile\n"
-    gdb_expect {
-	-re ".*<$url>.*" { pass $testname }
-	-re "$gdb_prompt" { fail $testname }
-	timeout { fail $testname }
-    }
 
-    set testname "Notice 1 URL with whitespace"
-    setenv DEBUGINFOD_URLS "  $url  "
-    clean_restart
-    send_gdb "file $binfile\n"
-    gdb_expect {
-	-re ".*<$url>.*" { pass $testname }
-	-re "$gdb_prompt" { fail $testname }
-	timeout { fail $testname }
-    }
+    test_urls $url \
+	"<$url>" \
+	"notice 1 URL"
+
+    test_urls "  $url  " \
+	"<$url>" \
+	"notice 1 URL with whitespace"
 
-    set testname "Notice 2 URLs"
     set url2 "127.0.0.1:$port"
-    setenv DEBUGINFOD_URLS "$url $url2"
-    clean_restart
-    send_gdb "file $binfile\n"
-    gdb_expect {
-	-re ".*<$url>\r\n.*<$url2>.*" { pass $testname }
-	-re "$gdb_prompt" { fail $testname }
-	timeout { fail $testname }
-    }
 
-    set testname "Notice 2 URLs with whitespace"
-    setenv DEBUGINFOD_URLS "  $url $url2  "
-    clean_restart
-    send_gdb "file $binfile\n"
-    gdb_expect {
-	-re ".*<$url>\r\n.*<$url2>.*" { pass $testname }
-	-re "$gdb_prompt" { fail $testname }
-	timeout { fail $testname }
-    }
+    test_urls "$url $url2" \
+	"<$url>\r\n +<$url2>" \
+	"notice 2 URLs"
+
+    test_urls "  $url $url2  " \
+	"<$url>\r\n +<$url2>" \
+	"notice 2 URLs with whitespace"
 }
 
 set envlist \

-- 
2.26.2


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

* Re: [PATCH] gdb/debuginfod: Prevent out_of_range exception
  2022-04-20 19:24             ` Pedro Alves
@ 2022-04-20 22:09               ` Aaron Merey
  2022-04-21 12:49               ` [pushed] gdb.debuginfod/fetch_src_and_symbols.exp: Fix "notice empty URL" test Pedro Alves
  1 sibling, 0 replies; 12+ messages in thread
From: Aaron Merey @ 2022-04-20 22:09 UTC (permalink / raw)
  To: Pedro Alves; +Cc: Simon Marchi, gdb-patches

On Wed, Apr 20, 2022 at 3:25 PM Pedro Alves <pedro@palves.net> wrote:
> Thanks.  I ended up (finally) installing Debuginfod, and playing with the testcase locally.
>
> Here's a patch that applies on top of yours, showing what I think this should look like.
>
> I added a helper procedure to avoid repetition, and also tightened all the matches, such that
> on the tests that expect 1 URL, we really only pass if one URL is printed.  Before, due to
> use of .*, the test would still pass if more than one URL was printed.  Same for expecting 2,
> printing >2, etc.
>
> I also lowercased the test messages, and added missing periods, and added a couple commments.

Thanks for taking the time to do this.

> I notice one odd thing -- so empty DEBUGINFOD_URLS disables Debuginfod, while DEBUGINFOD_URLS
> with only whitespaces does not.  I found that surprising, I would expect the "only whitespaces"
> case to behave the same as empty string, since effectively there are no URLs.  Maybe it's my
> expectations that are off.

Fair point, there is no reason to proceed with a query if the URL is only
whitespace.  I can change this in another patch.

> Feel free to squash this into your patch.

Pushed as commit 52449404c4e380.

Aaron


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

* [pushed] gdb.debuginfod/fetch_src_and_symbols.exp: Fix "notice empty URL" test
  2022-04-20 19:24             ` Pedro Alves
  2022-04-20 22:09               ` Aaron Merey
@ 2022-04-21 12:49               ` Pedro Alves
  1 sibling, 0 replies; 12+ messages in thread
From: Pedro Alves @ 2022-04-21 12:49 UTC (permalink / raw)
  To: Aaron Merey; +Cc: Simon Marchi, gdb-patches

On 2022-04-20 20:24, Pedro Alves wrote:
> +    gdb_test_multiple "with confirm off -- file $binfile" "notice empty URL" {
> +	-re ".*Enable debuginfod.*" {
> +	    fail $gdb_test_name
> +	}
> +	-re -wrap "" {
> +	    pass $gdb_test_name
> +	}
>      }

I noticed I messed up adjusting the fail case above.  I've pushed this follow up patch to fix it.

From 9cf71b1354f6829a999b39a675a18c5a85f19e17 Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Thu, 21 Apr 2022 13:35:09 +0100
Subject: [PATCH] gdb.debuginfod/fetch_src_and_symbols.exp: Fix "notice empty
 URL" test

The gdb_test_multiple pattern for the "notice empty URL" test in
gdb.debuginfod/fetch_src_and_symbols.exp misses expecting the prompt.
Fix it by using -re -wrap.

Also, by using "confirm off", the message GDB prints if Debuginfod
downloading is available doesn't contain "Enable debuginfod" any
longer.  E.g.:

~~~
  (gdb) file testsuite/outputs/gdb.debuginfod/fetch_src_and_symbols/fetch_src_and_symbols
  Reading symbols from testsuite/outputs/gdb.debuginfod/fetch_src_and_symbols/fetch_src_and_symbols...

  This GDB supports auto-downloading debuginfo from the following URLs:
    <http://localhost:123>
  Enable debuginfod for this session? (y or [n])
~~~

~~~
  (gdb) with confirm off -- file testsuite/outputs/gdb.debuginfod/fetch_src_and_symbols/fetch_src_and_symbols
  Reading symbols from testsuite/outputs/gdb.debuginfod/fetch_src_and_symbols/fetch_src_and_symbols...

  This GDB supports auto-downloading debuginfo from the following URLs:
    <http://127.0.0.1:8000>
    <127.0.0.1:8000>
  Debuginfod has been disabled.
  To make this setting permanent, add 'set debuginfod enabled off' to .gdbinit.
  (No debugging symbols found in testsuite/outputs/gdb.debuginfod/fetch_src_and_symbols/fetch_src_and_symbols)
  (gdb)
~~~

I handled that correctly in the other tests that use test_urls, but
had forgotten to update the "notice empty URL" one.

Change-Id: I00040c83466e1494b3875574eb009c571a1504bf
---
 gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp b/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp
index 6da9a86faa8..bd90bcd0cfe 100644
--- a/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp
+++ b/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp
@@ -310,7 +310,7 @@ proc local_url { } {
     # Disable confirmation to avoid having to deal with a query.  See
     # test_urls.
     gdb_test_multiple "with confirm off -- file $binfile" "notice empty URL" {
-	-re ".*Enable debuginfod.*" {
+	-re -wrap "This GDB supports auto-downloading.*" {
 	    fail $gdb_test_name
 	}
 	-re -wrap "" {

base-commit: 333cd559bae5a6be60832c020da479ae23fd2664
-- 
2.26.2


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

end of thread, other threads:[~2022-04-21 12:49 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-08 22:50 [PATCH] gdb/debuginfod: Prevent out_of_range exception Aaron Merey
2022-04-09 15:41 ` Simon Marchi
2022-04-14 18:56   ` Aaron Merey
2022-04-18 14:13     ` Tom Tromey
2022-04-18 16:25     ` Pedro Alves
2022-04-19 20:38       ` Aaron Merey
2022-04-19 20:44         ` Pedro Alves
2022-04-19 21:11           ` Aaron Merey
2022-04-20 19:24             ` Pedro Alves
2022-04-20 22:09               ` Aaron Merey
2022-04-21 12:49               ` [pushed] gdb.debuginfod/fetch_src_and_symbols.exp: Fix "notice empty URL" test Pedro Alves
2022-04-12 16:09 ` [PATCH] gdb/debuginfod: Prevent out_of_range exception Tom Tromey

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).