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

[Guinevere Larsen <guinevere@redhat.com>]

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
>