| Message ID | 1a87019f-b53e-5556-08bc-0ec373c26062@redhat.com |
|---|---|
| State | New |
| Headers | show |
> On 5 Jul 2019, at 19:36, Pedro Alves <palves@redhat.com> wrote: > > On 6/3/19 1:53 PM, Alan Hayward wrote: > >> --- a/gdb/symfile.c >> +++ b/gdb/symfile.c >> @@ -1672,7 +1672,19 @@ symbol_file_command (const char *args, int from_tty) >> >> validate_readnow_readnever (flags); >> >> + /* Set SYMFILE_DEFER_BP_RESET because the proper displacement for a PIE >> + (Position Independent Executable) main symbol file will only be >> + computed by the solib_create_inferior_hook below. Without it, >> + breakpoint_re_set would fail to insert the breakpoints with the zero >> + displacement. */ >> + add_flags |= SYMFILE_DEFER_BP_RESET; >> + >> symbol_file_add_main_1 (name, add_flags, flags, offset); >> + >> + solib_create_inferior_hook (0); > > solib_create_inferior_hook's parameter is "from_tty", so you should pass > it down instead of hardcoding false. > >> + >> + /* Now it's safe to re-add the breakpoints. */ >> + breakpoint_re_set (); >> } >> } >> >> diff --git a/gdb/testsuite/gdb.base/break-idempotent.exp b/gdb/testsuite/gdb.base/break-idempotent.exp >> index 902a5f818b..38e7cf4710 100644 >> --- a/gdb/testsuite/gdb.base/break-idempotent.exp >> +++ b/gdb/testsuite/gdb.base/break-idempotent.exp >> @@ -36,23 +36,6 @@ >> >> standard_testfile >> >> -if {[prepare_for_testing "failed to prepare" $testfile $srcfile debug]} { >> - return -1 >> -} >> - >> -if ![runto_main] then { >> - fail "can't run to main" >> - return 0 >> -} >> - >> -if [is_remote host] { >> - set arg [remote_download host $binfile] >> - if { $arg == "" } { >> - perror "download failed" >> - return -1 >> - } >> -} >> - >> # Force a breakpoint re-set in GDB. Currently this is done by >> # reloading symbols with the "file" command. >> >> @@ -62,11 +45,11 @@ proc force_breakpoint_re_set {} { >> set test "file \$binfile" >> gdb_test_multiple "file $binfile" $test { >> -re "Are you sure you want to change the file. .*y or n. $" { >> - send_gdb "y\n" >> + send_gdb "y\n" optional >> exp_continue >> } >> -re "Load new symbol table from \".*\".*y or n. $" { >> - send_gdb "y\n" >> + send_gdb "y\n" optional >> exp_continue >> } >> -re "Reading symbols from.*$gdb_prompt $" { >> @@ -123,7 +106,7 @@ proc set_breakpoint { break_command } { >> proc test_break { always_inserted break_command } { >> set cmd [lindex [split "$break_command"] 0] >> >> - with_test_prefix "always-inserted $always_inserted: $cmd" { >> + with_test_prefix "$cmd" { >> delete_breakpoints >> >> if ![runto_main] then { >> @@ -163,20 +146,45 @@ proc test_break { always_inserted break_command } { >> } >> } >> >> -foreach always_inserted { "off" "on" } { >> - test_break $always_inserted "break" >> +foreach_with_prefix pie { "nopie" "pie" } { > > It's not obvious from reading the testcase alone why > we do this nopie/pie. I think this warrants a comment. > >> + foreach_with_prefix always_inserted { "off" "on" } { >> >> - if {![skip_hw_breakpoint_tests]} { >> - test_break $always_inserted "hbreak" >> - } >> + set opts {debug} >> + lappend opts $pie >> >> - if {![skip_hw_watchpoint_tests]} { >> - test_break $always_inserted "watch" >> - } >> + if {[prepare_for_testing "failed to prepare" $testfile $srcfile $opts]} { >> + return -1 >> + } > > There's no need to rebuild for the always_inserted off/on variants. > I.e., you can compile twice instead of four times. Also, > to reproduce problems, it's better when each build generates its > own separate binary, instead of the last iteration overwriting > the previous iteration's file. > >> + >> + clean_restart $testfile > > prepare_for_testing already does this. > >> + >> + if ![runto_main] then { >> + fail "can't run to main" >> + return 0 >> + } > > This isn't really necessary since test_break calls > runto_main too. > >> + >> + if [is_remote host] { >> + set arg [remote_download host $binfile] >> + if { $arg == "" } { >> + perror "download failed" >> + return -1 >> + } >> + } > > Pedantically, these errors shouldn't cause the whole > testcase to return, but instead should only cause the > current iteration to be skipped. So e.g., on a system > that fails to build nopie, we'd still test pie. In > principle. > > Here's a patch you can merge with yours that addresses > the issues above. I really should have spotted most of those test case issues. Thanks for the review and additional patch. I pushed them. Alan. > > This is OK otherwise. Thanks. > > From c0b167cee9903e6e68fbe899e6960bd94f285132 Mon Sep 17 00:00:00 2001 > From: Pedro Alves <palves@redhat.com> > Date: Fri, 5 Jul 2019 19:16:06 +0100 > Subject: [PATCH] fixes > > --- > gdb/symfile.c | 2 +- > gdb/testsuite/gdb.base/break-idempotent.exp | 34 ++++++++++++++--------------- > 2 files changed, 17 insertions(+), 19 deletions(-) > > diff --git a/gdb/symfile.c b/gdb/symfile.c > index 0d353b47e57..dc6bdf3fd4a 100644 > --- a/gdb/symfile.c > +++ b/gdb/symfile.c > @@ -1681,7 +1681,7 @@ symbol_file_command (const char *args, int from_tty) > > symbol_file_add_main_1 (name, add_flags, flags, offset); > > - solib_create_inferior_hook (0); > + solib_create_inferior_hook (from_tty); > > /* Now it's safe to re-add the breakpoints. */ > breakpoint_re_set (); > diff --git a/gdb/testsuite/gdb.base/break-idempotent.exp b/gdb/testsuite/gdb.base/break-idempotent.exp > index 38e7cf4710e..96f91c50f90 100644 > --- a/gdb/testsuite/gdb.base/break-idempotent.exp > +++ b/gdb/testsuite/gdb.base/break-idempotent.exp > @@ -146,31 +146,29 @@ proc test_break { always_inserted break_command } { > } > } > > +# The testcase uses the "file" command to force breakpoint re-set in > +# GDB. Test both with and without PIE, as GDB used to mishandle > +# breakpoint re-set when reloading PIEs. > foreach_with_prefix pie { "nopie" "pie" } { > - foreach_with_prefix always_inserted { "off" "on" } { > - > - set opts {debug} > - lappend opts $pie > > - if {[prepare_for_testing "failed to prepare" $testfile $srcfile $opts]} { > - return -1 > - } > + set opts {debug} > + lappend opts $pie > > - clean_restart $testfile > + set binfile [standard_output_file $testfile-$pie] > > - if ![runto_main] then { > - fail "can't run to main" > - return 0 > - } > + if {[prepare_for_testing "failed to prepare" $binfile $srcfile $opts]} { > + continue > + } > > - if [is_remote host] { > - set arg [remote_download host $binfile] > - if { $arg == "" } { > - perror "download failed" > - return -1 > - } > + if [is_remote host] { > + set arg [remote_download host $binfile] > + if { $arg == "" } { > + untested "download failed" > + continue > } > + } > > + foreach_with_prefix always_inserted { "off" "on" } { > test_break $always_inserted "break" > > if {![skip_hw_breakpoint_tests]} { > -- > 2.14.5 >
diff --git a/gdb/symfile.c b/gdb/symfile.c index 0d353b47e57..dc6bdf3fd4a 100644 --- a/gdb/symfile.c +++ b/gdb/symfile.c @@ -1681,7 +1681,7 @@ symbol_file_command (const char *args, int from_tty) symbol_file_add_main_1 (name, add_flags, flags, offset); - solib_create_inferior_hook (0); + solib_create_inferior_hook (from_tty); /* Now it's safe to re-add the breakpoints. */ breakpoint_re_set (); diff --git a/gdb/testsuite/gdb.base/break-idempotent.exp b/gdb/testsuite/gdb.base/break-idempotent.exp index 38e7cf4710e..96f91c50f90 100644 --- a/gdb/testsuite/gdb.base/break-idempotent.exp +++ b/gdb/testsuite/gdb.base/break-idempotent.exp @@ -146,31 +146,29 @@ proc test_break { always_inserted break_command } { } } +# The testcase uses the "file" command to force breakpoint re-set in +# GDB. Test both with and without PIE, as GDB used to mishandle +# breakpoint re-set when reloading PIEs. foreach_with_prefix pie { "nopie" "pie" } { - foreach_with_prefix always_inserted { "off" "on" } { - - set opts {debug} - lappend opts $pie - if {[prepare_for_testing "failed to prepare" $testfile $srcfile $opts]} { - return -1 - } + set opts {debug} + lappend opts $pie - clean_restart $testfile + set binfile [standard_output_file $testfile-$pie] - if ![runto_main] then { - fail "can't run to main" - return 0 - } + if {[prepare_for_testing "failed to prepare" $binfile $srcfile $opts]} { + continue + } - if [is_remote host] { - set arg [remote_download host $binfile] - if { $arg == "" } { - perror "download failed" - return -1 - } + if [is_remote host] { + set arg [remote_download host $binfile] + if { $arg == "" } { + untested "download failed" + continue } + } + foreach_with_prefix always_inserted { "off" "on" } { test_break $always_inserted "break" if {![skip_hw_breakpoint_tests]} {