diff mbox

Use -qualified flag when setting temporary breakpoint in start command

Message ID e5e172dc-56e9-4469-d900-5affcf025824@efficios.com
State New
Headers show

Commit Message

Simon Marchi April 9, 2019, 4:19 p.m. UTC 
On 2019-04-09 11:27 a.m., Pedro Alves wrote:
> On 4/9/19 3:55 AM, Simon Marchi wrote:
>> From: Simon Marchi <simon.marchi@efficios.com>
>>
>> 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 <simon.marchi@efficios.com>
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

Comments

Pedro Alves April 9, 2019, 4:30 p.m. UTC | #1 
On 4/9/19 5:19 PM, Simon Marchi wrote:

> 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.
> 

Thanks.

> gdb/ChangeLog:
> 
> 	* infcmd.c (run_command_1): Pass -qualified to tbreak when usind
> 	the "start" command.

Typo "usind".

> 
> gdb/testsuite/ChangeLog:
> 
> 	* gdb.base/start-cpp.exp: New file.
> 	* gdb.base/start-cpp.cc: New file.

Note start-cpp.cc wasn't included in this patch -- don't forget
to git add it.

Otherwise OK.

Thanks,
Pedro Alves
diff mbox

Patch

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 <http://www.gnu.org/licenses/>.
+
+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"