From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@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, 09 Apr 2019 16:19:00 -0000 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: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2019-04/txt/msg00145.txt.bz2 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" -- 2.21.0