From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 2402 invoked by alias); 10 Jan 2020 03:46:03 -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 2380 invoked by uid 89); 10 Jan 2020 03:46:01 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-8.7 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_1,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.1 spammy=HX-Languages-Length:4839, H*r:10.0.0, company X-HELO: smtp.polymtl.ca Received: from smtp.polymtl.ca (HELO smtp.polymtl.ca) (132.207.4.11) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 10 Jan 2020 03:45:59 +0000 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 00A3jXBg008439 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 9 Jan 2020 22:45:38 -0500 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 00A3jXBg008439 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=polymtl.ca; s=default; t=1578627939; bh=1C1CEaNVyc54S/eyVx0YipqlFsECCnp/EAJ7DN3rw5E=; h=Subject:To:References:From:Date:In-Reply-To:From; b=LRZbSqndRwZDl1RLd3CgwXlAX2woKIpqed/AO8FnY/oWc3dgx9tYX8hec2ZwnOHPP zroqNPYpa/pNqRHHoq66jj45J1900xrcuCD2hB0dziXD9lWxKSqmXN4Q+2Oy7z9XoD Bw261uvK9axZmggdcKdnPIl+AZYSFgNNLRf5KPto= Received: from [10.0.0.11] (unknown [192.222.164.54]) (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 B6A7E1E508; Thu, 9 Jan 2020 22:45:33 -0500 (EST) Subject: Re: [PATCH] Dwarf 5: Handle debug_str_offsets and indexed attributes that have base offsets. To: Ali Tamur , gdb-patches@sourceware.org References: <20200109005745.217897-1-tamur@google.com> From: Simon Marchi Message-ID: <8fe864df-2fa2-2dc4-b461-3f9f55925a55@polymtl.ca> Date: Fri, 10 Jan 2020 03:46:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.3.0 MIME-Version: 1.0 In-Reply-To: <20200109005745.217897-1-tamur@google.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2020-01/txt/msg00236.txt.bz2 Hi Ali, Just a few more comments. We should be close nowthanks a lot for enduring my nitpicks :). > @@ -7182,7 +7200,35 @@ lookup_signatured_type (struct dwarf2_cu *cu, ULONGEST sig) > return entry; > } > } > - > + > +/* Return the address base of the compile unit, which, if exists, is stored > + either at the attribute DW_AT_GNU_addr_base, or DW_AT_addr_base. */ > +static gdb::optional > +lookup_addr_base (struct die_info *comp_unit_die) > +{ > + struct attribute *attr; > + attr = dwarf2_attr_no_follow (comp_unit_die, DW_AT_addr_base); > + if (attr == nullptr) > + attr = dwarf2_attr_no_follow (comp_unit_die, DW_AT_GNU_addr_base); > + if (attr == nullptr) > + return gdb::optional (); > + return DW_UNSND (attr); > +} > + > +/* Return range lists base of the compile unit, which, if exists, is stored > + either at the attribute DW_AT_rnglists_base or DW_AT_GNU_ranges_base. */ > +ULONGEST > +lookup_ranges_base (struct dwarf2_cu *cu, struct die_info *comp_unit_die) Remove the `cu` parameter. > @@ -18481,10 +18530,26 @@ read_full_die_1 (const struct die_reader_specs *reader, > attributes. */ > die->num_attrs = abbrev->num_attrs; > > + std::vector indexes_that_need_reprocess; > for (i = 0; i < abbrev->num_attrs; ++i) > - info_ptr = read_attribute (reader, &die->attrs[i], &abbrev->attrs[i], > - info_ptr); > + { > + bool need_reprocess; > + info_ptr = > + read_attribute (reader, &die->attrs[i], &abbrev->attrs[i], > + info_ptr, &need_reprocess); > + if (need_reprocess) > + indexes_that_need_reprocess.push_back (i); > + } > + > + struct attribute *attr = dwarf2_attr_no_follow (die, DW_AT_str_offsets_base); > + if (attr != nullptr) > + cu->str_offsets_base = DW_UNSND (attr); > > + auto maybe_addr_base = lookup_addr_base(die); > + if (maybe_addr_base.has_value ()) > + cu->addr_base = *maybe_addr_base; Could this be cu->addr_base = lookup_addr_base (die); ? > @@ -18996,12 +19061,22 @@ partial_die_info::read (const struct die_reader_specs *reader, > int has_high_pc_attr = 0; > int high_pc_relative = 0; > > + std::vector attr_vec (abbrev.num_attrs); > + std::vector indexes_that_need_reprocess; > for (i = 0; i < abbrev.num_attrs; ++i) > { > - struct attribute attr; > - > - info_ptr = read_attribute (reader, &attr, &abbrev.attrs[i], info_ptr); > + bool need_reprocess; > + info_ptr = read_attribute (reader, &attr_vec[i], &abbrev.attrs[i], > + info_ptr, &need_reprocess); > + /* String and address offsets that need to do the reprocessing have > + already been read at this point, so there is no need to wait until > + the loop terminates to do the reprocessing. */ > + if (need_reprocess) > + read_attribute_reprocess (reader, &attr_vec[i]); > + } > > + for (attribute &attr : attr_vec) > + { > /* Store the data if it is of an attribute we want to keep in a > partial symbol table. */ > switch (attr.name) I might just have a brain fart at the moment but... what is the reason we need to have two separate loops here again? What's the reason you can't just add the read_attribute_reprocess call in the existing loop? > @@ -19443,12 +19518,52 @@ partial_die_info::fixup (struct dwarf2_cu *cu) > fixup_called = 1; > } > > +/* Process the attributes that had to be skipped in the first round. These > + attributes are the ones that need str_offsets_base or addr_base attributes. > + They could not have been processed in the first round, because at the time > + the values of str_offsets_base or addr_base may not have been known. */ > +void read_attribute_reprocess (const struct die_reader_specs *reader, > + struct attribute *attr) > +{ > + struct dwarf2_cu *cu = reader->cu; > + switch (attr->form) > + { > + case DW_FORM_addrx: > + case DW_FORM_GNU_addr_index: > + DW_ADDR (attr) = read_addr_index (cu, DW_UNSND (attr)); > + break; > + case DW_FORM_strx: > + case DW_FORM_strx1: > + case DW_FORM_strx2: > + case DW_FORM_strx3: > + case DW_FORM_strx4: > + case DW_FORM_GNU_str_index: > + { > + unsigned int str_index = DW_UNSND (attr); > + if (reader->dwo_file != NULL) > + { > + DW_STRING (attr) = read_dwo_str_index (reader, str_index); > + DW_STRING_IS_CANONICAL (attr) = 0; > + } > + else if (cu->str_offsets_base.has_value ()) > + { > + DW_STRING (attr) = read_stub_str_index (cu, str_index); > + DW_STRING_IS_CANONICAL (attr) = 0; > + } What happens if we encounter one these indirect string forms (strx and company) here, but we don't enter any of the two "if" clauses above? If that happens, it means we'll leave a string-type attribute with its `DW_STRING` unset, and it will just break later. Is it: - a GDB bug, in which case we should add a gdb_assert_not_reached? - Wrong DWARF, in which case we should add a complain? - something else? Simon