[PATCH 2/2] Fix advance/until and multiple locations (PR gdb/26524)

[Pedro Alves]

On 8/26/20 10:01 PM, Simon Marchi wrote:
> On 2020-08-22 4:38 p.m., Pedro Alves wrote:

>> -  /* Breakpoint set at the return address in the caller frame.  May be
>> -     NULL.  */
>> -  breakpoint_up caller_breakpoint;
>> +  /* The breakpoint set at the return address in the caller frame,
>> +     plus breakpoints at the destination location.  */
> 
> I think it would make sense to use the plural "locations" in the comment, if we are
> talking about multiple destinations locations in terms of machine code.  Though it
> could be ok at the singular if you meant the source location, ofwhich there is only
> one.

Thanks for spotting that.  It was a typo.  I've added "all" as well:

  /* The breakpoint set at the return address in the caller frame,
     plus breakpoints at all the destination locations.  */
  std::vector<breakpoint_up> breakpoints;

>> +  for (const breakpoint_up &bp : breakpoints)
>> +    if (bpstat_find_breakpoint (tp->control.stop_bpstat,
>> +				bp.get ()) != NULL)
>> +      {
>> +	set_finished ();
>> +	break;
>> +      }
> 
> When using nested control structures, I thought we required braces for the
> outer ones, like this (as mentioned in the GNU coding standards):
> 
>   for (...)
>     {
>       if (...)
>         ...
>     }
> 

Hmm, where in the GNU coding standards?  I don't recall seeing that.

There's a rule for nested if-statements, at 
<https://www.gnu.org/prep/standards/standards.html#Writing-C>:

  "When you have an if-else statement nested in another if statement, always put braces around the if-else. Thus, never write like this: "

That helps with avoiding confusion and the dangling else problem,
though compilers warn about bad indentation nowadays catching
it too.

But here I don't see the benefits.

I don't see it in https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards
nor in the GCC standards either.

It's also frequently used in GDB:

 $ grep -rn "for (" gdb/ -A 1 | grep "if (" | wc -l
 564

Here in the whole tree:

 $ grep -rn "for (" -A 1 | grep "if (" | wc -l
 2472

And here in gcc, just under the gcc/ dir:

 $ grep -rn "for (" gcc/ -A 1 | grep "if (" | wc -l
 6469                                                                                                                                                                        

You can see how they typically look with e.g.:

 $ grep -rn "for (" -A 1 | grep "if (" -B 1 | less

> If that's not the case, I have been misleading people for a while!

I'm afraid you may have!

>> +
>> +  for (symtab_and_line &sal : sals)
>> +    {
>> +      resolve_sal_pc (&sal);
>> +
>> +      breakpoint_up location_breakpoint
>> +	= set_momentary_breakpoint (frame_gdbarch, sal,
>> +				    stop_frame_id, bp_until);
>> +      breakpoints.emplace_back (std::move (location_breakpoint));
>> +    }
> 
> I was a bit surprised to see multiple breakpoints being created here.  Since the
> parallel is that advance/until is like "tbreak <loc>; continue", I would have
> expected a single breakpoint with multiple locations.
> 
> I attributed that to the fact this chain of functions take a single sal:
> 
> - set_momentary_breakpoint, which calls
> - set_raw_breakpoint, which calls
> - init_raw_breakpoint
> 
> I presume that they could all be made to accept a list of sals instead of a single
> sal, and init_raw_breakpoint would add all of them to the breakpoint.
> 
> But since that is all internal things never seen by the user, I don't think it would
> be worth it to do this change.

Right, it's all internal.  Momentary breakpoints don't even have a breakpoint
number (it's always 0).  It wouldn't make a real difference.

> 
> So, the two patches LGTM.  I think they are safe enough to merge right now, before the
> gdb 10 branch is created.  They address new scenarios without risking too much breaking
> old ones.

Alright, thanks much!  I'm going to merge them.

Thanks,
Pedro Alves