[RFC] PPC: Skip call to __eabi in main()

[RFC] PPC: Skip call to __eabi in main()

[Kevin Buettner]

Some versions of gcc now generate a call to __eabi in main() for the
powerpc-eabi and powerpc-eabispe targets.  The patch below causes this
call to be skipped when placing a breakpoint at the first line of
main(), thus fixing quite a few failures when running the test suite.

I've tested this patch against powerpc-eabi and powerpc-eabispe
running on the simulator.

Comments?

	* rs6000-tdep.c (rs6000_skip_main_prologue): New function.
	(rs6000_gdb_arch_init): Register rs6000_skip_main_prologue.

Index: rs6000-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/rs6000-tdep.c,v
retrieving revision 1.318
diff -u -p -r1.318 rs6000-tdep.c
--- rs6000-tdep.c	15 Jul 2008 18:32:06 -0000	1.318
+++ rs6000-tdep.c	21 Jul 2008 22:39:00 -0000
@@ -1769,6 +1769,32 @@ rs6000_skip_prologue (struct gdbarch *gd
   return pc;
 }
 
+/* Check that the code pointed to by PC corresponds to a call to
+   __eabi, skip it if so.  Return PC otherwise.  */
+
+CORE_ADDR
+rs6000_skip_main_prologue (struct gdbarch *gdbarch, CORE_ADDR pc)
+{
+  gdb_byte buf[4];
+  unsigned long op;
+
+  if (target_read_memory (pc, buf, 4))
+    return pc;
+  op = extract_unsigned_integer (buf, 4);
+
+  if ((op & 0xfc000001) == 0x48000001)
+    {
+      CORE_ADDR displ = op & 0x03fffffc;
+      CORE_ADDR call_dest = pc + 4 + displ;
+      struct minimal_symbol *s = lookup_minimal_symbol_by_pc (call_dest);
+
+      if (s != NULL
+          && SYMBOL_LINKAGE_NAME (s) != NULL
+	  && strcmp (SYMBOL_LINKAGE_NAME (s), "__eabi") == 0)
+	pc += 4;
+    }
+  return pc;
+}
 
 /* All the ABI's require 16 byte alignment.  */
 static CORE_ADDR
