From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 10806 invoked by alias); 16 Aug 2019 18:34:43 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 9915 invoked by uid 89); 16 Aug 2019 18:34:43 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 spammy=belonging, HX-Languages-Length:3562, HX-Received:ce04 X-HELO: mail-wr1-f68.google.com Received: from mail-wr1-f68.google.com (HELO mail-wr1-f68.google.com) (209.85.221.68) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 16 Aug 2019 18:34:41 +0000 Received: by mail-wr1-f68.google.com with SMTP id p17so2411059wrf.11 for ; Fri, 16 Aug 2019 11:34:40 -0700 (PDT) Return-Path: Received: from ?IPv6:2001:8a0:f913:f700:56ee:75ff:fe8d:232b? ([2001:8a0:f913:f700:56ee:75ff:fe8d:232b]) by smtp.gmail.com with ESMTPSA id n12sm4037449wmc.24.2019.08.16.11.34.38 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 16 Aug 2019 11:34:38 -0700 (PDT) Subject: Re: [PATCH][gdb] Fix gdb.arch/amd64-tailcall-*.exp with -fPIE/-pie To: Tom de Vries , gdb-patches@sourceware.org References: <20190809104848.GA6563@delia> <57ada901-a8d8-b632-f7d8-e42283314b5a@redhat.com> <38825791-ad92-3f7e-d3ae-2ac123dd6422@suse.de> Cc: Jan Kratochvil From: Pedro Alves Message-ID: <8ce4d9fe-0169-35bd-ebbf-11551b45a12a@redhat.com> Date: Fri, 16 Aug 2019 18:34:00 -0000 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: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2019-08/txt/msg00378.txt.bz2 On 8/10/19 6:44 AM, Tom de Vries wrote: > On 09-08-19 19:38, Pedro Alves wrote: >> On 8/9/19 4:03 PM, Tom de Vries wrote: >> >>> >>> +/* Given an unrelocated address ADDR belonging to the text section of OBJFILE, >>> + return the relocated address. */ >>> + >>> +static CORE_ADDR >>> +relocate_text_addr (CORE_ADDR addr, struct objfile *objfile) >>> +{ >>> + CORE_ADDR baseaddr >>> + = ANOFFSET (objfile->section_offsets, SECT_OFF_TEXT (objfile)); >>> + struct gdbarch *gdbarch = get_objfile_arch (objfile); >>> + addr = gdbarch_adjust_dwarf2_addr (gdbarch, addr + baseaddr); >>> + return addr; >> >> I'd write: >> >> return gdbarch_adjust_dwarf2_addr (gdbarch, addr + baseaddr); >> > > Updated accordingly. > >>> +} >>> + >>> /* Find PC to be unwound from THIS_FRAME. THIS_FRAME must be a part of >>> CACHE. */ >>> >>> @@ -240,14 +255,25 @@ pretend_pc (struct frame_info *this_frame, struct tailcall_cache *cache) >>> gdb_assert (next_levels >= 0); >>> >>> if (next_levels < chain->callees) >>> - return chain->call_site[chain->length - next_levels - 1]->pc; >>> + { >>> + struct call_site *call_site >>> + = chain->call_site[chain->length - next_levels - 1]; >>> + struct objfile *objfile = call_site->per_cu->dwarf2_per_objfile->objfile; >>> + return relocate_text_addr (call_site->pc, objfile); >>> + } >>> next_levels -= chain->callees; >>> >>> /* Otherwise CHAIN->CALLEES are already covered by CHAIN->CALLERS. */ >>> if (chain->callees != chain->length) >>> { >>> if (next_levels < chain->callers) >>> - return chain->call_site[chain->callers - next_levels - 1]->pc; >>> + { >>> + struct call_site *call_site >>> + = chain->call_site[chain->callers - next_levels - 1]; >>> + struct objfile *objfile >>> + = call_site->per_cu->dwarf2_per_objfile->objfile; >>> + return relocate_text_addr (call_site->pc, objfile); >>> + } >> >> That seems fine, but it seems you could have factored out more, like: >> >> static CORE_ADDR >> call_site_relocated_pc (struct call_site *call_site) >> { >> struct objfile *objfile >> = call_site->per_cu->dwarf2_per_objfile->objfile; >> CORE_ADDR baseaddr >> = ANOFFSET (objfile->section_offsets, SECT_OFF_TEXT (objfile)); >> struct gdbarch *gdbarch = get_objfile_arch (objfile); >> return gdbarch_adjust_dwarf2_addr (gdbarch, call_size->pc + baseaddr); >> } >> >> Then the other hunks would look like: >> >> struct call_site *call_site >> = chain->call_site[chain->length - next_levels - 1]; >> return call_site_relocated_pc (call_site); >> >> ... >> >> struct call_site *call_site >> = chain->call_site[chain->callers - next_levels - 1]; >> return call_site_relocated_pc (call_site); >> >> call_site_relocated_pc could even be a method of struct call_site, I guess, >> so you'd write: >> >> return call_site->relocated_pc (); >> >> Any reason you didn't do something like that? > > I went for a utility function that can be maximally reused, as opposed > to a function that maximally factors out the code in this particular > patch. But we can do both, so this patch adds: > - relocate_text_addr (I've moved it to objfiles.h/objfiles.c) > - call_site::relocated_pc > > [ BTW, I should mention that this fix is dependent on the patch "[gdb] > Fix gdb.dwarf2/amd64-entry-value-param.exp with -fPIE/-pie" ( > https://sourceware.org/ml/gdb-patches/2019-08/msg00215.html ). ] > > OK like this? Thanks. It looks much better to me this way, but I think we should resolve the SECT_OFF_TEXT discussion in the other patch before moving forward. Thanks, Pedro Alves