Split part of insert_breakpoint_at into insert_breakpoint

2 files changed, 48 insertions, 49 deletions

diff --git a/breakpoint.h b/breakpoint.h
index 0ec1b37..3f63f63 100644
--- a/breakpoint.h
+++ b/breakpoint.h
@@ -102,17 +102,19 @@ int breakpoint_turn_on(struct breakpoint *bp, struct process *proc);
  * on failure.  */
 int breakpoint_turn_off(struct breakpoint *bp, struct process *proc);
 
-/* Utility function that does what typically needs to be done when a
- * breakpoint is to be inserted.  It checks whether there is another
- * breakpoint in PROC->LEADER for given ADDR.  If not, it allocates
- * memory for a new breakpoint on the heap, initializes it, and calls
- * PROC_ADD_BREAKPOINT to add the newly-created breakpoint.  For newly
- * added as well as preexisting breakpoints, it then calls
- * BREAKPOINT_TURN_ON.  If anything fails, it cleans up and returns
- * NULL.  Otherwise it returns the breakpoint for ADDR.  */
+/* This allocates and initializes new breakpoint at ADDR, then calls
+ * INSERT_BREAKPOINT.  Returns the new breakpoint or NULL if there are
+ * errors.  */
 struct breakpoint *insert_breakpoint_at(struct process *proc, arch_addr_t addr,
 					struct library_symbol *libsym);
 
+/* Check if there is a breakpoint on this address already.  If yes,
+ * return that breakpoint instead (BP was not added).  If no, try to
+ * PROC_ADD_BREAKPOINT and BREAKPOINT_TURN_ON.  If it all works,
+ * return BP.  Otherwise return NULL.  */
+struct breakpoint *insert_breakpoint(struct process *proc,
+				     struct breakpoint *bp);
+
 /* Name of a symbol associated with BP.  May be NULL.  */
 const char *breakpoint_name(const struct breakpoint *bp);
 
diff --git a/breakpoints.c b/breakpoints.c
index bc193ae..4b3cdd3 100644
--- a/breakpoints.c
+++ b/breakpoints.c
@@ -203,34 +203,42 @@ struct breakpoint *
 insert_breakpoint_at(struct process *proc, arch_addr_t addr,
 		     struct library_symbol *libsym)
 {
-	struct process *leader = proc->leader;
-
-	/* Only the group leader should be getting the breakpoints and
-	 * thus have ->breakpoint initialized.  */
-	assert(leader != NULL);
-	assert(leader->breakpoints != NULL);
-
 	debug(DEBUG_FUNCTION,
 	      "insert_breakpoint_at(pid=%d, addr=%p, symbol=%s)",
 	      proc->pid, addr, libsym ? libsym->name : "NULL");
 
 	assert(addr != 0);
 
-	/* We first create the breakpoint to find out what it's real
-	 * address is.  This makes a difference on ARM.
-	 *
-	 * XXX The real problem here is that to create a return
-	 * breakpoint ltrace calls get_return_addr and then
-	 * insert_breakpoint_at.  So get_return_addr needs to encode
-	 * all the information necessary for breakpoint_init into the
-	 * address itself, so ADDR is potentially mangled.  We filter
-	 * the noise out by first creating the breakpoint on stack,
-	 * and then looking at the address of the created breakpoint.
-	 * Replacing get_return_addr with get_return_breakpoint might
-	 * be a better solution.  */
-	struct breakpoint bp;
-	if (breakpoint_init(&bp, proc, addr, libsym) < 0)
+	struct breakpoint *bp = malloc(sizeof *bp);
+	if (bp == NULL || breakpoint_init(bp, proc, addr, libsym) < 0) {
+		free(bp);
 		return NULL;
+	}
+
+	/* N.B. (and XXX): BP->addr might differ from ADDR.  On ARM
+	 * this is a real possibility.  The problem here is that to
+	 * create a return breakpoint ltrace calls get_return_addr and
+	 * then insert_breakpoint_at.  So get_return_addr needs to
+	 * encode all the information necessary for breakpoint_init
+	 * into the address itself, so ADDR is potentially
+	 * mangled.  */
+
+	struct breakpoint *tmp = insert_breakpoint(proc, bp);
+	if (tmp != bp) {
+		breakpoint_destroy(bp);
+		free(bp);
+	}
+	return tmp;
+}
+
+struct breakpoint *
+insert_breakpoint(struct process *proc, struct breakpoint *bp)
+{
+	/* Only the group leader should be getting the breakpoints and
+	 * thus have ->breakpoint initialized.  */
+	struct process *leader = proc->leader;
+	assert(leader != NULL);
+	assert(leader->breakpoints != NULL);
 
 	/* XXX what we need to do instead is have a list of
 	 * breakpoints that are enabled at this address.  The
@@ -239,31 +247,20 @@ insert_breakpoint_at(struct process *proc, arch_addr_t addr,
 	 * will suffice, about the only realistic case where we need
 	 * to have more than one breakpoint per address is return from
 	 * a recursive library call.  */
-	struct breakpoint *sbp;
-	if (DICT_FIND_VAL(leader->breakpoints, &bp.addr, &sbp) >= 0) {
-		breakpoint_destroy(&bp);
-	} else {
-		sbp = malloc(sizeof(*sbp));
-		if (sbp == NULL
-		    || breakpoint_init(sbp, proc, addr, libsym) < 0) {
-			free(sbp);
-			return NULL;
-		}
-		if (proc_add_breakpoint(leader, sbp) < 0) {
-		fail:
-			free(sbp);
-			breakpoint_destroy(&bp);
+	struct breakpoint *ext_bp = bp;
+	if (DICT_FIND_VAL(leader->breakpoints, &bp->addr, &ext_bp) != 0) {
+		if (proc_add_breakpoint(leader, bp) < 0)
 			return NULL;
-		}
-		memcpy(sbp, &bp, sizeof(*sbp));
+		ext_bp = bp;
 	}
 
-	if (breakpoint_turn_on(sbp, proc) < 0) {
-		proc_remove_breakpoint(leader, sbp);
-		goto fail;
+	if (breakpoint_turn_on(ext_bp, proc) < 0) {
+		if (ext_bp != bp)
+			proc_remove_breakpoint(leader, bp);
+		return NULL;
 	}
 
-	return sbp;
+	return ext_bp;
 }
 
 void