@@ -3238,6 +3264,7 @@ rs6000_gdbarch_init (struct gdbarch_info
 
   set_gdbarch_skip_prologue (gdbarch, rs6000_skip_prologue);
   set_gdbarch_in_function_epilogue_p (gdbarch, rs6000_in_function_epilogue_p);
+  set_gdbarch_skip_main_prologue (gdbarch, rs6000_skip_main_prologue);
 
   set_gdbarch_inner_than (gdbarch, core_addr_lessthan);
   set_gdbarch_breakpoint_from_pc (gdbarch, rs6000_breakpoint_from_pc);

Re: [RFC] PPC: Skip call to __eabi in main()

[Thiago Jung Bauermann]

On Mon, 2008-07-21 at 15:53 -0700, Kevin Buettner wrote:
> +  if ((op & 0xfc000001) == 0x48000001)
> +    {
> +      CORE_ADDR displ = op & 0x03fffffc;

I think this would be more legible with #defines instead of these
hardcoded opcodes and bitmasks. Also, a comment above each #define
explaining what the value means, of course.
-- 
[]'s
Thiago Jung Bauermann
Software Engineer
IBM Linux Technology Center

Re: [RFC] PPC: Skip call to __eabi in main()

[Kevin Buettner]

On Tue, 22 Jul 2008 01:44:27 -0300
Thiago Jung Bauermann <bauerman@br.ibm.com> wrote:

> On Mon, 2008-07-21 at 15:53 -0700, Kevin Buettner wrote:
> > +  if ((op & 0xfc000001) == 0x48000001)
> > +    {
> > +      CORE_ADDR displ = op & 0x03fffffc;
> 
> I think this would be more legible with #defines instead of these
> hardcoded opcodes and bitmasks. Also, a comment above each #define
> explaining what the value means, of course.

I agree.

I found that this approach (of using macros for the masks and opcodes)
is used at two other locations in rs6000-tdep.c, but noticed that the
naming convention is not quite uniform.  The _MASK suffix is used in
both locations, but one location uses the _INSN suffix for opcode
values while the other location uses _INSTRUCTION.  I decided to go
with the longer name.  It does make the lines longer, but not long
enough to force me to have to split them in order to stay within
eighty columns.

I've placed the block of new macro definitions just ahead of
skip_prologue().  The existing convention seems to be that they are
placed just before the function in which they are first used.  Thus,
these macros would normally be placed just ahead of the new function,
rs6000_skip_main_prologue, that I'm introducing in this patch. 
However, skip_prologue() has a couple of spots which use hardcoded
constants for decoding `bl' instructions, thus it makes sense to put
these macros in a location where they'll be usable by both functions.

I'll post a later patch which updates skip_prologue() to also use
these macros instead of hardcoded constants for decoding the bl
instruction.

Further comments?

	* rs6000-tdep.c (BL_MASK, BL_INSTRUCTION, BL_DISPLACEMENT_MASK):
	New macros.
	(rs6000_skip_main_prologue): New function.
	(rs6000_gdb_arch_init): Register rs6000_skip_main_prologue.

Index: rs6000-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/rs6000-tdep.c,v
retrieving revision 1.318
diff -u -p -r1.318 rs6000-tdep.c
--- rs6000-tdep.c	15 Jul 2008 18:32:06 -0000	1.318
+++ rs6000-tdep.c	23 Jul 2008 00:43:05 -0000
@@ -1146,6 +1146,20 @@ bl_to_blrl_insn_p (CORE_ADDR pc, int ins
   return 0;
 }
 
+/* Masks for decoding a branch-and-link (bl) instruction.  
+
+   BL_MASK and BL_INSTRUCTION are used in combination with each other.
+   The former is anded with the opcode in question; if the result of
+   this masking operation is equal to BL_INSTRUCTION, then the opcode in
+   question is a ``bl'' instruction.
+   
+   BL_DISPLACMENT_MASK is anded with the opcode in order to extract
+   the branch displacement.  */
+
+#define BL_MASK 0xfc000001
+#define BL_INSTRUCTION 0x48000001
+#define BL_DISPLACEMENT_MASK 0x03fffffc
+
 /* return pc value after skipping a function prologue and also return
    information about a function frame.
 
@@ -1769,6 +1783,32 @@ rs6000_skip_prologue (struct gdbarch *gd
   return pc;
 }
 
+/* Check that the code pointed to by PC corresponds to a call to
+   __eabi, skip it if so.  Return PC otherwise.  */
+
+CORE_ADDR
+rs6000_skip_main_prologue (struct gdbarch *gdbarch, CORE_ADDR pc)
+{
+  gdb_byte buf[4];
+  unsigned long op;
+
+  if (target_read_memory (pc, buf, 4))
+    return pc;
+  op = extract_unsigned_integer (buf, 4);
+
+  if ((op & BL_MASK) == BL_INSTRUCTION)
+    {
+      CORE_ADDR displ = op & BL_DISPLACEMENT_MASK;
+      CORE_ADDR call_dest = pc + 4 + displ;
+      struct minimal_symbol *s = lookup_minimal_symbol_by_pc (call_dest);
+
+      if (s != NULL
+          && SYMBOL_LINKAGE_NAME (s) != NULL
+	  && strcmp (SYMBOL_LINKAGE_NAME (s), "__eabi") == 0)
+	pc += 4;
+    }
+  return pc;
+}
 
 /* All the ABI's require 16 byte alignment.  */
 static CORE_ADDR
@@ -3238,6 +3278,7 @@ rs6000_gdbarch_init (struct gdbarch_info
 
   set_gdbarch_skip_prologue (gdbarch, rs6000_skip_prologue);
   set_gdbarch_in_function_epilogue_p (gdbarch, rs6000_in_function_epilogue_p);
+  set_gdbarch_skip_main_prologue (gdbarch, rs6000_skip_main_prologue);
 
   set_gdbarch_inner_than (gdbarch, core_addr_lessthan);
   set_gdbarch_breakpoint_from_pc (gdbarch, rs6000_breakpoint_from_pc);

Re: [RFC] PPC: Skip call to __eabi in main()

[Kevin Buettner]

I've done some further testing of my patch and have found that
an additional test is needed to accomodate -fleading-underscore.
(See the comment in the patch below...)

I've also expanded the comment that precedes rs6000_skip_main_prologue().

I'll wait a few more days before committing this change.

Kevin

 	* rs6000-tdep.c (BL_MASK, BL_INSTRUCTION, BL_DISPLACEMENT_MASK):
 	New macros.
 	(rs6000_skip_main_prologue): New function.
 	(rs6000_gdb_arch_init): Register rs6000_skip_main_prologue.

Index: rs6000-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/rs6000-tdep.c,v
retrieving revision 1.318
diff -u -p -r1.318 rs6000-tdep.c
--- rs6000-tdep.c	15 Jul 2008 18:32:06 -0000	1.318
+++ rs6000-tdep.c	29 Jul 2008 00:11:52 -0000
@@ -1146,6 +1146,20 @@ bl_to_blrl_insn_p (CORE_ADDR pc, int ins
   return 0;
 }
 
+/* Masks for decoding a branch-and-link (bl) instruction.  
+
+   BL_MASK and BL_INSTRUCTION are used in combination with each other.
+   The former is anded with the opcode in question; if the result of
+   this masking operation is equal to BL_INSTRUCTION, then the opcode in
+   question is a ``bl'' instruction.
+   
+   BL_DISPLACMENT_MASK is anded with the opcode in order to extract
+   the branch displacement.  */
+
+#define BL_MASK 0xfc000001
+#define BL_INSTRUCTION 0x48000001
+#define BL_DISPLACEMENT_MASK 0x03fffffc
+
 /* return pc value after skipping a function prologue and also return
    information about a function frame.
 
@@ -1769,6 +1783,41 @@ rs6000_skip_prologue (struct gdbarch *gd
   return pc;
 }
 
+/* When compiling for EABI, some versions of GCC emit a call to __eabi
+   in the prologue of main().
+
+   The function below examines the code pointed at by PC and checks to
+   see if it corresponds to a call to __eabi.  If so, it returns the
+   address of the instruction following that call.  Otherwise, it simply
+   returns PC.  */
+
+CORE_ADDR
+rs6000_skip_main_prologue (struct gdbarch *gdbarch, CORE_ADDR pc)
+{
+  gdb_byte buf[4];
+  unsigned long op;
+
+  if (target_read_memory (pc, buf, 4))
+    return pc;
+  op = extract_unsigned_integer (buf, 4);
+
+  if ((op & BL_MASK) == BL_INSTRUCTION)
+    {
+      CORE_ADDR displ = op & BL_DISPLACEMENT_MASK;
+      CORE_ADDR call_dest = pc + 4 + displ;
+      struct minimal_symbol *s = lookup_minimal_symbol_by_pc (call_dest);
+
+      /* We check for ___eabi (three leading underscores) in addition
+         to __eabi in case the GCC option "-fleading-underscore" was
+	 used to compile the program.  */
+      if (s != NULL
+          && SYMBOL_LINKAGE_NAME (s) != NULL
+	  && (strcmp (SYMBOL_LINKAGE_NAME (s), "__eabi") == 0
+	      || strcmp (SYMBOL_LINKAGE_NAME (s), "___eabi") == 0))
+	pc += 4;
+    }
+  return pc;
+}
 
 /* All the ABI's require 16 byte alignment.  */
 static CORE_ADDR
@@ -3238,6 +3287,7 @@ rs6000_gdbarch_init (struct gdbarch_info
 
   set_gdbarch_skip_prologue (gdbarch, rs6000_skip_prologue);
   set_gdbarch_in_function_epilogue_p (gdbarch, rs6000_in_function_epilogue_p);
+  set_gdbarch_skip_main_prologue (gdbarch, rs6000_skip_main_prologue);
 
   set_gdbarch_inner_than (gdbarch, core_addr_lessthan);
   set_gdbarch_breakpoint_from_pc (gdbarch, rs6000_breakpoint_from_pc);

Re: [RFC] PPC: Skip call to __eabi in main()

[Luis Machado]

Hi Kevin,

Sorry it took a while to reply.

On Mon, 2008-07-28 at 17:22 -0700, Kevin Buettner wrote:
> +#define BL_MASK 0xfc000001

We have a very similar mask used for displaced stepping called
BRANCH_MASK (0xfc000000). It doesn't care about the LK bit though, its
purpose is just to check for a generic branch instruction.

Maybe we should rename BL_MASK to something else incorporating the
notion that we expect a LK bit? Or maybe doing the check for the LK bit
manually in the code and using BL_MASK as is.

> +#define BL_INSTRUCTION 0x48000001

Similarly, we also have B_INSN (0x48000000), lacking the LK bit.

> +#define BL_DISPLACEMENT_MASK 0x03fffffc

Should we make the naming more generic (B_LI_MASK maybe?) as the
displacement field is common to a variety of I-form branch instructions?
Just a suggestion, doesn't need to change if you don't feel it clarifies
things.

Regards,
Luis

Re: [RFC] PPC: Skip call to __eabi in main()

[Kevin Buettner]

Hi Luis,

Thanks for looking over my patch.  See below for my comments...

On Fri, 01 Aug 2008 15:45:14 -0300
Luis Machado <luisgpm@linux.vnet.ibm.com> wrote:

> On Mon, 2008-07-28 at 17:22 -0700, Kevin Buettner wrote:
> > +#define BL_MASK 0xfc000001
> 
> We have a very similar mask used for displaced stepping called
> BRANCH_MASK (0xfc000000). It doesn't care about the LK bit though, its
> purpose is just to check for a generic branch instruction.
> 
> Maybe we should rename BL_MASK to something else incorporating the
> notion that we expect a LK bit? Or maybe doing the check for the LK bit
> manually in the code and using BL_MASK as is.
> 
> > +#define BL_INSTRUCTION 0x48000001
> 
> Similarly, we also have B_INSN (0x48000000), lacking the LK bit.
> 
> > +#define BL_DISPLACEMENT_MASK 0x03fffffc
> 
> Should we make the naming more generic (B_LI_MASK maybe?) as the
> displacement field is common to a variety of I-form branch instructions?
> Just a suggestion, doesn't need to change if you don't feel it clarifies
> things.

Well, actually...

I'd prefer macros like IS_BL_INSTRUCTION and BL_DISPLACEMENT that would
work something like this:

  op = extract_unsigned_integer (buf, 4);

  if (IS_BL_INSTRUCTION (op))
    {
      CORE_ADDR displ = BL_DISPLACEMENT (op);

Contrast this with the code that I posted in my patch:

  op = extract_unsigned_integer (buf, 4);

  if ((op & BL_MASK) == BL_INSTRUCTION)
    {
      CORE_ADDR displ = op & BL_DISPLACEMENT_MASK;

I like the first form better because it removes the explicit mask and
comparison.  (To be clear, these operations would still exist, but
would be buried in the macro definitions.)  In my opinion, the first
form is easier to read and is less error prone.

If we were to use this approach, we'd presumably have other macros
such as IS_B_INSTRUCTION and B_DISPLACEMENT too.

We could write a version of B_DISPLACMENT which would work for BL
instructions, but I would prefer to have separate macros for each
case because it's easier to verify correctness.  Consider
the following:

  if (IS_BL_INSTRUCTION (op))
    {
      CORE_ADDR displ = B_DISPLACEMENT (op);

Many times, we create new code by copying and pasting existing code.  If
I, as a patch reviewer, were to see this in a patch, I might wonder if
B_DISPLACEMENT also works for BL instructions and would ultimately
have to refer to the macro and consult an instruction manual to verify
that the code is correct.

Using a separate BL-specific macro eliminates any concern that either
a typo was made or that an existing hunk of code was copied and not
entirely converted.

With regard to my patch, I'd prefer to commit it in its present form
and then address improvements to PowerPC instruction decoding at a
later time.  I considered using my preferred approach when I adjusted
my patch, but decided against doing so because a different approach
(that of using explicit masks and comparisons) was already in use.

FWIW, this isn't the only approach that I find compelling.  I recently
worked on a port which utilizes the instruction decoder in opcodes/. 
This decoder is also used for the disassembler and simulator.  It
completely decodes the instruction, returning a symbolic (via enums)
opcodes, and completely decoded instruction offsets, registers, etc.

Kevin

Re: [RFC] PPC: Skip call to __eabi in main()

[Daniel Jacobowitz]

On Wed, Aug 06, 2008 at 05:40:37PM -0700, Kevin Buettner wrote:
> With regard to my patch, I'd prefer to commit it in its present form
> and then address improvements to PowerPC instruction decoding at a
> later time.  I considered using my preferred approach when I adjusted
> my patch, but decided against doing so because a different approach
> (that of using explicit masks and comparisons) was already in use.

I think this is reasonable.

> FWIW, this isn't the only approach that I find compelling.  I recently
> worked on a port which utilizes the instruction decoder in opcodes/. 
> This decoder is also used for the disassembler and simulator.  It
> completely decodes the instruction, returning a symbolic (via enums)
> opcodes, and completely decoded instruction offsets, registers, etc.

I wish other ports provided enough detail in libopcodes to do this!
It's an excellent approach.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC] PPC: Skip call to __eabi in main()

[Luis Machado]

On Thu, 2008-08-07 at 10:03 -0400, Daniel Jacobowitz wrote:
> On Wed, Aug 06, 2008 at 05:40:37PM -0700, Kevin Buettner wrote:
> > With regard to my patch, I'd prefer to commit it in its present form
> > and then address improvements to PowerPC instruction decoding at a
> > later time.  I considered using my preferred approach when I adjusted
> > my patch, but decided against doing so because a different approach
> > (that of using explicit masks and comparisons) was already in use.
> 
> I think this is reasonable.
> 

Sounds good. We can unify the handling of those later most probably.

Regards,
Luis

Re: [RFC] PPC: Skip call to __eabi in main()

[Kevin Buettner]

On Mon, 28 Jul 2008 17:22:21 -0700
Kevin Buettner <kevinb@redhat.com> wrote:

>  	* rs6000-tdep.c (BL_MASK, BL_INSTRUCTION, BL_DISPLACEMENT_MASK):
>  	New macros.
>  	(rs6000_skip_main_prologue): New function.
>  	(rs6000_gdb_arch_init): Register rs6000_skip_main_prologue.

Committed.

Kevin