From patchwork Tue Apr 9 16:19:09 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Simon Marchi X-Patchwork-Id: 32221 Received: (qmail 67068 invoked by alias); 9 Apr 2019 16:19:30 -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 67060 invoked by uid 89); 9 Apr 2019 16:19:29 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-18.6 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=6047 X-HELO: mail.efficios.com Received: from mail.efficios.com (HELO mail.efficios.com) (167.114.142.138) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 09 Apr 2019 16:19:27 +0000 Received: from localhost (ip6-localhost [IPv6:::1]) by mail.efficios.com (Postfix) with ESMTP id EF69E7DB2D; Tue, 9 Apr 2019 12:19:25 -0400 (EDT) Received: from mail.efficios.com ([IPv6:::1]) by localhost (mail02.efficios.com [IPv6:::1]) (amavisd-new, port 10032) with ESMTP id LB_x9XIkWfCL; Tue, 9 Apr 2019 12:19:25 -0400 (EDT) Received: from localhost (ip6-localhost [IPv6:::1]) by mail.efficios.com (Postfix) with ESMTP id 008647DB23; Tue, 9 Apr 2019 12:19:25 -0400 (EDT) DKIM-Filter: OpenDKIM Filter v2.10.3 mail.efficios.com 008647DB23 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=efficios.com; s=default; t=1554826765; bh=K2bcjdZ6fM8OJMm7AD0Rq52d0lpCtTJSd8GjGwTDAsI=; h=To:From:Message-ID:Date:MIME-Version; b=BEDqPxwff25dp6u2gVmTjskIQ3e2mQ73pLzA+JLx0DUfATIJKJXKJS8+Pl3SlFZ7c MN++OQvfprG9l1f7KyxXPiMR33rnEFKuGjIBHdEg2XXP7FXqbJMtcVjXfObFU1Bt87 cyjCaJDN1o8SBas8FLEinCPZEP6zmMlYf8dP7O86s2LH1YP5wCjJXTpuAX9BuJaahW jzVRgrH5h8ZTLicASmzlb73mAIMAOqsyrIuuZac/ASS4mj1jf/7TFwyQ01ADZVtLUw IEr7MTYKdgBciZGZzEM8gmyVln/at6KFqTN9TsWK5tyVwLaJg1X0XyDT0n2WYcN4C8 vA2fXoTIl6ILA== Received: from mail.efficios.com ([IPv6:::1]) by localhost (mail02.efficios.com [IPv6:::1]) (amavisd-new, port 10026) with ESMTP id l3JWx1Vs_ArM; Tue, 9 Apr 2019 12:19:24 -0400 (EDT) Received: from [172.16.0.120] (192-222-157-41.qc.cable.ebox.net [192.222.157.41]) by mail.efficios.com (Postfix) with ESMTPSA id 8B7A77DB1C; Tue, 9 Apr 2019 12:19:10 -0400 (EDT) Subject: Re: [PATCH] Use -qualified flag when setting temporary breakpoint in start command To: Pedro Alves , Simon Marchi , gdb-patches@sourceware.org References: <20190409025557.28846-1-simon.marchi@polymtl.ca> From: Simon Marchi Message-ID: Date: Tue, 9 Apr 2019 12:19:09 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.6.1 MIME-Version: 1.0 In-Reply-To: On 2019-04-09 11:27 a.m., Pedro Alves wrote: > On 4/9/19 3:55 AM, Simon Marchi wrote: >> From: Simon Marchi >> >> When using the "start" command, GDB puts a temporary breakpoint on the >> "main" symbol (we literally invoke the tbreak command). However, since >> it does wild matching by default, it also puts a breakpoint on any C++ >> method or "main" function in a namespace. For example, when debugging >> GDB, it creates a total of 24 locations: >> >> (gdb) start >> Temporary breakpoint 1 at 0x198c1e9: main. (24 locations) >> >> as there are a bunch of methods called main in the selftests, such as >> >> selftests::string_view::capacity_1::main() >> >> If such method was called in the constructor of a global object, or a >> function marked with the attribute "constructor", then we would stop at >> the wrong place. Also, this causes a few extra symtabs (those that >> contain the "wrong" mains) to be expanded for nothing. >> >> The dummiest, most straightforward solution is to add -qualified when >> invoking tbreak. With this patch, "start" creates a single-location >> breakpoint, as expected. >> > > That seems fine. > >> I changed the start.exp test to use a C++ test file, which contains two >> main functions. The test now verifies that the output of "start" is the >> output we get when we set a single-location breakpoint. > > I'm mildly concerned that this drops testing with C, though. Given > that "start" is a basic command, and that C++ symbol/name matching > differs from C, and considering that some targets don't even > support C++ (considering extended-remote too), I'd think it to > be prudent to test both C and C++. I wouldn't say it's a big > deal, but, the .exp file is small, so it shouldn't be much of > a maintenance burden to copy & adjust it as a separate .exp > file, IMHO. I had initially changed start.exp to test both C and C++, then decided to just keep C++ for simplicity. But your point about coverage is good, so I've done as you suggested. >> >> gdb/ChangeLog: >> >> * infcmd.c (run_command_1): Pass -qualified to tbreak when usind >> the "start" command. >> >> gdb/testsuite/ChangeLog: >> >> * gdb.base/start.exp: Change test file to start.cpp. Enhance >> regexp to match output of single-location breakpoint. >> * gdb.base/start.cpp: New file. > > Nit: all other C++ source files in the testsuite use .cc extension. Fixed. >> +namespace foo >> +{ >> + >> +int main () >> +{ >> + return 1; >> +} >> + >> +} /* namespace foo */ >> + >> +int main() > > Watch out for GNU formatting. Woops. The blame is shared between the original start.c file and me not doing enough GDB these days. Here is the updated patch. From 232b274adfc6904d33bde3baa52e40836af6221b Mon Sep 17 00:00:00 2001 From: Simon Marchi Date: Mon, 8 Apr 2019 22:55:57 -0400 Subject: [PATCH] Use -qualified flag when setting temporary breakpoint in start command When using the "start" command, GDB puts a temporary breakpoint on the "main" symbol (we literally invoke the tbreak command). However, since it does wild matching by default, it also puts a breakpoint on any C++ method or "main" function in a namespace. For example, when debugging GDB, it creates a total of 24 locations: (gdb) start Temporary breakpoint 1 at 0x198c1e9: main. (24 locations) as there are a bunch of methods called main in the selftests, such as selftests::string_view::capacity_1::main() If such method was called in the constructor of a global object, or a function marked with the attribute "constructor", then we would stop at the wrong place. Also, this causes a few extra symtabs (those that contain the "wrong" mains) to be expanded for nothing. The dummiest, most straightforward solution is to add -qualified when invoking tbreak. With this patch, "start" creates a single-location breakpoint, as expected. I copied the start.exp test to start-cpp.exp and made it use a C++ test file, which contains two main functions. The new test verifies that the output of "start" is the output we get when we set a single-location breakpoint. gdb/ChangeLog: * infcmd.c (run_command_1): Pass -qualified to tbreak when usind the "start" command. gdb/testsuite/ChangeLog: * gdb.base/start-cpp.exp: New file. * gdb.base/start-cpp.cc: New file. --- gdb/infcmd.c | 5 +++- gdb/testsuite/gdb.base/start-cpp.exp | 37 ++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 1 deletion(-) create mode 100644 gdb/testsuite/gdb.base/start-cpp.exp diff --git a/gdb/infcmd.c b/gdb/infcmd.c index 3b26fd4a4671..178f89e95207 100644 --- a/gdb/infcmd.c +++ b/gdb/infcmd.c @@ -604,7 +604,10 @@ run_command_1 (const char *args, int from_tty, enum run_how run_how) /* Insert temporary breakpoint in main function if requested. */ if (run_how == RUN_STOP_AT_MAIN) - tbreak_command (main_name (), 0); + { + std::string arg = string_printf ("-qualified %s", main_name ()); + tbreak_command (arg.c_str (), 0); + } exec_file = get_exec_file (0); diff --git a/gdb/testsuite/gdb.base/start-cpp.exp b/gdb/testsuite/gdb.base/start-cpp.exp new file mode 100644 index 000000000000..5f98b92ffa41 --- /dev/null +++ b/gdb/testsuite/gdb.base/start-cpp.exp @@ -0,0 +1,37 @@ +# Copyright 2005-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 . + +standard_testfile .cc + +if {[prepare_for_testing "failed to prepare" $testfile $srcfile debug]} { + return -1 +} + +# This is a testcase specifically for the `start' GDB command. For regular +# stop-in-main goal in the testcases consider using `runto_main' instead. + +# In this C++ version of the test (as opposed to start.exp), we specifically +# test that the temporary breakpoint created by the start command has a single +# location, even if we have a function named "main" in a non-root namespace. + +# For C++ programs, "start" should stop in main(). +if { [gdb_start_cmd] < 0 } { + untested start + return -1 +} + +gdb_test "" \ + "Temporary breakpoint $decimal at $hex: file.*main \\(\\) at .*start-cpp.cc:.*" \ + "start"