[PATCH] gdb/testsuite: fix gdb.base/dlmopen-ns-ids.exp racy test

[PATCH] gdb/testsuite: fix gdb.base/dlmopen-ns-ids.exp racy test

[Guinevere Larsen]

The recently included gdb.base/dlmopen-ns-ids.exp test can sometimes
fail the call to get_integer_valueof when trying to check the namespace
ID of the fourth dlopened SO, for apparently no reason.

What's happening is that the call to get_first_so_ns doesn't necessarily
consume the GDB prompt, and so get_integer_valueof will see the prompt
immediately and not find the value the test is looking for.

To fix this, a call to gdb_test is added to the end of get_first_so_ns
to consume the prompt if it wasn't consumed by the previous
gdb_test_multiple call. If the namespace was found (and so ns is not
-1), we can guarantee that the prompt was not consumed.

I tried several other ways to solve that problem, but they each caused
different problems:
* using -wrap in the second block of the gdb_test_multiple resulted in
  every call timing out
* reducing the output with a pattern on the "info shared" call and
  adding -wrap caused the line to not be recognized
* using exp_continue caused the value of ns to be overwritten, returning
  the last SO_NS, instead of the first

With all these problems, adding the gdb_test call seemed the least
fragile option.
---
 gdb/testsuite/gdb.base/dlmopen-ns-ids.exp | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/gdb/testsuite/gdb.base/dlmopen-ns-ids.exp b/gdb/testsuite/gdb.base/dlmopen-ns-ids.exp
