Re: [PATCH 3/3] gdb/amd-dbgapi: add displaced stepping support

[Simon Marchi <simark@simark.ca>]

On 1/15/25 1:45 PM, Simon Marchi wrote:
>>> diff --git a/gdb/testsuite/gdb.rocm/displaced-stepping.exp b/gdb/testsuite/gdb.rocm/displaced-stepping.exp
>>> new file mode 100644
>>> index 000000000000..6b26e1adfd7a
>>> --- /dev/null
>>> +++ b/gdb/testsuite/gdb.rocm/displaced-stepping.exp
>>> @@ -0,0 +1,54 @@
>>> +# Copyright 2024 Free Software Foundation, Inc.
>>
>> Should probably 2025.
> 
> Fixed.
> 
>>> +
>>> +# This program is free software; you can redistribute it and/or modify
>>> +# it under the terms of the GNU General Public License as published by
>>> +# the Free Software Foundation; either version 3 of the License, or
>>> +# (at your option) any later version.
>>> +#
>>> +# This program is distributed in the hope that it will be useful,
>>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> +# GNU General Public License for more details.
>>> +#
>>> +# You should have received a copy of the GNU General Public License
>>> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
>>> +
>>> +# Test displaced stepping on AMD GPUs.
>>> +
>>> +load_lib rocm.exp
>>> +
>>> +standard_testfile simple.cpp
>>
>> I though the usage in GDB testsuite was to not share the .cpp across multiple testcases.  Should there be a displaced-stepping.cpp?
> 
> For "hello world" style programs (which this one is) I don't really
> mind.  But I will add displaced-stepping.cpp.
> 
>>
>>> +
>>> +require allow_hipcc_tests
>>> +
>>> +# Since GDB doesn't yet understand DWARF expressions generated by the HIP
>>> +# compiler, purposefully generate the binary without debug info.
>>> +if {[build_executable "failed to prepare" $testfile $srcfile {hip}]} {
>>> +    return
>>> +}
>>> +
>>> +proc do_test {} {
>>> +    clean_restart $::binfile
>>> +
>>> +    with_rocm_gpu_lock {
>>> +       if ![runto_main] {
>>> +           return
>>> +       }
>>> +
>>> +       gdb_test "with breakpoint pending on -- break do_an_addition" \
>>> +           "Breakpoint $::decimal \\(do_an_addition\\) pending."
>>> +
>>> +       gdb_test "continue" \
>>> +           "Thread $::decimal hit Breakpoint $::decimal, $::hex in do_an_addition.*"
>>> +
>>> +       gdb_test_no_output "set debug displaced on"
>>> +
>>> +       gdb_test "stepi" "displaced_step_prepare_throw: prepared successfully.*$::hex in do_an_addition.*"
>>
>> Just to reduce the noise in the gdb.log file when running the continue, this could be
>>
>> gdb_test "with debug displaced on -- stepi" ...
> 
> Good idea, I never remember to use "with".  Done.
> 
>>
>>> +
>>> +       gdb_test "continue" \
>>> +           "Inferior 1 .* exited normally.*" \
>>> +           "continue to end"
>>> +    }
>>> +}
>>> +
>>> +do_test
>>> -- 
>>> 2.47.1
>>>
>>
>> With those minor comments addressed, and given that it only touches amdgpu related code,
>> Approved-By: Lancelot Six <lancelot.six@amd.com> (amdgpu)
> 
> Thanks, added.
> 
> Simon

I pushed the series.

Simon