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 991573840C31 for ; Tue, 16 Jun 2020 08:08:18 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 991573840C31 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 p5so19697834wrw.9 for ; Tue, 16 Jun 2020 01:08:18 -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=FMerFk/fjzWqpiWHt1LRK+UbSRRp5FnX8R+u6TvGj4U=; b=U36WxZKKFphvJp2Kr4sv3ImTYA5r+LhT464f4AYdjJ+Q5KBZjZ8KXUem6kk9XUgVl0 ANu2bGRBPewVj9v0nDmW4na0o0CoWAe+DGypw/YTLQ/Kie11TRMNuMv3t782sl7Vm9mN R2ttZJiIzvfSu7s088tJSg7w6WVhlVBXqkiVBs3VoDRUvvYf32gA9KLZw6bOyKxl7ybL 2pGd9h8QVhhdvaPDbZVVbY5UvrJtyx7WB4IbO7xzZBtK/mymaoczFRmCa0LCFwZXYMcd qpi1WPS8hlU6+Ph+oisbS62VfNpElMPpY0btfKrXQ67WY1Pjw/JyJje39cF1GPAdfFhb At8A== 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=FMerFk/fjzWqpiWHt1LRK+UbSRRp5FnX8R+u6TvGj4U=; b=qs10dZPFLpq2E/eG29Dkw2k21uDUwfglhs3/R5EQQAfo7pQMwHr/N539vsOqYclpeY 4vQepRc2KYe9f4m60JREkbXQPBsoFl4lL4k4GV17P3m6sIESrDr8n9ORTV1Wa92oIo3U f3Zk6OAnldWEkF860jIh02rUX8mjEPM+3XyeP3eqz4EomOx0O1daeY30zvM7G09BUQ1I 0S9RQk3cuCVHX3D+DMpWAZxrRvMfCWNkwXzDz+FFBu2iQ4a3ehr2JLRbSb8w4P6S9+/a PJM1qxzjwLotjVEe52/BRtA22LSVCC7Wbio4BSCxB5p0xRETMMlY9vGLvQh6o7c8CDIS SIUA== X-Gm-Message-State: AOAM531QaR9vtWcMLPlCHX4ys4kxTNQqN7OMhwsoWACuWcQhOiY7Mwjo LcaXIJ+xr/j6XOs5ne05Qt9mYA== X-Google-Smtp-Source: ABdhPJx3UeM74vzEp7jsnA/24oPnF334VAWvnJ9SsJzlfVOriuS/+mQE7CyOff6P0iWSRPZHYo41ug== X-Received: by 2002:adf:9163:: with SMTP id j90mr1675605wrj.65.1592294897638; Tue, 16 Jun 2020 01:08:17 -0700 (PDT) Received: from localhost (host86-128-12-16.range86-128.btcentralplus.com. [86.128.12.16]) by smtp.gmail.com with ESMTPSA id v27sm30745404wrv.81.2020.06.16.01.08.16 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 16 Jun 2020 01:08:16 -0700 (PDT) Date: Tue, 16 Jun 2020 09:08:15 +0100 From: Andrew Burgess To: Bernd Edlinger Cc: "gdb-patches@sourceware.org" Subject: Re: [PATCH] Fix an undefined behavior in record_line Message-ID: <20200616080815.GQ2737@embecosm.com> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Operating-System: Linux/5.6.15-200.fc31.x86_64 (x86_64) X-Uptime: 09:00:18 up 7 days, 22:07, 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:08:22 -0000 * 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