From patchwork Wed Jun 12 12:34:03 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andrew Burgess X-Patchwork-Id: 33092 Received: (qmail 2965 invoked by alias); 12 Jun 2019 12:34:13 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 2957 invoked by uid 89); 12 Jun 2019 12:34:12 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.5 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.1 spammy=HX-Spam-Relays-External:209.85.128.67, H*RU:209.85.128.67, linespec.c, linespecc X-HELO: mail-wm1-f67.google.com Received: from mail-wm1-f67.google.com (HELO mail-wm1-f67.google.com) (209.85.128.67) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 12 Jun 2019 12:34:10 +0000 Received: by mail-wm1-f67.google.com with SMTP id x15so6399573wmj.3 for ; Wed, 12 Jun 2019 05:34:10 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=from:to:cc:subject:date:message-id; bh=E9dJIhtPGhg1e7+7dX0ZEON7aJ7RPpKEp4NM5Sysgtw=; b=K4fi//F5Rgo8/Q8f2qsaHgEbRSjjW45/CWN/yulHvY3EdXo1fxBhe7sSa4vkl8Y4ej woQ7H+G6gWvg+ftqIjYjfE1d6mUhO+LsKe7w1525l+MpRwbtJmbdgV9KnCeA3v7tUmcQ hWB/xGWLPtYARjwaMLfHOWeDNapZl1GtdtuYEoVDj1+nnSFciD8gZ9OSrItrS7enGRtI TnsjGIC+ShqNKclV82ZszRe4icj3DOqiJXPPYdCBJmi4G/tvUqTN732oHCJjedTPfLzb EZufUQxD1zxNAiZVQOExvDfae+qvjzL6Cb0JVVwXfLm/GnUrOMWMvfyPCDHpvDqiM+Jc XhRQ== Return-Path: Received: from localhost (host86-180-62-212.range86-180.btcentralplus.com. [86.180.62.212]) by smtp.gmail.com with ESMTPSA id d10sm26408159wrh.91.2019.06.12.05.34.06 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Wed, 12 Jun 2019 05:34:07 -0700 (PDT) From: Andrew Burgess To: gdb-patches@sourceware.org Cc: Andrew Burgess Subject: [PATCH] gdb: Don't skip prologue for explicit line breakpoints in assembler Date: Wed, 12 Jun 2019 13:34:03 +0100 Message-Id: <20190612123403.14348-1-andrew.burgess@embecosm.com> X-IsSubscribed: yes It was observed that in some cases, placing a breakpoint in an assembler file using filename:line-number syntax would result in the breakpoint being placed at a different line within the file. For example, consider this x86-64 assembler: test: push %rbp /* Break here. */ mov %rsp, %rbp nop /* Stops here. */ The user places the breakpoint using file:line notation targeting the line marked 'Break here', GDB actually stops at the line marked 'Stops here'. The reason is that the label 'test' is identified as the likely start of a function, and the call to symtab.c:skip_prologue_sal causes GDB to skip forward over the instructions that GDB believes to be part of the prologue. I believe however, that when debugging assembler code, where the user has instruction-by-instruction visibility, if they ask for a specific line, GDB should (as far as possible) stop on that line, and not perform any prologue skipping. I don't believe that the behaviour of higher level languages should change, in these cases skipping the prologue seems like the correct thing to do. In order to implement this change I needed to extend our current tracking of when the user has requested an explicit line number. We already tracked this in some cases, but not in others (see the changes in linespec.c). However, once I did this I started to see some additional failures (in tests gdb.base/break-include.exp gdb.base/ending-run.exp gdb.mi/mi-break.exp gdb.mi/mi-reverse.exp gdb.mi/mi-simplerun.exp) where we currently expected a breakpoint placed at one file and line number to be updated to reference a different line number, this was fixed by removing some code in symtab.c:skip_prologue_sal. My concern here is that removing this check didn't cause anything else to fail. I have a new test that covers my original case, this is written for x86-64 as most folk have access to such a target, however, any architecture that has a prologue scanner can be impacted by this change. gdb/ChangeLog: * linespec.c (decode_digits_list_mode): Set explicit_line to a bool value. (decode_digits_ordinary): Set explicit_line field in sal. * symtab.c (skip_prologue_sal): Don't skip prologue for a symtab_and_line that was set on an explicit line number in assembler code. Do always update the recorded symtab and line if we do skip the prologue. gdb/testsuite/ChangeLog: * gdb.arch/amd64-break-on-asm-line.S: New file. * gdb.arch/amd64-break-on-asm-line.exp: New file. --- gdb/ChangeLog | 10 +++++++ gdb/linespec.c | 3 +- gdb/symtab.c | 15 ++++++---- gdb/testsuite/ChangeLog | 5 ++++ gdb/testsuite/gdb.arch/amd64-break-on-asm-line.S | 35 ++++++++++++++++++++++ gdb/testsuite/gdb.arch/amd64-break-on-asm-line.exp | 35 ++++++++++++++++++++++ 6 files changed, 96 insertions(+), 7 deletions(-) create mode 100644 gdb/testsuite/gdb.arch/amd64-break-on-asm-line.S create mode 100644 gdb/testsuite/gdb.arch/amd64-break-on-asm-line.exp diff --git a/gdb/linespec.c b/gdb/linespec.c index 94400f3f336..1d8d2ca3273 100644 --- a/gdb/linespec.c +++ b/gdb/linespec.c @@ -4102,7 +4102,7 @@ decode_digits_list_mode (struct linespec_state *self, val.symtab = elt; val.pspace = SYMTAB_PSPACE (elt); val.pc = 0; - val.explicit_line = 1; + val.explicit_line = true; add_sal_to_sals (self, &values, &val, NULL, 0); } @@ -4136,6 +4136,7 @@ decode_digits_ordinary (struct linespec_state *self, sal.pspace = SYMTAB_PSPACE (elt); sal.symtab = elt; sal.line = line; + sal.explicit_line = true; sal.pc = pc; sals.push_back (std::move (sal)); } diff --git a/gdb/symtab.c b/gdb/symtab.c index 4920d94a247..c10e6b3e358 100644 --- a/gdb/symtab.c +++ b/gdb/symtab.c @@ -3693,6 +3693,15 @@ skip_prologue_sal (struct symtab_and_line *sal) if (sal->explicit_pc) return; + /* In assembly code, if the user asks for a specific line then we should + not adjust the SAL. The user already has instruction level + visibility in this case, so selecting a line other than one requested + is likely to be the wrong choice. */ + if (sal->symtab != nullptr + && sal->explicit_line + && SYMTAB_LANGUAGE (sal->symtab) == language_asm) + return; + scoped_restore_current_pspace_and_thread restore_pspace_thread; switch_to_program_space_and_thread (sal->pspace); @@ -3812,12 +3821,6 @@ skip_prologue_sal (struct symtab_and_line *sal) sal->pc = pc; sal->section = section; - - /* Unless the explicit_line flag was set, update the SAL line - and symtab to correspond to the modified PC location. */ - if (sal->explicit_line) - return; - sal->symtab = start_sal.symtab; sal->line = start_sal.line; sal->end = start_sal.end; diff --git a/gdb/testsuite/gdb.arch/amd64-break-on-asm-line.S b/gdb/testsuite/gdb.arch/amd64-break-on-asm-line.S new file mode 100644 index 00000000000..698c3e6d624 --- /dev/null +++ b/gdb/testsuite/gdb.arch/amd64-break-on-asm-line.S @@ -0,0 +1,35 @@ +/* Copyright 2019 Free Software Foundation, Inc. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see . + + This file is part of the gdb testsuite. + + Test that a breakpoint place by line number in an assembler file + will stop at the specified line. Previously versions of GDB have + incorrectly invoked the prologue analysis logic and skipped + forward. */ + + .text + .global main +main: + nop +test: + /* The next two instructions are required to look like an + x86-64 prologue so that GDB's prologue scanner will spot + them and skip forward. */ + push %rbp /* Break here. */ + mov %rsp, %rbp + nop /* Incorrect. */ + nop + nop diff --git a/gdb/testsuite/gdb.arch/amd64-break-on-asm-line.exp b/gdb/testsuite/gdb.arch/amd64-break-on-asm-line.exp new file mode 100644 index 00000000000..6218ce541bd --- /dev/null +++ b/gdb/testsuite/gdb.arch/amd64-break-on-asm-line.exp @@ -0,0 +1,35 @@ +# Copyright 2019 Free Software Foundation, Inc. + +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see . + +if { ![istarget x86_64-*-* ] || ![is_lp64_target] } { + return +} + +standard_testfile .S + +if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile} \ + { debug }] } { + untested "could not compile" + return -1 +} + +if ![runto_main] { + untested "could not run to main" + return -1 +} + +gdb_breakpoint [gdb_get_line_number "Break here"] +gdb_continue_to_breakpoint "Break on specified line" \ + ".*/\\* Break here\\. \\*/.*"