Re: [RFD+PATCH] ISA bit treatment on the MIPS platform

Maciej W. Rozycki
2012-05-18T22:01:36Z

Hello again,

As promised I am providing Linux results now. With the recent fixes to
gdbserver and mips-linux-tdep the number of Linux progressions matches, as
far as the order of magnitude is concerned, that of bare-iron ones and is
around 30 for MIPS16 and around 100 for microMIPS multilibs. The exact
new numbers are as follows (as usually, these are indicative only because
of intermittent failures, mainly in the thread tests):

standard MIPS/o32:

=== gdb Summary ===

# of expected passes 15667
# of unexpected failures 77
# of unexpected successes 2
# of expected failures 45
# of known failures 62
# of unresolved testcases 1
# of untested testcases 45
# of unsupported tests 161

standard MIPS/n64:

=== gdb Summary ===

# of expected passes 15407
# of unexpected failures 158
# of unexpected successes 2
# of expected failures 47
# of known failures 62
# of unresolved testcases 5
# of untested testcases 46
# of unsupported tests 162

MIPS16/o32:

=== gdb Summary ===

# of expected passes 15569
# of unexpected failures 145
# of unexpected successes 2
# of expected failures 45
# of known failures 62
# of unresolved testcases 3
# of untested testcases 46
# of unsupported tests 161

microMIPS/o32:

=== gdb Summary ===

# of expected passes 15480
# of unexpected failures 97
# of unexpected successes 2
# of expected failures 45
# of known failures 62
# of unresolved testcases 1
# of untested testcases 46
# of unsupported tests 162

Also as promised here's a test case to explicitly cover this issue,
similar in spirit to the examples used in the problem report. It fails
for MIPS16 multilibs with our current code (and for microMIPS ones with my
microMIPS support patch applied) and passes with my proposed ISA bit
changes included.

And last but not least, I've included an update to cover Linux microMIPS
signal trampolines, complementing the change just sent as a followup to
the original microMIPS support patch.

2012-05-18 Maciej W. Rozycki <macro@codesourcery.com>

gdb/testsuite/
* gdb.base/func-ptrs.c: New file.
* gdb.base/func-ptrs.exp: New file.

2012-05-18 Maciej W. Rozycki <macro@codesourcery.com>

gdb/
* mips-tdep.h (mips_unmake_compact_addr): New prototype.
* mips-tdep.c (unmake_compact_addr): Rename to...
(mips_unmake_compact_addr): ... this. Make external.
(make_compact_addr): Rename to...
(mips_make_compact_addr): ... this.
(mips_pc_is_mips): Update accordingly.
(mips_pc_is_mips16): Likewise.
(mips_pc_is_micromips): Likewise.
(mips_pc_isa): Likewise.
(mips_adjust_dwarf2_addr): Likewise.
(mips_fetch_instruction): Likewise.
(mips_breakpoint_from_pc): Likewise.
(mips_remote_breakpoint_from_pc): Likewise.
(mips_adjust_breakpoint_address): Likewise.
* mips-linux-tdep.c (micromips_linux_sigframe_validate): Strip
off the ISA bit from the address returned.

Maciej

