gdb/breakpoint: disable a bp location if condition is invalid at that location

Currently, for a conditional breakpoint, GDB checks if the condition
can be evaluated in the context of the first symtab and line (SAL).
In case of an error, defining the conditional breakpoint is aborted.
This prevents having a conditional breakpoint whose condition may
actually be meaningful for some of the location contexts.  This patch
makes it possible to define conditional BPs by checking all location
contexts.  If the condition is meaningful for even one context, the
breakpoint is defined.  The locations for which the condition gives
errors are disabled.

The bp_location struct is introduced a new field, 'disabled_by_cond'.
This field denotes whether the location is disabled automatically
because the condition was non-evaluatable.  Disabled-by-cond locations
cannot be enabled by the user.  But locations that are not
disabled-by-cond can be enabled/disabled by the user manually as
before.

For a concrete example, consider 3 contexts of a function 'func'.

  class Base
  {
  public:
    int b = 20;

    void func () {}
  };

  class A : public Base
  {
  public:
    int a = 10;

    void func () {}
  };

  class C : public Base
  {
  public:
    int c = 30;

    void func () {}
  };

Note that

* the variable 'a' is defined only in the context of A::func.
* the variable 'c' is defined only in the context of C::func.
* the variable 'b' is defined in all the three contexts.

With the existing GDB, it's not possible to define a conditional
breakpoint at 'func' if the condition refers to 'a' or 'c':

  (gdb) break func if a == 10
  No symbol "a" in current context.
  (gdb) break func if c == 30
  No symbol "c" in current context.
  (gdb) info breakpoints
  No breakpoints or watchpoints.

With this patch, it becomes possible:

  (gdb) break func if a == 10
  warning: failed to validate condition at location 1, disabling:
    No symbol "a" in current context.
  warning: failed to validate condition at location 3, disabling:
    No symbol "a" in current context.
  Breakpoint 1 at 0x11b6: func. (3 locations)
  (gdb) break func if c == 30
  Note: breakpoint 1 also set at pc 0x11ce.
  Note: breakpoint 1 also set at pc 0x11c2.
  Note: breakpoint 1 also set at pc 0x11b6.
  warning: failed to validate condition at location 1, disabling:
    No symbol "c" in current context.
  warning: failed to validate condition at location 2, disabling:
    No symbol "c" in current context.
  Breakpoint 2 at 0x11b6: func. (3 locations)
  (gdb) info breakpoints
  Num     Type           Disp Enb Address            What
  1       breakpoint     keep y   <MULTIPLE>
          stop only if a == 10
  1.1                         N*  0x00000000000011b6 in Base::func() at condbreak-multi-context.cc:23
  1.2                         y   0x00000000000011c2 in A::func() at condbreak-multi-context.cc:31
  1.3                         N*  0x00000000000011ce in C::func() at condbreak-multi-context.cc:39
  2       breakpoint     keep y   <MULTIPLE>
          stop only if c == 30
  2.1                         N*  0x00000000000011b6 in Base::func() at condbreak-multi-context.cc:23
  2.2                         N*  0x00000000000011c2 in A::func() at condbreak-multi-context.cc:31
  2.3                         y   0x00000000000011ce in C::func() at condbreak-multi-context.cc:39
  (*): Breakpoint condition is invalid at this location.

Here, uppercase 'N' denotes that the location is disabled because of
the invalid condition, as mentioned with a footnote in the legend of
the table.  Locations that are disabled by the user are still denoted
with lowercase 'n'.  Executing the code hits the breakpoints 1.2 and
2.3 as expected.

Defining a condition on an unconditional breakpoint gives the same
behavior above:

  (gdb) break func
  Breakpoint 1 at 0x11b6: func. (3 locations)
  (gdb) cond 1 a == 10
  warning: failed to validate condition at location 1.1, disabling:
    No symbol "a" in current context.
  warning: failed to validate condition at location 1.3, disabling:
    No symbol "a" in current context.
  (gdb) info breakpoints
  Num     Type           Disp Enb Address            What
  1       breakpoint     keep y   <MULTIPLE>
          stop only if a == 10
  1.1                         N*  0x00000000000011b6 in Base::func() at condbreak-multi-context.cc:23
  1.2                         y   0x00000000000011c2 in A::func() at condbreak-multi-context.cc:31
  1.3                         N*  0x00000000000011ce in C::func() at condbreak-multi-context.cc:39
  (*): Breakpoint condition is invalid at this location.

