RE: [PATCH 2/3] gdb/breakpoint: set the condition exp after parsing the condition successfully

["Aktemur, Tankut Baris" <tankut.baris.aktemur@intel.com>]

On Wednesday, July 22, 2020 6:06 PM, Simon Marchi wrote:
> On 2020-07-22 11:29 a.m., Aktemur, Tankut Baris wrote:
> > On Wednesday, July 22, 2020 3:28 PM, Simon Marchi wrote:
> >> Although, in the breakpoint case, when we have:
> >>
> >> 	  for (bp_location *loc = b->loc; loc != nullptr; loc = loc->next)
> >> 	    {
> >> 	      const char *arg = exp;
> >> 	      expression_up new_exp
> >> 		= parse_exp_1 (&arg, loc->address,
> >> 			       block_for_pc (loc->address), 0);
> >> 	      if (*arg != 0)
> >> 		error (_("Junk at end of expression"));
> >> 	      loc->cond = std::move (new_exp);
> >> 	    }
> >>
> >> Doesn't that mean that if the expression succeeds to parse for one location and then
> >> fails to parse for another location, we'll have updated one location and not the other?
> >
> > Ahh, yes.  The diff for the part above should have been:
> >
> >           struct bp_location *loc;
> >
> > +         /* Parse and set condition expressions.  We make two passes.
> > +            In the first, we parse the condition string to see if it
> > +            is valid in all locations.  If so, the condition would be
> > +            accepted.  So we go ahead and set the locations'
> > +            conditions.  In case a failing case is found, we throw
> > +            the error and the condition string will be rejected.
> > +            This two-pass approach is taken to avoid setting the
> > +            state of locations in case of a reject.  */
> > +         for (loc = b->loc; loc; loc = loc->next)
> > +           {
> > +             arg = exp;
> > +             parse_exp_1 (&arg, loc->address,
> > +                          block_for_pc (loc->address), 0);
> > +             if (*arg != 0)
> > +               error (_("Junk at end of expression"));
> > +           }
> > +
> > +         /* If we reach here, the condition is valid at all locations.  */
> >           for (loc = b->loc; loc; loc = loc->next)
> >             {
> >               arg = exp;
> >               loc->cond =
> >                 parse_exp_1 (&arg, loc->address,
> >                              block_for_pc (loc->address), 0);
> > -             if (*arg)
> > -               error (_("Junk at end of expression"));
> >             }
> >
> >> How does that work (or should work) when we have a multi-location breakpoint and the
> >> condition only makes sense in one of the locations?
> >
> > I'm in fact working on a follow-up patch on this topic, where the two-pass approach above
> > is used (hence I forgot to include it in this series).
> >
> > Currently, GDB expects the condition to be valid at all locations.  The patch that I'll
> > soon post proposes to accept the condition if there exist locations where it's valid.
> > The locations where the condition is invalid are disabled.  But in the current state, the
> > condition has to make sense at all locations.
> 
> Ok, so do you want to wait and post everything together, or do still want to consider
> merging this one on its own, since it's still a step forward?

I'd like to merge this on its own.

Thanks
-Baris


Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928