Use -qualified flag when setting temporary breakpoint in start command

  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.

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.

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.
---
 gdb/infcmd.c                     |  5 ++++-
 gdb/testsuite/gdb.base/start.cpp | 31 +++++++++++++++++++++++++++++++
 gdb/testsuite/gdb.base/start.exp |  4 ++--
 3 files changed, 37 insertions(+), 3 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/start.cpp
  

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.

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

> +namespace foo
> +{
> +
> +int main ()
> +{
> +  return 1;
> +}
> +
> +} /* namespace foo */
> +
> +int main()

Watch out for GNU formatting.

Thanks,
Pedro Alves
  

On Mon, Apr 08, 2019 at 10:55:57PM -0400, 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:

I wonder whether there's still a chance to have a(n additional) way
to specify the effect of -qualified using a syntax that is the same
across GDB versions.

I have pretty much the same effect like Simon for 'start' for a feature
'Break on abort()' in 'my' IDE, that post-8.1 triggers on any function
called 'abort'.

Even with the Python interface there seems to be no way to have
a single way to set the breakpoint short of having Pre/Post81Breakpoint
classes using different base constructors and try:/except...: 
to find the matching version.

To be honest, I am tempted to call the whole pattern matching on
function names a mis-feature. C++ name resolution is not really
compatible with regexps, so at the very least when naming the
global explicitly ('b ::abort') there should be no match on
'struct Foo { void abort() {} };'

Andre'
  

On 2019-04-09 13:12, André Pönitz wrote:
> I wonder whether there's still a chance to have a(n additional) way
> to specify the effect of -qualified using a syntax that is the same
> across GDB versions.
> 
> I have pretty much the same effect like Simon for 'start' for a feature
> 'Break on abort()' in 'my' IDE, that post-8.1 triggers on any function
> called 'abort'.
> 
> Even with the Python interface there seems to be no way to have
> a single way to set the breakpoint short of having Pre/Post81Breakpoint
> classes using different base constructors and try:/except...:
> to find the matching version.
> 
> To be honest, I am tempted to call the whole pattern matching on
> function names a mis-feature. C++ name resolution is not really
> compatible with regexps, so at the very least when naming the
> global explicitly ('b ::abort') there should be no match on
> 'struct Foo { void abort() {} };'

There is no pattern matching/regexp happening here.  The wild matching 
introduced in 8.1 just means that the identifier that you look up may be 
at any level, under some namespace or class.

Unless I am missing something, I believe that 'b ::abort' should do what 
you want, and the current behavior is simply buggy.

Simon
  

On Tue, Apr 09, 2019 at 06:02:09PM -0400, Simon Marchi wrote:
> On 2019-04-09 13:12, André Pönitz wrote:
> > I wonder whether there's still a chance to have a(n additional) way
> > to specify the effect of -qualified using a syntax that is the same
> > across GDB versions.
> > 
> > I have pretty much the same effect like Simon for 'start' for a feature
> > 'Break on abort()' in 'my' IDE, that post-8.1 triggers on any function
> > called 'abort'.
> > 
> > Even with the Python interface there seems to be no way to have
> > a single way to set the breakpoint short of having Pre/Post81Breakpoint
> > classes using different base constructors and try:/except...:
> > to find the matching version.
> > 
> > To be honest, I am tempted to call the whole pattern matching on
> > function names a mis-feature. C++ name resolution is not really
> > compatible with regexps, so at the very least when naming the
> > global explicitly ('b ::abort') there should be no match on
> > 'struct Foo { void abort() {} };'
> 
> There is no pattern matching/regexp happening here.  The wild matching
> introduced in 8.1 just means that the identifier that you look up may be at
> any level, under some namespace or class.

Ok. Sorry for not having checked the implementation before writing
the mail. 

I have some sympathy for the 'ignore namespaces' case as I am living in a 
world of user(!)-configurable namespaces myself, but I still do not see
much benefit in treating 'some_class::bar()' and 'some_ns::bar()' "the same"
under "default" circumstances. Ignoring the static some_class case these
are pretty much always really different functions. 

> Unless I am missing something, I believe that 'b ::abort' should do what you
> want, and the current behavior is simply buggy.

Buggy or not, it's certainly intentionally changed user-visible behaviour
without good way on the user side to distinguish between the versions.
This makes maintaining GDB 

I cannot reasonbly say "without any way" as one apparently can distinguish the
versions by trying to instantiate gdb.Breakpoint sub-classes with different
constructors and tell from the failures on which side of the fence one lives,
but that's, erm...

Anyway, mileage varies, and I'll rest my case, and remain happy enough that this
doesn't happen very often.

Andre'
  

>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:

Simon> When using the "start" command, GDB puts a temporary breakpoint on the
Simon> "main" symbol (we literally invoke the tbreak command).

[...]

Simon> The dummiest, most straightforward solution is to add -qualified when
Simon> invoking tbreak.  With this patch, "start" creates a single-location
Simon> breakpoint, as expected.

This seems like a good idea to me, with the possible caveat that it
should be tested for Rust and Ada main programs as well.  (Though
probably there are already test cases covering this...?)

thanks,
Tom
  

On 2019-04-25 10:10 a.m., Tom Tromey wrote:
> This seems like a good idea to me, with the possible caveat that it
> should be tested for Rust and Ada main programs as well.  (Though
> probably there are already test cases covering this...?)

I presume there is no Ada or Rust test that specifically check the situation
where a non-qualified breakpoint on "main" (or equivalent for the language) would
result in a multi-location breakpoint.

Can you help by providing Ada and Rust snippets that would result in multi-location
breakpoints on main (or whatever the entry point is called)?  I could then convert
them to test cases.

Thanks,

Simon
  

>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes:

Simon> I presume there is no Ada or Rust test that specifically check the situation
Simon> where a non-qualified breakpoint on "main" (or equivalent for the language) would
Simon> result in a multi-location breakpoint.

Probably not.  I think my main concern is just that it continues to work
when the name of "main" is something that already has qualifiers.

Simon> Can you help by providing Ada and Rust snippets that would result in multi-location
Simon> breakpoints on main (or whatever the entry point is called)?  I could then convert
Simon> them to test cases.

I think actually my worry would be addressed by any "start" test in
those languages, because the main to stop at is always qualified.

Tom