Locations that are disabled because of a condition cannot be enabled
by the user:

  ...
  (gdb) enable 1.1
  Breakpoint 1's condition is invalid at location 1, cannot enable.

Resetting the condition enables the locations back:

  ...
  (gdb) cond 1
  Breakpoint 1's condition is now valid at location 1, enabling.
  Breakpoint 1's condition is now valid at location 3, enabling.
  Breakpoint 1 now unconditional.
  (gdb) info breakpoints
  Num     Type           Disp Enb Address            What
  1       breakpoint     keep y   <MULTIPLE>
  1.1                         y   0x00000000000011b6 in Base::func() at condbreak-multi-context.cc:23
  1.2                         y   0x00000000000011c2 in A::func() at condbreak-multi-context.cc:31
  1.3                         y   0x00000000000011ce in C::func() at condbreak-multi-context.cc:39

If a location is disabled by the user, a condition can still be defined
but the location will remain disabled even if the condition is meaningful
for the disabled location:

  ...
  (gdb) disable 1.2
  (gdb) cond 1 a == 10
  warning: failed to validate condition at location 1.1, disabling:
    No symbol "a" in current context.
  warning: failed to validate condition at location 1.3, disabling:
    No symbol "a" in current context.
  (gdb) info breakpoints
  Num     Type           Disp Enb Address            What
  1       breakpoint     keep y   <MULTIPLE>
          stop only if a == 10
  1.1                         N*  0x00000000000011b6 in Base::func() at condbreak-multi-context.cc:23
  1.2                         n   0x00000000000011c2 in A::func() at condbreak-multi-context.cc:31
  1.3                         N*  0x00000000000011ce in C::func() at condbreak-multi-context.cc:39
  (*): Breakpoint condition is invalid at this location.

The condition of a breakpoint can be changed.  Locations'
enable/disable states are updated accordingly.

  ...
  (gdb) cond 1 c == 30
  warning: failed to validate condition at location 1.1, disabling:
    No symbol "c" in current context.
  Breakpoint 1's condition is now valid at location 3, enabling.
  (gdb) info breakpoints
  Num     Type           Disp Enb Address            What
  1       breakpoint     keep y   <MULTIPLE>
          stop only if c == 30
  1.1                         N*  0x00000000000011b6 in Base::func() at condbreak-multi-context.cc:23
  1.2                         N*  0x00000000000011c2 in A::func() at condbreak-multi-context.cc:31
  1.3                         y   0x00000000000011ce in C::func() at condbreak-multi-context.cc:39
  (*): Breakpoint condition is invalid at this location.

  (gdb) cond 1 b == 20
  Breakpoint 1's condition is now valid at location 1, enabling.
  (gdb) info breakpoints
  Num     Type           Disp Enb Address            What
  1       breakpoint     keep y   <MULTIPLE>
          stop only if b == 20
  1.1                         y   0x00000000000011b6 in Base::func() at condbreak-multi-context.cc:23
  1.2                         n   0x00000000000011c2 in A::func() at condbreak-multi-context.cc:31
  1.3                         y   0x00000000000011ce in C::func() at condbreak-multi-context.cc:39
  # Note that location 1.2 was disabled by the user previously.

If the condition expression is bad for all the locations, it will be
rejected.

  (gdb) cond 1 garbage
  No symbol "garbage" in current context.

For conditions that are invalid or valid for all the locations of a
breakpoint, the existing behavior is preserved.

Regression-tested on X86_64 Linux.

gdb/ChangeLog:
2020-10-27  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>

	* breakpoint.h (class bp_location) <disabled_by_cond>: New field.
	* breakpoint.c (set_breakpoint_location_condition): New function.
	(set_breakpoint_condition): Disable a breakpoint location if parsing
	the condition string gives an error.
	(should_be_inserted): Update to consider the 'disabled_by_cond' field.
	(build_target_condition_list): Ditto.
	(build_target_command_list): Ditto.
	(build_bpstat_chain): Ditto.
	(print_one_breakpoint_location): Ditto.
	(print_one_breakpoint): Ditto.
	(breakpoint_1): Ditto.
	(bp_location::bp_location): Ditto.
	(locations_are_equal): Ditto.
	(update_breakpoint_locations): Ditto.
	(enable_disable_bp_num_loc): Ditto.
	(init_breakpoint_sal): Use set_breakpoint_location_condition.
	(find_condition_and_thread_for_sals): New static function.
	(create_breakpoint): Call find_condition_and_thread_for_sals.
	(location_to_sals): Call find_condition_and_thread_for_sals instead
	of find_condition_and_thread.

