From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [205.139.110.61]) by sourceware.org (Postfix) with ESMTP id 76C34385DC00 for ; Sun, 31 May 2020 16:38:03 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 76C34385DC00 Received: from mail-wr1-f69.google.com (mail-wr1-f69.google.com [209.85.221.69]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-448-wd6GL0QXNl-tVuNyxF_74A-1; Sun, 31 May 2020 12:38:01 -0400 X-MC-Unique: wd6GL0QXNl-tVuNyxF_74A-1 Received: by mail-wr1-f69.google.com with SMTP id o1so3588565wrm.17 for ; Sun, 31 May 2020 09:38:00 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=JdIgE3PNMZj2xbV4a0/gjv/Rf4EoiT8SrvgmW9zsT6c=; b=OTZrwVM8RxloO1+0u8OPuqrX+Q605Rr6B+gFd6/+p7Nlt8JjPhG+5cuAIF97v5wKxy f+dIZmHvRGizqVe1UmXsU9O9nLazqKpGH2lXvsm7degbseLSuj1P95Mu0QF6UeR7fuC6 T110Pj8OqZbHunK7mzbaNuIebsS6XIeqYCUthIQwVwg5P+K02JRmKbZZo6gvkSr+xPZr ZOs/SyC5bsNDnAHh9PHghu3y9XEI+j6ar3cLD/Jh0jPFD7zJ7iB8EevuhPn0Yn0mrUaP YaMjUXPPcL3uMDcb/FycUyvG02N3mxxWJCa5+4RRK0QSMZGqAD5cDvB7WQ6GcIvOTfoj wEkg== X-Gm-Message-State: AOAM531Ttib3YO/iU2s7uRIcpzLK5jNvTC1Fg2S0ToTYd/LtSy1f/TOO mZzg2URH8DAQXJK0+7tMqwq8to5z7KyWafa7YwNcZT8N8hhZf+gnymnUvGDC9Z+3wE105MXHXD4 QA6vpx8OBaIO4N7D3LjmsEg== X-Received: by 2002:a1c:acc8:: with SMTP id v191mr18788603wme.154.1590943079803; Sun, 31 May 2020 09:37:59 -0700 (PDT) X-Google-Smtp-Source: ABdhPJxlTEXtW/FEXp8bXoKled16hmxYPcNLjAsTI5vNaAO4iA9ngnjfvtFJWi5sd3n3uktM229n2Q== X-Received: by 2002:a1c:acc8:: with SMTP id v191mr18788591wme.154.1590943079535; Sun, 31 May 2020 09:37:59 -0700 (PDT) Received: from ?IPv6:2001:8a0:f909:7b00:56ee:75ff:fe8d:232b? ([2001:8a0:f909:7b00:56ee:75ff:fe8d:232b]) by smtp.gmail.com with ESMTPSA id k12sm15838609wrn.42.2020.05.31.09.37.58 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sun, 31 May 2020 09:37:59 -0700 (PDT) Subject: Re: [PATCH 7/7] Reset Windows hardware breakpoints on executable's entry point To: Hannes Domani , gdb-patches@sourceware.org References: <20200525185659.59346-1-ssbssa@yahoo.de> <20200525185659.59346-8-ssbssa@yahoo.de> From: Pedro Alves Message-ID: Date: Sun, 31 May 2020 17:37:57 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: <20200525185659.59346-8-ssbssa@yahoo.de> Content-Language: en-US X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-10.2 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, 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, 31 May 2020 16:38:05 -0000 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 continues > execution. Missing ChangeLog entry. > --- > gdb/windows-tdep.c | 130 +++++++++++++++++++++++++++++++++++++++++++++ > 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 @@ > #include "coff/internal.h" > #include "libcoff.h" > #include "solist.h" > +#include "observable.h" > > #define CYGWIN_DLL_NAME "cygwin1.dll" > > @@ -870,6 +871,99 @@ windows_get_siginfo_type (struct gdbarch *gdbarch) > return siginfo_type; > } > > +/* Windows-specific cached data. This is used by GDB for caching > + purposes for each inferior. This helps reduce the overhead of > + transfering data from a remote target to the local host. */ > +struct windows_info > +{ > + CORE_ADDR entry_point = 0; > +}; > + > +/* Per-inferior data key. */ > +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. > + > +/* Frees whatever allocated space there is to be freed and sets INF's > + Windows cache data pointer to NULL. */ > + > +static void > +invalidate_windows_cache_inf (struct inferior *inf) > +{ > + windows_inferior_data.clear (inf); > +} > + > +/* Fetch the Windows cache info for INF. This function always returns a > + valid INFO pointer. */ > + > +static struct windows_info * > +get_windows_inferior_data (void) Drop the (void), only old pre-C++ code has it. You can also drop redundant "struct" throughout if you like. > +{ > + struct windows_info *info; > + struct inferior *inf = current_inferior (); > + > + info = windows_inferior_data.get (inf); > + if (info == NULL) > + info = windows_inferior_data.emplace (inf); > + > + return info; > +} > + > +/* Breakpoint on entry point where any active hardware breakpoints will > + be reset. */ 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. */ > + > +static bool > +reset_hardware_breakpoints (struct breakpoint *b) > +{ > + if (b->type != bp_hardware_breakpoint > + && b->type != bp_hardware_watchpoint > + && b->type != bp_read_watchpoint > + && b->type != bp_access_watchpoint) > + 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". > + > + struct bp_location *loc; > + for (loc = b->loc; loc; loc = loc->next) > + if (loc->enabled && loc->pspace == current_program_space > + && b->ops->remove_location (loc, REMOVE_BREAKPOINT) == 0) > + 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. > + > + return false; > +} > + > +/* This breakpoint type should never stop, but when reached, reset > + the active hardware breakpoints. */ hardware breakpoints and watchpoints. > + > +static void > +startup_breakpoint_check_status (bpstat bs) > +{ > + /* Never stop. */ > + bs->stop = 0; > + > + iterate_over_breakpoints (reset_hardware_breakpoints); > +} > + > +/* Update the breakpoint location to the current entry point. */ > + > +static void > +startup_breakpoint_re_set (struct breakpoint *b) > +{ This is called if/when the loaded executable changes, even without re-starting an inferior. E.g., if you use the "file" command after starting the inferior. So this should re-fetch the new entry point from the executable. Again, take a look at the jit.c code. > + struct windows_info *info = get_windows_inferior_data (); > + CORE_ADDR entry_point = info->entry_point; > + > + /* Do nothing if the entry point didn't change. */ > + struct bp_location *loc; > + for (loc = b->loc; loc; loc = loc->next) > + if (loc->pspace == current_program_space && loc->address == entry_point) > + return; > + > + event_location_up location > + = new_address_location (entry_point, nullptr, 0); > + std::vector sals; > + sals = b->ops->decode_location (b, location.get (), current_program_space); Merge the two statements, so that you end up copy initialization, instead of initialization, and then assignment: std::vector sals = b->ops->decode_location (b, location.get (), current_program_space); > + update_breakpoint_locations (b, current_program_space, sals, {}); > +} > + > /* Implement the "solib_create_inferior_hook" target_so_ops method. */ > > static void > @@ -914,6 +1008,30 @@ windows_solib_create_inferior_hook (int from_tty) > if (vmaddr != exec_base) > objfile_rebase (symfile_objfile, exec_base - vmaddr); > } > + > + /* Create the entry point breakpoint if it doesn't exist already. */ > + if (target_has_execution && exec_base != 0) > + { > + struct windows_info *info = get_windows_inferior_data (); > + CORE_ADDR entry_point = exec_base > + + pe_data (exec_bfd)->pe_opthdr.AddressOfEntryPoint; > + info->entry_point = entry_point; > + > + breakpoint *startup_breakpoint > + = iterate_over_breakpoints ([] (breakpoint *bp) > + { > + return bp->ops == &entry_point_breakpoint_ops; > + }); > + if (startup_breakpoint == nullptr) > + { > + event_location_up location > + = new_address_location (entry_point, nullptr, 0); > + 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. 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. > + nullptr, 0, 1, bp_breakpoint, 0, > + AUTO_BOOLEAN_FALSE, &entry_point_breakpoint_ops, > + 0, 1, 1, 0); Thanks, Pedro Alves