From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id sHrmOWWxgF9oEAAAWB0awg (envelope-from ) for ; Fri, 09 Oct 2020 14:52:21 -0400 Received: by simark.ca (Postfix, from userid 112) id EAD321EF6F; Fri, 9 Oct 2020 14:52:21 -0400 (EDT) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-1.1 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,MAILING_LIST_MULTI,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.2 Received: from sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id 523171E58D for ; Fri, 9 Oct 2020 14:52:20 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id DFA61386F023; Fri, 9 Oct 2020 18:52:19 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org DFA61386F023 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1602269539; bh=+sq7O6LBgh9qEzCqX5DpM7AFYYTqnnkGQDtQZtJNUYs=; h=Date:To:In-Reply-To:References:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=yASx0jTOauF1u4TCpcK1IcpLZ73cXOU/QUVMkZ40H4P2N/SCCKJfyOIlA20SFeX2X 30Y/rTK+hjTzKpvQskVP4K8+FXV7SrnAwvrUJu2l2I5iHaAEryZSoo/P+ornpTFQzc R2FaVVnLoXNg8r3DtQbYJvMm1+4RyADA/Ax7x8Hs= Received: from sonic310-11.consmr.mail.ir2.yahoo.com (sonic310-11.consmr.mail.ir2.yahoo.com [77.238.177.32]) by sourceware.org (Postfix) with ESMTPS id C74693861000 for ; Fri, 9 Oct 2020 18:52:14 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org C74693861000 X-SONIC-DKIM-SIGN: v=1; a=rsa-sha256; c=relaxed/relaxed; d=yahoo.de; s=s2048; t=1602269533; bh=C0bAhbwB5SuMuyRFcQXTqvOXnTGQiAJs6cMWt81Iy+4=; h=Date:From:To:Subject; b=cu3R64/GztgBv5keAIAR2UDIYI8ThuFdYecB03NoL1daR7FIKV/dKjvZQuow2gSqYrNwL3MjXgZat235pD6RtCwt68f8K4JuqSVvLlmhFBG6WGD5ysXetM5pqMxZbu+dYN88ZUXRIkbBGtC5YVh+3GuO3hhhf4NmOnY6YZOPx19w469R6wnNnMDOmA37GOEwhX/YRkBz8DjIYd/8GtR+FtvM5vKs8NGFcL8hpl8715oqa/+5/JhXaI6auMtMHMpPcZUd4d3hR26+08oqdtdZgX9gJW/rohUj1XIP5VBsL6bY8XxYCM07GWGdA4OKeFge2j45ecymJBY7q125rWVP6g== X-YMail-OSG: XUG3mM8VM1lrkn9pt2fNJo7_gbj7OM5I1sAEToAIvCMCuC2V4iVvSVsHB6FBe6_ 6uNJce40Xp.QTQb5sCP7zGbuXua_uf_pwTEGxUVz4wjMjiPkc8YoDa.B.731xlOio5uuLaurIe4b L5L._5IsOoDhUtRC41XftRZIo497EQ6lKk00QocOdxVCgCD_s9iYlaAAEP9OWAMvzKKIpxWrSwg2 hLZqFjyP49eopsqLaGfrJH2QM_E0iqbjUJG0vofUXWBcXISjHIF7wNFyz9qbbToeV5dtVa_uJ10X eqBxiqKlP076vVC1icxDFAuMCMCv51dOYdlsqPjp6qlP9EgZO0ZBGbVo4fSnd8Ea4DdbaFcNBiuo VHU_3690JAjqwmK97bhOpnubVBYfZJXEnCDLjpxMHCtzXYezg.KKYMgT3wJHaxGT2jw24Uj4wolZ kSgp5aBT4S.5FA_rrVWmOMK.QoClAfgGcNTKvte5NwEf3H5ofhuhK47lLyfuQcZ2mCrR07tXc6hl aB93E3Tyk3L9ClzmW5dmFCGK2G1AeCHjrkEaHEV.ee6zwiq7SK1fduA.tGD3Diq3bcQhY_JTiLVc 9tNJTH0S9UtLGo4BzKkPMP5gRYPKeVh2xqOADE8IBfneKBYM783a2oyQoV9iWyo_mragI0wQFPvR LKHbQ4_IHBpG7844Bafk9opzVu8nN9jqt3XSJp2_xuF8OSMs6N9Jp09oEHG3i2kjW3KHHvHpDKh5 xesIa.zP_SzYdNYAkhp.DpcGSwjAh9k5Eg4rvl8fWbXSeBTpwt86EEca2AHntG1qZYorRZAwGQhZ KWNBMZfEsFNUW2RRpU78JBEBAoN9zCQFyChwA4QF3bQN0sU7eUFwgGPcPI6DDjSLLOQD.mCQOmiM kqmnUrexmW47xbg_vOPIntarRCOsh3SJFM0.a7dzPL5iO9jnRCitpexPei_ehKM1Plw8KgpA3BNB apD7jYvFzvPlA6f8QkPSXsHEh8fgNfj9Xu0ZegY3YOeAsAOMxAvYN1lOtFUib1uJ0YSQTSdNuVT2 j1pgixjxEr6G3wCc458f4BHO2LaTN704RJmAw32hhX7pVTzzJnCw9q3KFZg9HomfeZhzMfV5UD_. jAK8ehwp4YK39tOLQNuL2jCRFau4weudQHx1QIETgzL.S7vg4J3FqURoP3BfBfQq0dK8c6QKtubQ 2FRYrirjDAf8z2x6tFYDUiF82OXscf6g5TyRQsl30yxvWnUC3O9ikAXIBe_rw734vyU2G6hS.lMk cMv8GE6Q6vFpAIsfYOq3zXlSExdk5IBKFpnZq_7FCmwkNVTkRwwKuskPzbemErJvc5._.AcMM3mr fQ.lfrlr6JRY4fJj2XU9gmRzC3ftPMEXDkiWFFsBdqBr.xME3yjf3J_2iZCzKdhenRxK6zNbL9bg cRY8TMnhK5b36y7bMGOK7z_A3N.PWeV4.M6EG9f78Da.pjN5jk5QE4KcQUZe14wNqTlK5qeysgxg _NuhMeGAnZ5ffSVeURCm_ZplHfy6HnaLhb1pSOSbVrdueupZBGX6e7N2Q5jIPmx5bg_k- Received: from sonic.gate.mail.ne1.yahoo.com by sonic310.consmr.mail.ir2.yahoo.com with HTTP; Fri, 9 Oct 2020 18:52:13 +0000 Date: Fri, 9 Oct 2020 18:51:53 +0000 (UTC) To: Gdb-patches , Pedro Alves Message-ID: <1338819464.1964114.1602269513009@mail.yahoo.com> In-Reply-To: <13b5d541-81c8-e7cb-1e9b-29e85b1c940b@palves.net> References: <20200525185659.59346-1-ssbssa@yahoo.de> <20200525185659.59346-8-ssbssa@yahoo.de> <1116841542.798992.1591534575390@mail.yahoo.com> <13b5d541-81c8-e7cb-1e9b-29e85b1c940b@palves.net> 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.16795 YMailNorrin Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:76.0) Gecko/20100101 Firefox/76.0 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: , From: Hannes Domani via Gdb-patches Reply-To: Hannes Domani Errors-To: gdb-patches-bounces@sourceware.org Sender: "Gdb-patches" Am Freitag, 9. Oktober 2020, 20:22:21 MESZ hat Pedro Alves Folgendes geschrieben: > Hi Hannes, > > On 6/7/20 1:56 PM, Hannes Domani via Gdb-patches wrote: > >=C2=A0 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=C2=A0 gdb/windows-tdep.c | 130 ++++++++++++++++++++++++++++++++= +++++++++++++ > >>>=C2=A0=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=C2=A0 #include "coff/internal.h" > >>>=C2=A0=C2=A0 #include "libcoff.h" > >>>=C2=A0=C2=A0 #include "solist.h" > >>> +#include "observable.h" > >>> > >>>=C2=A0=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=C2=A0 return siginfo_type; > >>>=C2=A0=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 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? > > These notifications are more about the event that happened than about > invalidating the inferior.=C2=A0 Some observers may use the notification > for that, others for other things. > > Probably nobody found a need for matching program space notifications > because so far the existing ones have covered all needs. I see. > > > >>> + > >>> +/* 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 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 && 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, inst= ead of > >> initialization, and then assignment: > >> > >>=C2=A0=C2=A0=C2=A0 std::vector sals > >>=C2=A0=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=C2=A0 /* Implement the "solib_create_inferior_hook" target_so_o= ps method.=C2=A0 */ > >>> > >>>=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=C2=A0 if (vmaddr !=3D exec_= base) > >>>=C2=A0=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 } > >>> + > >>> +=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-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 > > > > I looked better at the code, and as long as startup_breakpoint_re_set > is called whenever an inferior is added, I agree it should work. > > However, does the breakpoint go away when the inferior exits, though? > I'm wondering what happens when, say: > > #1 - you load a Windows binary in inferior 1. > #2 - you run the inferior, and it exits > #3 - you now load a non-Windows binary in inferior 1 (say, a GNU/Linux pr= ogram) > #4 - you run inferior 1 > > Don't we end up in startup_breakpoint_re_set in step #3 or #4? > If so, that calls get_windows_inferior_data() which a new > windows_info object, and then creates a location at address 0, > I presume.=C2=A0 All while the inferior isn't a Windows inferior. > > Or what if you load a Windows program in inferior 1, and a GNU/Linux > program in inferior 2?=C2=A0 Doesn't a similar problem happen? You're probably right. I've never build a gdb which understands both Windows and Linux executables (I think), so I can't test it. It looks like it keeps the breakpoint on the same location as the last Windows executable. What would you suggest how to fix this? Hannes