Re: [PATCH v3 10/16] [gdb/aarch64] sme: Fixup sigframe gdbarch when vg/svg changes

[Luis Machado <luis.machado@arm.com>]

On 7/28/23 02:01, Thiago Jung Bauermann wrote:
> 
> Luis Machado via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
>>  	case AARCH64_SVE_MAGIC:
>>  	  {
>>  	    /* Check if the section is followed by a full SVE dump, and set
>>  	       sve_regs if it is.  */
>>  	    gdb_byte buf[4];
>> -	    uint16_t flags;
>> -
>> -	    if (!tdep->has_sve ())
>> -	      break;
>>  
>> +	    /* Extract the vector length.  */
>>  	    if (target_read_memory (section + AARCH64_SVE_CONTEXT_VL_OFFSET,
>>  				    buf, 2) != 0)
>>  	      {
>> +		warning (_("Failed to read the vector length from the SVE "
>> +			   " signal frame context."));
> 
> Extraneous space between "SVE" and "signal".
> 

Fixed.

>>  		section += size;
>>  		break;
>>  	      }
> 
>>  	case AARCH64_ZA_MAGIC:
>>  	  {
>> -	    if (!tdep->has_sme ())
>> -	      {
>> -		section += size;
>> -		break;
>> -	      }
>> -
>>  	    /* Check if the section is followed by a full ZA dump, and set
>>  	       za_state if it is.  */
>>  	    gdb_byte buf[2];
>>  
>> +	    /* Extract the streaming vector length.  */
>>  	    if (target_read_memory (section + AARCH64_SVE_CONTEXT_VL_OFFSET,
> 
> Same comment from the previous patch about using the SME macro instead
> of the SVE one.
> 
>>  				    buf, 2) != 0)
>>  	      {
>> +		warning (_("Failed to read the streaming vector length from "
>> +			   " ZA signal frame context."));
> 
> Extraneous space between "SVE" and "signal".
> 

Fixed.

>>  		section += size;
>>  		break;
>>  	      }
> 
>> @@ -477,11 +499,49 @@ aarch64_linux_sigframe_init (const struct tramp_frame *self,
>>        if (!extra_found && section > section_end)
>>  	break;
>>      }
>> +}
>> +
>> +/* Implement the "init" method of struct tramp_frame.  */
>> +
>> +static void
>> +aarch64_linux_sigframe_init (const struct tramp_frame *self,
>> +			     frame_info_ptr this_frame,
>> +			     struct trad_frame_cache *this_cache,
>> +			     CORE_ADDR func)
>> +{
>> +  /* Read the signal context information.  */
>> +  struct aarch64_linux_sigframe signal_frame;
>> +  aarch64_linux_read_signal_frame_info (this_frame, signal_frame);
>> +
>> +  /* Now we have all the data required to restore the registers from the
>> +     signal frame.  */
>> +
>> +  /* Restore the general purpose registers.  */
>> +  CORE_ADDR offset
>> +    = signal_frame.sigcontext_address + AARCH64_SIGCONTEXT_XO_OFFSET;
> 
> You can simplify the above to
> 
>   CORE_ADDR offset = signal_frame.gpr_section;
> 

Yep. Agreed. Changed it now.

>> +  for (int i = 0; i < 31; i++)
>> +    {
>> +      trad_frame_set_reg_addr (this_cache, AARCH64_X0_REGNUM + i, offset);
>> +      offset += AARCH64_SIGCONTEXT_REG_SIZE;
>> +    }
>> +  trad_frame_set_reg_addr (this_cache, AARCH64_SP_REGNUM, offset);
>> +  offset += AARCH64_SIGCONTEXT_REG_SIZE;
>> +  trad_frame_set_reg_addr (this_cache, AARCH64_PC_REGNUM, offset);
> 
>> diff --git a/gdb/tramp-frame.h b/gdb/tramp-frame.h
>> index fa0241acb2d..563c34ae595 100644
>> --- a/gdb/tramp-frame.h
>> +++ b/gdb/tramp-frame.h
>> @@ -42,6 +42,13 @@ struct trad_frame_cache;
>>     instruction sequence.  */
>>  #define TRAMP_SENTINEL_INSN ULONGEST_MAX
>>  
>> +/* Assuming the frame chain: (outer) prev <-> this <-> next (inner);
>> +   use THIS frame, and implicitly the NEXT frame's register unwind
>> +   method, return PREV frame's architecture.  */
> 
> Maybe s/return/to return/ ?
> 

Yes, "to return" is what I had in mind.

>> +
>> +typedef struct gdbarch *(frame_prev_arch_ftype) (frame_info_ptr this_frame,
>> +						 void **this_prologue_cache);
>> +
>>  struct tramp_frame
>>  {
>>    /* The trampoline's type, some a signal trampolines, some are normal
>> @@ -75,6 +82,11 @@ struct tramp_frame
>>    int (*validate) (const struct tramp_frame *self,
>>  		   frame_info_ptr this_frame,
>>  		   CORE_ADDR *pc);
>> +
>> +  /* Given the current frame in THIS_FRAME and a frame cache in FRAME_CACHE,
>> +     return the architecture of the previous frame.  */
>> +  struct gdbarch *(*prev_arch) (frame_info_ptr this_frame,
>> +				void **frame_cache);
> 
> Shouldn't the typedef defined above be used here?
> 

Yep. Change it to use the typedef now.

>>  };
> 

Thanks!