From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 45107 invoked by alias); 26 Sep 2017 18:48:53 -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 44745 invoked by uid 89); 26 Sep 2017 18:48:53 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-21.4 required=5.0 tests=BAYES_00,FREEMAIL_FROM,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,HTML_MESSAGE,RCVD_IN_DNSWL_NONE,RCVD_IN_SORBS_SPAM,SPF_PASS autolearn=ham version=3.3.2 spammy=H*c:alternative X-HELO: mail-vk0-f46.google.com Received: from mail-vk0-f46.google.com (HELO mail-vk0-f46.google.com) (209.85.213.46) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 26 Sep 2017 18:48:50 +0000 Received: by mail-vk0-f46.google.com with SMTP id i1so5880614vke.12 for ; Tue, 26 Sep 2017 11:48:50 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=tfjIVJ1mCabmBd4YtAxVn27hABPQFzEh+VEQFGDnE9M=; b=UsBfJxoLTMZfgmFvZ+31ZRd+ZlC3jgekL/uYMkW7FUhqPr/8wlZmPOsZjJot3svljS 27y1L+7zIbXtlcLGmtPaFqDdWbS5YojejSgvBfp1Dfl8tv7oTpSOVD/ksuGLO88yzEuP 7KOH/1JwH9d8AKUQojw0PBU1J3QogsTOGep8O2hxtYkYY3f7Va7jMOfBnuKr43LfO1WL OhoGUd4ZoTGpvBoZFbOTuXdU2A3r1IwAZnpz4mWlSoxUBBiLz7i2LiV6Po185fEGjieR n4kBaL3vC0fo/QzJstQW29VoN6Bm8yo/0oVPrFzL4YyINn6DeHH3EDYT3Dx4hSvH0GGT sO9g== X-Gm-Message-State: AHPjjUgtv/o0eIrKybpm9gF049oUpJMx01hVhcBnheUpp5BAfkuvzCKP YLsd9CEmBwsE3LD1Vpx6onAKLcNdw70m1OPStnc= X-Google-Smtp-Source: AOwi7QATzKvsKZJ/9JSU0HNW3ebmHDhqxj0avGXAASyHDK69+6gxsoE4QY2q5kszXUih1pJac3e7dtM3rxXzIC2nyvc= X-Received: by 10.31.165.11 with SMTP id o11mr9496492vke.103.1506451727863; Tue, 26 Sep 2017 11:48:47 -0700 (PDT) MIME-Version: 1.0 Received: by 10.159.50.193 with HTTP; Tue, 26 Sep 2017 11:48:47 -0700 (PDT) In-Reply-To: References: From: Alexander Shaposhnikov Date: Tue, 26 Sep 2017 18:48:00 -0000 Message-ID: Subject: Re: [PATCH] Fix reading .dwp files without .debug_tu_index To: Doug Evans Cc: gdb-patches Content-Type: text/plain; charset="UTF-8" X-SW-Source: 2017-09/txt/msg00814.txt.bz2 (updated patch + changelog) changelog: 2017-09-26 Alexander Shaposhnikov * dwarf2read.c (open_and_init_dwp_file): Protect against dwp_file having NULL cus or tus. patch: diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c index b3c5fabfc0..4e6bf27364 100644 --- a/gdb/dwarf2read.c +++ b/gdb/dwarf2read.c @@ -11196,7 +11196,8 @@ open_and_init_dwp_file (void) dwp_file->tus = create_dwp_hash_table (dwp_file, 1); /* The DWP file version is stored in the hash table. Oh well. */ - if (dwp_file->cus->version != dwp_file->tus->version) + if (dwp_file->cus && dwp_file->tus + && dwp_file->cus->version != dwp_file->tus->version) { /* Technically speaking, we should try to limp along, but this is pretty bizarre. We use pulongest here because that's the established @@ -11206,7 +11207,13 @@ open_and_init_dwp_file (void) pulongest (dwp_file->cus->version), pulongest (dwp_file->tus->version), dwp_name.c_str ()); } - dwp_file->version = dwp_file->cus->version; + + if (dwp_file->cus) + dwp_file->version = dwp_file->cus->version; + else if (dwp_file->tus) + dwp_file->version = dwp_file->tus->version; + else + dwp_file->version = 2; if (dwp_file->version == 2) bfd_map_over_sections (dwp_file->dbfd, dwarf2_locate_v2_dwp_sections, Kind regards, Alexander Shaposhnikov On Tue, Sep 26, 2017 at 10:51 AM, Doug Evans wrote: > On Mon, Sep 25, 2017 at 9:13 PM, Alexander Shaposhnikov > wrote: > > > > >I'd go with something like (untested and likely improperly formatted): > > >if (dwp_file->cus) > > > dwp_file->version = dwp_file-cus->version; > > > else if (dwp_file->tus) > > > dwp_file->version = dwp_file->tus->version > > > else > > > dwp_file->version = 2; > > > > i'm ok with that change, many thanks, > > do i need to resend the patch ? (and yeah, i can double check / test it > when > > i get home) > > No need, if you're confident all the nits are taken care of [ChangeLog > syntax and contents, patch whitespace, testing, and so on ... :-)] > > > regarding the dependence on the order of sections - (if i am not > mistaken) > > yeah, it's a separate issue - i can provide more details > > when i get to my laptop. > > Thanks. > > > > > > Best regards, > > Alexander Shaposhnikov > > > > On Mon, Sep 25, 2017 at 8:14 PM, Doug Evans wrote: > >> > >> On Tue, Sep 19, 2017 at 12:58 PM, Alexander Shaposhnikov > >> wrote: > >> > This patch fixes segmentation fault (due to dereferencing of a null > >> > pointer) > >> > in dwarf2read.c in the code dwp_file->cus->version != > >> > dwp_file->tus->version > >> > by adding defensive checks similarly to how it's already done > >> > at the lines 11208 - 11215 (in the same file dwarf2read.c). > >> > The issue can be reproduced with dwp packages built by llvm-dwp > utility > >> > (from the current upstream) since at the moment llvm-dwp doesn't > create > >> > .debug_tu_index section, thus dwp_file->tus will be null. > >> > > >> > Test plan: > >> > > >> > BUILD: > >> > main.cpp: > >> > int f() { > >> > int x = 0; > >> > int y = 1; > >> > return x + y; > >> > } > >> > g++ -fPIC -gsplit-dwarf -g -O0 main.cpp -o main.exe > >> > llvm-dwp main.dwo -o main.exe.dwp > >> > # This step is a workaround to a separate issue (unrelated to this > >> > patch). > >> > # At the moment llvm tools & clang put .strtab section first (its > index > >> > is > >> > 1), > >> > # while gcc/gold/binutils put it at the end. > >> > # Unfortunately gdb (in the code reading dwps) appears to depend on > the > >> > order > >> > # of sections, > >> > >> That's odd. Depends on the order of sections how? > >> > >> > to workaround this (to reproduce the issue which this patch > >> > # aims to address) we use objcopy to do the following trick: > >> > # if one asks to remove .strtab objcopy will do that but at the same > >> > time > >> > # it will create a new .shstrtab section at the end. > >> > objcopy --remove-section .strtab main.exe.dwp > >> > RUN: > >> > gdb main.exe > >> > One can observe that now there is no crash and debugging functionality > >> > works as expected (setting breakpoints, printing local variable, > >> > exploring > >> > the stack). > >> > > >> > gdb/ChangeLog: > >> > yyyy-mm-dd Alexander Shaposhnikov gmail.com> > >> > > >> > * dwarf2read.c: Fix segmentation fault on reading > >> > > >> > .dwp files without .debug_tu_index section. > >> > > >> > Patch: > >> > > >> > diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c > >> > > >> > index b1914cf876..547e3f034e 100644 > >> > --- a/gdb/dwarf2read.c > >> > +++ b/gdb/dwarf2read.c > >> > @@ -11185,7 +11185,8 @@ open_and_init_dwp_file (void) > >> > dwp_file->tus = create_dwp_hash_table (dwp_file, 1); > >> > > >> > /* The DWP file version is stored in the hash table. Oh well. */ > >> > - if (dwp_file->cus->version != dwp_file->tus->version) > >> > + if (dwp_file->cus && dwp_file->tus > >> > + && dwp_file->cus->version != dwp_file->tus->version) > >> > { > >> > /* Technically speaking, we should try to limp along, but this > is > >> > pretty bizarre. We use pulongest here because that's the > >> > established > >> > @@ -11195,7 +11196,7 @@ open_and_init_dwp_file (void) > >> > pulongest (dwp_file->cus->version), > >> > pulongest (dwp_file->tus->version), dwp_name.c_str ()); > >> > } > >> > - dwp_file->version = dwp_file->cus->version; > >> > + dwp_file->version = dwp_file->cus ? dwp_file->cus->version : 0; > >> > >> A version of 0 doesn't seem right. > >> I'd go with something like (untested and likely improperly formatted): > >> > >> if (dwp_file->cus) > >> dwp_file->version = dwp_file-cus->version; > >> else if (dwp_file->tus) > >> dwp_file->version = dwp_file->tus->version > >> else > >> dwp_file->version = 2; > >> > >> ok with that change. > >> > >> > > >> > if (dwp_file->version == 2) > >> > bfd_map_over_sections (dwp_file->dbfd, > >> > dwarf2_locate_v2_dwp_sections, > >> > > >> > > >> > Kind regards, > >> > Alexander Shaposhnikov > > > > >