* [PATCH 0/5] Fix some Python Inferior methods @ 2023-07-07 15:07 Tom Tromey 2023-07-07 15:07 ` [PATCH 1/5] Minor cleanups in py-inferior.exp Tom Tromey ` (4 more replies) 0 siblings, 5 replies; 14+ messages in thread From: Tom Tromey @ 2023-07-07 15:07 UTC (permalink / raw) To: gdb-patches A user pointed out a bug in Inferior.search_memory. This series is the result. Most of the patches are minor things I noticed while working on the final patch, which fixes the bug. Regression tested on x86-64 Fedora 36. --- Tom Tromey (5): Minor cleanups in py-inferior.exp Refactor py-inferior.exp Rename Python variable in py-inferior.exp Remove obsolete comment from gdbthread.h Use correct inferior in Inferior.read_memory et al gdb/gdbthread.h | 2 +- gdb/python/py-inferior.c | 36 +++++++++++++-- gdb/testsuite/gdb.python/py-inferior.exp | 79 ++++++++++++++++++++++++-------- 3 files changed, 92 insertions(+), 25 deletions(-) --- base-commit: 13f5f57e0d2fb3e06e15c57d67a40499a5910ba6 change-id: 20230707-py-inf-fixes-30615-668ef09475ab Best regards, -- Tom Tromey <tromey@adacore.com> ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/5] Minor cleanups in py-inferior.exp 2023-07-07 15:07 [PATCH 0/5] Fix some Python Inferior methods Tom Tromey @ 2023-07-07 15:07 ` Tom Tromey 2023-07-07 18:06 ` Pedro Alves 2023-07-07 15:07 ` [PATCH 2/5] Refactor py-inferior.exp Tom Tromey ` (3 subsequent siblings) 4 siblings, 1 reply; 14+ messages in thread From: Tom Tromey @ 2023-07-07 15:07 UTC (permalink / raw) To: gdb-patches While working on this series, I noticed a couple of oddities in py-inferior.exp. One is an obviously incorrect comment, and the other is a confusing test name. This patch fixes these. --- gdb/testsuite/gdb.python/py-inferior.exp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/gdb/testsuite/gdb.python/py-inferior.exp b/gdb/testsuite/gdb.python/py-inferior.exp index 58deece5d5f..33bc27ee86d 100644 --- a/gdb/testsuite/gdb.python/py-inferior.exp +++ b/gdb/testsuite/gdb.python/py-inferior.exp @@ -206,8 +206,7 @@ if [isnative] { } } -# Test Inferior is_valid. This must always be the last test in -# this testcase as it kills the inferior. +# Test Inferior is_valid. with_test_prefix "is_valid" { gdb_py_test_silent_cmd "python inf_list = gdb.inferiors()" "get initial list" 1 @@ -247,7 +246,7 @@ with_test_prefix "is_valid" { gdb_test "python print (inf_list\[1\].is_valid())" "True" \ "check inferior validity 3" - gdb_test_no_output "remove-inferiors 2" "remove-inferiors 3" + gdb_test_no_output "remove-inferiors 2" "remove-inferiors 2" gdb_test "python print (inf_list\[0\].is_valid())" "True" \ "check inferior validity 4" -- 2.40.1 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/5] Minor cleanups in py-inferior.exp 2023-07-07 15:07 ` [PATCH 1/5] Minor cleanups in py-inferior.exp Tom Tromey @ 2023-07-07 18:06 ` Pedro Alves 2023-07-11 15:05 ` Tom Tromey 0 siblings, 1 reply; 14+ messages in thread From: Pedro Alves @ 2023-07-07 18:06 UTC (permalink / raw) To: Tom Tromey, gdb-patches On 2023-07-07 16:07, Tom Tromey via Gdb-patches wrote: > @@ -247,7 +246,7 @@ with_test_prefix "is_valid" { > gdb_test "python print (inf_list\[1\].is_valid())" "True" \ > "check inferior validity 3" > > - gdb_test_no_output "remove-inferiors 2" "remove-inferiors 3" > + gdb_test_no_output "remove-inferiors 2" "remove-inferiors 2" You could just remove the second argument completely, it defaults to the command string. > gdb_test "python print (inf_list\[0\].is_valid())" "True" \ > "check inferior validity 4" There's also: gdb_test_no_output "remove-inferiors 3" "remove second inferior" ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/5] Minor cleanups in py-inferior.exp 2023-07-07 18:06 ` Pedro Alves @ 2023-07-11 15:05 ` Tom Tromey 0 siblings, 0 replies; 14+ messages in thread From: Tom Tromey @ 2023-07-11 15:05 UTC (permalink / raw) To: Pedro Alves; +Cc: Tom Tromey, gdb-patches >>>>> "Pedro" == Pedro Alves <pedro@palves.net> writes: Pedro> On 2023-07-07 16:07, Tom Tromey via Gdb-patches wrote: >> @@ -247,7 +246,7 @@ with_test_prefix "is_valid" { >> gdb_test "python print (inf_list\[1\].is_valid())" "True" \ >> "check inferior validity 3" >> >> - gdb_test_no_output "remove-inferiors 2" "remove-inferiors 3" >> + gdb_test_no_output "remove-inferiors 2" "remove-inferiors 2" Pedro> You could just remove the second argument completely, it defaults to the Pedro> command string. >> gdb_test "python print (inf_list\[0\].is_valid())" "True" \ >> "check inferior validity 4" Pedro> There's also: Pedro> gdb_test_no_output "remove-inferiors 3" "remove second inferior" I made both these changes for v2. Tom ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/5] Refactor py-inferior.exp 2023-07-07 15:07 [PATCH 0/5] Fix some Python Inferior methods Tom Tromey 2023-07-07 15:07 ` [PATCH 1/5] Minor cleanups in py-inferior.exp Tom Tromey @ 2023-07-07 15:07 ` Tom Tromey 2023-07-07 18:12 ` Pedro Alves 2023-07-07 15:07 ` [PATCH 3/5] Rename Python variable in py-inferior.exp Tom Tromey ` (2 subsequent siblings) 4 siblings, 1 reply; 14+ messages in thread From: Tom Tromey @ 2023-07-07 15:07 UTC (permalink / raw) To: gdb-patches This changes py-inferior.exp to make it a bit more robust when adding new inferiors during the course of the test. --- gdb/testsuite/gdb.python/py-inferior.exp | 43 +++++++++++++++++++++----------- 1 file changed, 29 insertions(+), 14 deletions(-) diff --git a/gdb/testsuite/gdb.python/py-inferior.exp b/gdb/testsuite/gdb.python/py-inferior.exp index 33bc27ee86d..a29624f4fd5 100644 --- a/gdb/testsuite/gdb.python/py-inferior.exp +++ b/gdb/testsuite/gdb.python/py-inferior.exp @@ -40,6 +40,20 @@ if {![runto_main]} { return 0 } +# The most recently added inferior. +set most_recent_inf 1 + +# A helper function that adds a new inferior. It returns the expected +# number of the new inferior. ARG is a string to pass to +# add-inferior. +proc add_inferior {{arg ""}} { + global most_recent_inf + incr most_recent_inf + gdb_test "add-inferior $arg" "Added inferior $most_recent_inf.*" \ + "add inferior $most_recent_inf" + return $most_recent_inf +} + # Test basic gdb.Inferior attributes and methods. gdb_py_test_silent_cmd "python inferiors = gdb.inferiors ()" "get inferiors list" 1 @@ -234,7 +248,7 @@ with_test_prefix "is_valid" { "gdb.events.inferior_deleted.connect(del_inf_handler)" "" \ "end" "" - gdb_test "add-inferior" "Added inferior 2.*" "add empty inferior 2" + set num [add_inferior] gdb_py_test_silent_cmd "python inf_list = gdb.inferiors()" "get new list" 1 gdb_test "python print (len(inf_list))" "2" "get inferior list length 2" gdb_test "python print (inf_list\[0\].is_valid())" "True" \ @@ -246,7 +260,7 @@ with_test_prefix "is_valid" { gdb_test "python print (inf_list\[1\].is_valid())" "True" \ "check inferior validity 3" - gdb_test_no_output "remove-inferiors 2" "remove-inferiors 2" + gdb_test_no_output "remove-inferiors $num" "remove-inferiors $num" gdb_test "python print (inf_list\[0\].is_valid())" "True" \ "check inferior validity 4" @@ -287,15 +301,16 @@ with_test_prefix "selected_inferior" { # Figure out if inf 1 has a native target. set inf_1_is_native [gdb_is_target_native] - gdb_test "add-inferior -no-connection" "Added inferior 3" "create new inferior" - gdb_test "inferior 3" ".*" "switch to third inferior" - gdb_test "py print (gdb.selected_inferior().num)" "3" "third inferior selected" + set num [add_inferior "-no-connection"] + gdb_test "inferior $num" ".*" "switch to inferior $num" + gdb_test "py print (gdb.selected_inferior().num)" "$num" \ + "inferior $num selected" gdb_test "py print (gdb.selected_inferior().connection_num)" "None" \ - "third inferior's None connection number" + "inferior $num's None connection number" gdb_test "py print (gdb.selected_inferior().connection)" "None" \ - "third inferior's None connection" + "inferior $num's None connection" gdb_test "target native" "Done. Use the \"run\" command to start a process." \ - "target for the third inferior" + "target for inferior $num" # If inf 1 has a native target, inf 3's target is shared with 1's. # Otherwise, it must have created a new target with a new number. @@ -306,10 +321,10 @@ with_test_prefix "selected_inferior" { } gdb_test "py print (gdb.selected_inferior().connection_num)" \ "$expected_connection_num" \ - "third inferior's native connection number" + "inferior $num's native connection number" gdb_test "py print (gdb.selected_inferior().connection.num)" \ "$expected_connection_num" \ - "third inferior's native connection number, though connection object" + "inferior $num's native connection number, though connection object" # Test printing of gdb.TargetConnection object. gdb_test "py print (gdb.selected_inferior().connection)" \ @@ -317,18 +332,18 @@ with_test_prefix "selected_inferior" { "print a connection object" gdb_test "inferior 1" ".*" "switch back to first inferior" - gdb_test_no_output "remove-inferiors 3" "remove second inferior" + gdb_test_no_output "remove-inferiors $num" "remove inferior $num" } # Test repr()/str() with_test_prefix "__repr__" { - gdb_test "add-inferior" "Added inferior 4 on connection .*" "add inferior 4" + set num [add_inferior] gdb_py_test_silent_cmd "python infs = gdb.inferiors()" "get inferior list" 1 gdb_test "python print (infs\[0\])" "<gdb.Inferior num=1, pid=$decimal>" gdb_test "python print (infs)" \ - "\\\(<gdb.Inferior num=1, pid=$decimal>, <gdb.Inferior num=4, pid=$decimal>\\\)" \ + "\\\(<gdb.Inferior num=1, pid=$decimal>, <gdb.Inferior num=$num, pid=$decimal>\\\)" \ "print all inferiors 1" - gdb_test_no_output "remove-inferiors 4" + gdb_test_no_output "remove-inferiors $num" gdb_test "python print (infs)" \ "\\\(<gdb.Inferior num=1, pid=$decimal>, <gdb.Inferior \\\(invalid\\\)>\\\)" \ "print all inferiors 2" -- 2.40.1 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/5] Refactor py-inferior.exp 2023-07-07 15:07 ` [PATCH 2/5] Refactor py-inferior.exp Tom Tromey @ 2023-07-07 18:12 ` Pedro Alves 0 siblings, 0 replies; 14+ messages in thread From: Pedro Alves @ 2023-07-07 18:12 UTC (permalink / raw) To: Tom Tromey, gdb-patches On 2023-07-07 16:07, Tom Tromey via Gdb-patches wrote: > This changes py-inferior.exp to make it a bit more robust when adding > new inferiors during the course of the test. Ah, this fixes the "There's also:" case I pointed out in patch #1 anyhow. Approved-By: Pedro Alves <pedro@palves.net> Pedro Alves > --- > gdb/testsuite/gdb.python/py-inferior.exp | 43 +++++++++++++++++++++----------- > 1 file changed, 29 insertions(+), 14 deletions(-) > > diff --git a/gdb/testsuite/gdb.python/py-inferior.exp b/gdb/testsuite/gdb.python/py-inferior.exp > index 33bc27ee86d..a29624f4fd5 100644 > --- a/gdb/testsuite/gdb.python/py-inferior.exp > +++ b/gdb/testsuite/gdb.python/py-inferior.exp > @@ -40,6 +40,20 @@ if {![runto_main]} { > return 0 > } > > +# The most recently added inferior. > +set most_recent_inf 1 > + > +# A helper function that adds a new inferior. It returns the expected > +# number of the new inferior. ARG is a string to pass to > +# add-inferior. > +proc add_inferior {{arg ""}} { > + global most_recent_inf > + incr most_recent_inf > + gdb_test "add-inferior $arg" "Added inferior $most_recent_inf.*" \ > + "add inferior $most_recent_inf" > + return $most_recent_inf > +} > + > # Test basic gdb.Inferior attributes and methods. > > gdb_py_test_silent_cmd "python inferiors = gdb.inferiors ()" "get inferiors list" 1 > @@ -234,7 +248,7 @@ with_test_prefix "is_valid" { > "gdb.events.inferior_deleted.connect(del_inf_handler)" "" \ > "end" "" > > - gdb_test "add-inferior" "Added inferior 2.*" "add empty inferior 2" > + set num [add_inferior] > gdb_py_test_silent_cmd "python inf_list = gdb.inferiors()" "get new list" 1 > gdb_test "python print (len(inf_list))" "2" "get inferior list length 2" > gdb_test "python print (inf_list\[0\].is_valid())" "True" \ > @@ -246,7 +260,7 @@ with_test_prefix "is_valid" { > gdb_test "python print (inf_list\[1\].is_valid())" "True" \ > "check inferior validity 3" > > - gdb_test_no_output "remove-inferiors 2" "remove-inferiors 2" > + gdb_test_no_output "remove-inferiors $num" "remove-inferiors $num" > gdb_test "python print (inf_list\[0\].is_valid())" "True" \ > "check inferior validity 4" > > @@ -287,15 +301,16 @@ with_test_prefix "selected_inferior" { > # Figure out if inf 1 has a native target. > set inf_1_is_native [gdb_is_target_native] > > - gdb_test "add-inferior -no-connection" "Added inferior 3" "create new inferior" > - gdb_test "inferior 3" ".*" "switch to third inferior" > - gdb_test "py print (gdb.selected_inferior().num)" "3" "third inferior selected" > + set num [add_inferior "-no-connection"] > + gdb_test "inferior $num" ".*" "switch to inferior $num" > + gdb_test "py print (gdb.selected_inferior().num)" "$num" \ > + "inferior $num selected" > gdb_test "py print (gdb.selected_inferior().connection_num)" "None" \ > - "third inferior's None connection number" > + "inferior $num's None connection number" > gdb_test "py print (gdb.selected_inferior().connection)" "None" \ > - "third inferior's None connection" > + "inferior $num's None connection" > gdb_test "target native" "Done. Use the \"run\" command to start a process." \ > - "target for the third inferior" > + "target for inferior $num" > > # If inf 1 has a native target, inf 3's target is shared with 1's. > # Otherwise, it must have created a new target with a new number. > @@ -306,10 +321,10 @@ with_test_prefix "selected_inferior" { > } > gdb_test "py print (gdb.selected_inferior().connection_num)" \ > "$expected_connection_num" \ > - "third inferior's native connection number" > + "inferior $num's native connection number" > gdb_test "py print (gdb.selected_inferior().connection.num)" \ > "$expected_connection_num" \ > - "third inferior's native connection number, though connection object" > + "inferior $num's native connection number, though connection object" > > # Test printing of gdb.TargetConnection object. > gdb_test "py print (gdb.selected_inferior().connection)" \ > @@ -317,18 +332,18 @@ with_test_prefix "selected_inferior" { > "print a connection object" > > gdb_test "inferior 1" ".*" "switch back to first inferior" > - gdb_test_no_output "remove-inferiors 3" "remove second inferior" > + gdb_test_no_output "remove-inferiors $num" "remove inferior $num" > } > > # Test repr()/str() > with_test_prefix "__repr__" { > - gdb_test "add-inferior" "Added inferior 4 on connection .*" "add inferior 4" > + set num [add_inferior] > gdb_py_test_silent_cmd "python infs = gdb.inferiors()" "get inferior list" 1 > gdb_test "python print (infs\[0\])" "<gdb.Inferior num=1, pid=$decimal>" > gdb_test "python print (infs)" \ > - "\\\(<gdb.Inferior num=1, pid=$decimal>, <gdb.Inferior num=4, pid=$decimal>\\\)" \ > + "\\\(<gdb.Inferior num=1, pid=$decimal>, <gdb.Inferior num=$num, pid=$decimal>\\\)" \ > "print all inferiors 1" > - gdb_test_no_output "remove-inferiors 4" > + gdb_test_no_output "remove-inferiors $num" > gdb_test "python print (infs)" \ > "\\\(<gdb.Inferior num=1, pid=$decimal>, <gdb.Inferior \\\(invalid\\\)>\\\)" \ > "print all inferiors 2" > ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 3/5] Rename Python variable in py-inferior.exp 2023-07-07 15:07 [PATCH 0/5] Fix some Python Inferior methods Tom Tromey 2023-07-07 15:07 ` [PATCH 1/5] Minor cleanups in py-inferior.exp Tom Tromey 2023-07-07 15:07 ` [PATCH 2/5] Refactor py-inferior.exp Tom Tromey @ 2023-07-07 15:07 ` Tom Tromey 2023-07-07 18:14 ` Pedro Alves 2023-07-07 15:08 ` [PATCH 4/5] Remove obsolete comment from gdbthread.h Tom Tromey 2023-07-07 15:08 ` [PATCH 5/5] Use correct inferior in Inferior.read_memory et al Tom Tromey 4 siblings, 1 reply; 14+ messages in thread From: Tom Tromey @ 2023-07-07 15:07 UTC (permalink / raw) To: gdb-patches py-inferior.exp creates a Python variable named 'str'. This clashes with the built-in type of the same name and can be confusing when trying to evaluate Python code when debugging the test case. This patch renames it. --- gdb/testsuite/gdb.python/py-inferior.exp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/gdb/testsuite/gdb.python/py-inferior.exp b/gdb/testsuite/gdb.python/py-inferior.exp index a29624f4fd5..41e26878031 100644 --- a/gdb/testsuite/gdb.python/py-inferior.exp +++ b/gdb/testsuite/gdb.python/py-inferior.exp @@ -88,13 +88,13 @@ gdb_continue_to_breakpoint "cont to Break here." ".*Break here\..*" gdb_py_test_silent_cmd "python addr = gdb.selected_frame ().read_var ('str')" \ "read str address" 0 -gdb_py_test_silent_cmd "python str = gdb.inferiors()\[0\].read_memory (addr, 5); print(str)" \ +gdb_py_test_silent_cmd "python astr = gdb.inferiors()\[0\].read_memory (addr, 5); print(str)" \ "read str contents" 1 gdb_py_test_silent_cmd "python a = bytes('a', 'ascii')" "" 0 -gdb_py_test_silent_cmd "python str\[1\] = a" "change str" 0 -gdb_py_test_silent_cmd "python gdb.inferiors()\[0\].write_memory (addr, str)" \ +gdb_py_test_silent_cmd "python astr\[1\] = a" "change str" 0 +gdb_py_test_silent_cmd "python gdb.inferiors()\[0\].write_memory (addr, astr)" \ "write str" 1 -gdb_test "print (str)" " = \"hallo, testsuite\"" \ +gdb_test "print str" " = \"hallo, testsuite\"" \ "ensure str was changed in the inferior" # Test memory search. -- 2.40.1 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/5] Rename Python variable in py-inferior.exp 2023-07-07 15:07 ` [PATCH 3/5] Rename Python variable in py-inferior.exp Tom Tromey @ 2023-07-07 18:14 ` Pedro Alves 2023-07-11 15:19 ` Tom Tromey 0 siblings, 1 reply; 14+ messages in thread From: Pedro Alves @ 2023-07-07 18:14 UTC (permalink / raw) To: Tom Tromey, gdb-patches On 2023-07-07 16:07, Tom Tromey via Gdb-patches wrote: > py-inferior.exp creates a Python variable named 'str'. This clashes > with the built-in type of the same name and can be confusing when > trying to evaluate Python code when debugging the test case. This > patch renames it. > --- > gdb/testsuite/gdb.python/py-inferior.exp | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/gdb/testsuite/gdb.python/py-inferior.exp b/gdb/testsuite/gdb.python/py-inferior.exp > index a29624f4fd5..41e26878031 100644 > --- a/gdb/testsuite/gdb.python/py-inferior.exp > +++ b/gdb/testsuite/gdb.python/py-inferior.exp > @@ -88,13 +88,13 @@ gdb_continue_to_breakpoint "cont to Break here." ".*Break here\..*" > > gdb_py_test_silent_cmd "python addr = gdb.selected_frame ().read_var ('str')" \ > "read str address" 0 > -gdb_py_test_silent_cmd "python str = gdb.inferiors()\[0\].read_memory (addr, 5); print(str)" \ > +gdb_py_test_silent_cmd "python astr = gdb.inferiors()\[0\].read_memory (addr, 5); print(str)" \ Shouldn't the "print(str)" after the ; be adjusted as well? How can this pass as is? > "read str contents" 1 > gdb_py_test_silent_cmd "python a = bytes('a', 'ascii')" "" 0 > -gdb_py_test_silent_cmd "python str\[1\] = a" "change str" 0 > -gdb_py_test_silent_cmd "python gdb.inferiors()\[0\].write_memory (addr, str)" \ > +gdb_py_test_silent_cmd "python astr\[1\] = a" "change str" 0 > +gdb_py_test_silent_cmd "python gdb.inferiors()\[0\].write_memory (addr, astr)" \ > "write str" 1 > -gdb_test "print (str)" " = \"hallo, testsuite\"" \ > +gdb_test "print str" " = \"hallo, testsuite\"" \ > "ensure str was changed in the inferior" > > # Test memory search. > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/5] Rename Python variable in py-inferior.exp 2023-07-07 18:14 ` Pedro Alves @ 2023-07-11 15:19 ` Tom Tromey 0 siblings, 0 replies; 14+ messages in thread From: Tom Tromey @ 2023-07-11 15:19 UTC (permalink / raw) To: Pedro Alves; +Cc: Tom Tromey, gdb-patches >>>>> "Pedro" == Pedro Alves <pedro@palves.net> writes: >> +gdb_py_test_silent_cmd "python astr = gdb.inferiors()\[0\].read_memory (addr, 5); print(str)" \ Pedro> Shouldn't the "print(str)" after the ; be adjusted as well? How can this pass as is? Yes, it should be. It passes because gdb_py_test_silent_cmd doesn't seem to check the output beyond looking for an exception, and 'str' is a built-in type, so this passes. I changed it not to use gdb_test and to look for <memory ...> in v2. Tom ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 4/5] Remove obsolete comment from gdbthread.h 2023-07-07 15:07 [PATCH 0/5] Fix some Python Inferior methods Tom Tromey ` (2 preceding siblings ...) 2023-07-07 15:07 ` [PATCH 3/5] Rename Python variable in py-inferior.exp Tom Tromey @ 2023-07-07 15:08 ` Tom Tromey 2023-07-07 18:15 ` Pedro Alves 2023-07-07 15:08 ` [PATCH 5/5] Use correct inferior in Inferior.read_memory et al Tom Tromey 4 siblings, 1 reply; 14+ messages in thread From: Tom Tromey @ 2023-07-07 15:08 UTC (permalink / raw) To: gdb-patches A comment in gdbthread.h refers to a global that no longer exists. --- gdb/gdbthread.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h index 7135515bf45..d294be6762b 100644 --- a/gdb/gdbthread.h +++ b/gdb/gdbthread.h @@ -764,7 +764,7 @@ extern int thread_count (process_stratum_target *proc_target); /* Return true if we have any thread in any inferior. */ extern bool any_thread_p (); -/* Switch context to thread THR. Also sets the STOP_PC global. */ +/* Switch context to thread THR. */ extern void switch_to_thread (struct thread_info *thr); /* Switch context to no thread selected. */ -- 2.40.1 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 4/5] Remove obsolete comment from gdbthread.h 2023-07-07 15:08 ` [PATCH 4/5] Remove obsolete comment from gdbthread.h Tom Tromey @ 2023-07-07 18:15 ` Pedro Alves 0 siblings, 0 replies; 14+ messages in thread From: Pedro Alves @ 2023-07-07 18:15 UTC (permalink / raw) To: Tom Tromey, gdb-patches On 2023-07-07 16:08, Tom Tromey via Gdb-patches wrote: > A comment in gdbthread.h refers to a global that no longer exists. Approved-By: Pedro Alves <pedro@palves.net> Thanks, Pedro Alves > --- > gdb/gdbthread.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h > index 7135515bf45..d294be6762b 100644 > --- a/gdb/gdbthread.h > +++ b/gdb/gdbthread.h > @@ -764,7 +764,7 @@ extern int thread_count (process_stratum_target *proc_target); > /* Return true if we have any thread in any inferior. */ > extern bool any_thread_p (); > > -/* Switch context to thread THR. Also sets the STOP_PC global. */ > +/* Switch context to thread THR. */ > extern void switch_to_thread (struct thread_info *thr); > > /* Switch context to no thread selected. */ > ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 5/5] Use correct inferior in Inferior.read_memory et al 2023-07-07 15:07 [PATCH 0/5] Fix some Python Inferior methods Tom Tromey ` (3 preceding siblings ...) 2023-07-07 15:08 ` [PATCH 4/5] Remove obsolete comment from gdbthread.h Tom Tromey @ 2023-07-07 15:08 ` Tom Tromey 2023-07-07 18:26 ` Pedro Alves 4 siblings, 1 reply; 14+ messages in thread From: Tom Tromey @ 2023-07-07 15:08 UTC (permalink / raw) To: gdb-patches A user noticed that Inferior.read_memory and a few other Python APIs will always use the currently selected inferior, not the one passed to the call. This patch fixes the bug by arranging to switch to the inferior. I found this same issue in several APIs, so this fixes them all. I also found out that setting current_inferior isn't enough when reading memory -- one must also set inferior_ptid. This seems very confusing to me, especially considering that current_inferior must be set properly anyway in order to access the target stack. Finally, I also added a few missing calls to INFPY_REQUIRE_VALID to these methods. Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30615 --- gdb/python/py-inferior.c | 36 ++++++++++++++++++++++++++++---- gdb/testsuite/gdb.python/py-inferior.exp | 25 ++++++++++++++++++++++ 2 files changed, 57 insertions(+), 4 deletions(-) diff --git a/gdb/python/py-inferior.c b/gdb/python/py-inferior.c index af8bd8855a3..7fb422f621e 100644 --- a/gdb/python/py-inferior.c +++ b/gdb/python/py-inferior.c @@ -30,6 +30,7 @@ #include "gdbsupport/gdb_signals.h" #include "py-event.h" #include "py-stopevent.h" +#include "progspace-and-thread.h" #include <unordered_map> using thread_map_t @@ -528,11 +529,14 @@ gdbpy_inferiors (PyObject *unused, PyObject *unused2) static PyObject * infpy_read_memory (PyObject *self, PyObject *args, PyObject *kw) { + inferior_object *inf = (inferior_object *) self; CORE_ADDR addr, length; gdb::unique_xmalloc_ptr<gdb_byte> buffer; PyObject *addr_obj, *length_obj; static const char *keywords[] = { "address", "length", NULL }; + INFPY_REQUIRE_VALID (inf); + if (!gdb_PyArg_ParseTupleAndKeywords (args, kw, "OO", keywords, &addr_obj, &length_obj)) return NULL; @@ -543,6 +547,12 @@ infpy_read_memory (PyObject *self, PyObject *args, PyObject *kw) try { + scoped_restore_current_inferior restore_inferior; + scoped_restore_current_pspace_and_thread restore_thread; + + thread_info *thr = any_thread_of_inferior (inf->inferior); + switch_to_thread_no_regs (thr); + buffer.reset ((gdb_byte *) xmalloc (length)); read_memory (addr, buffer.get (), length); @@ -565,6 +575,7 @@ infpy_read_memory (PyObject *self, PyObject *args, PyObject *kw) static PyObject * infpy_write_memory (PyObject *self, PyObject *args, PyObject *kw) { + inferior_object *inf = (inferior_object *) self; struct gdb_exception except; Py_ssize_t buf_len; const gdb_byte *buffer; @@ -573,6 +584,8 @@ infpy_write_memory (PyObject *self, PyObject *args, PyObject *kw) static const char *keywords[] = { "address", "buffer", "length", NULL }; Py_buffer pybuf; + INFPY_REQUIRE_VALID (inf); + if (!gdb_PyArg_ParseTupleAndKeywords (args, kw, "Os*|O", keywords, &addr_obj, &pybuf, &length_obj)) return NULL; @@ -591,6 +604,12 @@ infpy_write_memory (PyObject *self, PyObject *args, PyObject *kw) try { + scoped_restore_current_inferior restore_inferior; + scoped_restore_current_pspace_and_thread restore_thread; + + thread_info *thr = any_thread_of_inferior (inf->inferior); + switch_to_thread_no_regs (thr); + write_memory_with_notification (addr, buffer, length); } catch (gdb_exception &ex) @@ -604,7 +623,7 @@ infpy_write_memory (PyObject *self, PyObject *args, PyObject *kw) } /* Implementation of - gdb.search_memory (address, length, pattern). ADDRESS is the + Inferior.search_memory (address, length, pattern). ADDRESS is the address to start the search. LENGTH specifies the scope of the search from ADDRESS. PATTERN is the pattern to search for (and must be a Python object supporting the buffer protocol). @@ -614,6 +633,7 @@ infpy_write_memory (PyObject *self, PyObject *args, PyObject *kw) static PyObject * infpy_search_memory (PyObject *self, PyObject *args, PyObject *kw) { + inferior_object *inf = (inferior_object *) self; struct gdb_exception except; CORE_ADDR start_addr, length; static const char *keywords[] = { "address", "length", "pattern", NULL }; @@ -624,6 +644,8 @@ infpy_search_memory (PyObject *self, PyObject *args, PyObject *kw) int found = 0; Py_buffer pybuf; + INFPY_REQUIRE_VALID (inf); + if (!gdb_PyArg_ParseTupleAndKeywords (args, kw, "OOs*", keywords, &start_addr_obj, &length_obj, &pybuf)) @@ -656,6 +678,12 @@ infpy_search_memory (PyObject *self, PyObject *args, PyObject *kw) try { + scoped_restore_current_inferior restore_inferior; + scoped_restore_current_pspace_and_thread restore_thread; + + thread_info *thr = any_thread_of_inferior (inf->inferior); + switch_to_thread_no_regs (thr); + found = target_search_memory (start_addr, length, buffer, pattern_size, &found_addr); @@ -912,10 +940,10 @@ infpy_get_main_name (PyObject *self, void *closure) /* This is unfortunate but the implementation of main_name can reach into memory. */ scoped_restore_current_inferior restore_inferior; - set_current_inferior (inf->inferior); + scoped_restore_current_pspace_and_thread restore_thread; - scoped_restore_current_program_space restore_current_progspace; - set_current_program_space (inf->inferior->pspace); + thread_info *thr = any_thread_of_inferior (inf->inferior); + switch_to_thread_no_regs (thr); name = main_name (); } diff --git a/gdb/testsuite/gdb.python/py-inferior.exp b/gdb/testsuite/gdb.python/py-inferior.exp index 41e26878031..9b0643d63e2 100644 --- a/gdb/testsuite/gdb.python/py-inferior.exp +++ b/gdb/testsuite/gdb.python/py-inferior.exp @@ -90,6 +90,7 @@ gdb_py_test_silent_cmd "python addr = gdb.selected_frame ().read_var ('str')" \ "read str address" 0 gdb_py_test_silent_cmd "python astr = gdb.inferiors()\[0\].read_memory (addr, 5); print(str)" \ "read str contents" 1 +gdb_test "python print(astr\[0\])" .* gdb_py_test_silent_cmd "python a = bytes('a', 'ascii')" "" 0 gdb_py_test_silent_cmd "python astr\[1\] = a" "change str" 0 gdb_py_test_silent_cmd "python gdb.inferiors()\[0\].write_memory (addr, astr)" \ @@ -97,6 +98,10 @@ gdb_py_test_silent_cmd "python gdb.inferiors()\[0\].write_memory (addr, astr)" \ gdb_test "print str" " = \"hallo, testsuite\"" \ "ensure str was changed in the inferior" +# Add a new inferior here, so we can test that operations work on the +# correct inferior. +set num [add_inferior] + # Test memory search. set hex_number {0x[0-9a-fA-F][0-9a-fA-F]*} @@ -114,6 +119,9 @@ with_test_prefix "string" { gdb_test_no_output "py start_addr = search_buf.address" gdb_test_no_output "py length = search_buf.type.sizeof" + # Switch to the new inferior before testing. + gdb_test "inferior $num" ".*" "switch to inferior $num" + gdb_test "py print (gdb.inferiors()\[0\].search_memory (start_addr, length, 'aaa'))" \ "${one_pattern_found}" "find string pattern" @@ -127,6 +135,23 @@ with_test_prefix "string" { "${one_pattern_found}" "pattern found at end of range" } +# While still in the new inferior, test reading and writing memory +# again. +gdb_py_test_silent_cmd "python astr = gdb.inferiors()\[0\].read_memory (addr, 5); print(str)" \ + "read str while other inferior selected" 1 +gdb_test "python print(astr\[1\])" "b'a'" \ + "print a character from the string" +gdb_py_test_silent_cmd "python astr\[1\] = b'X'" "change str again" 0 +gdb_py_test_silent_cmd "python gdb.inferiors()\[0\].write_memory (addr, astr)" \ + "write str while other inferior selected" 1 + +gdb_test "inferior 1" ".*" "switch back to inferior 1" + +gdb_test "print str" " = \"hXllo, testsuite\"" \ + "ensure str was changed while other inferior selected" + +gdb_test_no_output "remove-inferiors $num" "remove-inferiors $num" + # Import struct to pack the following patterns. gdb_test_no_output "py from struct import *" -- 2.40.1 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 5/5] Use correct inferior in Inferior.read_memory et al 2023-07-07 15:08 ` [PATCH 5/5] Use correct inferior in Inferior.read_memory et al Tom Tromey @ 2023-07-07 18:26 ` Pedro Alves 2023-07-07 18:27 ` Pedro Alves 0 siblings, 1 reply; 14+ messages in thread From: Pedro Alves @ 2023-07-07 18:26 UTC (permalink / raw) To: Tom Tromey, gdb-patches On 2023-07-07 16:08, Tom Tromey via Gdb-patches wrote: > A user noticed that Inferior.read_memory and a few other Python APIs > will always use the currently selected inferior, not the one passed to > the call. > > This patch fixes the bug by arranging to switch to the inferior. I > found this same issue in several APIs, so this fixes them all. > > I also found out that setting current_inferior isn't enough when > reading memory -- one must also set inferior_ptid. This seems very > confusing to me, especially considering that current_inferior must be > set properly anyway in order to access the target stack. That is because we must be able to remove breakpoints from fork children with "set detach-on-fork off", in which case there's no inferior to represent the child at all. (removing breakpoints == writing to memory to replace breakpoint insn with original instruction). > Finally, I also added a few missing calls to INFPY_REQUIRE_VALID to > these methods. > > Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30615 > --- > gdb/python/py-inferior.c | 36 ++++++++++++++++++++++++++++---- > gdb/testsuite/gdb.python/py-inferior.exp | 25 ++++++++++++++++++++++ > 2 files changed, 57 insertions(+), 4 deletions(-) > > diff --git a/gdb/python/py-inferior.c b/gdb/python/py-inferior.c > index af8bd8855a3..7fb422f621e 100644 > --- a/gdb/python/py-inferior.c > +++ b/gdb/python/py-inferior.c > @@ -30,6 +30,7 @@ > #include "gdbsupport/gdb_signals.h" > #include "py-event.h" > #include "py-stopevent.h" > +#include "progspace-and-thread.h" > #include <unordered_map> > > using thread_map_t > @@ -528,11 +529,14 @@ gdbpy_inferiors (PyObject *unused, PyObject *unused2) > static PyObject * > infpy_read_memory (PyObject *self, PyObject *args, PyObject *kw) > { > + inferior_object *inf = (inferior_object *) self; > CORE_ADDR addr, length; > gdb::unique_xmalloc_ptr<gdb_byte> buffer; > PyObject *addr_obj, *length_obj; > static const char *keywords[] = { "address", "length", NULL }; > > + INFPY_REQUIRE_VALID (inf); > + > if (!gdb_PyArg_ParseTupleAndKeywords (args, kw, "OO", keywords, > &addr_obj, &length_obj)) > return NULL; > @@ -543,6 +547,12 @@ infpy_read_memory (PyObject *self, PyObject *args, PyObject *kw) > > try > { > + scoped_restore_current_inferior restore_inferior; > + scoped_restore_current_pspace_and_thread restore_thread; > + > + thread_info *thr = any_thread_of_inferior (inf->inferior); > + switch_to_thread_no_regs (thr); > + I think this is going to break any Python unwinder that needs to read memory, because switching threads invalidates the frame chain. See how proc-service.c:ps_xfer_memory avoids switching threads, switching only inferior_ptid. I think this needs to do the same. Pedro Alves ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 5/5] Use correct inferior in Inferior.read_memory et al 2023-07-07 18:26 ` Pedro Alves @ 2023-07-07 18:27 ` Pedro Alves 0 siblings, 0 replies; 14+ messages in thread From: Pedro Alves @ 2023-07-07 18:27 UTC (permalink / raw) To: Tom Tromey, gdb-patches On 2023-07-07 19:26, Pedro Alves wrote: > On 2023-07-07 16:08, Tom Tromey via Gdb-patches wrote: >> A user noticed that Inferior.read_memory and a few other Python APIs >> will always use the currently selected inferior, not the one passed to >> the call. >> >> This patch fixes the bug by arranging to switch to the inferior. I >> found this same issue in several APIs, so this fixes them all. >> >> I also found out that setting current_inferior isn't enough when >> reading memory -- one must also set inferior_ptid. This seems very >> confusing to me, especially considering that current_inferior must be >> set properly anyway in order to access the target stack. > > That is because we must be able to remove breakpoints from fork children > with "set detach-on-fork off", in which case there's no inferior to represent Err, I meant "set detach-on-fork on", of course. Pedro Alves ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2023-07-11 15:19 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-07-07 15:07 [PATCH 0/5] Fix some Python Inferior methods Tom Tromey 2023-07-07 15:07 ` [PATCH 1/5] Minor cleanups in py-inferior.exp Tom Tromey 2023-07-07 18:06 ` Pedro Alves 2023-07-11 15:05 ` Tom Tromey 2023-07-07 15:07 ` [PATCH 2/5] Refactor py-inferior.exp Tom Tromey 2023-07-07 18:12 ` Pedro Alves 2023-07-07 15:07 ` [PATCH 3/5] Rename Python variable in py-inferior.exp Tom Tromey 2023-07-07 18:14 ` Pedro Alves 2023-07-11 15:19 ` Tom Tromey 2023-07-07 15:08 ` [PATCH 4/5] Remove obsolete comment from gdbthread.h Tom Tromey 2023-07-07 18:15 ` Pedro Alves 2023-07-07 15:08 ` [PATCH 5/5] Use correct inferior in Inferior.read_memory et al Tom Tromey 2023-07-07 18:26 ` Pedro Alves 2023-07-07 18:27 ` Pedro Alves
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).