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:
> +struct sframe_header
> +{
> +	struct sframe_preamble preamble;
> +	/* Information about the arch (endianness) and ABI. */
> +	uint8_t abi_arch;
> +	/*
> +	 * Offset for the Frame Pointer (FP) from CFA may be fixed for some
> +	 * ABIs (e.g, in AMD64 when -fno-omit-frame-pointer is used).  When fixed,
> +	 * this field specifies the fixed stack frame offset and the individual
> +	 * FREs do not need to track it.  When not fixed, it is set to
> +	 * SFRAME_CFA_FIXED_FP_INVALID, and the individual FREs may provide
> +	 * the applicable stack frame offset, if any.
> +	 */
> +	int8_t cfa_fixed_fp_offset;
> +	/*
> +	 * Offset for the Return Address from CFA is fixed for some ABIs
> +	 * (e.g., AMD64 has it as CFA-8).  When fixed, the header specifies the
> +	 * fixed stack frame offset and the individual FREs do not track it.  When
> +	 * not fixed, it is set to SFRAME_CFA_FIXED_RA_INVALID, and individual
> +	 * FREs provide the applicable stack frame offset, if any.
> +	 */
> +	int8_t cfa_fixed_ra_offset;
> +	/*
> +	 * Number of bytes making up the auxiliary header, if any.
> +	 * Some ABI/arch, in the future, may use this space for extending the
> +	 * information in SFrame header.  Auxiliary header is contained in
> +	 * bytes sequentially following the sframe_header.
> +	 */
> +	uint8_t auxhdr_len;
> +	/* Number of SFrame FDEs in this SFrame section. */
> +	uint32_t num_fdes;
> +	/* Number of SFrame Frame Row Entries. */
> +	uint32_t num_fres;
> +	/* Number of bytes in the SFrame Frame Row Entry section. */
> +	uint32_t fre_len;
> +	/* Offset of SFrame Function Descriptor Entry section. */
> +	uint32_t fdeoff;
> +	/* Offset of SFrame Frame Row Entry section. */
> +	uint32_t freoff;
> +} __packed;

So I know there's people that hate hard on tail comments, but personally
I can't read the above, something like:

	/*
	 * Number of bytes making up the auxiliary header, if any.
	 * Some ABI/arch, in the future, may use this space for extending the
	 * information in SFrame header.  Auxiliary header is contained in
	 * bytes sequentially following the sframe_header.
	 */
	uint8_t auxhdr_len;
	uint32_t num_fdes; /* Number of SFrame FDEs in this SFrame section. */
	uint32_t num_fres; /* Number of SFrame Frame Row Entries. */
	uint32_t fre_len;  /* Number of bytes in the SFrame Frame Row Entry section. */
	uint32_t fdeoff;   /* Offset of SFrame Function Descriptor Entry section. */
	uint32_t freoff;   /* Offset of SFrame Frame Row Entry section. */

Is far more readable (but yes, busts the line length a little).

An alternative form might be:

	/*
	 * @auxhdr_len: Number of bytes making up the auxiliary header, if any.
	 *   Some ABI/arch, in the future, may use this space for extending the
	 *   information in SFrame header.  Auxiliary header is contained in
	 *   bytes sequentially following the sframe_header.
	 * @num_fdes: Number of SFrame FDEs in this SFrame section.
	 * @num_fres: Number of SFrame Frame Row Entries.
	 * @fre_len:  Number of bytes in the SFrame Frame Row Entry section.
	 * @fdeoff:   Offset of SFrame Function Descriptor Entry section.
	 * @freoff:   Offset of SFrame Frame Row Entry section.
	 */
	uint8_t auxhdr_len;
	uint32_t num_fdes;
	uint32_t num_fres;
	uint32_t fre_len;
	uint32_t fdeoff;
	uint32_t freoff;