From patchwork Tue Sep 18 21:22:40 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Tom Tromey X-Patchwork-Id: 29451 Received: (qmail 50751 invoked by alias); 18 Sep 2018 21:22:59 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 50739 invoked by uid 89); 18 Sep 2018 21:22:58 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.3 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=5586, Style, STOP, sk:startup X-HELO: gateway34.websitewelcome.com Received: from gateway34.websitewelcome.com (HELO gateway34.websitewelcome.com) (192.185.148.200) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 18 Sep 2018 21:22:56 +0000 Received: from cm15.websitewelcome.com (cm15.websitewelcome.com [100.42.49.9]) by gateway34.websitewelcome.com (Postfix) with ESMTP id B61EC9D744 for ; Tue, 18 Sep 2018 16:22:54 -0500 (CDT) Received: from box5379.bluehost.com ([162.241.216.53]) by cmsmtp with SMTP id 2NSDgjsFpbXuJ2NSKg1jw4; Tue, 18 Sep 2018 16:22:53 -0500 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=tromey.com; s=default; h=Content-Type:MIME-Version:Message-ID:In-Reply-To:Date: References:Subject:Cc:To:From:Sender:Reply-To:Content-Transfer-Encoding: Content-ID:Content-Description:Resent-Date:Resent-From:Resent-Sender: Resent-To:Resent-Cc:Resent-Message-ID:List-Id:List-Help:List-Unsubscribe: List-Subscribe:List-Post:List-Owner:List-Archive; bh=sOZ6rZr3z6lAbwb8v33KupjkClFmThRx+XD9MieaqrE=; b=UUUFmPqKgidJhfJ8kE1cWhGlYm 3+WikGKhcLjmj3v4iedfuTAnMb3ARxhiQEGpICVMJCOOKiKJqoCzW7ANusMB5ypLFiZR1pBdrdsht iDSe9OMoq/T+IxGkspjxgwjRg; Received: from 97-122-190-66.hlrn.qwest.net ([97.122.190.66]:53416 helo=pokyo) by box5379.bluehost.com with esmtpsa (TLSv1.2:ECDHE-RSA-AES256-GCM-SHA384:256) (Exim 4.91) (envelope-from ) id 1g2NSD-003K1B-Bv; Tue, 18 Sep 2018 16:22:41 -0500 From: Tom Tromey To: Simon Marchi Cc: Xavier Roirand , gdb-patches@sourceware.org, brobecker@adacore.com Subject: Re: [RFA 2/5] Darwin: Handle unrelocated dyld. References: <1534932677-9496-1-git-send-email-roirand@adacore.com> <1534932677-9496-3-git-send-email-roirand@adacore.com> <18e995c1bee8c82df212dd431136d259@polymtl.ca> Date: Tue, 18 Sep 2018 15:22:40 -0600 In-Reply-To: <18e995c1bee8c82df212dd431136d259@polymtl.ca> (Simon Marchi's message of "Wed, 22 Aug 2018 09:55:37 -0400") Message-ID: <87lg7ysdpb.fsf@tromey.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1.50 (gnu/linux) MIME-Version: 1.0 >>>>> "Simon" == Simon Marchi writes: Simon> With this patch, there seems to be two way the notifier breakpoint can Simon> be set: in darwin_solib_create_inferior_hook and in Simon> darwin_handle_solib_event. Can you explain why do we need both? Simon> In particular, I don't understand when the one in Simon> darwin_handle_solib_event can be useful. There are two ways Simon> darwin_handle_solib_event can be called: [...] Today I took Xavier's patch and instrumented it with some printfs. What happens here is that at startup, darwin_solib_create_inferior_hook clears the notifier_set flag: + notifier += dyld_relocated_base_address; + info->notifier_set = false; ... but it still creates an solib event breakpoint. This later causes darwin_handle_solib_event to be called. In my case the address of the solib event breakpoint is at the same location in both cases. But, there is also this comment: + (Apparently dyld doesn't need to relocate itself on x86-64 darwin, + but don't assume that). What this says to me is that perhaps there is an architecture where darwin_handle_solib_event computes a different solib breakpoint address. My conclusion is that the patch is generally ok (certainly it works) and, while this one part is unusual, it isn't fatally so. >> + /* True when a breakpoint has been added on dyld notifier. */ >> + unsigned int notifier_set; Simon> Use bool? I did this. Simon> Also, from what I understand from the rest of the patch (I might be Simon> wrong), this is also set if we have tried to add it but failed Simon> (because we failed to get info about the dynamic loader, for example). Simon> If this is right, can you update the comment to reflect this? I don't think this is quite accurate; and the existing comments were enough for me. But there's one case where it sets notifier=0 -- this is the one I don't understand. >> +static gdb_bfd_ref_ptr >> +darwin_get_dyld_bfd (void) Simon> The comment above the function would need to be updated. I did it. >> { >> char *interp_name; >> - CORE_ADDR load_addr = 0; >> >> /* This method doesn't work with an attached process. */ >> if (current_inferior ()->attach_flag) >> - return; >> + return NULL; Simon> Should this last snippet (check for attach_flag) be here or in Simon> darwin_solib_get_all_image_info_addr_at_init? From what I understand, Simon> there is nothing preventing darwin_get_dyld_bfd to work when working Simon> with an attached process. I didn't really understand this choice either, but all the calls to darwin_get_dyld_bfd ultimately come from darwin_solib_create_inferior_hook anyway, so maybe it is intentional. >> + CORE_ADDR notifier; Simon> Style nit: you can declare variables where you are using them the Simon> first time. I made this change too. Tom commit 71b7becde98dd066de4e1e3a5142233c4dd16581 Author: Xavier Roirand Date: Wed Aug 22 12:11:14 2018 +0200 Darwin: Handle unrelocated dyld. On Darwin, debugging an helloworld program with GDB does not work and ends with: (gdb) set startup-with-shell off (gdb) start Temporary breakpoint 1 at 0x100000fb4: file /tmp/helloworld.c, line 1. Starting program: /private/tmp/helloworld [New Thread 0x2703 of process 18906] [New Thread 0x2603 of process 18906] [1]+ Stopped ./gdb/gdb /tmp/helloworld When debugging with lldb, instead of having the STOP signal, we can see that a breakpoint is not set to a proper location: Warning: Cannot insert breakpoint -1. Cannot access memory at address 0xf726 Command aborted. The inserted breakpoint is the one used when GDB has to stop the target when a shared library is loaded or unloaded. The notifier address used for adding the breakpoint is wrong thus the above failure. This notifier address is an offset relative to dyld base address, so the value calculation has to be updated to reflect this. This patch fix this. gdb/ChangeLog: * solib-darwin.c (struct darwin_info): Add notifier_set field. (darwin_get_dyld_bfd): New function. (darwin_solib_get_all_image_info_addr_at_init): Update call. (darwin_handle_solib_event): New function. (darwin_solib_create_inferior_hook): Handle unrelocated dyld. (_initialize_darwin_solib): Add solib_event. Change-Id: I7dde5008c9158f17b78dc89bd7f4bd8a12d4a6e1 diff --git a/gdb/solib-darwin.c b/gdb/solib-darwin.c index ed8e0c13365..2633dc04d43 100644 --- a/gdb/solib-darwin.c +++ b/gdb/solib-darwin.c @@ -77,6 +77,10 @@ struct darwin_info /* Gdb copy of dyld_all_info_infos. */ struct gdb_dyld_all_image_infos all_image; + + /* True when a breakpoint has been added on dyld notifier, or when + an attempt to do so has been made and failed. */ + bool notifier_set; }; /* Per-program-space data key. */ @@ -429,23 +433,21 @@ gdb_bfd_mach_o_fat_extract (bfd *abfd, bfd_format format, return gdb_bfd_ref_ptr (result); } -/* Extract dyld_all_image_addr when the process was just created, assuming the - current PC is at the entry of the dynamic linker. */ +/* Return the BFD for the program interpreter. */ -static void -darwin_solib_get_all_image_info_addr_at_init (struct darwin_info *info) +static gdb_bfd_ref_ptr +darwin_get_dyld_bfd () { char *interp_name; - CORE_ADDR load_addr = 0; /* This method doesn't work with an attached process. */ if (current_inferior ()->attach_flag) - return; + return NULL; /* Find the program interpreter. */ interp_name = find_program_interpreter (); if (!interp_name) - return; + return NULL; /* Create a bfd for the interpreter. */ gdb_bfd_ref_ptr dyld_bfd (gdb_bfd_open (interp_name, gnutarget, -1)); @@ -459,6 +461,18 @@ darwin_solib_get_all_image_info_addr_at_init (struct darwin_info *info) else dyld_bfd.release (); } + return dyld_bfd; +} + +/* Extract dyld_all_image_addr when the process was just created, assuming the + current PC is at the entry of the dynamic linker. */ + +static void +darwin_solib_get_all_image_info_addr_at_init (struct darwin_info *info) +{ + CORE_ADDR load_addr = 0; + gdb_bfd_ref_ptr dyld_bfd (darwin_get_dyld_bfd ()); + if (dyld_bfd == NULL) return; @@ -502,6 +516,37 @@ darwin_solib_read_all_image_info_addr (struct darwin_info *info) info->all_image_addr = extract_unsigned_integer (buf, len, BFD_ENDIAN_BIG); } +/* Callback called on solib event. + Used when a breakpoint has to be added on the entry point, to wait for + dyld initialization. */ + +static void +darwin_handle_solib_event (void) +{ + struct darwin_info *info = get_darwin_info (); + + /* Nothing to do if notifier was already set. */ + if (info->notifier_set) + return; + info->notifier_set = true; + + /* Remove breakpoint on entry, but not now (as our caller is iterating on + breakpoints). */ + remove_solib_event_breakpoints_at_next_stop (); + + /* Reload dyld infos. */ + darwin_load_image_infos (info); + + if (info->all_image.count == 0) + { + /* Not expected: no dylibs. */ + return; + } + + /* Insert the real solib event on the dyld notifier. */ + create_solib_event_breakpoint (target_gdbarch (), info->all_image.notifier); +} + /* Shared library startup support. See documentation in solib-svr4.c. */ static void @@ -528,10 +573,6 @@ darwin_solib_create_inferior_hook (int from_tty) return; } - /* Add the breakpoint which is hit by dyld when the list of solib is - modified. */ - create_solib_event_breakpoint (target_gdbarch (), info->all_image.notifier); - if (info->all_image.count != 0) { /* Possible relocate the main executable (PIE). */ @@ -558,6 +599,54 @@ darwin_solib_create_inferior_hook (int from_tty) if (vmaddr != load_addr) objfile_rebase (symfile_objfile, load_addr - vmaddr); } + + /* Set solib notifier (to reload list of shared libraries). */ + CORE_ADDR notifier = info->all_image.notifier; + info->notifier_set = true; + + if (info->all_image.count == 0) + { + /* Dyld hasn't yet relocated itself, so the notifier address may be + incorrect (as it has to be relocated). + (Apparently dyld doesn't need to relocate itself on x86-64 darwin, + but don't assume that). + Set an event breakpoint at the entry point. */ + CORE_ADDR start = bfd_get_start_address (exec_bfd); + if (start == 0) + notifier = 0; + else + { + gdb_bfd_ref_ptr dyld_bfd (darwin_get_dyld_bfd ()); + if (dyld_bfd != NULL) + { + CORE_ADDR dyld_bfd_start_address; + CORE_ADDR dyld_relocated_base_address; + CORE_ADDR pc; + + dyld_bfd_start_address = bfd_get_start_address (dyld_bfd.get()); + + /* We find the dynamic linker's base address by examining + the current pc (which should point at the entry point + for the dynamic linker) and subtracting the offset of + the entry point. */ + + pc = regcache_read_pc (get_current_regcache ()); + dyld_relocated_base_address = pc - dyld_bfd_start_address; + + /* We get the proper notifier relocated address by + adding the dyld relocated base address to the current + notifier offset value. */ + + notifier += dyld_relocated_base_address; + info->notifier_set = false; + } + } + } + + /* Add the breakpoint which is hit by dyld when the list of solib is + modified. */ + if (notifier != 0) + create_solib_event_breakpoint (target_gdbarch (), notifier); } static void @@ -658,4 +747,5 @@ _initialize_darwin_solib (void) darwin_so_ops.in_dynsym_resolve_code = darwin_in_dynsym_resolve_code; darwin_so_ops.lookup_lib_global_symbol = darwin_lookup_lib_symbol; darwin_so_ops.bfd_open = darwin_bfd_open; + darwin_so_ops.handle_event = darwin_handle_solib_event; }