From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id rfq0DWSqgF98DwAAWB0awg (envelope-from ) for ; Fri, 09 Oct 2020 14:22:28 -0400 Received: by simark.ca (Postfix, from userid 112) id 29BC11EF6F; Fri, 9 Oct 2020 14:22:28 -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.0 required=5.0 tests=MAILING_LIST_MULTI 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 64C601E58D for ; Fri, 9 Oct 2020 14:22:26 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id E852F385780E; Fri, 9 Oct 2020 18:22:25 +0000 (GMT) Received: from mail-wr1-f68.google.com (mail-wr1-f68.google.com [209.85.221.68]) by sourceware.org (Postfix) with ESMTPS id 91DC7385780E for ; Fri, 9 Oct 2020 18:22:22 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 91DC7385780E Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=palves.net Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=alves.ped@gmail.com Received: by mail-wr1-f68.google.com with SMTP id x7so2653709wrl.3 for ; Fri, 09 Oct 2020 11:22:22 -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=lWr3y+nw2RVYXV3r+2D+lOhptq6FLvYSdovb7ztRP+U=; b=M3jT25gr/ygDWJvhzxzyOJRZjxC5fplHyWgc7moj0elANnjnRbQqdXNfxDS6g3Q7Os VLfelC1k8gJhD4UTJ3uqu9CmZFTLWDLc+ZxCcC7n20lJ4vHe0exPPkBGtRsChLRvVs9Q Zmuzq6oaiqo6v/s/ezAIsdQtHQ98qOM0XR0yHclQScbk9H0oYGWgpcre2aACOX+zwS8Z AYxbscaG/gUKNgoKAvE9m4Gc/qSi6Hg7h0gr+cqC4IRuFNeslE1zZVWC/p25KWrmkDcQ IntyzkCMZMERoezvjQVa9QoozcWkVD0laRj250afglL6IMHyDQDlMVH1remoU90fatb0 4FQw== X-Gm-Message-State: AOAM532JeMBkT0/ubVDxfimvzMjYZcSVEX5Idna1Wbw4DrWjbSqaWMyN eGESruTd91Xgrq6/wlHhPAYJMKLPxpq6aA== X-Google-Smtp-Source: ABdhPJw1IJEeoy8dH+rFgpcPuacC/CuwvlcKfgvNlyP31+WUC2+jgH/1hx+XZw3Soj/h7vcN+ul3bA== X-Received: by 2002:adf:80e4:: with SMTP id 91mr16337454wrl.223.1602267740121; Fri, 09 Oct 2020 11:22:20 -0700 (PDT) Received: from ?IPv6:2001:8a0:f91e:6d00:c80a:ea25:47ef:5f73? ([2001:8a0:f91e:6d00:c80a:ea25:47ef:5f73]) by smtp.gmail.com with ESMTPSA id c14sm12619433wrm.64.2020.10.09.11.22.18 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 09 Oct 2020 11:22:18 -0700 (PDT) Subject: Re: [PATCH 7/7] Reset Windows hardware breakpoints on executable's entry point To: Hannes Domani , Gdb-patches References: <20200525185659.59346-1-ssbssa@yahoo.de> <20200525185659.59346-8-ssbssa@yahoo.de> <1116841542.798992.1591534575390@mail.yahoo.com> From: Pedro Alves Message-ID: <13b5d541-81c8-e7cb-1e9b-29e85b1c940b@palves.net> Date: Fri, 9 Oct 2020 19:22:17 +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: <1116841542.798992.1591534575390@mail.yahoo.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit 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: , Errors-To: gdb-patches-bounces@sourceware.org Sender: "Gdb-patches" Hi Hannes, On 6/7/20 1:56 PM, Hannes Domani via Gdb-patches wrote: > 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 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. > > 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?: > >   /* Observers used to invalidate the cache when needed.  */ >   gdb::observers::inferior_exit.attach (invalidate_windows_cache_inf); >   gdb::observers::inferior_appeared.attach (invalidate_windows_cache_inf); > > Are they not needed for some reason? These notifications are more about the event that happened than about invalidating the inferior. 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. > > >>> + >>> +/* 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. > > 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. > > >>> +  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. > > 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     Type           Disp Enb Address            What > -1      breakpoint     del  y   > -1.1                        y   0x00000000004015b0 in mainCRTStartup at C:/gcc/src/mingw-w64-v7.0.0/mingw-w64-crt/crt/crtexe.c:218:1 inf 2 > -1.2                        y   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 program) #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. 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? Doesn't a similar problem happen? Thanks, Pedro Alves