gdb/testsuite/ChangeLog:
2020-10-27  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>

	* gdb.base/condbreak-multi-context.cc: New file.
	* gdb.base/condbreak-multi-context.exp: New file.

gdb/doc/ChangeLog:
2020-10-27  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>

	* gdb.texinfo (Set Breaks): Document disabling of breakpoint
	locations for which the breakpoint condition is invalid.

1 files changed, 185 insertions, 50 deletions

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 631bee5..0d3fd0c 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -830,6 +830,56 @@ get_first_locp_gte_addr (CORE_ADDR address)
   return locp_found;
 }
 
+/* Parse COND_STRING in the context of LOC and set as the condition
+   expression of LOC.  BP_NUM is the number of LOC's owner, LOC_NUM is
+   the number of LOC within its owner.  In case of parsing error, mark
+   LOC as DISABLED_BY_COND.  In case of success, unset DISABLED_BY_COND.  */
+
+static void
+set_breakpoint_location_condition (const char *cond_string, bp_location *loc,
+				   int bp_num, int loc_num)
+{
+  bool has_junk = false;
+  try
+    {
+      expression_up new_exp = parse_exp_1 (&cond_string, loc->address,
+					   block_for_pc (loc->address), 0);
+      if (*cond_string != 0)
+	has_junk = true;
+      else
+	{
+	  loc->cond = std::move (new_exp);
+	  if (loc->disabled_by_cond && loc->enabled)
+	    printf_filtered (_("Breakpoint %d's condition is now valid at "
+			       "location %d, enabling.\n"),
+			     bp_num, loc_num);
+
+	  loc->disabled_by_cond = false;
+	}
+    }
+  catch (const gdb_exception_error &e)
+    {
+      if (loc->enabled)
+	{
+	  /* Warn if a user-enabled location is now becoming disabled-by-cond.
+	     BP_NUM is 0 if the breakpoint is being defined for the first
+	     time using the "break ... if ..." command, and non-zero if
+	     already defined.  */
+	  if (bp_num != 0)
+	    warning (_("failed to validate condition at location %d.%d, "
+		       "disabling:\n  %s"), bp_num, loc_num, e.what ());
+	  else
+	    warning (_("failed to validate condition at location %d, "
+		       "disabling:\n  %s"), loc_num, e.what ());
+	}
+
+      loc->disabled_by_cond = true;
+    }
+
+  if (has_junk)
+    error (_("Garbage '%s' follows condition"), cond_string);
+}
+
 void
 set_breakpoint_condition (struct breakpoint *b, const char *exp,
 			  int from_tty)