gdb-test-func-ptrs.diff
Index: gdb-fsf-trunk-quilt/gdb/testsuite/gdb.base/func-ptrs.c
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ gdb-fsf-trunk-quilt/gdb/testsuite/gdb.base/func-ptrs.c 2012-05-14 22:46:46.185606207 +0100
@@ -0,0 +1,50 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2012 Free Software Foundation, Inc.
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <http://www.gnu.org/licenses/>. */
+
+void
+sentinel (void)
+{
+ return;
+}
+
+int
+incr (int i)
+{
+ sentinel ();
+ return i + 1;
+}
+
+int
+decr (int i)
+{
+ sentinel ();
+ return i - 1;
+}
+
+int (*calc) (int) = incr;
+
+int
+main (void)
+{
+ int i = -1;
+
+ i = calc (i);
+ i = calc (i);
+ i = calc (i);
+
+ return i;
+}
Index: gdb-fsf-trunk-quilt/gdb/testsuite/gdb.base/func-ptrs.exp
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ gdb-fsf-trunk-quilt/gdb/testsuite/gdb.base/func-ptrs.exp 2012-05-14 23:17:51.115558348 +0100
@@ -0,0 +1,95 @@
+# Copyright 2012 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+
+set testname func-ptrs
+set srcfile ${testname}.c
+if { [prepare_for_testing ${testname}.exp ${testname} ${srcfile}] } {
+ return -1
+}
+
+if { ![runto_main] } {
+ untested ${testname}.exp
+ return -1
+}
+
+
+# First set our breakpoints.
+
+set breakpoint_re \
+ "Breakpoint $decimal at $hex: file .*${srcfile}, line $decimal\\."
+gdb_test "break sentinel if calc == decr" \
+ "${breakpoint_re}" \
+ "breakpoint at sentinel"
+gdb_test "break incr" \
+ "${breakpoint_re}" \
+ "breakpoint at incr"
+gdb_test "break decr" \
+ "${breakpoint_re}" \
+ "breakpoint at decr"
+
+
+# Check if we run through to the breakpoint in incr.
+
+gdb_test "continue" \
+ "Breakpoint $decimal, incr \\(i=-1\\)\[ \r\n\]+at .*${srcfile}:$decimal\[\r\n\]+.*" \
+ "continue to incr, first time"
+
+
+# Go back up, make sure the return value is 0.
+
+gdb_test "finish" \
+ "Run till exit from #0 +incr \\(i=-1\\)\[ \r\n\]+at .*${srcfile}:$decimal\[\r\n\]+($hex in )?main \\(\\)\[ \r\n\]+at .*${srcfile}:$decimal\[\r\n\]+.*Value returned is \\$$decimal = 0" \
+ "go back to main from incr, first time"
+
+
+# Redirect calc and see if we run to the breakpoint in decr instead.
+
+gdb_test_no_output "set calc = decr" "set calc to decr"
+gdb_test "continue" \
+ "Breakpoint $decimal, decr \\(i=0\\)\[ \r\n\]+at .*${srcfile}:$decimal\[\r\n\]+.*" \
+ "continue to decr"
+
+
+# Go back up, check if we stop in sentinel instead.
+
+gdb_test "finish" \
+ "Run till exit from #0 +decr \\(i=0\\)\[ \r\n\]+at .*${srcfile}:$decimal\[\r\n\]+Breakpoint $decimal, sentinel \\(\\)\[ \r\n\]+at .*${srcfile}:$decimal\[\r\n\]+.*" \
+ "stop in sentinel"
+
+
+# Go back all the way up to main, make sure the return value is -1.
+
+gdb_test_no_output "up-silently" "move up to decr"
+gdb_test "finish" \
+ "Run till exit from #1 +($hex in )?decr \\(i=0\\)\[ \r\n\]+at .*${srcfile}:$decimal\[\r\n\]+($hex in )?main \\(\\)\[ \r\n\]+at .*${srcfile}:$decimal\[\r\n\]+.*Value returned is \\$$decimal = -1" \
+ "go back to main from decr"
+
+
+# Reset calc and see if we run to the breakpoint in incr again.
+
+gdb_test_no_output "set calc = incr" "set calc to incr"
+gdb_test "continue" \
+ "Breakpoint $decimal, incr \\(i=-1\\)\[ \r\n\]+at .*${srcfile}:$decimal\[\r\n\]+.*" \
+ "continue to incr, second time"
+
+
+# Go back up again, make sure the return value is 0.
+
+gdb_test "finish" \
+ "Run till exit from #0 +incr \\(i=-1\\)\[ \r\n\]+at .*${srcfile}:$decimal\[\r\n\]+($hex in )?main \\(\\)\[ \r\n\]+at .*${srcfile}:$decimal\[\r\n\]+.*Value returned is \\$$decimal = 0" \
+ "go back to main from incr, second time"
+
+
+# All done!

