From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.polymtl.ca (smtp.polymtl.ca [132.207.4.11]) by sourceware.org (Postfix) with ESMTPS id 378303858C39 for ; Thu, 30 Sep 2021 14:27:06 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 378303858C39 Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id 18UEQx2G018408 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 30 Sep 2021 10:27:04 -0400 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 18UEQx2G018408 Received: from [10.0.0.11] (192-222-157-6.qc.cable.ebox.net [192.222.157.6]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 9C01A1EDF7; Thu, 30 Sep 2021 10:26:59 -0400 (EDT) Message-ID: <5940137d-6a6f-2309-673f-d9491ed654b5@polymtl.ca> Date: Thu, 30 Sep 2021 10:26:58 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.1.1 Subject: Re: [PATCH][gdb/symtab] Relocate call_site_htab Content-Language: en-US To: Tom de Vries , gdb-patches@sourceware.org Cc: Tom Tromey References: <20210930095608.GA11467@delia> From: Simon Marchi In-Reply-To: <20210930095608.GA11467@delia> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Thu, 30 Sep 2021 14:26:59 +0000 X-Spam-Status: No, score=-10.6 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) 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: Thu, 30 Sep 2021 14:27:09 -0000 On 2021-09-30 05:56, Tom de Vries via Gdb-patches wrote: > [ CC-ing maintainers that reviewed previous submission. ] > > Hi, > > When running test-case gdb.arch/amd64-entry-value-inline.exp with target board > unix/-no-pie/-fno-PIE we have: > ... > (gdb) continue^M > Continuing.^M > ^M > Breakpoint 2, fn2 (y=y@entry=25, x=x@entry=6) at \ > gdb.arch/amd64-entry-value-inline.c:32^M > 32 y = -2 + x; /* break-here */^M > (gdb) PASS: gdb.arch/amd64-entry-value-inline.exp: \ > continue to breakpoint: break-here > p y^M > $1 = 25^M > (gdb) PASS: gdb.arch/amd64-entry-value-inline.exp: p y > ... > > But with target board unix/-pie/-fPIE we have instead: > ... > p y^M > $1 = ^M > (gdb) FAIL: gdb.arch/amd64-entry-value-inline.exp: p y > ... > > The test-case uses a .S file, which was generated using gcc 4.8.0, but I can > reproduce the same problem using the original C file and gcc 4.8.5. > > The problem is that in order to access the value, call_site information is > accessed, which is both: > - unrelocated, and > - accessed as if it were relocated. > > I've submitted an attempt at fixing this before, trying to handle this at all > points where the information is used ( > https://sourceware.org/pipermail/gdb-patches/2019-August/159631.html ). > > Instead, fix this more reliably by relocating the call_site information. > > This fixes for me all remaining regressions for unix/-pie/-fPIE vs > unix/-no-pie/-fno-PIE (not counting ada compilation FAILs). > > Tested on x86_64-linux. > > Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=24892 > > Any comments? Instead of relocating the data structure, isn't the tendency now (to make sharing between objfiles easier) to keep the data structure unrelocated, and relocate things when looking things up? I could imagine a compunit_symtab method called "find_call_site", where you pass a relocated PC. The method internally unrelocates the PC to do the lookup in the htab. call_site would store an unrelocated pc, and a call_site::pc method would return the (computed) relocated pc. Simiarly, the call_site's target would keep holding an unrelocated pc, and it would be computed on the fly. See crude patch implementing this at the bottom of this message. Some observations I made while reading about this. - In call_site_to_target_addr, when the target is defined using a DWARF expression (FIELD_LOC_KIND_DWARF_BLOCK), is the result of computing that expression relocated or not? I would presume that it's not relocated. Do we need to relocate it at that point? That looks like a bug to me, it would probably be caught by an appropriate test. - In read_call_site_scope, we do an htab lookup to look for duplicates. We do: slot = htab_find_slot (cu->call_site_htab, &call_site_local, INSERT); where call_site_local is a call_site object. However, the htab works using the address as index, it passes core_addr_hash and core_addr_eq as hash/eq functions. And in call_site_for_pc, the lookup is done by passing a pointer to a CORE_ADDR. Therefore, the lookup in read_call_site_scope looks wrong to me. It just works because call_site::pc happens to be the first field of call_site. Otherwise, comments on this version: > > Thanks, > - Tom > > [gdb/symtab] Relocate call_site_htab > > --- > gdb/objfiles.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 58 insertions(+) > > diff --git a/gdb/objfiles.c b/gdb/objfiles.c > index b65fa8820ca..ceff0fcfba4 100644 > --- a/gdb/objfiles.c > +++ b/gdb/objfiles.c > @@ -626,6 +626,60 @@ relocate_one_symbol (struct symbol *sym, struct objfile *objfile, > } > } > > +/* Relocate call_site S using offset DELTA. */ > + > +static void > +call_site_relocate (struct call_site *s, CORE_ADDR delta) > +{ > + s->pc += delta; > + if (FIELD_LOC_KIND (s->target) == FIELD_LOC_KIND_PHYSADDR) > + FIELD_STATIC_PHYSADDR (s->target) += delta; > +} > + > +/* Relocate HTAB, which is a COMPUNIT_CALL_SITE_HTAB using offset DELTA. */ > + > +static void > +compunit_call_site_htab_relocate (htab_t htab, CORE_ADDR delta) > +{ > + /* Changing the pc field changes the hashcode, so we can't just update the > + elements. Instead, we move them to this var, and then reinsert them. */ > + std::vector tmp; Instead of using a temporary vector, would it be simpler to just create a new htab, fill it while traversing the original one, and move it in place of the original one? The original symtab might still be referenced in dwarf2_cu, but it's probably not needed there anymore after we have moved it to compunit_symtab. So in process_full_comp_unit, after moving the htab, we could set dwarf2_cu::call_site_htab to nullptr to make sure we don't ever use it again. > + > + /* Copy elements to tmp. */ > + auto visitor_func > + = [] (void **slot, void *info) -> int > + { > + /* Copy element to tmp. */ > + struct call_site *s = (struct call_site *) *slot; > + std::vector *tmp_ptr > + = (std::vector *)info; > + tmp_ptr->push_back (s); > + > + /* Keep going. */ > + return 1; > + }; > + htab_traverse (htab, visitor_func, &tmp); > + > + /* Make hashtable empty. This does not destroy the elements because the > + hashtable is created with del_f == nullptr. */ > + htab_empty (htab); > + > + /* Relocate and reinsert elements. */ > + for (struct call_site *s : tmp) { > + /* Relocate element. */ > + call_site_relocate (s, delta); > + > + /* Reinsert element. */ > + struct call_site call_site_local; > + call_site_local.pc = s->pc; > + void **slot > + = htab_find_slot (htab, &call_site_local, INSERT); As explained above, since the htab functions are core_addr_eq/core_addr_hash, we should pass a pointer to a CORE_ADDR. > + gdb_assert (slot != NULL); > + gdb_assert (*slot == NULL); > + *slot = s; > + } Watch the formatting above. >From 8e8b9679da453bd6e5f4573412da275038a945dc Mon Sep 17 00:00:00 2001 From: Simon Marchi Date: Thu, 30 Sep 2021 09:08:43 -0400 Subject: [PATCH] blah Change-Id: I6c73686653d6c4daad331f3536a257cbcea77c64 --- gdb/block.c | 13 ++++++------- gdb/dwarf2/frame-tailcall.c | 4 ++-- gdb/dwarf2/loc.c | 27 +++++++++++++++++---------- gdb/dwarf2/read.c | 10 +++++----- gdb/gdbtypes.c | 10 ++++++++++ gdb/gdbtypes.h | 7 ++++--- gdb/symtab.c | 24 ++++++++++++++++++++++++ gdb/symtab.h | 6 ++++-- 8 files changed, 72 insertions(+), 29 deletions(-) diff --git a/gdb/block.c b/gdb/block.c index 4cb957313960..426ccb630e25 100644 --- a/gdb/block.c +++ b/gdb/block.c @@ -224,16 +224,15 @@ blockvector_contains_pc (const struct blockvector *bv, CORE_ADDR pc) struct call_site * call_site_for_pc (struct gdbarch *gdbarch, CORE_ADDR pc) { - struct compunit_symtab *cust; - void **slot = NULL; + call_site *cs = nullptr; /* -1 as tail call PC can be already after the compilation unit range. */ - cust = find_pc_compunit_symtab (pc - 1); + compunit_symtab *cust = find_pc_compunit_symtab (pc - 1); - if (cust != NULL && COMPUNIT_CALL_SITE_HTAB (cust) != NULL) - slot = htab_find_slot (COMPUNIT_CALL_SITE_HTAB (cust), &pc, NO_INSERT); + if (cust != nullptr) + cs = cust->find_call_site (pc); - if (slot == NULL) + if (cs == nullptr) { struct bound_minimal_symbol msym = lookup_minimal_symbol_by_pc (pc); @@ -247,7 +246,7 @@ call_site_for_pc (struct gdbarch *gdbarch, CORE_ADDR pc) : msym.minsym->print_name ())); } - return (struct call_site *) *slot; + return cs; } /* Return the blockvector immediately containing the innermost lexical block diff --git a/gdb/dwarf2/frame-tailcall.c b/gdb/dwarf2/frame-tailcall.c index f112b4ecca48..9fe498b0924e 100644 --- a/gdb/dwarf2/frame-tailcall.c +++ b/gdb/dwarf2/frame-tailcall.c @@ -240,14 +240,14 @@ 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; + return chain->call_site[chain->length - next_levels - 1]->pc (); 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; + return chain->call_site[chain->callers - next_levels - 1]->pc (); next_levels -= chain->callers; } diff --git a/gdb/dwarf2/loc.c b/gdb/dwarf2/loc.c index 2322a01f396a..16e0be73d027 100644 --- a/gdb/dwarf2/loc.c +++ b/gdb/dwarf2/loc.c @@ -654,10 +654,10 @@ call_site_to_target_addr (struct gdbarch *call_site_gdbarch, { struct bound_minimal_symbol msym; - msym = lookup_minimal_symbol_by_pc (call_site->pc - 1); + msym = lookup_minimal_symbol_by_pc (call_site->pc () - 1); throw_error (NO_ENTRY_VALUE_ERROR, _("DW_AT_call_target is not specified at %s in %s"), - paddress (call_site_gdbarch, call_site->pc), + paddress (call_site_gdbarch, call_site->pc ()), (msym.minsym == NULL ? "???" : msym.minsym->print_name ())); @@ -666,12 +666,12 @@ call_site_to_target_addr (struct gdbarch *call_site_gdbarch, { struct bound_minimal_symbol msym; - msym = lookup_minimal_symbol_by_pc (call_site->pc - 1); + msym = lookup_minimal_symbol_by_pc (call_site->pc () - 1); throw_error (NO_ENTRY_VALUE_ERROR, _("DW_AT_call_target DWARF block resolving " "requires known frame which is currently not " "available at %s in %s"), - paddress (call_site_gdbarch, call_site->pc), + paddress (call_site_gdbarch, call_site->pc ()), (msym.minsym == NULL ? "???" : msym.minsym->print_name ())); @@ -700,11 +700,11 @@ call_site_to_target_addr (struct gdbarch *call_site_gdbarch, msym = lookup_minimal_symbol (physname, NULL, NULL); if (msym.minsym == NULL) { - msym = lookup_minimal_symbol_by_pc (call_site->pc - 1); + msym = lookup_minimal_symbol_by_pc (call_site->pc () - 1); throw_error (NO_ENTRY_VALUE_ERROR, _("Cannot find function \"%s\" for a call site target " "at %s in %s"), - physname, paddress (call_site_gdbarch, call_site->pc), + physname, paddress (call_site_gdbarch, call_site->pc ()), (msym.minsym == NULL ? "???" : msym.minsym->print_name ())); @@ -713,7 +713,14 @@ call_site_to_target_addr (struct gdbarch *call_site_gdbarch, } case FIELD_LOC_KIND_PHYSADDR: - return FIELD_STATIC_PHYSADDR (call_site->target); + { + dwarf2_per_objfile *per_objfile = call_site->per_objfile; + compunit_symtab *cust = per_objfile->get_symtab (call_site->per_cu); + int sect_idx = COMPUNIT_BLOCK_LINE_SECTION (cust); + CORE_ADDR delta = per_objfile->objfile->section_offsets[sect_idx]; + + return FIELD_STATIC_PHYSADDR (call_site->target) + delta; + } default: internal_error (__FILE__, __LINE__, _("invalid call site target kind")); @@ -810,7 +817,7 @@ func_verify_no_selftailcall (struct gdbarch *gdbarch, CORE_ADDR verify_addr) static void tailcall_dump (struct gdbarch *gdbarch, const struct call_site *call_site) { - CORE_ADDR addr = call_site->pc; + CORE_ADDR addr = call_site->pc (); struct bound_minimal_symbol msym = lookup_minimal_symbol_by_pc (addr - 1); fprintf_unfiltered (gdb_stdlog, " %s(%s)", paddress (gdbarch, addr), @@ -986,7 +993,7 @@ call_site_find_chain_1 (struct gdbarch *gdbarch, CORE_ADDR caller_pc, if (target_call_site) { - if (addr_hash.insert (target_call_site->pc).second) + if (addr_hash.insert (target_call_site->pc ()).second) { /* Successfully entered TARGET_CALL_SITE. */ @@ -1005,7 +1012,7 @@ call_site_find_chain_1 (struct gdbarch *gdbarch, CORE_ADDR caller_pc, call_site = chain.back (); chain.pop_back (); - size_t removed = addr_hash.erase (call_site->pc); + size_t removed = addr_hash.erase (call_site->pc ()); gdb_assert (removed == 1); target_call_site = call_site->tail_call_next; diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c index 00aa64dd0abc..bb5767aae673 100644 --- a/gdb/dwarf2/read.c +++ b/gdb/dwarf2/read.c @@ -9510,7 +9510,7 @@ process_full_comp_unit (dwarf2_cu *cu, enum language pretend_language) if (gcc_4_minor >= 5) cust->epilogue_unwind_valid = 1; - cust->call_site_htab = cu->call_site_htab; + cust->set_call_site_htab (cu->call_site_htab); } per_objfile->set_symtab (cu->per_cu, cust); @@ -13341,7 +13341,7 @@ read_call_site_scope (struct die_info *die, struct dwarf2_cu *cu) struct gdbarch *gdbarch = objfile->arch (); CORE_ADDR pc, baseaddr; struct attribute *attr; - struct call_site *call_site, call_site_local; + struct call_site *call_site; void **slot; int nparams; struct die_info *child_die; @@ -13364,13 +13364,13 @@ read_call_site_scope (struct die_info *die, struct dwarf2_cu *cu) } pc = attr->as_address () + baseaddr; pc = gdbarch_adjust_dwarf2_addr (gdbarch, pc); + pc -= baseaddr; if (cu->call_site_htab == NULL) cu->call_site_htab = htab_create_alloc_ex (16, core_addr_hash, core_addr_eq, NULL, &objfile->objfile_obstack, hashtab_obstack_allocate, NULL); - call_site_local.pc = pc; - slot = htab_find_slot (cu->call_site_htab, &call_site_local, INSERT); + slot = htab_find_slot (cu->call_site_htab, &pc, INSERT); if (*slot != NULL) { complaint (_("Duplicate PC %s for DW_TAG_call_site " @@ -13406,7 +13406,7 @@ read_call_site_scope (struct die_info *die, struct dwarf2_cu *cu) + (sizeof (*call_site->parameter) * (nparams - 1)))); *slot = call_site; memset (call_site, 0, sizeof (*call_site) - sizeof (*call_site->parameter)); - call_site->pc = pc; + call_site->m_unrelocated_pc = pc; if (dwarf2_flag_true_p (die, DW_AT_call_tail_call, cu) || dwarf2_flag_true_p (die, DW_AT_GNU_tail_call, cu)) diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c index be7c74ac6cfd..2585fe03a138 100644 --- a/gdb/gdbtypes.c +++ b/gdb/gdbtypes.c @@ -37,6 +37,7 @@ #include "cp-support.h" #include "bcache.h" #include "dwarf2/loc.h" +#include "dwarf2/read.h" #include "gdbcore.h" #include "floatformat.h" #include "f-lang.h" @@ -6309,6 +6310,15 @@ objfile_type (struct objfile *objfile) return objfile_type; } +CORE_ADDR +call_site::pc () const +{ + compunit_symtab *cust = this->per_objfile->get_symtab (this->per_cu); + CORE_ADDR delta + = this->per_objfile->objfile->section_offsets[COMPUNIT_BLOCK_LINE_SECTION (cust)]; + return m_unrelocated_pc + delta; +} + void _initialize_gdbtypes (); void _initialize_gdbtypes () diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h index 2a641122aec0..e47bc46374d3 100644 --- a/gdb/gdbtypes.h +++ b/gdb/gdbtypes.h @@ -1783,11 +1783,12 @@ struct call_site_parameter struct call_site { - /* * Address of the first instruction after this call. It must be + CORE_ADDR pc () const; + + /* Unrelocated address of the first instruction after this call. It must be the first field as we overload core_addr_hash and core_addr_eq for it. */ - - CORE_ADDR pc; + CORE_ADDR m_unrelocated_pc; /* * List successor with head in FUNC_TYPE.TAIL_CALL_LIST. */ diff --git a/gdb/symtab.c b/gdb/symtab.c index a30d900cf8d6..e536754db02e 100644 --- a/gdb/symtab.c +++ b/gdb/symtab.c @@ -329,6 +329,30 @@ search_domain_name (enum search_domain e) } } +call_site * +compunit_symtab::find_call_site (CORE_ADDR pc) const +{ + if (m_call_site_htab == nullptr) + return nullptr; + + CORE_ADDR delta + = this->objfile->section_offsets[COMPUNIT_BLOCK_LINE_SECTION (this)]; + CORE_ADDR unrelocated_pc = pc - delta; + + void **slot = htab_find_slot (m_call_site_htab, &unrelocated_pc, NO_INSERT); + if (slot == nullptr) + return nullptr; + + return (call_site *) *slot; +} + +void +compunit_symtab::set_call_site_htab (htab_t call_site_htab) +{ + gdb_assert (m_call_site_htab == nullptr); + m_call_site_htab = call_site_htab; +} + /* See symtab.h. */ struct symtab * diff --git a/gdb/symtab.h b/gdb/symtab.h index 5182f51672e3..748f49c42435 100644 --- a/gdb/symtab.h +++ b/gdb/symtab.h @@ -1449,6 +1449,9 @@ struct symtab struct compunit_symtab { + call_site *find_call_site (CORE_ADDR pc) const; + void set_call_site_htab (htab_t call_site_htab); + /* Unordered chain of all compunit symtabs of this objfile. */ struct compunit_symtab *next; @@ -1503,7 +1506,7 @@ struct compunit_symtab unsigned int epilogue_unwind_valid : 1; /* struct call_site entries for this compilation unit or NULL. */ - htab_t call_site_htab; + htab_t m_call_site_htab; /* The macro table for this symtab. Like the blockvector, this is shared between different symtabs in a given compilation unit. @@ -1538,7 +1541,6 @@ using compunit_symtab_range = next_range; #define COMPUNIT_BLOCK_LINE_SECTION(cust) ((cust)->block_line_section) #define COMPUNIT_LOCATIONS_VALID(cust) ((cust)->locations_valid) #define COMPUNIT_EPILOGUE_UNWIND_VALID(cust) ((cust)->epilogue_unwind_valid) -#define COMPUNIT_CALL_SITE_HTAB(cust) ((cust)->call_site_htab) #define COMPUNIT_MACRO_TABLE(cust) ((cust)->macro_table) /* A range adapter to allowing iterating over all the file tables -- 2.33.0