@@ -843,9 +893,16 @@ set_breakpoint_condition (struct breakpoint *b, const char *exp,
 	static_cast<watchpoint *> (b)->cond_exp.reset ();
       else
 	{
+	  int loc_num = 1;
 	  for (bp_location *loc = b->loc; loc != nullptr; loc = loc->next)
 	    {
 	      loc->cond.reset ();
+	      if (loc->disabled_by_cond && loc->enabled)
+		printf_filtered (_("Breakpoint %d's condition is now valid at "
+				   "location %d, enabling.\n"),
+				 b->number, loc_num);
+	      loc->disabled_by_cond = false;
+	      loc_num++;
 
 	      /* No need to free the condition agent expression
 		 bytecode (if we have one).  We will handle this
@@ -873,29 +930,37 @@ set_breakpoint_condition (struct breakpoint *b, const char *exp,
 	{
 	  /* 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
+	     is valid in at least one location.  If so, the condition
+	     would be accepted.  So we go ahead and set the locations'
+	     conditions.  In case no valid 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 (bp_location *loc = b->loc; loc != nullptr; loc = loc->next)
 	    {
-	      const char *arg = exp;
-	      parse_exp_1 (&arg, loc->address,
-			   block_for_pc (loc->address), 0);
-	      if (*arg != 0)
-		error (_("Junk at end of expression"));
+	      try
+		{
+		  const char *arg = exp;
+		  parse_exp_1 (&arg, loc->address,
+			       block_for_pc (loc->address), 0);
+		  if (*arg != 0)
+		    error (_("Junk at end of expression"));
+		  break;
+		}
+	      catch (const gdb_exception_error &e)
+		{
+		  /* Condition string is invalid.  If this happens to
+		     be the last loc, abandon.  */
+		  if (loc->next == nullptr)
+		    throw;
+		}
 	    }
 
-	  /* If we reach here, the condition is valid at all locations.  */
-	  for (bp_location *loc = b->loc; loc != nullptr; loc = loc->next)
-	    {
-	      const char *arg = exp;
-	      loc->cond =
-		parse_exp_1 (&arg, loc->address,
-			     block_for_pc (loc->address), 0);
-	    }
+	  /* If we reach here, the condition is valid at some locations.  */
+          int loc_num = 1;
+          for (bp_location *loc = b->loc; loc != nullptr;
+               loc = loc->next, loc_num++)
+            set_breakpoint_location_condition (exp, loc, b->number, loc_num);
 	}
 
       /* We know that the new condition parsed successfully.  The
@@ -2010,7 +2075,8 @@ should_be_inserted (struct bp_location *bl)
   if (bl->owner->disposition == disp_del_at_next_stop)
     return 0;
 
-  if (!bl->enabled || bl->shlib_disabled || bl->duplicate)
+  if (!bl->enabled || bl->disabled_by_cond
+      || bl->shlib_disabled || bl->duplicate)
     return 0;
 
   if (user_breakpoint_p (bl->owner) && bl->pspace->executing_startup)
@@ -2205,7 +2271,8 @@ build_target_condition_list (struct bp_location *bl)
 	  && is_breakpoint (loc->owner)
 	  && loc->pspace->num == bl->pspace->num
 	  && loc->owner->enable_state == bp_enabled
-	  && loc->enabled)
+	  && loc->enabled
+	  && !loc->disabled_by_cond)
 	{
 	  /* Add the condition to the vector.  This will be used later
 	     to send the conditions to the target.  */
@@ -2395,7 +2462,8 @@ build_target_command_list (struct bp_location *bl)
 	  && is_breakpoint (loc->owner)
 	  && loc->pspace->num == bl->pspace->num
 	  && loc->owner->enable_state == bp_enabled
