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

[Kevin Buettner <kevinb at redhat dot com>]

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