From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from sonic307-53.consmr.mail.ir2.yahoo.com (sonic307-53.consmr.mail.ir2.yahoo.com [87.248.110.30]) by sourceware.org (Postfix) with ESMTPS id 87B39386F432 for ; Sun, 7 Jun 2020 12:56:20 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 87B39386F432 X-YMail-OSG: hW.UZQAVM1kOyr_KV__TjzojhAQrbb8kfiE3uxkinUfJWTNv_L47eUdq1VN1Z5B pj8rT2AJtabxVjClM.slgFyk_4aYZWcUgbiK9oN4F4_mXtQq1SxVqbfJUh02uO1hkgk8kuvC9A9c CLOW4kafMAfNXLBw7JLzbC35XEng7FizlfVNyZl7nu2WVNTZQcJdR8PYKJVz8GRHhOfjePgsbF9y ncmXSZYmGOTXn7ukHmqvSBi63XnxeAcKfAQFpYR4BYzBvMpS_3Q6QGfmIyN1UFioLnfwm5QRxLXC uUogIl303aYqldQ2ILuai4GprtIhUkJhbNMU7mw0vO3ECGphd58XCp7lfhxCp4tlbnYlKl8QPInV xOr17Cciqwk0DZk0pIvc6iXYEgK11g8C9s_q1fCZYhn27TieRMJjR.R.r4dw_qz.ZnsTrakmuWkK vCs.w0UqfCcO8GVCdB043uwIlJKLSicMal2P5j0GgoIKOWzPquBnGe0NwxX1zjfh7LlAqA4H1xAm Wv6UYaoRd2yxbAjV.AapgLNv_xNe.0cxzpCoS0cBmnj3DErtsSTJc3aOh31Vr8RTv_qKCfUD4Ezq 6M8SG4bDvQJt2uw3Yw3aK4QVFB391UT1pEo4pB9NTWbWStC8bNMZtLele65HpP4B2R7gUzDNAm3a fVG27xX0DHx1jelHOkkMX8SbFApzmIvw7d4L.2mmzkJiNW2AS0dFhT4INP0Fziu5Yc1JG2chi1JN hHSfrdyUkF530m2b90GnGeXM_k44PIFU9.PWhrjKdSSLuz8jgGTXhUqp9_7D3TqmEofUmoi5ttPR nKif.EY_XAAQ1WftPMKyHETAvp0xMWw8klzAYAxlVKP3wj70TgjLqxFHGx2wn2tyOKj8pgRda0et bUgPlrH4nCOL9wIvs6ahGrK5TS_K5b0ucq.mLk2zmHj6f.NarWvEEK_B0bOvuqjnHKdFEujN9z0Y Dmb5cxJTx5SKN8cxfu4xKAzTHDDDA1HlqoEsNx9g.NA3zmojJOuH8VXJY1Ts8.IdErzxsl9eLAZN 7QtMGQRVNW6b53w7OX.io_IbLrJu0FEx0FoRGcCzOz6mL4Ebsf1YgCo.OHvur..o4hG1yIg9AMae bmJYbllXPcx7fIfmCCiXscsoo0CL9w0Egh_ghwUF98JRRLsEB.F43rV76V3YJKSTzybXOX61mBgs fkPpeqTC7vCl6vfrFzcbzGu56rNBQeRS0BLzPfS7gANujcx9AkQVvmKNOXRO_Tbcg3BpgJxxz0ry NFDJtyNcQrouHEFMdQYC.j9iBfnMG7J_TRnSB3Nn1SKNq9.FqFP8NyiBuLEYtRZQOQsMCgn3N3On 5EnRfPAv7h8uyTIT7AWVsEUquyw-- Received: from sonic.gate.mail.ne1.yahoo.com by sonic307.consmr.mail.ir2.yahoo.com with HTTP; Sun, 7 Jun 2020 12:56:19 +0000 Date: Sun, 7 Jun 2020 12:56:15 +0000 (UTC) From: Hannes Domani To: Gdb-patches Cc: Pedro Alves Message-ID: <1116841542.798992.1591534575390@mail.yahoo.com> In-Reply-To: References: <20200525185659.59346-1-ssbssa@yahoo.de> <20200525185659.59346-8-ssbssa@yahoo.de> Subject: Re: [PATCH 7/7] Reset Windows hardware breakpoints on executable's entry point MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-Mailer: WebService/1.1.16072 YMailNorrin Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:76.0) Gecko/20100101 Firefox/76.0 X-Spam-Status: No, score=-8.5 required=5.0 tests=BAYES_00, BODY_8BITS, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 07 Jun 2020 12:56:24 -0000 Am Sonntag, 31. Mai 2020, 18:38:06 MESZ hat Pedro Alves Folgendes geschrieben: > On 5/25/20 7:56 PM, Hannes Domani via Gdb-patches wrote: > > > This patch creates an internal breakpoint on the process entry point, w= hich > > when it is reached, resets all active hardware breakpoints, and continu= es > > execution. > > Missing ChangeLog entry. > > > --- > >=C2=A0 gdb/windows-tdep.c | 130 ++++++++++++++++++++++++++++++++++++++++= +++++ > >=C2=A0 1 file changed, 130 insertions(+) > > > > diff --git a/gdb/windows-tdep.c b/gdb/windows-tdep.c > > index aa0adeba99..90e4794fc5 100644 > > --- a/gdb/windows-tdep.c > > +++ b/gdb/windows-tdep.c > > @@ -37,6 +37,7 @@ > >=C2=A0 #include "coff/internal.h" > >=C2=A0 #include "libcoff.h" > >=C2=A0 #include "solist.h" > > +#include "observable.h" > > > >=C2=A0 #define CYGWIN_DLL_NAME "cygwin1.dll" > > > > @@ -870,6 +871,99 @@ windows_get_siginfo_type (struct gdbarch *gdbarch) > >=C2=A0=C2=A0=C2=A0 return siginfo_type; > >=C2=A0 } > > > > +/* Windows-specific cached data.=C2=A0 This is used by GDB for caching > > +=C2=A0 purposes for each inferior.=C2=A0 This helps reduce the overhea= d of > > +=C2=A0 transfering data from a remote target to the local host.=C2=A0 = */ > > +struct windows_info > > +{ > > +=C2=A0 CORE_ADDR entry_point =3D 0; > > +}; > > + > > +/* Per-inferior data key.=C2=A0 */ > > +static const struct inferior_key windows_inferior_data; > > This should be program_space_key / per-program_space data, instead of > per-inferior data. > > You may want to take a look at the jit.c code, which is doing similar > things. OK. I will change it to program_space_key, but why are there no observers to invalidate the a program-space, like these for inferiors?: =C2=A0 /* Observers used to invalidate the cache when needed.=C2=A0 */ =C2=A0 gdb::observers::inferior_exit.attach (invalidate_windows_cache_inf); =C2=A0 gdb::observers::inferior_appeared.attach (invalidate_windows_cache_i= nf); Are they not needed for some reason? > > + > > +/* Frees whatever allocated space there is to be freed and sets INF's > > +=C2=A0 Windows cache data pointer to NULL.=C2=A0 */ > > + > > +static void > > +invalidate_windows_cache_inf (struct inferior *inf) > > +{ > > +=C2=A0 windows_inferior_data.clear (inf); > > +} > > + > > +/* Fetch the Windows cache info for INF.=C2=A0 This function always re= turns a > > +=C2=A0 valid INFO pointer.=C2=A0 */ > > + > > +static struct windows_info * > > +get_windows_inferior_data (void) > > Drop the (void), only old pre-C++ code has it.=C2=A0 You can also drop > redundant "struct" throughout if you like. > > > +{ > > +=C2=A0 struct windows_info *info; > > +=C2=A0 struct inferior *inf =3D current_inferior (); > > + > > +=C2=A0 info =3D windows_inferior_data.get (inf); > > +=C2=A0 if (info =3D=3D NULL) > > +=C2=A0=C2=A0=C2=A0 info =3D windows_inferior_data.emplace (inf); > > + > > +=C2=A0 return info; > > +} > > + > > +/* Breakpoint on entry point where any active hardware breakpoints wil= l > > +=C2=A0 be reset.=C2=A0 */ > > Please expand the comments, explaining why this is necessary > in the first place. > > > +static struct breakpoint_ops entry_point_breakpoint_ops; > > + > > +/* Reset active hardware breakpoints.=C2=A0 */ > > + > > +static bool > > +reset_hardware_breakpoints (struct breakpoint *b) > > +{ > > +=C2=A0 if (b->type !=3D bp_hardware_breakpoint > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 && b->type !=3D bp_hardware_watchpoint > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 && b->type !=3D bp_read_watchpoint > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 && b->type !=3D bp_access_watchpoint) > > +=C2=A0=C2=A0=C2=A0 return false; > > This should instead look at locations and their bp_loc_type, > looking for bp_loc_hardware_breakpoint / bp_loc_hardware_watchpoint. > There are situations where the breakpoint is a software breakpoint, > but GDB still inserts a hardware breakpoint, like e.g., due > to "set breakpoint auto-hw". > > > + > > +=C2=A0 struct bp_location *loc; > > +=C2=A0 for (loc =3D b->loc; loc; loc =3D loc->next) > > +=C2=A0=C2=A0=C2=A0 if (loc->enabled && loc->pspace =3D=3D current_prog= ram_space > > +=C2=A0=C2=A0=C2=A0 && b->ops->remove_location (loc, REMOVE_BREAKPOINT)= =3D=3D 0) > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 b->ops->insert_location (loc); > > This is incorrect for not considering whether a > breakpoint location was enabled but not inserted (e.g., the overall > breakpoint was disabled), or whether a breakpoint location was > a duplicate. > > You should instead look at loc->inserted. > > > + > > +=C2=A0 return false; > > +} > > + > > +/* This breakpoint type should never stop, but when reached, reset > > +=C2=A0 the active hardware breakpoints.=C2=A0 */ > > hardware breakpoints and watchpoints. > > > + > > +static void > > +startup_breakpoint_check_status (bpstat bs) > > +{ > > +=C2=A0 /* Never stop.=C2=A0 */ > > +=C2=A0 bs->stop =3D 0; > > + > > +=C2=A0 iterate_over_breakpoints (reset_hardware_breakpoints); > > +} > > + > > +/* Update the breakpoint location to the current entry point.=C2=A0 */ > > + > > +static void > > +startup_breakpoint_re_set (struct breakpoint *b) > > +{ > > This is called if/when the loaded executable changes, even > without re-starting an inferior.=C2=A0 E.g., if you use the > "file" command after starting the inferior.=C2=A0 So this > should re-fetch the new entry point from the executable. > Again, take a look at the jit.c code. If I do "file" after "start", then windows_solib_create_inferior_hook is called before startup_breakpoint_re_set, so the new entry point was fetched already. > > +=C2=A0 struct windows_info *info =3D get_windows_inferior_data (); > > +=C2=A0 CORE_ADDR entry_point =3D info->entry_point; > > + > > +=C2=A0 /* Do nothing if the entry point didn't change.=C2=A0 */ > > +=C2=A0 struct bp_location *loc; > > +=C2=A0 for (loc =3D b->loc; loc; loc =3D loc->next) > > +=C2=A0=C2=A0=C2=A0 if (loc->pspace =3D=3D current_program_space && loc= ->address =3D=3D entry_point) > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return; > > + > > +=C2=A0 event_location_up location > > +=C2=A0=C2=A0=C2=A0 =3D new_address_location (entry_point, nullptr, 0); > > +=C2=A0 std::vector sals; > > +=C2=A0 sals =3D b->ops->decode_location (b, location.get (), current_p= rogram_space); > > Merge the two statements, so that you end up copy initialization, instead= of > initialization, and then assignment: > >=C2=A0=C2=A0 std::vector sals >=C2=A0=C2=A0=C2=A0=C2=A0 =3D b->ops->decode_location (b, location.get (), = current_program_space); > > > +=C2=A0 update_breakpoint_locations (b, current_program_space, sals, {}= ); > > +} > > + > >=C2=A0 /* Implement the "solib_create_inferior_hook" target_so_ops metho= d.=C2=A0 */ > > > >=C2=A0 static void > > @@ -914,6 +1008,30 @@ windows_solib_create_inferior_hook (int from_tty) > >=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (vmaddr !=3D exec_base) > >=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 objfile_rebase (symfile_objfile, exec_bas= e - vmaddr); > >=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 } > > + > > +=C2=A0 /* Create the entry point breakpoint if it doesn't exist alread= y.=C2=A0 */ > > +=C2=A0 if (target_has_execution && exec_base !=3D 0) > > +=C2=A0=C2=A0=C2=A0 { > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 struct windows_info *info =3D get_windo= ws_inferior_data (); > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 CORE_ADDR entry_point =3D exec_base > > +=C2=A0=C2=A0=C2=A0 + pe_data (exec_bfd)->pe_opthdr.AddressOfEntryPoint= ; > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 info->entry_point =3D entry_point; > > + > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 breakpoint *startup_breakpoint > > +=C2=A0=C2=A0=C2=A0 =3D iterate_over_breakpoints ([] (breakpoint *bp) > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 { > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return bp->ops =3D=3D &entr= y_point_breakpoint_ops; > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 }); > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (startup_breakpoint =3D=3D nullptr) > > +=C2=A0=C2=A0=C2=A0 { > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 event_location_up location > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 =3D new_address_location (e= ntry_point, nullptr, 0); > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 create_breakpoint (target_gdbarch(), lo= cation.get(), nullptr, -1, > > Space before parens. > > This looking up for the pre-existing breakpoint doesn't work > correctly when you consider multiple inferiors, where each will > need a location for its own entry pointer.=C2=A0 The Windows backend > doesn't support multi-process, but OTOH, if you do it like jit.c > does, which just basically always create a breakpoint and > stores the pointer in the per-pspace data, you're practically > good to go, and you'll make it easier for whomever comes next > and decides to all multi-process support. I'm not sure what part here will not work. I actually tested with multiple inferiors (not running at the same time), and update_breakpoint_locations made a breakpoint location for each: (gdb) maint info br Num=C2=A0=C2=A0=C2=A0=C2=A0 Type=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0 Disp Enb Address=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0 What -1=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 breakpoint=C2=A0=C2=A0=C2=A0=C2=A0 del=C2= =A0 y=C2=A0=C2=A0 -1.1=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 y=C2= =A0=C2=A0 0x00000000004015b0 in mainCRTStartup at C:/gcc/src/mingw-w64-v7.0= .0/mingw-w64-crt/crt/crtexe.c:218:1 inf 2 -1.2=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 y=C2= =A0=C2=A0 0x000000000117ee20 in mainCRTStartup at heob.c:7094:1 inf 1 Hannes