-	  && loc->enabled)
+	  && loc->enabled
+	  && !loc->disabled_by_cond)
 	{
 	  /* Add the command to the vector.  This will be used later
 	     to send the commands to the target.  */
@@ -5278,7 +5346,7 @@ build_bpstat_chain (const address_space *aspace, CORE_ADDR bp_addr,
 	  if (b->type == bp_hardware_watchpoint && bl != b->loc)
 	    break;
 
-	  if (!bl->enabled || bl->shlib_disabled)
+	  if (!bl->enabled || bl->disabled_by_cond || bl->shlib_disabled)
 	    continue;
 
 	  if (!bpstat_check_location (bl, aspace, bp_addr, ws))
@@ -5987,7 +6055,8 @@ print_one_breakpoint_location (struct breakpoint *b,
      breakpoints with single disabled location.  */
   if (loc == NULL 
       && (b->loc != NULL 
-	  && (b->loc->next != NULL || !b->loc->enabled)))
+	  && (b->loc->next != NULL
+	      || !b->loc->enabled || b->loc->disabled_by_cond)))
     header_of_multiple = 1;
   if (loc == NULL)
     loc = b->loc;
@@ -6018,7 +6087,8 @@ print_one_breakpoint_location (struct breakpoint *b,
   /* 4 */
   annotate_field (3);
   if (part_of_multiple)
-    uiout->field_string ("enabled", loc->enabled ? "y" : "n");
+    uiout->field_string ("enabled", (loc->disabled_by_cond ? "N*"
+				     : (loc->enabled ? "y" : "n")));
   else
     uiout->field_fmt ("enabled", "%c", bpenables[(int) b->enable_state]);
 
@@ -6318,7 +6388,9 @@ print_one_breakpoint (struct breakpoint *b,
 	  && (!is_catchpoint (b) || is_exception_catchpoint (b)
 	      || is_ada_exception_catchpoint (b))
 	  && (allflag
-	      || (b->loc && (b->loc->next || !b->loc->enabled))))
+	      || (b->loc && (b->loc->next
+			     || !b->loc->enabled
+			     || b->loc->disabled_by_cond))))
 	{
 	  gdb::optional<ui_out_emit_list> locations_list;
 
@@ -6412,6 +6484,7 @@ breakpoint_1 (const char *bp_num_list, bool show_internal,
   int print_address_bits = 0;
   int print_type_col_width = 14;
   struct ui_out *uiout = current_uiout;
+  bool has_disabled_by_cond_location = false;
 
   get_user_print_options (&opts);
 
@@ -6512,7 +6585,12 @@ breakpoint_1 (const char *bp_num_list, bool show_internal,
 	/* We only print out user settable breakpoints unless the
 	   show_internal is set.  */
 	if (show_internal || user_breakpoint_p (b))
-	  print_one_breakpoint (b, &last_loc, show_internal);
+	  {
+	    print_one_breakpoint (b, &last_loc, show_internal);
+	    for (bp_location *loc = b->loc; loc != NULL; loc = loc->next)
+	      if (loc->disabled_by_cond)
+		has_disabled_by_cond_location = true;
+	  }
       }
   }
 
@@ -6533,6 +6611,10 @@ breakpoint_1 (const char *bp_num_list, bool show_internal,
     {
       if (last_loc && !server_command)
 	set_next_address (last_loc->gdbarch, last_loc->address);
+
+      if (has_disabled_by_cond_location)
+	uiout->message (_("(*): Breakpoint condition is invalid at this "
+			  "location.\n"));
     }
 
   /* FIXME?  Should this be moved up so that it is only called when
@@ -6960,6 +7042,7 @@ bp_location::bp_location (breakpoint *owner, bp_loc_type type)
   this->cond_bytecode = NULL;
   this->shlib_disabled = 0;
   this->enabled = 1;
+  this->disabled_by_cond = false;
 
   this->loc_type = type;
 
@@ -8832,15 +8915,9 @@ init_breakpoint_sal (struct breakpoint *b, struct gdbarch *gdbarch,
 	    loc->inserted = 1;
 	}
 
-      if (b->cond_string)
-	{
-	  const char *arg = b->cond_string;
-
-	  loc->cond = parse_exp_1 (&arg, loc->address,
-				   block_for_pc (loc->address), 0);
-	  if (*arg)
-              error (_("Garbage '%s' follows condition"), arg);
-	}
+      /* Do not set breakpoint locations conditions yet.  As locations
+	 are inserted, they get sorted based on their addresses.  Let
+	 the list stabilize to have reliable location numbers.  */
 
       /* Dynamic printf requires and uses additional arguments on the
 	 command line, otherwise it's an error.  */
@@ -8855,6 +8932,19 @@ init_breakpoint_sal (struct breakpoint *b, struct gdbarch *gdbarch,
 	error (_("Garbage '%s' at end of command"), b->extra_string);
     }
 
+
+  /* The order of the locations is now stable.  Set the location
+     condition using the location's number.  */
+  int loc_num = 1;
+  for (bp_location *loc = b->loc; loc != nullptr; loc = loc->next)
+    {
+      if (b->cond_string != nullptr)
+	set_breakpoint_location_condition (b->cond_string, loc, b->number,
+					   loc_num);
+
+      ++loc_num;
+    }
+
   b->display_canonical = display_canonical;
   if (location != NULL)
     b->location = std::move (location);
@@ -9143,6 +9233,50 @@ find_condition_and_thread (const char *tok, CORE_ADDR pc,
     }
 }
 
+/* Call 'find_condition_and_thread' for each sal in SALS until a parse
+   succeeds.  The parsed values are written to COND_STRING, THREAD,
+   TASK, and REST.  See the comment of 'find_condition_and_thread'
+   for the description of these parameters and INPUT.  */
+
+static void
+find_condition_and_thread_for_sals (const std::vector<symtab_and_line> &sals,
+				    const char *input, char **cond_string,
+				    int *thread, int *task, char **rest)
+{
+  int num_failures = 0;
+  for (auto &sal : sals)
+    {
+      char *cond = nullptr;
+      int thread_id = 0;
+      int task_id = 0;
+      char *remaining = nullptr;
+
+      /* Here we want to parse 'arg' to separate condition from thread
+	 number.  But because parsing happens in a context and the
+	 contexts of sals might be different, try each until there is
+	 success.  Finding one successful parse is sufficient for our
+	 goal.  When setting the breakpoint we'll re-parse the
+	 condition in the context of each sal.  */
+      try
+	{
+	  find_condition_and_thread (input, sal.pc, &cond, &thread_id,
+				     &task_id, &remaining);
+	  *cond_string = cond;
+	  *thread = thread_id;
+	  *task = task_id;
+	  *rest = remaining;
+	  break;
+	}
+      catch (const gdb_exception_error &e)
+	{
+	  num_failures++;
+	  /* If no sal remains, do not continue.  */
+	  if (num_failures == sals.size ())
+	    throw;
+	}
+    }
+}
+
 /* Decode a static tracepoint marker spec.  */
 
 static std::vector<symtab_and_line>
@@ -9306,13 +9440,8 @@ create_breakpoint (struct gdbarch *gdbarch,
 
 	  const linespec_sals &lsal = canonical.lsals[0];
 
-	  /* Here we only parse 'arg' to separate condition
-	     from thread number, so parsing in context of first
-	     sal is OK.  When setting the breakpoint we'll
-	     re-parse it in context of each sal.  */
-
-	  find_condition_and_thread (extra_string, lsal.sals[0].pc,
-				     &cond, &thread, &task, &rest);
+	  find_condition_and_thread_for_sals (lsal.sals, extra_string,
+					      &cond, &thread, &task, &rest);
 	  cond_string_copy.reset (cond);
 	  extra_string_copy.reset (rest);
         }
@@ -13414,6 +13543,9 @@ locations_are_equal (struct bp_location *a, struct bp_location *b)
       if (a->enabled != b->enabled)
 	return 0;
 
+      if (a->disabled_by_cond != b->disabled_by_cond)
+	return 0;
+
       a = a->next;
       b = b->next;
     }
