Re: [POC 3/5] sframe: add new SFrame library

[Peter Zijlstra <peterz@infradead.org>]

On Mon, May 01, 2023 at 01:04:08PM -0700, Indu Bhagat wrote:
> +static unsigned int sframe_get_fre_type(struct sframe_func_desc_entry *fdep)
> +{
> +	return (fdep) ? SFRAME_V1_FUNC_FRE_TYPE(fdep->func_info) : 0;
> +}
> +
> +static unsigned int sframe_get_fde_type(struct sframe_func_desc_entry *fdep)
> +{
> +	return (fdep) ? SFRAME_V1_FUNC_FDE_TYPE(fdep->func_info) : 0;
> +}

vs.

> +unsigned int sframe_fre_get_base_reg_id(struct sframe_fre *frep,
> +					int *errp)
> +{
> +	if (!frep)
> +		return sframe_set_errno(errp, SFRAME_ERR_FRE_INVAL);
> +
> +	return SFRAME_V1_FRE_CFA_BASE_REG_ID(frep->fre_info);
> +}

Inconsistent codying style here... FWIW, I prefer the latter.


> +int32_t sframe_fre_get_fp_offset(struct sframe_sec *sfsec,
> +				 struct sframe_fre *frep, int *errp)
> +{
> +	uint32_t fp_offset_idx = 0;

There's an unconditional assignment later, why are you initializing to 0
here? Also, that xmas thing Steve mentioned.

> +	int8_t fp_offset = sframe_sec_get_fixed_fp_offset(sfsec);
> +	/*
> +	 * If the FP offset is not being tracked, return the fixed FP offset
> +	 * from the SFrame header.
> +	 */
> +	if (fp_offset != SFRAME_CFA_FIXED_FP_INVALID) {
> +		*errp = 0;
> +		return fp_offset;
> +	}
> +
> +	/*
> +	 * In some ABIs, the stack offset to recover RA (using the CFA) from is
> +	 * fixed (like AMD64).  In such cases, the stack offset to recover FP
> +	 * will appear at the second index.
> +	 */
> +	fp_offset_idx = ((sframe_sec_get_fixed_ra_offset(sfsec)
> +			  != SFRAME_CFA_FIXED_RA_INVALID)
> +			 ? SFRAME_FRE_RA_OFFSET_IDX
> +			 : SFRAME_FRE_FP_OFFSET_IDX);

And as such, I'll suggest you write this like:

	fp_offset_idx = SFRAME_FRE_FP_OFFSET_IDX;
	if (sframe_sec_get_fixed_ra_offset(sfrec) != SFRAME_CFA_FIXED_RA_INVALID)
		fp_offset_idx = SFRAME_FRE_RA_OFFSET_IDX;

And also, if you'd use shorter identifiers, you'd not constantly run
into the line limit like you do.

Basically never use ?: unless you _have_ to :-)

> +	return sframe_get_fre_offset(frep, fp_offset_idx, errp);
> +}