From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from sonic313-21.consmr.mail.ir2.yahoo.com (sonic313-21.consmr.mail.ir2.yahoo.com [77.238.179.188]) by sourceware.org (Postfix) with ESMTPS id 40FCB3858D34 for ; Wed, 8 Jul 2020 17:43:48 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 40FCB3858D34 Authentication-Results: sourceware.org; dmarc=pass (p=reject dis=none) header.from=yahoo.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=ssbssa@yahoo.de DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=yahoo.de; s=s2048; t=1594230227; bh=Yhd5u8HSD6x/evbMH+X2B+ruwhg/qmga8yIw0rZc2fo=; h=Date:From:To:Cc:In-Reply-To:References:Subject:From:Subject; b=aNLa+xmWUW6IwtjEwPO0cTzb3lECa+jN2PrcCVYmZbOV39TGGXrX4asIZ9C+us9n040aT0pS1GYcP2xocCOWGAzI8QYMBFaEjfdvgFQLlJYSjJ8UYPOuueeg41VA2y0GHzhM8KBm2qVCuXj8VZ3lWxq9QdT2Ru57j7DCMA4tiiGHXFKUSLlKlOnPdYXUWEH8OAMbzYe2Mz4RJy0NDRqzuowtTk6oN6uc0E1az8wbRzzIj0oVp/SA0WHAsr2Fw89eSU+N9T2jVbfTCzm9PAsK9VR2nYPRoc/aepvS6zBRpoajKCYhP6EYwpF9I1873zhkCkTzFPnCbywU4BNiED/y8A== X-YMail-OSG: K5A0488VM1lsIu0HBYfhNLzq7rK7r7RdWrZqAQz1xYuzvmV5VdJu5lgEHzY04j5 ogvlfFc0TlXk3csffQRgEa1thd.Dvr1GiTQewYrXDsFGrfznkGTgO8uw.yDLufPF87okHAmEvIpl 7V1fHdnHvXF06jvN.xsIVQUBuzIP27orPwdY7p3lKrqpFG2tLnp8AcE3iN4pRAY3.691s1uEyOax j2Bdew0n52R5gJndqLbXzPg9kDAb2RAMm1qdzm5WtMgALuW_QzZ_28pSxBp4Zm2gAh98.WvIr0Oq dqXgpHAoBclbFv0qWyqnlOiyQE0dNRRoqfHE2_Ss5FMNKgLfT3RlJyukef5FXA5f7c1Z2XZY3z.I uL38lqFA8fkP9SZhEJV5hEDI.euonU91vHHWW4yrAZUJqdQuLNs7BdeW.ehItFn3FjGMJ1g6e.MZ Ip5REioTmQy91oEW_stb_xDU5RiwAWszjfSzg1_uHzS16V3wgxLjtc1eyNDwBGF6ptJK92.n7Am5 sKEEywMlk8OkLvl8LUuELe5QuL32TgpiyhAw.wP93KuefHm9v0UfeDAxDGLZXsKGzF7sG6MOJ_DF U21q7GwE1bIpU21q54CdoRhLVHdeW2IjmgD_an75HVa5DCzNcEBam3YQRc_0O2kAKw59CXa_akXf 1sVs7oTnvO7p2UFxq9BTnsYsfaBsLnVpbMiwt_wxmx5zdfbUderlSOLT_IPPOeBt6uK824qqToRm lQMi61Ew3C3z46oGbrt73H3WPQ4pTAEwVTh_uR0yLeZtGXUeqEOHrLlmcHWSOMBSqE7etahrgFlq 3S77H7Tnc8tSoaBFFRWUew4GLK8.JW5Q4GnISUC4uqS.QAbFQIEaGoJgGLw9MDCDHZ1.7ZHHenTX rF_at2mCMWEtf_iLrkIPE8NYbMqvlY8bWVGuRfUcdELqgxqA4S9Au_roNQAQiPbrT7BQ562im.G1 qAf3qgnj_6s8nN0mkLvp2cIoTcf9tkQOwnT.LDhd_qnzkNkPHcALwAWm.wXSSUWZtobY8m52yvj1 FpoHCMLb0hWtgy17TTSF6jmEiR2Ppzf00jXODnjxIAhMf0c1.hjfGpeumuUYxT3GI53s53zc9AYf Z9oy9XRpj_39u6o2y_v_Rqe2kx.pj4UtaQetPjXt7Cmjysy_mZ4.wtty5qSed622hajWNddvCdyd P4pZyEjQAyM4qQBS1GmdN7gz4h5MBI52JhMh0Ce1mhSX7qLp9gNQ3ZINSURQNC7Kd8FMzJJXOsda O5mm5tMOyocD5l6mbP3foicZ7lq2pJipEjUD00TLu4oUh_i.3Vcvj22DJPT_5CLLjNLe0wtiLfB0 CINWN7T8gzK25VkwiS4AJFNBd6M3HlrACgVWz82M- Received: from sonic.gate.mail.ne1.yahoo.com by sonic313.consmr.mail.ir2.yahoo.com with HTTP; Wed, 8 Jul 2020 17:43:47 +0000 Date: Wed, 8 Jul 2020 17:43:42 +0000 (UTC) From: Hannes Domani To: Gdb-patches Cc: Pedro Alves Message-ID: <1063873041.7900814.1594230222719@mail.yahoo.com> In-Reply-To: <1116841542.798992.1591534575390@mail.yahoo.com> References: <20200525185659.59346-1-ssbssa@yahoo.de> <20200525185659.59346-8-ssbssa@yahoo.de> <1116841542.798992.1591534575390@mail.yahoo.com> 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.16197 YMailNorrin Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:76.0) Gecko/20100101 Firefox/76.0 X-Spam-Status: No, score=-8.9 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: Wed, 08 Jul 2020 17:43:50 -0000 Ping. Am Sonntag, 7. Juni 2020, 14:56:29 MESZ hat Hannes Domani via Gdb-patches <= gdb-patches@sourceware.org> 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 point,= which > > > when it is reached, resets all active hardware breakpoints, and conti= nues > > > 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 *gdbarc= h) > > >=C2=A0=C2=A0=C2=A0 return siginfo_type; > > >=C2=A0 } > > > > > > +/* Windows-specific cached data.=C2=A0 This is used by GDB for cachi= ng > > > +=C2=A0 purposes for each inferior.=C2=A0 This helps reduce the overh= ead 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=C2=A0 /* Observers used to invalidate the cache when needed.=C2=A0 = */ >=C2=A0=C2=A0 gdb::observers::inferior_exit.attach (invalidate_windows_cach= e_inf); >=C2=A0=C2=A0 gdb::observers::inferior_appeared.attach (invalidate_windows_= cache_inf); > > 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 = 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 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 w= ill > > > +=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_watchpoin= t > > > +=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_pr= ogram_space > > > +=C2=A0=C2=A0=C2=A0 && b->ops->remove_location (loc, REMOVE_BREAKPOIN= T) =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 && l= oc->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= _program_space); > > > > Merge the two statements, so that you end up copy initialization, inste= ad 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 met= hod.=C2=A0 */ > > > > > >=C2=A0 static void > > > @@ -914,6 +1008,30 @@ windows_solib_create_inferior_hook (int from_tt= y) > > >=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_b= ase - vmaddr); > > >=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 } > > > + > > > +=C2=A0 /* Create the entry point breakpoint if it doesn't exist alre= ady.=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_win= dows_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.AddressOfEntryPoi= nt; > > > +=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 &en= try_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 = (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-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