@@ -13521,10 +13653,7 @@ update_breakpoint_locations (struct breakpoint *b,
 	    }
 	  catch (const gdb_exception_error &e)
 	    {
-	      warning (_("failed to reevaluate condition "
-			 "for breakpoint %d: %s"), 
-		       b->number, e.what ());
-	      new_loc->enabled = 0;
+	      new_loc->disabled_by_cond = true;
 	    }
 	}
 
@@ -13549,7 +13678,7 @@ update_breakpoint_locations (struct breakpoint *b,
 
     for (; e; e = e->next)
       {
-	if (!e->enabled && e->function_name)
+	if ((!e->enabled || e->disabled_by_cond) && e->function_name)
 	  {
 	    struct bp_location *l = b->loc;
 	    if (have_ambiguous_names)
@@ -13565,7 +13694,8 @@ update_breakpoint_locations (struct breakpoint *b,
 		       enough.  */
 		    if (breakpoint_locations_match (e, l, true))
 		      {
-			l->enabled = 0;
+			l->enabled = e->enabled;
+			l->disabled_by_cond = e->disabled_by_cond;
 			break;
 		      }
 		  }
@@ -13576,7 +13706,8 @@ update_breakpoint_locations (struct breakpoint *b,
 		  if (l->function_name
 		      && strcmp (e->function_name, l->function_name) == 0)
 		    {
-		      l->enabled = 0;
+		      l->enabled = e->enabled;
+		      l->disabled_by_cond = e->disabled_by_cond;
 		      break;
 		    }
 	      }
@@ -13650,9 +13781,9 @@ location_to_sals (struct breakpoint *b, struct event_location *location,
 	  char *cond_string, *extra_string;
 	  int thread, task;
 
-	  find_condition_and_thread (b->extra_string, sals[0].pc,
-				     &cond_string, &thread, &task,
-				     &extra_string);
+	  find_condition_and_thread_for_sals (sals, b->extra_string,
+					      &cond_string, &thread,
+					      &task, &extra_string);
 	  gdb_assert (b->cond_string == NULL);
 	  if (cond_string)
 	    b->cond_string = cond_string;
@@ -14137,6 +14268,10 @@ enable_disable_bp_num_loc (int bp_num, int loc_num, bool enable)
   struct bp_location *loc = find_location_by_number (bp_num, loc_num);
   if (loc != NULL)
     {
+      if (loc->disabled_by_cond && enable)
+	error (_("Breakpoint %d's condition is invalid at location %d, "
+		 "cannot enable."), bp_num, loc_num);
+
       if (loc->enabled != enable)
 	{
 	  loc->enabled = enable;