gdb-mips16-isa-bit-sigtramp.diff
Index: gdb-fsf-trunk-quilt/gdb/mips-tdep.c
===================================================================
--- gdb-fsf-trunk-quilt.orig/gdb/mips-tdep.c 2012-05-18 20:56:25.545667440 +0100
+++ gdb-fsf-trunk-quilt/gdb/mips-tdep.c 2012-05-18 20:46:31.615619743 +0100
@@ -321,8 +321,8 @@ is_micromips_addr (struct gdbarch *gdbar

/* Strip the ISA (compression) bit off from ADDR. */

-static CORE_ADDR
-unmake_compact_addr (CORE_ADDR addr)
+CORE_ADDR
+mips_unmake_compact_addr (CORE_ADDR addr)
{
return ((addr) & ~(CORE_ADDR) 1);
}
@@ -330,7 +330,7 @@ unmake_compact_addr (CORE_ADDR addr)
/* Add the ISA (compression) bit to ADDR. */

static CORE_ADDR
-make_compact_addr (CORE_ADDR addr)
+mips_make_compact_addr (CORE_ADDR addr)
{
return ((addr) | (CORE_ADDR) 1);
}
@@ -1147,7 +1147,7 @@ mips_pc_is_mips (CORE_ADDR memaddr)
stored by elfread.c in the high bit of the info field. Use this
to decide if the function is standard MIPS. Otherwise if bit 0
of the address is clear, then this is a standard MIPS function. */
- sym = lookup_minimal_symbol_by_pc (make_compact_addr (memaddr));
+ sym = lookup_minimal_symbol_by_pc (mips_make_compact_addr (memaddr));
if (sym)
return msymbol_is_mips (sym);
else
@@ -1165,7 +1165,7 @@ mips_pc_is_mips16 (struct gdbarch *gdbar
elfread.c in the high bit of the info field. Use this to decide
if the function is MIPS16. Otherwise if bit 0 of the address is
set, then ELF file flags will tell if this is a MIPS16 function. */
- sym = lookup_minimal_symbol_by_pc (make_compact_addr (memaddr));
+ sym = lookup_minimal_symbol_by_pc (mips_make_compact_addr (memaddr));
if (sym)
return msymbol_is_mips16 (sym);
else
@@ -1184,7 +1184,7 @@ mips_pc_is_micromips (struct gdbarch *gd
if the function is microMIPS. Otherwise if bit 0 of the address
is set, then ELF file flags will tell if this is a microMIPS
function. */
- sym = lookup_minimal_symbol_by_pc (make_compact_addr (memaddr));
+ sym = lookup_minimal_symbol_by_pc (mips_make_compact_addr (memaddr));
if (sym)
return msymbol_is_micromips (sym);
else
@@ -1204,7 +1204,7 @@ mips_pc_isa (struct gdbarch *gdbarch, CO
this to decide if the function is MIPS16 or microMIPS or normal
MIPS. Otherwise if bit 0 of the address is set, then ELF file
flags will tell if this is a MIPS16 or a microMIPS function. */
- sym = lookup_minimal_symbol_by_pc (make_compact_addr (memaddr));
+ sym = lookup_minimal_symbol_by_pc (mips_make_compact_addr (memaddr));
if (sym)
{
if (msymbol_is_micromips (sym))
@@ -1230,8 +1230,8 @@ mips_pc_isa (struct gdbarch *gdbarch, CO
static CORE_ADDR
mips_adjust_dwarf2_addr (CORE_ADDR pc)
{
- pc = unmake_compact_addr (pc);
- return mips_pc_is_mips (pc) ? pc : make_compact_addr (pc);
+ pc = mips_unmake_compact_addr (pc);
+ return mips_pc_is_mips (pc) ? pc : mips_make_compact_addr (pc);
}

/* Recalculate the line record requested so that the resulting PC has the
@@ -1374,7 +1374,7 @@ mips_fetch_instruction (struct gdbarch *
case ISA_MICROMIPS:
case ISA_MIPS16:
instlen = MIPS_INSN16_SIZE;
- addr = unmake_compact_addr (addr);
+ addr = mips_unmake_compact_addr (addr);
break;
case ISA_MIPS:
instlen = MIPS_INSN32_SIZE;
@@ -6798,7 +6798,7 @@ mips_breakpoint_from_pc (struct gdbarch
if (mips_pc_is_mips16 (gdbarch, pc))
{
static gdb_byte mips16_big_breakpoint[] = { 0xe8, 0xa5 };
- *pcptr = unmake_compact_addr (pc);
+ *pcptr = mips_unmake_compact_addr (pc);
*lenptr = sizeof (mips16_big_breakpoint);
return mips16_big_breakpoint;
}
@@ -6813,7 +6813,7 @@ mips_breakpoint_from_pc (struct gdbarch
insn = mips_fetch_instruction (gdbarch, ISA_MICROMIPS, pc, &status);
size = status ? 2
: mips_insn_size (ISA_MICROMIPS, insn) == 2 ? 2 : 4;
- *pcptr = unmake_compact_addr (pc);
+ *pcptr = mips_unmake_compact_addr (pc);
*lenptr = size;
return (size == 2) ? micromips16_big_breakpoint
: micromips32_big_breakpoint;
@@ -6849,7 +6849,7 @@ mips_breakpoint_from_pc (struct gdbarch
if (mips_pc_is_mips16 (gdbarch, pc))
{
static gdb_byte mips16_little_breakpoint[] = { 0xa5, 0xe8 };
- *pcptr = unmake_compact_addr (pc);
+ *pcptr = mips_unmake_compact_addr (pc);
*lenptr = sizeof (mips16_little_breakpoint);
return mips16_little_breakpoint;
}
@@ -6864,7 +6864,7 @@ mips_breakpoint_from_pc (struct gdbarch
insn = mips_fetch_instruction (gdbarch, ISA_MICROMIPS, pc, &status);
size = status ? 2
: mips_insn_size (ISA_MICROMIPS, insn) == 2 ? 2 : 4;
- *pcptr = unmake_compact_addr (pc);
+ *pcptr = mips_unmake_compact_addr (pc);
*lenptr = size;
return (size == 2) ? micromips16_little_breakpoint
: micromips32_little_breakpoint;
@@ -6908,7 +6908,7 @@ mips_remote_breakpoint_from_pc (struct g

if (mips_pc_is_mips16 (gdbarch, pc))
{
- *pcptr = unmake_compact_addr (pc);
+ *pcptr = mips_unmake_compact_addr (pc);
*kindptr = 2;
}
else if (mips_pc_is_micromips (gdbarch, pc))
@@ -6919,7 +6919,7 @@ mips_remote_breakpoint_from_pc (struct g

insn = mips_fetch_instruction (gdbarch, ISA_MICROMIPS, pc, &status);
size = status ? 2 : mips_insn_size (ISA_MICROMIPS, insn) == 2 ? 2 : 4;
- *pcptr = unmake_compact_addr (pc);
+ *pcptr = mips_unmake_compact_addr (pc);
*kindptr = size | 1;
}
else
@@ -7169,7 +7169,7 @@ mips_adjust_breakpoint_address (struct g
CORE_ADDR addr, jmpaddr;
int i;

- boundary = unmake_compact_addr (boundary);
+ boundary = mips_unmake_compact_addr (boundary);

/* The only MIPS16 instructions with delay slots are JAL, JALX,
JALR and JR. An absolute JAL/JALX is always 4 bytes long,
@@ -7187,7 +7187,7 @@ mips_adjust_breakpoint_address (struct g
addr = bpaddr;
for (i = 1; i < 4; i++)
{
- if (unmake_compact_addr (addr) == boundary)
+ if (mips_unmake_compact_addr (addr) == boundary)
break;
addr -= MIPS_INSN16_SIZE;
if (i == 1 && instruction_has_delay_slot (gdbarch, addr, 0))
Index: gdb-fsf-trunk-quilt/gdb/mips-linux-tdep.c
===================================================================
--- gdb-fsf-trunk-quilt.orig/gdb/mips-linux-tdep.c 2012-05-18 20:47:17.000000000 +0100
+++ gdb-fsf-trunk-quilt/gdb/mips-linux-tdep.c 2012-05-18 20:56:18.315621662 +0100
@@ -1237,7 +1237,10 @@ micromips_linux_sigframe_validate (const
struct frame_info *this_frame,
CORE_ADDR pc)
{
- return mips_pc_is_micromips (get_frame_arch (this_frame), pc) ? pc : 0;
+ if (mips_pc_is_micromips (get_frame_arch (this_frame), pc))
+ return mips_unmake_compact_addr (pc);
+ else
+ return 0;
}

/* Implement the "write_pc" gdbarch method. */
Index: gdb-fsf-trunk-quilt/gdb/mips-tdep.h
===================================================================
--- gdb-fsf-trunk-quilt.orig/gdb/mips-tdep.h 2012-05-18 20:40:32.000000000 +0100
+++ gdb-fsf-trunk-quilt/gdb/mips-tdep.h 2012-05-18 20:43:04.005621216 +0100
@@ -161,6 +161,9 @@ enum
/* Single step based on where the current instruction will take us. */
extern int mips_software_single_step (struct frame_info *frame);

+/* Strip the ISA (compression) bit off from ADDR. */
+extern CORE_ADDR mips_unmake_compact_addr (CORE_ADDR addr);
+
/* Tell if the program counter value in MEMADDR is in a standard
MIPS function. */
extern int mips_pc_is_mips (bfd_vma memaddr);