From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-x441.google.com (mail-wr1-x441.google.com [IPv6:2a00:1450:4864:20::441]) by sourceware.org (Postfix) with ESMTPS id C6292383F84F for ; Tue, 16 Jun 2020 08:11:48 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org C6292383F84F 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-x441.google.com with SMTP id e1so19770445wrt.5 for ; Tue, 16 Jun 2020 01:11:48 -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=Dvgpw8qmL/DfommjOysrZJYUKLO1ClrXw+224cesh4I=; b=Pq/y5tL619pXfYVfL/uirH2Kaekcg1QQKTJB0IZr/wTSXP4PH+gGSEh7ir+KLTJJ4+ 32hrSzZ/josTs4Z7FapTYZ6JvKXlUHjTCOgOzpcY3dDw1Zp3WkzMwwHQ8d7j0qMCUtkU 2NpX9LmO/jghcJwUqcA2S8qYrcU6rxryw6S5x5hA4YPqfO+26T905cAFQ8bUgOv714gw GIimxcmYYSzJlDRRrtCS39KkvxYUWo6z598n3tzlsavbpTWC0916YvXOxGcz1yZghwkl diLwCVY35EggsV1Rtp/PTKzYV+PhS34I7ArPyqnhEby4SlqdqIxO2cXPgpCEjbjWrVVo +KTg== 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=Dvgpw8qmL/DfommjOysrZJYUKLO1ClrXw+224cesh4I=; b=cHe7nTu7UqAO55o47bCVbsneC4QTORbxkX4lc2zu519DBF6pYB5Op6YtwsXjD0SYVL nYLdCgC8NdaXWm4ahCuR6/F5EglGiXv9Lf6WdgAUWDiqYzqdgwprd3Kp2d3o19nKemyM Q0IemAPaGjSVSXWe9JFEapk0cHSP8R9RKsbj+QVYpVkas4euX+zvIdTWLp5CJfN9jyqu 8rQLB1H/nitZd/x/SJdY3gHDcAmaCjxYL4Tpthvsamgi+itA7h4iZWTZEhck1BOqbcRU zBZNgc0vMrRRmj3LZg6ST/GU4LMwQj7i6KDYMbc2ALAeLfshY74STIaMREpmZhfO6Ow3 u6bg== X-Gm-Message-State: AOAM531MecHY2+AbXLqE++HyD6dnBLQB37lRuqZQyCign/d+9TKadvtD DI5RREk2ZCR5nEFezXsxQYZVnDmgAgY= X-Google-Smtp-Source: ABdhPJxj3fd9R5sh9EaEGxyogPgJ28tODXu+qD8nZsUz/eGq+5eXfnmxvvUfKINHApR3iYpv+rA9cw== X-Received: by 2002:a5d:6283:: with SMTP id k3mr1681010wru.422.1592295107869; Tue, 16 Jun 2020 01:11:47 -0700 (PDT) Received: from localhost (host86-128-12-16.range86-128.btcentralplus.com. [86.128.12.16]) by smtp.gmail.com with ESMTPSA id s8sm27721076wrm.96.2020.06.16.01.11.47 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 16 Jun 2020 01:11:47 -0700 (PDT) Date: Tue, 16 Jun 2020 09:11:46 +0100 From: Andrew Burgess To: Bernd Edlinger Cc: "gdb-patches@sourceware.org" Subject: Re: [PATCH] Fix an undefined behavior in record_line Message-ID: <20200616081146.GR2737@embecosm.com> References: <20200616080815.GQ2737@embecosm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200616080815.GQ2737@embecosm.com> X-Operating-System: Linux/5.6.15-200.fc31.x86_64 (x86_64) X-Uptime: 09:09:56 up 7 days, 22:16, X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] X-Spam-Status: No, score=-10.1 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_BARRACUDACENTRAL, 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, 16 Jun 2020 08:11:50 -0000 Bernd, I apologise for replying to this old email. For some reason my mail client brought this to the top of my list - I think I tagged it as 'todo' long ago and never untagged it. I'm not really sure :-/ Anyway, I should have double checked the date before replying. I didn't, and I apologise. Thanks, Andrew * Andrew Burgess [2020-06-16 09:08:15 +0100]: > * Bernd Edlinger [2020-03-13 12:15:35 +0100]: > > > Additionally do not completely remove symbols > > at the same PC than the end marker, instead > > make them non-is-stmt breakpoints. > > You need to be careful, symbols means something completely different > within GDB, you're talking about line table entries here. > > Also, we generally split different functionality into different > patches, so I would expect to see at least two patches here, one > fixing the undefined behaviour and resize issue, and another for the > change in behaviour deleting line markers vs setting them to > non-is-stmt. > > The patch for changing from deleting empty lines to making them > non-is-stmt would need some additional justification text I think, why > is this important? Also, it would ideally have some tests attached, > or, if this change fixes some existing test, would reference that test > in the commit message. > > I do remember looking at a version of this patch before, and I worked > through the code and did manage to figure out what the undefined > behaviour was that you were fixing. But I can't remember now what the > problem was. > > Please could you expand the commit message to explain what the > undefined behaviour actually is that your fixing. > > > > > Also fix the condition when the line table need to be resized, > > that was wasting one element. > > > > 2020-03-10 Bernd Edlinger > > * buildsym.c (record_line): Fix ub and preserve lines at eof. > > Please expand 'ub' to 'undefined behaviour' or at the very least > capitalise to UB, it's not a commonly used acronym and it just looks > like a type (to me). > > Thanks, > Andrew > > > --- > > gdb/buildsym.c | 28 +++++++++++----------------- > > 1 file changed, 11 insertions(+), 17 deletions(-) > > > > diff --git a/gdb/buildsym.c b/gdb/buildsym.c > > index 7155db3..e090fdb 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)))); > > } > > > > - /* 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. */ > > 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) > > + break; > > + e->is_stmt = 0; > > } > > + while (e > subfile->line_vector->item); > > } > > > > e = subfile->line_vector->item + subfile->line_vector->nitems++; > > -- > > 1.9.1