index 03b7a527af5..5c65162d98f 100644
--- a/gdb/testsuite/gdb.base/dlmopen-ns-ids.exp
+++ b/gdb/testsuite/gdb.base/dlmopen-ns-ids.exp
@@ -45,7 +45,7 @@ proc get_first_so_ns {} {
 	-re "From\\s+To\\s+\(NS\\s+\)?Syms\\s+Read\\s+Shared Object Library\r\n" {
 	    exp_continue
 	}
-	-re "^$::hex\\s+$::hex\\s+\\\[\\\[($::decimal)\\\]\\\]\\s+\[^\r\n]+$::binfile_lib.*" {
+	-re "^$::hex\\s+$::hex\\s+\\\[\\\[($::decimal)\\\]\\\]\\s+\[^\r\n]+$::binfile_lib\r\n" {
 	    set ns $expect_out(1,string)
 	}
 	-re "^$::gdb_prompt $" {
@@ -54,6 +54,14 @@ proc get_first_so_ns {} {
 	    exp_continue
 	}
     }
+    # Clear the prompt from the previous test, if it was not consumed.
+    # This can't be done in the previous test because we need to leave on
+    # the first occurrence of the library, otherwise we'll overwrite the
+    # namespace found, and just adding gdb_prompt to the end of the success
+    # case caused several timeouts.
+    if {$ns != -1} {
+	gdb_test -nopass "" ".*"
+    }
     return $ns
 }
 

base-commit: d8c3e4901650e35ddf5083f693b8a21b9d24fcc6
-- 
2.49.0

Re: [PATCH] gdb/testsuite: fix gdb.base/dlmopen-ns-ids.exp racy test

[Tom de Vries]

On 4/9/25 20:25, Guinevere Larsen wrote:
> * using exp_continue caused the value of ns to be overwritten, returning
>    the last SO_NS, instead of the first

Hi Gwen,

my guess is that this is the right solution, and that you can just check 
for $ns == -1 to make sure only the first value is picked.

Thanks,
- Tom

Re: [PATCH] gdb/testsuite: fix gdb.base/dlmopen-ns-ids.exp racy test

[Tom de Vries]

On 4/10/25 06:40, Tom de Vries wrote:
> On 4/9/25 20:25, Guinevere Larsen wrote:
>> * using exp_continue caused the value of ns to be overwritten, returning
>>    the last SO_NS, instead of the first
> 
> Hi Gwen,
> 
> my guess is that this is the right solution, and that you can just check 
> for $ns == -1 to make sure only the first value is picked.

I tried this out, and got it working, after fixing the following.

There is a problem with the gdb_test_multiple.

It uses -lbl, which is the style of processing one line at a time, with 
\r\n at the start, and (?=\r\n) at the end, which matches but not 
consumes it (lookahead), always leaving an \r\n at the start of the next 
line.

This matches what -wrap (which is based on gdb_test) does which requires 
an \r\n before the prompt.

I got the test-case working using:
...
diff --git a/gdb/testsuite/gdb.base/dlmopen-ns-ids.exp 
b/gdb/testsuite/gdb.base/dlmopen-ns-ids.exp
index 03b7a527af5..d3584a0a402 100644
--- a/gdb/testsuite/gdb.base/dlmopen-ns-ids.exp
+++ b/gdb/testsuite/gdb.base/dlmopen-ns-ids.exp
@@ -42,17 +42,17 @@ if { [build_executable "failed to build" $testfile 
$srcfile \
  proc get_first_so_ns {} {
      set ns -1
      gdb_test_multiple "info sharedlibrary" "get SO namespace" -lbl {
-	-re "From\\s+To\\s+\(NS\\s+\)?Syms\\s+Read\\s+Shared Object Library\r\n" {
+	-re "\r\nFrom\\s+To\\s+\(NS\\s+\)?Syms\\s+Read\\s+Shared Object 
Library(?=\r\n)" {
  	    exp_continue
  	}
-	-re 
"^$::hex\\s+$::hex\\s+\\\[\\\[($::decimal)\\\]\\\]\\s+\[^\r\n]+$::binfile_lib.*" 
{
-	    set ns $expect_out(1,string)
-	}
-	-re "^$::gdb_prompt $" {
-	}
-	-re "^\[^\r\n\]+\r\n" {
+	-re 
"\r\n$::hex\\s+$::hex\\s+\\\[\\\[($::decimal)\\\]\\\]\\s+\[^\r\n]+${::binfile_lib}(?=\r\n
)" {
+	    if { $ns == -1 } {
+		set ns $expect_out(1,string)
+	    }
  	    exp_continue
  	}
+	-re -wrap "" {
+	}
      }
      return $ns
  }
...

I also think that binfile_lib needs to be wrapped in string_to_regexp.

Thanks,
- Tom

Re: [PATCH] gdb/testsuite: fix gdb.base/dlmopen-ns-ids.exp racy test

[Guinevere Larsen]

On 4/10/25 5:11 AM, Tom de Vries wrote:
> On 4/10/25 06:40, Tom de Vries wrote:
>> On 4/9/25 20:25, Guinevere Larsen wrote:
>>> * using exp_continue caused the value of ns to be overwritten, 
>>> returning
>>>    the last SO_NS, instead of the first
>>
>> Hi Gwen,
>>
>> my guess is that this is the right solution, and that you can just 
>> check for $ns == -1 to make sure only the first value is picked.
>
> I tried this out, and got it working, after fixing the following.
>
> There is a problem with the gdb_test_multiple.
>
> It uses -lbl, which is the style of processing one line at a time, 
> with \r\n at the start, and (?=\r\n) at the end, which matches but not 
> consumes it (lookahead), always leaving an \r\n at the start of the 
> next line.
>
> This matches what -wrap (which is based on gdb_test) does which 
> requires an \r\n before the prompt.
>
> I got the test-case working using:
> ...
> diff --git a/gdb/testsuite/gdb.base/dlmopen-ns-ids.exp 
> b/gdb/testsuite/gdb.base/dlmopen-ns-ids.exp
> index 03b7a527af5..d3584a0a402 100644
> --- a/gdb/testsuite/gdb.base/dlmopen-ns-ids.exp
> +++ b/gdb/testsuite/gdb.base/dlmopen-ns-ids.exp
> @@ -42,17 +42,17 @@ if { [build_executable "failed to build" $testfile 
> $srcfile \
>  proc get_first_so_ns {} {
>      set ns -1
>      gdb_test_multiple "info sharedlibrary" "get SO namespace" -lbl {
> -    -re "From\\s+To\\s+\(NS\\s+\)?Syms\\s+Read\\s+Shared Object 
> Library\r\n" {
> +    -re "\r\nFrom\\s+To\\s+\(NS\\s+\)?Syms\\s+Read\\s+Shared Object 
> Library(?=\r\n)" {
>          exp_continue
>      }
> -    -re 
> "^$::hex\\s+$::hex\\s+\\\[\\\[($::decimal)\\\]\\\]\\s+\[^\r\n]+$::binfile_lib.*" 
> {
> -        set ns $expect_out(1,string)
> -    }
> -    -re "^$::gdb_prompt $" {
> -    }
> -    -re "^\[^\r\n\]+\r\n" {
> +    -re 
> "\r\n$::hex\\s+$::hex\\s+\\\[\\\[($::decimal)\\\]\\\]\\s+\[^\r\n]+${::binfile_lib}(?=\r\n
> )" {
> +        if { $ns == -1 } {
> +        set ns $expect_out(1,string)
> +        }
>          exp_continue
>      }
> +    -re -wrap "" {
> +    }
>      }
>      return $ns
>  }
> ...
>
Ah, ok. I guess there's quite a bit I still need to learn about -lbl.

I was thinking that this could be quite a bit slower, but I noticed that 
this also fixes issues with using the lib name in the "info 
sharedlibrary" command, so I'll add this to a v2

> I also think that binfile_lib needs to be wrapped in string_to_regexp.
Will also do this for v2.

-- 
Cheers,
Guinevere Larsen
She/Her/Hers

>
> Thanks,
> - Tom
>

[PATCH v2] gdb/testsuite: fix gdb.base/dlmopen-ns-ids.exp racy test

[Guinevere Larsen]

The recently included gdb.base/dlmopen-ns-ids.exp test can sometimes
fail the call to get_integer_valueof when trying to check the namespace
ID of the fourth dlopened SO, for apparently no reason.

What's happening is that the call to get_first_so_ns doesn't necessarily
consume the GDB prompt, and so get_integer_valueof will see the prompt
immediately and not find the value the test is looking for.

To fix this, the test was changed so that we consume all of the output
of the command "info sharedlibrary", but only set the namespace ID for
the first occurence of the SO we're looking for.  The command now also
gets the solib name as a parameter, to reduce the amount of output.

Co-Authored-By: tdevries@suse.de
---
 gdb/testsuite/gdb.base/dlmopen-ns-ids.exp | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/gdb/testsuite/gdb.base/dlmopen-ns-ids.exp b/gdb/testsuite/gdb.base/dlmopen-ns-ids.exp
index 03b7a527af5..3ddc07e7773 100644
--- a/gdb/testsuite/gdb.base/dlmopen-ns-ids.exp
+++ b/gdb/testsuite/gdb.base/dlmopen-ns-ids.exp
@@ -24,7 +24,8 @@ require allow_dlmopen_tests
 standard_testfile -main.c -lib.c
 
 set srcfile_lib $srcfile2
-set binfile_lib [standard_output_file dlmopen-lib.so]
+set so_name dlmopen-lib.so
+set binfile_lib [standard_output_file $so_name]
 
 if { [build_executable "build shlib" $binfile_lib $srcfile_lib \
 	  [list debug shlib]] == -1 } {
@@ -41,18 +42,19 @@ if { [build_executable "failed to build" $testfile $srcfile \
 # for the so
 proc get_first_so_ns {} {
     set ns -1
-    gdb_test_multiple "info sharedlibrary" "get SO namespace" -lbl {
-	-re "From\\s+To\\s+\(NS\\s+\)?Syms\\s+Read\\s+Shared Object Library\r\n" {
+    set lib_regexp [string_to_regexp ${::binfile_lib}]
+    gdb_test_multiple "info sharedlibrary $::so_name" "get SO namespace" -lbl {
+	-re "\r\nFrom\\s+To\\s+\(NS\\s+\)?Syms\\s+Read\\s+Shared Object Library(?=\r\n)" {
 	    exp_continue
 	}
-	-re "^$::hex\\s+$::hex\\s+\\\[\\\[($::decimal)\\\]\\\]\\s+\[^\r\n]+$::binfile_lib.*" {
-	    set ns $expect_out(1,string)
-	}
-	-re "^$::gdb_prompt $" {
-	}
-	-re "^\[^\r\n\]+\r\n" {
+	-re "\r\n$::hex\\s+$::hex\\s+\\\[\\\[($::decimal)\\\]\\\]\\s+\[^\r\n]+${lib_regexp}(?=\r\n)" {
+	    if {$ns == -1} {
+		set ns $expect_out(1,string)
+	    }
 	    exp_continue
 	}
+	-re -wrap "" {
+	}
     }
     return $ns
 }

base-commit: d1458933830456e54223d9fc61f0d9b3a19256f5
-- 
2.49.0

Re: [PATCH v2] gdb/testsuite: fix gdb.base/dlmopen-ns-ids.exp racy test

[Tom de Vries]

On 4/10/25 15:01, Guinevere Larsen wrote:
> The recently included gdb.base/dlmopen-ns-ids.exp test can sometimes
> fail the call to get_integer_valueof when trying to check the namespace
> ID of the fourth dlopened SO, for apparently no reason.
> 
> What's happening is that the call to get_first_so_ns doesn't necessarily
> consume the GDB prompt, and so get_integer_valueof will see the prompt
> immediately and not find the value the test is looking for.
> 
> To fix this, the test was changed so that we consume all of the output
> of the command "info sharedlibrary", but only set the namespace ID for
> the first occurence of the SO we're looking for.  The command now also
> gets the solib name as a parameter, to reduce the amount of output.
> 

Hi Gwen,

LGTM.

I've also ran it through make-check-all.sh, and found no issues.

I did notice a typo, and indeed, the proposed codespell-log check 
confirmed it ( 
https://sourceware.org/pipermail/gdb-patches/2025-April/217118.html ):
...
codespell-log............................................................Passed
- hook id: codespell-log
- duration: 0.19s

codespell-log-internal...................................................Failed
- hook id: codespell
- exit code: 65

.git/COMMIT_EDITMSG:13: occurence ==> occurrence

...

OK with that fixed.

Approved-By: Tom de Vries <tdevries@suse.de>

FWIW, I don't much like the style of regexp used, I find it hard to parse.

I prefer something more spaced out and less escape-heavy, like:
...
     set re1 [list "From" "To" "(NS" ")?Syms" "Read" "Shared Object 
Library"]
     set re1 [join $re1 {\s+}]

     set re_ns [list {\[\[} ($::decimal) {\]\]}]
     set re_ns [join $re_ns ""]
     set re2 [list "$::hex" "$::hex" $re_ns "\[^\r\n]+${lib_regexp}"]
     set re2 [join $re2 {\s+}]
...

But that's a pre-exising style issue, so I suppose it's better to not 
address this in this bug-fix commit.

Thanks,
- Tom

> Co-Authored-By: tdevries@suse.de
> ---
>   gdb/testsuite/gdb.base/dlmopen-ns-ids.exp | 20 +++++++++++---------
>   1 file changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/gdb/testsuite/gdb.base/dlmopen-ns-ids.exp b/gdb/testsuite/gdb.base/dlmopen-ns-ids.exp
> index 03b7a527af5..3ddc07e7773 100644
> --- a/gdb/testsuite/gdb.base/dlmopen-ns-ids.exp
> +++ b/gdb/testsuite/gdb.base/dlmopen-ns-ids.exp
> @@ -24,7 +24,8 @@ require allow_dlmopen_tests
>   standard_testfile -main.c -lib.c
>   
>   set srcfile_lib $srcfile2
> -set binfile_lib [standard_output_file dlmopen-lib.so]
> +set so_name dlmopen-lib.so
> +set binfile_lib [standard_output_file $so_name]
>   
>   if { [build_executable "build shlib" $binfile_lib $srcfile_lib \
>   	  [list debug shlib]] == -1 } {
> @@ -41,18 +42,19 @@ if { [build_executable "failed to build" $testfile $srcfile \
>   # for the so
>   proc get_first_so_ns {} {
>       set ns -1
> -    gdb_test_multiple "info sharedlibrary" "get SO namespace" -lbl {
> -	-re "From\\s+To\\s+\(NS\\s+\)?Syms\\s+Read\\s+Shared Object Library\r\n" {
> +    set lib_regexp [string_to_regexp ${::binfile_lib}]
> +    gdb_test_multiple "info sharedlibrary $::so_name" "get SO namespace" -lbl {
> +	-re "\r\nFrom\\s+To\\s+\(NS\\s+\)?Syms\\s+Read\\s+Shared Object Library(?=\r\n)" {
>   	    exp_continue
>   	}
> -	-re "^$::hex\\s+$::hex\\s+\\\[\\\[($::decimal)\\\]\\\]\\s+\[^\r\n]+$::binfile_lib.*" {
> -	    set ns $expect_out(1,string)
> -	}
> -	-re "^$::gdb_prompt $" {
> -	}
> -	-re "^\[^\r\n\]+\r\n" {
> +	-re "\r\n$::hex\\s+$::hex\\s+\\\[\\\[($::decimal)\\\]\\\]\\s+\[^\r\n]+${lib_regexp}(?=\r\n)" {
> +	    if {$ns == -1} {
> +		set ns $expect_out(1,string)
> +	    }
>   	    exp_continue
>   	}
> +	-re -wrap "" {
> +	}
>       }
>       return $ns
>   }
> 
> base-commit: d1458933830456e54223d9fc61f0d9b3a19256f5

Re: [PATCH v2] gdb/testsuite: fix gdb.base/dlmopen-ns-ids.exp racy test

[Guinevere Larsen]

On 4/11/25 7:26 AM, Tom de Vries wrote:
> On 4/10/25 15:01, Guinevere Larsen wrote:
>> The recently included gdb.base/dlmopen-ns-ids.exp test can sometimes
>> fail the call to get_integer_valueof when trying to check the namespace
>> ID of the fourth dlopened SO, for apparently no reason.
>>
>> What's happening is that the call to get_first_so_ns doesn't necessarily
>> consume the GDB prompt, and so get_integer_valueof will see the prompt
>> immediately and not find the value the test is looking for.
>>
>> To fix this, the test was changed so that we consume all of the output
>> of the command "info sharedlibrary", but only set the namespace ID for
>> the first occurence of the SO we're looking for.  The command now also
>> gets the solib name as a parameter, to reduce the amount of output.
>>
>
> Hi Gwen,
>
> LGTM.
>
> I've also ran it through make-check-all.sh, and found no issues.
>
> I did notice a typo, and indeed, the proposed codespell-log check 
> confirmed it ( 
> https://sourceware.org/pipermail/gdb-patches/2025-April/217118.html ):
> ...
> codespell-log............................................................Passed 
>
> - hook id: codespell-log
> - duration: 0.19s
>
> codespell-log-internal...................................................Failed 
>
> - hook id: codespell
> - exit code: 65
>
> .git/COMMIT_EDITMSG:13: occurence ==> occurrence
>
> ...
>
> OK with that fixed.
>
> Approved-By: Tom de Vries <tdevries@suse.de>
Thanks, I pushed this (while also fixing the Co-Authored-By tag)
>
> FWIW, I don't much like the style of regexp used, I find it hard to 
> parse.

This makes sense. I'm also not a fan of the final regexp either, i just 
didn't know how to make it better. Considering I'm going to propose 
further changes to the test, I will try to remember to make this change 
as well

-- 
Cheers,
Guinevere Larsen
She/Her/Hers

>
> I prefer something more spaced out and less escape-heavy, like:
> ...
>     set re1 [list "From" "To" "(NS" ")?Syms" "Read" "Shared Object 
> Library"]
>     set re1 [join $re1 {\s+}]
>
>     set re_ns [list {\[\[} ($::decimal) {\]\]}]
>     set re_ns [join $re_ns ""]
>     set re2 [list "$::hex" "$::hex" $re_ns "\[^\r\n]+${lib_regexp}"]
>     set re2 [join $re2 {\s+}]
> ...
>
> But that's a pre-exising style issue, so I suppose it's better to not 
> address this in this bug-fix commit.
>
> Thanks,
> - Tom
>
>> Co-Authored-By: tdevries@suse.de
>> ---
>>   gdb/testsuite/gdb.base/dlmopen-ns-ids.exp | 20 +++++++++++---------
>>   1 file changed, 11 insertions(+), 9 deletions(-)
>>
>> diff --git a/gdb/testsuite/gdb.base/dlmopen-ns-ids.exp 
>> b/gdb/testsuite/gdb.base/dlmopen-ns-ids.exp
>> index 03b7a527af5..3ddc07e7773 100644
>> --- a/gdb/testsuite/gdb.base/dlmopen-ns-ids.exp
>> +++ b/gdb/testsuite/gdb.base/dlmopen-ns-ids.exp
>> @@ -24,7 +24,8 @@ require allow_dlmopen_tests
>>   standard_testfile -main.c -lib.c
>>     set srcfile_lib $srcfile2
>> -set binfile_lib [standard_output_file dlmopen-lib.so]
>> +set so_name dlmopen-lib.so
>> +set binfile_lib [standard_output_file $so_name]
>>     if { [build_executable "build shlib" $binfile_lib $srcfile_lib \
>>         [list debug shlib]] == -1 } {
>> @@ -41,18 +42,19 @@ if { [build_executable "failed to build" 
>> $testfile $srcfile \
>>   # for the so
>>   proc get_first_so_ns {} {
>>       set ns -1
>> -    gdb_test_multiple "info sharedlibrary" "get SO namespace" -lbl {
>> -    -re "From\\s+To\\s+\(NS\\s+\)?Syms\\s+Read\\s+Shared Object 
>> Library\r\n" {
>> +    set lib_regexp [string_to_regexp ${::binfile_lib}]
>> +    gdb_test_multiple "info sharedlibrary $::so_name" "get SO 
>> namespace" -lbl {
>> +    -re "\r\nFrom\\s+To\\s+\(NS\\s+\)?Syms\\s+Read\\s+Shared Object 
>> Library(?=\r\n)" {
>>           exp_continue
>>       }
>> -    -re 
>> "^$::hex\\s+$::hex\\s+\\\[\\\[($::decimal)\\\]\\\]\\s+\[^\r\n]+$::binfile_lib.*" 
>> {
>> -        set ns $expect_out(1,string)
>> -    }
>> -    -re "^$::gdb_prompt $" {
>> -    }
>> -    -re "^\[^\r\n\]+\r\n" {
>> +    -re 
>> "\r\n$::hex\\s+$::hex\\s+\\\[\\\[($::decimal)\\\]\\\]\\s+\[^\r\n]+${lib_regexp}(?=\r\n)" 
>> {
>> +        if {$ns == -1} {
>> +        set ns $expect_out(1,string)
>> +        }
>>           exp_continue
>>       }
>> +    -re -wrap "" {
>> +    }
>>       }
>>       return $ns
>>   }
>>
>> base-commit: d1458933830456e54223d9fc61f0d9b3a19256f5
>