Re: RFA: ia64 tdep patch

[Kevin Buettner <kevinb@redhat.com>]

On Oct 17,  3:58pm, J. Johnston wrote:

> The attached ia64 patch fixes a few problems, most notably
> backtracing through signal handlers.

I think these changes are mostly okay, but...

In your ChangeLog entry, you say:

> 	* ia64-tdep.c: Change all references of DEPRECATED_REGISTER_RAW_SIZE
> 	to use register_size() instead.

Yet, later on, in the patch, I see that you're reintroducing a use of
DEPRECATED_REGISTER_RAW_SIZE:

  +  else if (regnum == IA64_BR0_REGNUM)
  +    {
  +      CORE_ADDR br0 = 0;
  +      CORE_ADDR addr = cache->saved_regs[IA64_BR0_REGNUM];
  +      if (addr != 0)
  +	{
  +	  *lvalp = lval_memory;
  +	  *addrp = addr;
  +	  read_memory (addr, buf, DEPRECATED_REGISTER_RAW_SIZE (IA64_BR0_REGNUM));
  +	  br0 = extract_unsigned_integer (buf, 8);
  +	}
  +      store_unsigned_integer (valuep, 8, br0);
  +    }

Also, regarding:

       /* We want to calculate the previous bsp as the end of the previous register stack frame.
 	 This corresponds to what the hardware bsp register will be if we pop the frame
 	 back which is why we might have been called.  We know the beginning of the current
-         frame is cache->bsp - cache->sof.  This value in the previous frame points to
+	 frame is cache->bsp - cache->sof.  This value in the previous frame points to
 	 the start of the output registers.  We can calculate the end of that frame by adding
 	 the size of output (sof (size of frame) - sol (size of locals)).  */
       ia64_frame_prev_register (next_frame, this_cache, IA64_CFM_REGNUM,
 				&cfm_optim, &cfm_lval, &cfm_addr, &cfm_realnum, cfm_valuep);
       prev_cfm = extract_unsigned_integer (cfm_valuep, 8);
-
+      
       bsp = rse_address_add (cache->bsp, -(cache->sof));
       prev_bsp = rse_address_add (bsp, (prev_cfm & 0x7f) - ((prev_cfm >> 7) & 0x7f));
-
+      

The first white space change is okay, but the latter two are not.  I'd
really prefer to see whitespace changes occur via a separate patch anyway.

So, if you don't mind, could you please submit separate patches for:

  1) whitespace changes
  2) DEPRECATED_... elimination
  3) the CFM / backtracing changes.

Patches (1) and (2) are preapproved -- just make sure that you don't
introduce extra white space on an otherwise blank line.  For (1) and
(2), please post the patches that you end up committing.

Once (1) and (2) are split out, I'd like another chance to review what's
left (patch 3).

With regard to (3), one of the questions that I already have concerns
the following line in ia64_sigtramp_frame_init_saved_regs():

+      CORE_ADDR sp = cache->base + 16;

Could you explain what this is about?  (Preferably with a comment in the
code?)

Thanks,

Kevin

P.S. If you'd prefer to reverse things and submit patch 3 first, that'd
be okay too.