Re: [PATCH 3/5] Refactor in mips-linux-nat.c

Yao Qi
2013-07-28T00:44:48Z

On 07/24/2013 08:26 AM, Maciej W. Rozycki wrote:
> This fits on one line, please adjust.
>
>> >@@ -910,21 +930,25 @@ try_one_watch (struct pt_watch_regs *regs, CORE_ADDR addr,
>> > {
>> > /* Try to split it across several registers. */
>> > regs_copy = *regs;
>> >- for (i = 0; i < get_num_valid (&regs_copy); i++)
>> >+ for (i = 0;
>> >+ i < mips_linux_watch_get_num_valid (&regs_copy);
>> >+ i++)
> Likewise. OK with these changes, thanks.

Fixed. Patch below is what I committed.

--
Yao (é½å°§)

---
gdb/ChangeLog | 36 +++++++++
gdb/mips-linux-nat.c | 206 +++++++++++++++++++++++++++++---------------------
2 files changed, 155 insertions(+), 87 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 6350b53..2259dd4 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,41 @@
2013-07-27 Yao Qi <yao@codesourcery.com>

+ * mips-linux-nat.c (get_irw_mask): Rename to ...
+ (mips_linux_watch_get_irw_mask): ... this. Rename parameter
+ 'set' to 'n'. Update function comment. All callers changed.
+ (get_reg_mask): Rename parameter 'set' to 'n'. Update
+ function comment. All callers changed.
+ (get_num_valid): Rename to ...
+ (mips_linux_watch_get_num_valid): ... this. Rename parameter
+ 'set' to 'n'. Update function comment. All callers changed.
+ (get_watchlo): Rename to ...
+ (mips_linux_watch_get_watchlo): ... this. Rename parameter
+ 'set' to 'n'. Update function comment. All callers changed.
+ (set_watchlo): Rename to ...
+ (mips_linux_watch_set_watchlo): ... this. Rename parameter
+ 'set' to 'n'. Update function comment. All callers changed.
+ (get_watchhi): Rename to ...
+ (mips_linux_watch_get_watchhi): ... this. Update function
+ comment. All callers changed.
+ (set_watchhi): Rename to ...
+ (mips_linux_watch_set_watchhi): ... this. Update function
+ comment. All callers changed.
+ (mips_linux_read_watch_registers): Update function comment.
+ Add new parameters 'lwpid', 'watch_readback', and
+ 'watch_readback_valid'. Update.
+ (type_to_irw): Rename to ...
+ (mips_linux_watch_type_to_irw): ... this. Update function
+ comment. All callers changed.
+ (fill_mask): Update function comment.
+ (try_one_watch): Rename to ...
+ (mips_linux_watch_try_one_watch): ... this. Change the type
+ of parameter 'irw' from 'unsigned' to 'uint32_t'.
+ (populate_regs_from_watches): Rename to ...
+ (mips_linux_watch_populate_regs): ... this. Add parameter
+ 'current_watches'. All callers changed.
+
+2013-07-27 Yao Qi <yao@codesourcery.com>
+
* mips-linux-nat.c (MAX_DEBUG_REGISTER): Move it earlier in
the code.
(PTRACE_SET_WATCH_REGS, enum pt_watch_style): Remove.
diff --git a/gdb/mips-linux-nat.c b/gdb/mips-linux-nat.c
index 63cc140..d792cb3 100644
--- a/gdb/mips-linux-nat.c
+++ b/gdb/mips-linux-nat.c
@@ -556,44 +556,46 @@ static struct mips_watchpoint *current_watches;

static struct pt_watch_regs watch_mirror;

-/* Assuming usable watch registers, return the irw_mask. */
+/* Assuming usable watch registers REGS, return the irw_mask of
+ register N. */

static uint32_t
-get_irw_mask (struct pt_watch_regs *regs, int set)
+mips_linux_watch_get_irw_mask (struct pt_watch_regs *regs, int n)
{
switch (regs->style)
{
case pt_watch_style_mips32:
- return regs->mips32.watch_masks[set] & IRW_MASK;
+ return regs->mips32.watch_masks[n] & IRW_MASK;
case pt_watch_style_mips64:
- return regs->mips64.watch_masks[set] & IRW_MASK;
+ return regs->mips64.watch_masks[n] & IRW_MASK;
default:
internal_error (__FILE__, __LINE__,
_("Unrecognized watch register style"));
}
}

-/* Assuming usable watch registers, return the reg_mask. */
+/* Assuming usable watch registers REGS, return the reg_mask of
+ register N. */

static uint32_t
-get_reg_mask (struct pt_watch_regs *regs, int set)
+get_reg_mask (struct pt_watch_regs *regs, int n)
{
switch (regs->style)
{
case pt_watch_style_mips32:
- return regs->mips32.watch_masks[set] & ~IRW_MASK;
+ return regs->mips32.watch_masks[n] & ~IRW_MASK;
case pt_watch_style_mips64:
- return regs->mips64.watch_masks[set] & ~IRW_MASK;
+ return regs->mips64.watch_masks[n] & ~IRW_MASK;
default:
internal_error (__FILE__, __LINE__,
_("Unrecognized watch register style"));
}
}

-/* Assuming usable watch registers, return the num_valid. */
+/* Assuming usable watch registers REGS, return the num_valid. */

static uint32_t
-get_num_valid (struct pt_watch_regs *regs)
+mips_linux_watch_get_num_valid (struct pt_watch_regs *regs)
{
switch (regs->style)
{
@@ -607,37 +609,40 @@ get_num_valid (struct pt_watch_regs *regs)
}
}

-/* Assuming usable watch registers, return the watchlo. */
+/* Assuming usable watch registers REGS, return the watchlo of
+ register N. */

static CORE_ADDR
-get_watchlo (struct pt_watch_regs *regs, int set)
+mips_linux_watch_get_watchlo (struct pt_watch_regs *regs, int n)
{
switch (regs->style)
{
case pt_watch_style_mips32:
- return regs->mips32.watchlo[set];
+ return regs->mips32.watchlo[n];
case pt_watch_style_mips64:
- return regs->mips64.watchlo[set];
+ return regs->mips64.watchlo[n];
default:
internal_error (__FILE__, __LINE__,
_("Unrecognized watch register style"));
}
}

-/* Assuming usable watch registers, set a watchlo value. */
+/* Assuming usable watch registers REGS, set watchlo of register N to
+ VALUE. */

static void
-set_watchlo (struct pt_watch_regs *regs, int set, CORE_ADDR value)
+mips_linux_watch_set_watchlo (struct pt_watch_regs *regs, int n,
+ CORE_ADDR value)
{
switch (regs->style)
{
case pt_watch_style_mips32:
/* The cast will never throw away bits as 64 bit addresses can
never be used on a 32 bit kernel. */
- regs->mips32.watchlo[set] = (uint32_t)value;
+ regs->mips32.watchlo[n] = (uint32_t) value;
break;
case pt_watch_style_mips64:
- regs->mips64.watchlo[set] = value;
+ regs->mips64.watchlo[n] = value;
break;
default:
internal_error (__FILE__, __LINE__,
@@ -645,10 +650,11 @@ set_watchlo (struct pt_watch_regs *regs, int set, CORE_ADDR value)
}
}

-/* Assuming usable watch registers, return the watchhi. */
+/* Assuming usable watch registers REGS, return the watchhi of
+ register N. */

static uint32_t
-get_watchhi (struct pt_watch_regs *regs, int n)
+mips_linux_watch_get_watchhi (struct pt_watch_regs *regs, int n)
{
switch (regs->style)
{
@@ -662,10 +668,12 @@ get_watchhi (struct pt_watch_regs *regs, int n)
}
}

-/* Assuming usable watch registers, set a watchhi value. */
+/* Assuming usable watch registers REGS, set watchhi of register N to
+ VALUE. */

static void
-set_watchhi (struct pt_watch_regs *regs, int n, uint16_t value)
+mips_linux_watch_set_watchhi (struct pt_watch_regs *regs, int n,
+ uint16_t value)
{
switch (regs->style)
{
@@ -701,57 +709,60 @@ mips_show_dr (const char *func, CORE_ADDR addr,
for (i = 0; i < MAX_DEBUG_REGISTER; i++)
printf_unfiltered ("\tDR%d: lo=%s, hi=%s\n", i,
paddress (target_gdbarch (),
- get_watchlo (&watch_mirror, i)),
+ mips_linux_watch_get_watchlo (&watch_mirror,
+ i)),
paddress (target_gdbarch (),
- get_watchhi (&watch_mirror, i)));
+ mips_linux_watch_get_watchhi (&watch_mirror,
+ i)));
}

-/* Return 1 if watch registers are usable. Cached information is used
- unless force is true. */
+/* Read the watch registers of process LWPID and store it in
+ WATCH_READBACK. Save true to *WATCH_READBACK_VALID if watch
+ registers are valid. Return 1 if watch registers are usable.
+ Cached information is used unless FORCE is true. */

static int
-mips_linux_read_watch_registers (int force)
+mips_linux_read_watch_registers (long lwpid,
+ struct pt_watch_regs *watch_readback,
+ int *watch_readback_valid, int force)
{
- int tid;
-
- if (force || watch_readback_valid == 0)
+ if (force || *watch_readback_valid == 0)
{
- tid = ptid_get_lwp (inferior_ptid);
- if (ptrace (PTRACE_GET_WATCH_REGS, tid, &watch_readback) == -1)
+ if (ptrace (PTRACE_GET_WATCH_REGS, lwpid, watch_readback) == -1)
{
- watch_readback_valid = -1;
+ *watch_readback_valid = -1;
return 0;
}
- switch (watch_readback.style)
+ switch (watch_readback->style)
{
case pt_watch_style_mips32:
- if (watch_readback.mips32.num_valid == 0)
+ if (watch_readback->mips32.num_valid == 0)
{
- watch_readback_valid = -1;
+ *watch_readback_valid = -1;
return 0;
}
break;
case pt_watch_style_mips64:
- if (watch_readback.mips64.num_valid == 0)
+ if (watch_readback->mips64.num_valid == 0)
{
- watch_readback_valid = -1;
+ *watch_readback_valid = -1;
return 0;
}
break;
default:
- watch_readback_valid = -1;
+ *watch_readback_valid = -1;
return 0;
}
/* Watch registers appear to be usable. */
- watch_readback_valid = 1;
+ *watch_readback_valid = 1;
}
- return (watch_readback_valid == 1) ? 1 : 0;
+ return (*watch_readback_valid == 1) ? 1 : 0;
}

-/* Convert GDB's type to an IRW mask. */
+/* Convert GDB's TYPE to an IRW mask. */

-static unsigned
-type_to_irw (int type)
+static uint32_t
+mips_linux_watch_type_to_irw (int type)
{
switch (type)
{
@@ -775,7 +786,9 @@ mips_linux_can_use_hw_breakpoint (int type, int cnt, int ot)
int i;
uint32_t wanted_mask, irw_mask;

- if (!mips_linux_read_watch_registers (0))
+ if (!mips_linux_read_watch_registers (ptid_get_lwp (inferior_ptid),
+ &watch_readback,
+ &watch_readback_valid, 0))
return 0;

switch (type)
@@ -793,9 +806,11 @@ mips_linux_can_use_hw_breakpoint (int type, int cnt, int ot)
return 0;
}

- for (i = 0; i < get_num_valid (&watch_readback) && cnt; i++)
+ for (i = 0;
+ i < mips_linux_watch_get_num_valid (&watch_readback) && cnt;
+ i++)
{
- irw_mask = get_irw_mask (&watch_readback, i);
+ irw_mask = mips_linux_watch_get_irw_mask (&watch_readback, i);
if ((irw_mask & wanted_mask) == wanted_mask)
cnt--;
}
@@ -812,13 +827,15 @@ mips_linux_stopped_by_watchpoint (void)
int n;
int num_valid;

- if (!mips_linux_read_watch_registers (1))
+ if (!mips_linux_read_watch_registers (ptid_get_lwp (inferior_ptid),
+ &watch_readback,
+ &watch_readback_valid, 1))
return 0;

- num_valid = get_num_valid (&watch_readback);
+ num_valid = mips_linux_watch_get_num_valid (&watch_readback);

for (n = 0; n < MAX_DEBUG_REGISTER && n < num_valid; n++)
- if (get_watchhi (&watch_readback, n) & (R_MASK | W_MASK))
+ if (mips_linux_watch_get_watchhi (&watch_readback, n) & (R_MASK | W_MASK))
return 1;

return 0;
@@ -836,12 +853,13 @@ mips_linux_stopped_data_address (struct target_ops *t, CORE_ADDR *paddr)
return 0;
}

-/* Set any low order bits in mask that are not set. */
+/* Set any low order bits in MASK that are not set. */

static CORE_ADDR
fill_mask (CORE_ADDR mask)
{
CORE_ADDR f = 1;
+
while (f && f < mask)
{
mask |= f;
@@ -850,15 +868,16 @@ fill_mask (CORE_ADDR mask)
return mask;
}

-/* Try to add a single watch to the specified registers. Return 1 on
- success, 0 on failure. */
+/* Try to add a single watch to the specified registers REGS. The
+ address of added watch is ADDR, the length is LEN, and the mask
+ is IRW. Return 1 on success, 0 on failure. */

static int
-try_one_watch (struct pt_watch_regs *regs, CORE_ADDR addr,
- int len, unsigned irw)
+mips_linux_watch_try_one_watch (struct pt_watch_regs *regs,
+ CORE_ADDR addr, int len, uint32_t irw)
{
CORE_ADDR base_addr, last_byte, break_addr, segment_len;
- CORE_ADDR mask_bits, t_low, t_low_end;
+ CORE_ADDR mask_bits, t_low;
uint16_t t_hi;
int i, free_watches;
struct pt_watch_regs regs_copy;
@@ -871,29 +890,30 @@ try_one_watch (struct pt_watch_regs *regs, CORE_ADDR addr,
base_addr = addr & ~mask_bits;

/* Check to see if it is covered by current registers. */
- for (i = 0; i < get_num_valid (regs); i++)
+ for (i = 0; i < mips_linux_watch_get_num_valid (regs); i++)
{
- t_low = get_watchlo (regs, i);
- if (t_low != 0 && irw == ((unsigned)t_low & irw))
+ t_low = mips_linux_watch_get_watchlo (regs, i);
+ if (t_low != 0 && irw == ((uint32_t) t_low & irw))
{
- t_hi = get_watchhi (regs, i) | IRW_MASK;
- t_low &= ~(CORE_ADDR)t_hi;
+ t_hi = mips_linux_watch_get_watchhi (regs, i) | IRW_MASK;
+ t_low &= ~(CORE_ADDR) t_hi;
if (addr >= t_low && last_byte <= (t_low + t_hi))
return 1;
}
}
/* Try to find an empty register. */
free_watches = 0;
- for (i = 0; i < get_num_valid (regs); i++)
+ for (i = 0; i < mips_linux_watch_get_num_valid (regs); i++)
{
- t_low = get_watchlo (regs, i);
- if (t_low == 0 && irw == (get_irw_mask (regs, i) & irw))
+ t_low = mips_linux_watch_get_watchlo (regs, i);
+ if (t_low == 0
+ && irw == (mips_linux_watch_get_irw_mask (regs, i) & irw))
{
if (mask_bits <= (get_reg_mask (regs, i) | IRW_MASK))
{
/* It fits, we'll take it. */
- set_watchlo (regs, i, base_addr | irw);
- set_watchhi (regs, i, mask_bits & ~IRW_MASK);
+ mips_linux_watch_set_watchlo (regs, i, base_addr | irw);
+ mips_linux_watch_set_watchhi (regs, i, mask_bits & ~IRW_MASK);
return 1;
}
else
@@ -907,21 +927,23 @@ try_one_watch (struct pt_watch_regs *regs, CORE_ADDR addr,
{
/* Try to split it across several registers. */
regs_copy = *regs;
- for (i = 0; i < get_num_valid (&regs_copy); i++)
+ for (i = 0; i < mips_linux_watch_get_num_valid (&regs_copy); i++)
{
- t_low = get_watchlo (&regs_copy, i);
+ t_low = mips_linux_watch_get_watchlo (&regs_copy, i);
t_hi = get_reg_mask (&regs_copy, i) | IRW_MASK;
if (t_low == 0 && irw == (t_hi & irw))
{
- t_low = addr & ~(CORE_ADDR)t_hi;
+ t_low = addr & ~(CORE_ADDR) t_hi;
break_addr = t_low + t_hi + 1;
if (break_addr >= addr + len)
segment_len = len;
else
segment_len = break_addr - addr;
mask_bits = fill_mask (addr ^ (addr + segment_len - 1));
- set_watchlo (&regs_copy, i, (addr & ~mask_bits) | irw);
- set_watchhi (&regs_copy, i, mask_bits & ~IRW_MASK);
+ mips_linux_watch_set_watchlo (&regs_copy, i,
+ (addr & ~mask_bits) | irw);
+ mips_linux_watch_set_watchhi (&regs_copy, i,
+ mask_bits & ~IRW_MASK);
if (break_addr >= addr + len)
{
*regs = regs_copy;
@@ -945,17 +967,18 @@ mips_linux_region_ok_for_hw_watchpoint (CORE_ADDR addr, int len)
struct pt_watch_regs dummy_regs;
int i;

- if (!mips_linux_read_watch_registers (0))
+ if (!mips_linux_read_watch_registers (ptid_get_lwp (inferior_ptid),
+ &watch_readback,
+ &watch_readback_valid, 0))
return 0;

dummy_regs = watch_readback;
/* Clear them out. */
- for (i = 0; i < get_num_valid (&dummy_regs); i++)
- set_watchlo (&dummy_regs, i, 0);
- return try_one_watch (&dummy_regs, addr, len, 0);
+ for (i = 0; i < mips_linux_watch_get_num_valid (&dummy_regs); i++)
+ mips_linux_watch_set_watchlo (&dummy_regs, i, 0);
+ return mips_linux_watch_try_one_watch (&dummy_regs, addr, len, 0);
}

-
/* Write the mirrored watch register values for each thread. */

static int
@@ -981,7 +1004,9 @@ mips_linux_new_thread (struct lwp_info *lp)
{
int tid;

- if (!mips_linux_read_watch_registers (0))
+ if (!mips_linux_read_watch_registers (ptid_get_lwp (inferior_ptid),
+ &watch_readback,
+ &watch_readback_valid, 0))
return;

tid = ptid_get_lwp (lp->ptid);
@@ -989,25 +1014,29 @@ mips_linux_new_thread (struct lwp_info *lp)
perror_with_name (_("Couldn't write debug register"));
}

-/* Fill in the watch registers with the currently cached watches. */
+/* Fill in the watch registers REGS with the currently cached
+ watches CURRENT_WATCHES. */

static void
-populate_regs_from_watches (struct pt_watch_regs *regs)
+mips_linux_watch_populate_regs (struct mips_watchpoint *current_watches,
+ struct pt_watch_regs *regs)
{
struct mips_watchpoint *w;
int i;

/* Clear them out. */
- for (i = 0; i < get_num_valid (regs); i++)
+ for (i = 0; i < mips_linux_watch_get_num_valid (regs); i++)
{
- set_watchlo (regs, i, 0);
- set_watchhi (regs, i, 0);
+ mips_linux_watch_set_watchlo (regs, i, 0);
+ mips_linux_watch_set_watchhi (regs, i, 0);
}

w = current_watches;
while (w)
{
- i = try_one_watch (regs, w->addr, w->len, type_to_irw (w->type));
+ uint32_t irw = mips_linux_watch_type_to_irw (w->type);
+
+ i = mips_linux_watch_try_one_watch (regs, w->addr, w->len, irw);
/* They must all fit, because we previously calculated that they
would. */
gdb_assert (i);
@@ -1029,7 +1058,9 @@ mips_linux_insert_watchpoint (CORE_ADDR addr, int len, int type,
int i;
int retval;

- if (!mips_linux_read_watch_registers (0))
+ if (!mips_linux_read_watch_registers (ptid_get_lwp (inferior_ptid),
+ &watch_readback,
+ &watch_readback_valid, 0))
return -1;

if (len <= 0)
@@ -1037,10 +1068,11 @@ mips_linux_insert_watchpoint (CORE_ADDR addr, int len, int type,

regs = watch_readback;
/* Add the current watches. */
- populate_regs_from_watches (&regs);
+ mips_linux_watch_populate_regs (current_watches, &regs);

/* Now try to add the new watch. */
- if (!try_one_watch (&regs, addr, len, type_to_irw (type)))
+ if (!mips_linux_watch_try_one_watch (&regs, addr, len,
+ mips_linux_watch_type_to_irw (type)))
return -1;

/* It fit. Stick it on the end of the list. */
@@ -1102,7 +1134,7 @@ mips_linux_remove_watchpoint (CORE_ADDR addr, int len, int type,
gdb_assert (watch_readback_valid == 1);

watch_mirror = watch_readback;
- populate_regs_from_watches (&watch_mirror);
+ mips_linux_watch_populate_regs (current_watches, &watch_mirror);

retval = write_watchpoint_regs ();

--
1.7.7.6