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

[Simon Marchi <simark@simark.ca>]

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?

Simon