From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-x442.google.com (mail-wr1-x442.google.com [IPv6:2a00:1450:4864:20::442]) by sourceware.org (Postfix) with ESMTPS id E66CA3851C39 for ; Mon, 20 Jul 2020 16:57:23 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org E66CA3851C39 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=embecosm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=andrew.burgess@embecosm.com Received: by mail-wr1-x442.google.com with SMTP id a15so3619692wrh.10 for ; Mon, 20 Jul 2020 09:57:23 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=E8VXeKLgy2doeejDiEjGgJ+v6mnXHLv1cC4GaJW/rGA=; b=FTNTtQlzHhUSHTiMr9SBzxAeMYDpWmNz9VamGxXHJvD6NR0CNFdVTCa9rFpoWmMONn d5oiicQe7CssTKpjwGCTnm9gZrmFUDB4kvQabZKP9ZamH6rBteOGGxg97/R6FC9BSoE9 ICoTfqukby+GIrk8MS+FbFenuiS/lxQXuhrjEDGhs05IayI/EpoVL6hkGhUf5bSfqlB6 pNRaQbk1wFEZd7z0FbSqqFuUNJCgwtXPO1Vi60w34p4+jupC7gbsO3OdwuaIvqp192o1 BKfxnwNM7nh/Yiz5rAPtwZk6sm37H4RPAhY3zXDC8Se3q8HN1f8fBouBtScr0H2bnj6d w5jQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=E8VXeKLgy2doeejDiEjGgJ+v6mnXHLv1cC4GaJW/rGA=; b=WxKP2T//Ozd6So1O8kHSnQ48rjBfv93JoduUFyB7BQkVi4CBmIuSryjufwWLcXabKO gOsDFLI2ImWOnZInM8a6vyJIOLxKyOisFe+a8XHUYCIZ8yrPuQuA8j2ii/iXdv9Gb8UF pu6CLxYPpBMk+FSZqmSucsW0JuIZY/4VUh7XvTipn2dHUk3lwoIQfyCyfydEk3zAIySl 2lLGD5Y4MD7E4Ic1c2GwpVeTDgy5X0tFpjEN0cQwyIgYWexhL853moSDGVqxT0xRKIfa l7niaxs47GJ2vqr5s6N5K6xQZkTCF1+b8KbG09sEW4Gb7Rz3xjivSe53zTNGxu1KcMeu f/jA== X-Gm-Message-State: AOAM530A9ksj/5PgdpneGk5xPTySH4+/fdM9PpaE5D3p+yGWBaytzzZW ZY5xC/HkPEljPfg/T2OjWd6Rukt7zzY= X-Google-Smtp-Source: ABdhPJw03Ot2GssvHW0OgptQmTo49lglSzfu8vVTQbP5eYUZEC8lADWcsmm0Tj65tr2FWrq5CVLHRg== X-Received: by 2002:adf:e68f:: with SMTP id r15mr14530917wrm.196.1595264242939; Mon, 20 Jul 2020 09:57:22 -0700 (PDT) Received: from localhost (host86-134-151-238.range86-134.btcentralplus.com. [86.134.151.238]) by smtp.gmail.com with ESMTPSA id w16sm37401748wrg.95.2020.07.20.09.57.21 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 20 Jul 2020 09:57:22 -0700 (PDT) Date: Mon, 20 Jul 2020 17:57:20 +0100 From: Andrew Burgess To: Simon Marchi Cc: gdb-patches@sourceware.org Subject: Re: [PATCH] gdb: Don't hard code 0 as end marker in GDB's line table Message-ID: <20200720165720.GA853475@embecosm.com> References: <20200720125505.1506140-1-andrew.burgess@embecosm.com> <81619b06-4380-7543-1c0c-aa504bb0a0ea@simark.ca> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <81619b06-4380-7543-1c0c-aa504bb0a0ea@simark.ca> X-Operating-System: Linux/5.6.15-200.fc31.x86_64 (x86_64) X-Uptime: 17:55:35 up 2 days, 2:10, X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] X-Spam-Status: No, score=-4.8 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) 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: Mon, 20 Jul 2020 16:57:25 -0000 * Simon Marchi [2020-07-20 12:01:21 -0400]: > On 2020-07-20 8:55 a.m., Andrew Burgess wrote: > > Currently GDB hard-codes the use of 0 as the end of sequence marker in > > its line tables. After this commit GDB now uses a named > > constant (linetable_entry::end_marker) as the end of sequence marker. > > The value of this constant is -1, not 0. This change was made in > > order to aid fixing bug PR gdb/26243. > > > > Bug PR gdb/26243 is about allowing line number 0 to be used to > > indicate a program address that has no corresponding source line (this > > use is specified in the DWARF standard). Currently GDB can't use line > > number 0 as this line number is hard coded as the end of sequence > > marker, but, after this commit line number 0 no longer has any special > > meaning. > > > > This commit does not fix PR gdb/26243, but it is step towards allowing > > that issue to be fixed. > > > > gdb/ChangeLog: > > > > PR gdb/26243 > > * buildsym.c (buildsym_compunit::record_line): Add an assert for > > the incoming line number. Update comments to not mention 0 > > specifically. Update to check for linetable_entry::end_marker > > rather than 0. > > * disasm.c (do_mixed_source_and_assembly_deprecated): Check for > > linetable_entry::end_marker not 0. > > (do_mixed_source_and_assembly): Likewise. > > * dwarf2/read.c (dwarf_finish_line): Pass > > linetable_entry::end_marker not 0. > > * mdebugread.c (add_line): Set is_stmt field. > > * python/py-linetable.c (ltpy_get_all_source_lines): Check for > > linetable_entry::end_marker not 0, update comments. > > (ltpy_iternext): Likewise. > > * record-btrace.c (btrace_find_line_range): Likewise. > > * symmisc.c (maintenance_print_one_line_table): Likewise. > > * symtab.c (find_pc_sect_line): Likewise. > > (find_line_symtab): Likewise. > > (skip_prologue_using_sal): Likewise. > > * symtab.h (linetable_entry::end_marker): New const static member > > variable. Add a static assert for this field. > > * xcoffread.c (arrange_linetable): Check for > > linetable_entry::end_marker not 0. > > I think the idea is good. But we need to make sure to audit all the places > that use linetable_entry::line to see if they now need to treat the value 0 > specially. One case that came to mind while reading your patch is > "disassemble /s". Here's an example using bug 26243's reproducer, showing > the end of "disassemble /s tree_insert". The instruction at 0x40135f maps > to line 0. > > This is with GDB 8.3: > > --- > 38 else > 39 root->right = make_node(val); > 0x000000000040134a <+138>: mov -0xc(%rbp),%edi > 0x000000000040134d <+141>: callq 0x401250 > 0x0000000000401352 <+146>: mov -0x8(%rbp),%rcx > 0x0000000000401356 <+150>: mov %rax,0x10(%rcx) > > 40 } > 0x000000000040135a <+154>: jmpq 0x40135f > 0x000000000040135f <+159>: jmpq 0x401364 > > 41 } > 0x0000000000401364 <+164>: add $0x10,%rsp > 0x0000000000401368 <+168>: pop %rbp > 0x0000000000401369 <+169>: retq > --- > > This made 0x40135f appear as if it was part of the previous line. > > This is with GDB master: > > --- > 38 else > 39 root->right = make_node(val); > 0x000000000040134a <+138>: mov -0xc(%rbp),%edi > 0x000000000040134d <+141>: call 0x401250 <_Z9make_nodei> > 0x0000000000401352 <+146>: mov -0x8(%rbp),%rcx > 0x0000000000401356 <+150>: mov %rax,0x10(%rcx) > > 40 } > 0x000000000040135a <+154>: jmp 0x40135f <_Z11tree_insertP4nodei+159> > > unknown: > --- no source info for this pc --- > 0x000000000040135f <+159>: jmp 0x401364 <_Z11tree_insertP4nodei+164> > > test.cpp: > 26 { > 27 if (val < root->id) > 28 { > 29 if (root->left) > 30 tree_insert (root->left, val); > 31 else > 32 root->left = make_node(val); > 33 } > 34 else if (val > root->id) > 35 { > 36 if (root->right) > 37 tree_insert (root->right, val); > 38 else > 39 root->right = make_node(val); > 40 } > 41 } > 0x0000000000401364 <+164>: add $0x10,%rsp > 0x0000000000401368 <+168>: pop %rbp > 0x0000000000401369 <+169>: ret > End of assembler dump. > --- > > I don't know what happens here, but it doesn't look good. The "no source info for this pc" > part is ok, but the "unknown:" and source lines 26-40 look wrong. > > This is with GDB master + your patch: > > --- > 38 else > 39 root->right = make_node(val); > 0x000000000040134a <+138>: mov -0xc(%rbp),%edi > 0x000000000040134d <+141>: call 0x401250 <_Z9make_nodei> > 0x0000000000401352 <+146>: mov -0x8(%rbp),%rcx > 0x0000000000401356 <+150>: mov %rax,0x10(%rcx) > > 40 } > 0x000000000040135a <+154>: jmp 0x40135f <_Z11tree_insertP4nodei+159> > > Line number 0 out of range; test.cpp has 80 lines. > --- > > The disassembly gets interrupted, because the line number is invalid. > > In this case, line 0 should probably get handled specially so that we print > the "no source info for this pc" message and keep printing the rest correctly. > > So, I think we just need to be careful and look at the possible behavior changes > of all these spots, not just blindly change `line == 0` for `line == end_marker`. Thanks for pointing this out. I'll investigate. I'm confused as to how we could ever have been handling 'line == 0' as anything other than 'line == end_marker', or how these 'line == 0' entries were coming from, but I'll start with the example you gave, and I'm sure it'll make sense. Thanks, Andrew