From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from sonic302-20.consmr.mail.ir2.yahoo.com (sonic302-20.consmr.mail.ir2.yahoo.com [87.248.110.83]) by sourceware.org (Postfix) with ESMTPS id A10E1385780E for ; Sun, 4 Oct 2020 13:34:55 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org A10E1385780E X-YMail-OSG: IYkv_cMVM1nxg6S7A.VaDHDZD6vEm27qqlsL.Ygw6osdpumchcnP0l0t6Anznhn HCeyPsddQhGCFka.v5RbiEEBbO73aUj2jWIGnVyqfbTE8h7XAsnpHlWJhCRGMWxcoRJRbT3pl0Sz GF6eb91ou1RtOSK6PPbhgNQh2Im4Po.tUsG3IsUcJrdgXkqgcoaLnLDVMQAM_XSQl3sXxZN0thfN tRhwOgT5l2dJR42FueIlOAIudxw2LQAKvQih3ZAwpKg58TPm4SSD.zVVFWLFOv6tIdjdJkBn5oZY R8h9WUEsRaAtQUeBpMPMf.WZ_W_XyUYdfedZXFsv65pWjtc1El435JWVicwKWDvsCJXrrJEHzBss 0JitKWWUyPdxnpkVrPr2a4DAOhRF6sw7fkXfzfQPBoVfiaV4Wm0y38wOVoRPsZ_jV1ZYQQeucbJG E_zGxqJJxlimGW.wCAScW1gpiMqG0Vo7K1aUtRuLc2o7wbPwEdzzDN5YeKGXU.uEtpCg1PQqUrc6 i0eX1HzRyL1UvrcoGZqQat.ByJd410UG1ibJtjoDU0k8dMZZ4XxB3BcP9._jLwJKJD3CWRLFKGfC W75LQARiDErlaI9naX7T3SHTOKi8hP9zIBH_JT.ta0JeY.m0Qo9PJgHS44VL4PEz6ucE5BjZ_jmm JCVRvM1RTdUccaQ05lvaoFeNKZhN9KB429Ds3N3J68GdJCs6JC8LyiXwluaHiwQkDK9ZtLPMJJ8G JMMk7w5o4k9ylq7qIdZH7YZ9S3xHFmwLSs8V6XBWubOFnZqUd.7riE1P4VB8sR_i30wwxhfIkVdf 3lgSWQXdHkPVh.GPrNXCkBvFfD7zePm6fnTWwga.U7QOVHBlOPidMeDAVK3CYWMsvieKjmAPbFZc SJ8ML3SR6l41waq6U76dPRl3e3bFM6Mkt1JROkgeomlx5hizpK2S8fJ1OfE1Svyz.WqPMxpH5cCy oLj9d2DQ533e1uD2.5wz94AraJ96RUrolIGOkI0ZdIArJYWW8_W3_esiwX9W5Ro6TO8vuHJX.ZER TDwgQEv6TnuhTCxkcRviFCqaw7rzKqdoLT._hREc_e14mGtBMipsZDtzNRZJHArX.mgRhxpyB4JQ jQShXdz9NwDnp2WmLZOd_8_gEwZaGlor8SvDW5xp5fuK2ftkEXMiimFLaJjljNxa2ZX6YjWn12ft G_RULQ.170IRL1u9QWkGzxBQn61koI_H8bz9lkBNLWNNaXy6VdF6phwZKt6wCgpgfpLbgvnxamp2 tcLLK.I22su73IjMCF6hjQE1tcWP61sg6.U_iz5xpW15rXx0ZrGH7wcIuTyQFDayDXT4q57g0hzf bQTnAZJ1kwxdAuss7NJscNqltOpZE2dG6T7tHQh4HRE4dBj19RDvvHsT8E6NhFWF5L6y0XnC5Tds hq8wqpfApjo5jtq0ks6GkcRyDljBbQLJdfbsPZV3Lu2pVoZo7.MaYghMUGD6rFAsflYZQ9l5N6NM D.0Qc_p2R Received: from sonic.gate.mail.ne1.yahoo.com by sonic302.consmr.mail.ir2.yahoo.com with HTTP; Sun, 4 Oct 2020 13:34:54 +0000 Date: Sun, 4 Oct 2020 13:34:49 +0000 (UTC) From: Hannes Domani To: Gdb-patches Cc: Pedro Alves Message-ID: <990229213.2562185.1601818489842@mail.yahoo.com> Subject: [PING v2][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 References: <990229213.2562185.1601818489842.ref@mail.yahoo.com> X-Mailer: WebService/1.1.16718 YMailNorrin Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:76.0) Gecko/20100101 Firefox/76.0 X-Spam-Status: No, score=-8.1 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, 04 Oct 2020 13:34:57 -0000 Am Mittwoch, 8. Juli 2020, 19:43:52 MESZ hat Hannes Domani Folgendes geschrieben: Ping. > Ping. > > Am Sonntag, 7. Juni 2020, 14:56:29 MESZ hat Hannes Domani via Gdb-patches= Folgendes geschrieben: > > > 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 poin= t, which > > > > when it is reached, resets all active hardware breakpoints, and con= tinues > > > > 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 *gdba= rch) > > > >=C2=A0=C2=A0=C2=A0 return siginfo_type; > > > >=C2=A0 } > > > > > > > > +/* Windows-specific cached data.=C2=A0 This is used by GDB for cac= hing > > > > +=C2=A0 purposes for each inferior.=C2=A0 This helps reduce the ove= rhead 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_da= ta; > > > > > > 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 observe= rs > > to invalidate the a program-space, like these for inferiors?: > > > >=C2=A0=C2=A0 /* Observers used to invalidate the cache when needed.=C2= =A0 */ > >=C2=A0=C2=A0 gdb::observers::inferior_exit.attach (invalidate_windows_ca= che_inf); > >=C2=A0=C2=A0 gdb::observers::inferior_appeared.attach (invalidate_window= s_cache_inf); > > > > Are they not needed for some reason? > > > > > > > > + > > > > +/* Frees whatever allocated space there is to be freed and sets IN= F'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 alway= s returns 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 dro= p > > > 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= will > > > > +=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_watchpo= int > > > > +=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_watchpoin= t) > > > > +=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_= program_space > > > > +=C2=A0=C2=A0=C2=A0 && b->ops->remove_location (loc, REMOVE_BREAKPO= INT) =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 i= s > > 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 (), curre= nt_program_space); > > > > > > Merge the two statements, so that you end up copy initialization, ins= tead 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 m= ethod.=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= _base - vmaddr); > > > >=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 } > > > > + > > > > +=C2=A0 /* Create the entry point breakpoint if it doesn't exist al= ready.=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_w= indows_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.AddressOfEntryP= oint; > > > > +=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 *b= p) > > > > +=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 &= entry_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 nullp= tr) > > > > +=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_locatio= n (entry_point, nullptr, 0); > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 create_breakpoint (target_gdbarch()= , location.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-v= 7.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