From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-x443.google.com (mail-wr1-x443.google.com [IPv6:2a00:1450:4864:20::443]) by sourceware.org (Postfix) with ESMTPS id 3E27A385B834 for ; Tue, 24 Mar 2020 09:10:16 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 3E27A385B834 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-x443.google.com with SMTP id 65so3726966wrl.1 for ; Tue, 24 Mar 2020 02:10:16 -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:user-agent; bh=6yEjraYUGfxWkFKd1idaeegE8Q4GEhwRMbw0QV2b84E=; b=Qahx1axlHuHQIn5PR6tKaR1P18V2QOrwe8sg4rLA9w5Qj1puYIYUIT6uXFFpbsaNM2 iWeoLI4ytMGpht/uemabeIjMrddlFvtl4nYVHLMz49KbK3d70ucSlvJbtpM2SHBU+8Vy EzhC8DxD7wevqaCv0K1EkLGpYh4hExId5STnSJ42KgmlwnQBAyjk+e9GtwDrOEv8azoC fwITvdhU+pnYRRKUCrL2pAeT7qS/pNwVwyUcUqWShe4m69KsyOZfZP0KOuY2ew8JxnKc Sb21rgPtbtbTJuHnU9dIL3EbQNq1FcgMnCl00L1awHKVKBM/JJlUr7k60zM1pT+GH9pi 0nfg== 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:user-agent; bh=6yEjraYUGfxWkFKd1idaeegE8Q4GEhwRMbw0QV2b84E=; b=LB0uo3UZMKDyasa65j0Yyf7m9QZei/dznyGYp9SukOoGkr9q5AfQr9tZ8DRwP8l43i PNgE3SXq05mUR5www1m4AJ/1Cqwl5DPHRNDYo53IqJOmEDnYXX/w8AqQ6Pfiw7h4ecxC 1LWzY5HEbZ7/dbJqE5j6PCL6fpsFX6unHoT9FvFDEM8JePL4hS20Tp5K/OtlxkmGzeRz JcVTvLR9thTRSsRPLGQ5KsVPHQK8z7sMMuyhqvmPQDVMkx+qqSMvsgArIA2+mW2OQO1v URmvKIEMTn+BlHr7sjnat7TiUy3zcSYiRXgtJc/i11g8cjbJGhzL5Yv6CTlQnbRLVDJT FHMg== X-Gm-Message-State: ANhLgQ3Y6fULoMdPL/Dg89mmqrjRrDYcn1+QtcKE7YdEdZEjpMBo5GFM Q1U2hcoaRVk/zIFoyeNZpO9ZeA== X-Google-Smtp-Source: ADFU+vu6p2n/T1pYNPCrJNTs1k9EFSeTXCpzddpQOYwmH3HjrMh3hbFUxgoMatybeh8B1u4KE1hqQA== X-Received: by 2002:adf:e5cd:: with SMTP id a13mr35935953wrn.275.1585041015176; Tue, 24 Mar 2020 02:10:15 -0700 (PDT) Received: from localhost (host86-186-80-207.range86-186.btcentralplus.com. [86.186.80.207]) by smtp.gmail.com with ESMTPSA id b187sm3584381wmc.14.2020.03.24.02.10.14 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 24 Mar 2020 02:10:14 -0700 (PDT) Date: Tue, 24 Mar 2020 09:10:13 +0000 From: Andrew Burgess To: Bernd Edlinger Cc: "gdb-patches@sourceware.org" Subject: Re: [PATCHv2] Fix an undefined behavior in record_line Message-ID: <20200324091013.GT3317@embecosm.com> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Operating-System: Linux/4.18.19-100.fc27.x86_64 (x86_64) X-Uptime: 08:59:04 up 38 days, 20:27, X-Fortune: I've got a bad feeling about this. X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] User-Agent: Mutt/1.9.2 (2017-12-15) X-Spam-Status: No, score=-26.3 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, 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: Tue, 24 Mar 2020 09:10:17 -0000 * Bernd Edlinger [2020-03-23 22:25:42 +0100]: > On 3/22/20 4:25 AM, Bernd Edlinger wrote: > > On 3/13/20 12:55 PM, Bernd Edlinger wrote: > >> Additionally do not completely remove symbols > >> at the same PC than the end marker, instead > >> make them non-is-stmt breakpoints. > >> > >> Also fix the condition when the line table need to be resized, > >> that was wasting one element. I suspect this commit message has evolved overtime - having the first word be "additionally" seems a little strange. > >> > >> 2020-03-10 Bernd Edlinger > >> * buildsym.c (record_line): Fix ub and preserve lines at eof. Typo: ub -> up > >> --- > >> gdb/buildsym.c | 28 +++++++++++----------------- > >> 1 file changed, 11 insertions(+), 17 deletions(-) > >> > >> diff --git a/gdb/buildsym.c b/gdb/buildsym.c > >> index 7155db3..960a36c 100644 > >> --- a/gdb/buildsym.c > >> +++ b/gdb/buildsym.c > >> @@ -695,7 +695,7 @@ struct blockvector * > >> } > >> } > >> > >> - if (subfile->line_vector->nitems + 1 >= subfile->line_vector_length) > >> + if (subfile->line_vector->nitems >= subfile->line_vector_length) > >> { > >> subfile->line_vector_length *= 2; > >> subfile->line_vector = (struct linetable *) > >> @@ -705,27 +705,21 @@ struct blockvector * > >> * sizeof (struct linetable_entry)))); > >> } This part seems separate to what comes below I think. This should be a separate commit. > >> > >> - /* Normally, we treat lines as unsorted. But the end of sequence > >> - marker is special. We sort line markers at the same PC by line > >> - number, so end of sequence markers (which have line == 0) appear > >> - first. This is right if the marker ends the previous function, > >> - and there is no padding before the next function. But it is > >> - wrong if the previous line was empty and we are now marking a > >> - switch to a different subfile. We must leave the end of sequence > >> - marker at the end of this group of lines, not sort the empty line > >> - to after the marker. The easiest way to accomplish this is to > >> - delete any empty lines from our table, if they are followed by > >> - end of sequence markers. All we lose is the ability to set > >> - breakpoints at some lines which contain no instructions > >> - anyway. */ > >> + /* The end of sequence marker is special. We need to reset the > >> + is_stmt flag on previous lines at the same PC, otherwise these > >> + lines may cause problems. All we lose is the ability to set > >> + breakpoints at some lines which contain no instructions > >> - anyway. */ You need to expand on what "problems" means here. Someone coming back to this code in the future will have no idea why we're making this change, and with no tests for this commit they can't even try to figure out the "problems" by looking at a test. > >> if (line == 0 && subfile->line_vector->nitems > 0) > >> { > >> - e = subfile->line_vector->item + subfile->line_vector->nitems - 1; > >> - while (subfile->line_vector->nitems > 0 && e->pc == pc) > >> + e = subfile->line_vector->item + subfile->line_vector->nitems; > >> + do > >> { > >> e--; > >> - subfile->line_vector->nitems--; > >> + if (e->pc != pc || e->line == 0) > >> + break; > >> + e->is_stmt = 0; > >> } > >> + while (e > subfile->line_vector->item); > >> } > >> > >> e = subfile->line_vector->item + subfile->line_vectoms++; > >> > > Andrew, this is the place where currently the is-stmt entries > are deleted. With your is-stmt patch this code is executed in more > cases than before. Therefore I would suggest to convert them > to !is_stmt lines for now, but maybe in the long run add a new flag > that allows them to be used in the file:line case, but make these > lines behave differently when stepping, I am only trying to fix > the case where you step out of the subroutine. I'm super uncomfortable with any code that changes is-stmt to !is-stmt, as I worry about what we might be giving up. You say "All we lose is the ability to set breakpoints at some lines which contain no instructions anyway.", but I'll need to work through some examples to see what this actually means in practice before I can be happy with this change. Thanks, Andrew