Kevin Buettner wrote:
> 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).
> 

Ok, done.  A patch for 1) and 2) has been checked in.  See the new attached patch.

2003-10-20  Jeff Johnston  <jjohnstn@redhat.com>

	* ia64-tdep.c: (ia64_frame_cache): Add new prev_cfm field.
	(ia64_alloc_frame_cache):  Initialize new prev_cfm field to 0.
	(floatformat_valid): New static routine.
	(floatformat_ia64_ext): Add name field and set up is_valid routine
	to floatformat_valid().
	(examine_prologue):  For the previous cfm, use frame_unwind_register()
	if the cfm is not stored in a register-stack register.  Save the
	previous cfm value in the prev_cfm field.  Add debug output.
	(ia64_frame_this_id): Use frame_id_build_special() to also register
	the bsp.  Add debug output.
	(ia64_sigtramp_frame_this_id): Ditto.
	(ia64_frame_prev_register):  Look at cache saved_regs for a few more
	registers and also add some checks for framelessness before accepting
	current register values for fields such as return address.  For cfm,
	use the cached prev_cfm field if available.  Add debug output.
	(ia64_sigtramp_frame_init_saved_regs): Bump up base by 16 to get
	sp needed for calling lower level
	ia64_linux_sigcontext_register_address().  Also save the
	bsp and sp address as part of initialization.
	(ia64_sigtramp_frame_cache): Hard-code stack size as it can't be
	calculated.  Cache the bsp and cfm values.
	(ia64_sigtramp_frame_prev_register): Flesh this routine out instead of
	using ia64_frame_prev_register().  The saved values for bsp and sp can
	be taken from the cache.  Add debug output.
	(ia64_push_dummy_call): Use frame_id_build_special() to also register
	the bsp.


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

I've simplified it.  It is the mem stack size that should have been added to the 
base.  I have added a comment that it cannot be calculated via prologue examination.

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