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

["Maciej W. Rozycki" <macro at codesourcery dot com>]

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);