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

[Indu Bhagat <indu.bhagat@oracle.com>]

On 5/1/23 15:40, Steven Rostedt wrote:
> On Mon,  1 May 2023 13:04:08 -0700
> Indu Bhagat <indu.bhagat@oracle.com> wrote:
> 
>> This patch adds an implementation to read SFrame stack trace data from
>> a .sframe section.  Some APIs are also provided to find stack tracing
>> information per PC, e.g., given a PC, find the SFrame FRE.
>>
>> These routines are provided in the sframe_read.h and sframe_read.c.
>>
>> This implmentation is malloc-free.
>>
>> Signed-off-by: Indu Bhagat <indu.bhagat@oracle.com>
>> ---
>>   lib/Makefile             |   1 +
>>   lib/sframe/Makefile      |   5 +
>>   lib/sframe/sframe.h      | 263 +++++++++++++++++++++
>>   lib/sframe/sframe_read.c | 498 +++++++++++++++++++++++++++++++++++++++
>>   lib/sframe/sframe_read.h |  75 ++++++
>>   5 files changed, 842 insertions(+)
>>   create mode 100644 lib/sframe/Makefile
>>   create mode 100644 lib/sframe/sframe.h
>>   create mode 100644 lib/sframe/sframe_read.c
>>   create mode 100644 lib/sframe/sframe_read.h
>>
>> diff --git a/lib/Makefile b/lib/Makefile
>> index 876fcdeae34e..cb02d16dbffd 100644
>> --- a/lib/Makefile
>> +++ b/lib/Makefile
>> @@ -198,6 +198,7 @@ obj-$(CONFIG_ZSTD_COMPRESS) += zstd/
>>   obj-$(CONFIG_ZSTD_DECOMPRESS) += zstd/
>>   obj-$(CONFIG_XZ_DEC) += xz/
>>   obj-$(CONFIG_RAID6_PQ) += raid6/
>> +obj-$(CONFIG_USER_UNWINDER_SFRAME) += sframe/
>>   
>>   lib-$(CONFIG_DECOMPRESS_GZIP) += decompress_inflate.o
>>   lib-$(CONFIG_DECOMPRESS_BZIP2) += decompress_bunzip2.o
>> diff --git a/lib/sframe/Makefile b/lib/sframe/Makefile
>> new file mode 100644
>> index 000000000000..4e4291d9294f
>> --- /dev/null
>> +++ b/lib/sframe/Makefile
>> @@ -0,0 +1,5 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +##################################
>> +obj-$(CONFIG_USER_UNWINDER_SFRAME) += sframe_read.o \
> 
> The ending backslash is only needed if there's a line wrap, which you don't
> have here.
> 
>> +
>> +CFLAGS_sframe_read.o += -I $(srctree)/lib/sframe/
>> diff --git a/lib/sframe/sframe.h b/lib/sframe/sframe.h
>> new file mode 100644
>> index 000000000000..b1290e92839a
>> --- /dev/null
>> +++ b/lib/sframe/sframe.h
>> @@ -0,0 +1,263 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * Copyright (C) 2023, Oracle and/or its affiliates.
>> + */
>> +
>> +#ifndef	SFRAME_H
>> +#define	SFRAME_H
>> +
>> +#include <linux/types.h>
>> +
>> +/* This file contains definitions for the SFrame stack tracing format, which is
>> + * documented at https://sourceware.org/binutils/docs */
>> +
>> +#define SFRAME_VERSION_1	1
>> +#define SFRAME_MAGIC		0xdee2
>> +#define SFRAME_VERSION	SFRAME_VERSION_1
>> +
>> +/* Function Descriptor Entries are sorted on PC. */
>> +#define SFRAME_F_FDE_SORTED	0x1
>> +/* Frame-pointer based stack tracing. Defined, but not set. */
>> +#define SFRAME_F_FRAME_POINTER 0x2
>> +
>> +#define SFRAME_CFA_FIXED_FP_INVALID 0
>> +#define SFRAME_CFA_FIXED_RA_INVALID 0
>> +
>> +/* Supported ABIs/Arch. */
>> +#define SFRAME_ABI_AARCH64_ENDIAN_BIG      1 /* AARCH64 big endian. */
>> +#define SFRAME_ABI_AARCH64_ENDIAN_LITTLE   2 /* AARCH64 little endian. */
>> +#define SFRAME_ABI_AMD64_ENDIAN_LITTLE     3 /* AMD64 little endian. */
>> +
>> +/* SFrame FRE types. */
>> +#define SFRAME_FRE_TYPE_ADDR1	0
>> +#define SFRAME_FRE_TYPE_ADDR2	1
>> +#define SFRAME_FRE_TYPE_ADDR4	2
>> +
>> +/*
>> + * SFrame Function Descriptor Entry types.
>> + *
>> + * The SFrame format has two possible representations for functions.  The
>> + * choice of which type to use is made according to the instruction patterns
>> + * in the relevant program stub.
>> + */
>> +
>> +/* Unwinders perform a (PC >= FRE_START_ADDR) to look up a matching FRE. */
>> +#define SFRAME_FDE_TYPE_PCINC   0
>> +/*
>> + * Unwinders perform a (PC & FRE_START_ADDR_AS_MASK >= FRE_START_ADDR_AS_MASK)
>> + * to look up a matching FRE.  Typical usecases are pltN entries, trampolines
>> + * etc.
>> + */
>> +#define SFRAME_FDE_TYPE_PCMASK  1
>> +
>> +struct sframe_preamble
>> +{
> 
> Note, for Linux coding style, structs should have the '{' on the same line:
> 
> struct sframe_preamble {
> 
> This is required because a common way to find a struct definition is to
> search the name followed by a '{'.
> 
> 
>> +	/* Magic number (SFRAME_MAGIC). */
>> +	uint16_t magic;
>> +	/* Data format version number (SFRAME_VERSION). */
>> +	uint8_t version;
>> +	/* Various flags. */
>> +	uint8_t flags;
>> +} __packed;
>> +
>> +struct sframe_header
>> +{
> 
> Here too.
> 
>> +	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;
>> +
>> +#define SFRAME_V1_HDR_SIZE(sframe_hdr)	\
>> +  ((sizeof (struct sframe_header) + (sframe_hdr).auxhdr_len))
>> +
>> +/* Two possible keys for executable (instruction) pointers signing. */
>> +#define SFRAME_AARCH64_PAUTH_KEY_A    0 /* Key A. */
>> +#define SFRAME_AARCH64_PAUTH_KEY_B    1 /* Key B. */
>> +
>> +struct sframe_func_desc_entry
>> +{
> 
> Basically all of them.
> 
>> +	/*
>> +	 * Function start address.  Encoded as a signed offset, relative to the
>> +	 * beginning of the current FDE.
>> +	 */
>> +	int32_t func_start_address;
>> +	/* Size of the function in bytes. */
>> +	uint32_t func_size;
>> +	/*
>> +	 * Offset of the first SFrame Frame Row Entry of the function, relative to the
>> +	 * beginning of the SFrame Frame Row Entry sub-section.
>> +	 */
>> +	uint32_t func_start_fre_off;
>> +	/* Number of frame row entries for the function. */
>> +	uint32_t func_num_fres;
>> +	/*
>> +	 * Additional information for deciphering the unwind information for the
>> +	 * function.
>> +	 *   - 4-bits: Identify the FRE type used for the function.
>> +	 *   - 1-bit: Identify the FDE type of the function - mask or inc.
>> +	 *   - 1-bit: PAC authorization A/B key (aarch64).
>> +	 *   - 2-bits: Unused.
>> +	 * --------------------------------------------------------------------------
>> +	 * |     Unused    |  PAC auth A/B key (aarch64) |  FDE type |   FRE type   |
>> +	 * |               |        Unused (amd64)       |           |              |
>> +	 * --------------------------------------------------------------------------
>> +	 * 8               6                             5           4              0
>> +	 */
>> +	uint8_t func_info;
>> +} __packed;
>> +
>> +/* Note: Set PAC auth key to SFRAME_AARCH64_PAUTH_KEY_A by default.  */
>> +#define SFRAME_V1_FUNC_INFO(fde_type, fre_enc_type) \
>> +  (((SFRAME_AARCH64_PAUTH_KEY_A & 0x1) << 5) | \
>> +   (((fde_type) & 0x1) << 4) | ((fre_enc_type) & 0xf))
>> +
>> +#define SFRAME_V1_FUNC_FRE_TYPE(data)	  ((data) & 0xf)
>> +#define SFRAME_V1_FUNC_FDE_TYPE(data)	  (((data) >> 4) & 0x1)
>> +#define SFRAME_V1_FUNC_PAUTH_KEY(data)	  (((data) >> 5) & 0x1)
>> +
>> +/*
>> + * Size of stack frame offsets in an SFrame Frame Row Entry.  A single
>> + * SFrame FRE has all offsets of the same size.  Offset size may vary
>> + * across frame row entries.
>> + */
>> +#define SFRAME_FRE_OFFSET_1B	  0
>> +#define SFRAME_FRE_OFFSET_2B	  1
>> +#define SFRAME_FRE_OFFSET_4B	  2
>> +
>> +/* An SFrame Frame Row Entry can be SP or FP based.  */
>> +#define SFRAME_BASE_REG_FP	0
>> +#define SFRAME_BASE_REG_SP	1
>> +
>> +/*
>> + * The index at which a specific offset is presented in the variable length
>> + * bytes of an FRE.
>> + */
>> +#define SFRAME_FRE_CFA_OFFSET_IDX   0
>> +/*
>> + * The RA stack offset, if present, will always be at index 1 in the variable
>> + * length bytes of the FRE.
>> + */
>> +#define SFRAME_FRE_RA_OFFSET_IDX    1
>> +/*
>> + * The FP stack offset may appear at offset 1 or 2, depending on the ABI as RA
>> + * may or may not be tracked.
>> + */
>> +#define SFRAME_FRE_FP_OFFSET_IDX    2
>> +
>> +struct sframe_fre_info
>> +{
>> +  /* Information about
>> +   *   - 1 bit: base reg for CFA
>> +   *   - 4 bits: Number of offsets (N).  A value of upto 3 is allowed to track
>> +   *     all three of CFA, FP and RA (fixed implicit order).
>> +   *   - 2 bits: information about size of the offsets (S) in bytes.
>> +   *     Valid values are SFRAME_FRE_OFFSET_1B, SFRAME_FRE_OFFSET_2B,
>> +   *     SFRAME_FRE_OFFSET_4B
>> +   *   - 1 bit: Mangled RA state bit (aarch64 only).
>> +   * -----------------------------------------------------------------------------------
>> +   * | Mangled-RA (aarch64) |  Size of offsets   |   Number of offsets    |   base_reg |
>> +   * |  Unused (amd64)      |                    |                        |            |
>> +   * -----------------------------------------------------------------------------------
>> +   * 8                     7                    5                        1            0
>> +   */
>> +  uint8_t fre_info;
>> +};
>> +
>> +/* Macros to compose and decompose FRE info. */
>> +
>> +/* Note: Set mangled_ra_p to zero by default. */
>> +#define SFRAME_V1_FRE_INFO(base_reg_id, offset_num, offset_size) \
>> +  (((0 & 0x1) << 7) | (((offset_size) & 0x3) << 5) | \
>> +   (((offset_num) & 0xf) << 1) | ((base_reg_id) & 0x1))
>> +
>> +/* Set the mangled_ra_p bit as indicated. */
>> +#define SFRAME_V1_FRE_INFO_UPDATE_MANGLED_RA_P(mangled_ra_p, fre_info) \
>> +  ((((mangled_ra_p) & 0x1) << 7) | ((fre_info) & 0x7f))
>> +
>> +#define SFRAME_V1_FRE_CFA_BASE_REG_ID(data)	  ((data) & 0x1)
>> +#define SFRAME_V1_FRE_OFFSET_COUNT(data)	  (((data) >> 1) & 0xf)
>> +#define SFRAME_V1_FRE_OFFSET_SIZE(data)		  (((data) >> 5) & 0x3)
>> +#define SFRAME_V1_FRE_MANGLED_RA_P(data)	  (((data) >> 7) & 0x1)
>> +
>> +/* SFrame Frame Row Entry definitions. */
>> +
>> +/*
>> + * Used when SFRAME_FRE_TYPE_ADDR1 is specified as FRE type.
>> + * Upper limit of start address in sframe_frame_row_entry_addr1 if 0x100 (not
>> + * inclusive).
>> + */
>> +struct sframe_frame_row_entry_addr1
>> +{
>> +	/*
>> +	 * Start address of the frame row entry.  Encoded as an 1-byte unsigned
>> +	 * offset, relative to the start address of the function.
>> +	 */
>> +	uint8_t fre_start_ip_offset;
>> +	struct sframe_fre_info fre_info;
>> +} __packed;
>> +
>> +/*
>> + * Used when SFRAME_FRE_TYPE_ADDR2 is specified as FRE type.
>> + * Upper limit of start address in sframe_frame_row_entry_addr2 is 0x10000 (not
>> + * inclusive).
>> + */
>> +struct sframe_frame_row_entry_addr2
>> +{
>> +	/*
>> +	 * Start address of the frame row entry.  Encoded as an 2-byte unsigned
>> +	 * offset, relative to the start address of the function.
>> +	 */
>> +	uint16_t fre_start_ip_offset;
>> +	struct sframe_fre_info fre_info;
>> +} __packed;
>> +
>> +/*
>> + * Used when SFRAME_FRE_TYPE_ADDR4 is specified as FRE type.
>> + * Upper limit of start address in sframe_frame_row_entry_addr2
>> + * is 0x100000000 (not inclusive).
>> + */
>> +struct sframe_frame_row_entry_addr4
>> +{
>> +	/*
>> +	 * Start address of the frame row entry.  Encoded as a 4-byte unsigned
>> +	 * offset, relative to the start address of the function.
>> +	 */
>> +	uint32_t fre_start_ip_offset;
>> +	struct sframe_fre_info fre_info;
>> +} __packed;
>> +
>> +#endif	/* SFRAME_H */
>> diff --git a/lib/sframe/sframe_read.c b/lib/sframe/sframe_read.c
>> new file mode 100644
>> index 000000000000..9d6558d62c54
>> --- /dev/null
>> +++ b/lib/sframe/sframe_read.c
>> @@ -0,0 +1,498 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Copyright (C) 2023, Oracle and/or its affiliates.
>> + */
>> +
>> +#include <linux/string.h>
>> +
>> +#include "sframe_read.h"
>> +
>> +struct sframe_sec {
>> +	/* SFrame header. */
>> +	struct sframe_header header;
>> +	/* SFrame Function Desc Entries. */
>> +	void *fdes;
>> +	/* SFrame Frame Row Entries. */
>> +	void *fres;
>> +	/* Number of bytes needed for SFrame FREs. */
>> +	uint32_t fre_nbytes;
>> +};
>> +
>> +static int sframe_set_errno(int *errp, int error)
>> +{
>> +	if (errp != NULL)
>> +		*errp = error;
>> +	return SFRAME_ERR;
>> +}
>> +
>> +static uint32_t sframe_sec_get_hdr_size(struct sframe_header *sfh)
>> +{
>> +	return SFRAME_V1_HDR_SIZE(*sfh);
>> +}
>> +
>> +static unsigned int sframe_fre_get_offset_count(unsigned char fre_info)
>> +{
>> +	return SFRAME_V1_FRE_OFFSET_COUNT(fre_info);
>> +}
>> +
>> +static unsigned int sframe_fre_get_offset_size(unsigned char fre_info)
>> +{
>> +	return SFRAME_V1_FRE_OFFSET_SIZE(fre_info);
>> +}
>> +
>> +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;
>> +}
>> +
>> +static bool sframe_header_sanity_check_p(struct sframe_header *hp)
>> +{
>> +	unsigned char all_flags = SFRAME_F_FDE_SORTED | SFRAME_F_FRAME_POINTER;
> 
> Add a space here.
> 
>> +	/* Check that the preamble is valid. */
>> +	if ((hp->preamble.magic != SFRAME_MAGIC)
>> +	    || (hp->preamble.version != SFRAME_VERSION)
>> +	    || ((hp->preamble.flags | all_flags) != all_flags))
>> +		return false;
>> +
>> +	/* Check that the offsets are valid. */
>> +	if (hp->fdeoff > hp->freoff)
>> +		return false;
>> +
>> +	return true;
>> +}
>> +
>> +static bool sframe_fre_sanity_check_p(struct sframe_fre *frep)
>> +{
>> +	unsigned int offset_size, offset_cnt;
>> +
>> +	if (frep == NULL)
>> +		return false;
>> +
>> +	offset_size = sframe_fre_get_offset_size(frep->fre_info);
>> +
>> +	if (offset_size != SFRAME_FRE_OFFSET_1B
>> +	    && offset_size != SFRAME_FRE_OFFSET_2B
>> +	    && offset_size != SFRAME_FRE_OFFSET_4B)
>> +		return false;
>> +
>> +	offset_cnt = sframe_fre_get_offset_count(frep->fre_info);
>> +	if (offset_cnt > MAX_NUM_STACK_OFFSETS)
>> +		return false;
>> +
>> +	return true;
>> +}
>> +
>> +static int32_t sframe_get_fre_offset(struct sframe_fre *frep, uint32_t idx,
>> +				     int *errp)
>> +{
>> +	int offset_cnt, offset_size;
>> +
>> +	if (frep == NULL || !sframe_fre_sanity_check_p(frep))
>> +		return sframe_set_errno(errp, SFRAME_ERR_FRE_INVAL);
>> +
>> +	offset_cnt = sframe_fre_get_offset_count(frep->fre_info);
>> +	offset_size = sframe_fre_get_offset_size(frep->fre_info);
>> +
>> +	if (offset_cnt < idx + 1)
>> +		return sframe_set_errno(errp, SFRAME_ERR_FREOFFSET_NOPRESENT);
>> +
>> +	if (errp != NULL)
>> +		*errp = 0; /* Offset Valid. */
>> +
>> +	if (offset_size == SFRAME_FRE_OFFSET_1B) {
>> +		int8_t *stack_offsets = (int8_t *)frep->fre_offsets;
>> +		return stack_offsets[idx];
>> +	} else if (offset_size == SFRAME_FRE_OFFSET_2B) {
>> +		int16_t *stack_offsets = (int16_t *)frep->fre_offsets;
>> +		return stack_offsets[idx];
>> +	} else {
>> +		int32_t *stack_offsets = (int32_t *)frep->fre_offsets;
>> +		return stack_offsets[idx];
>> +	}
>> +}
>> +
>> +static struct sframe_header *sframe_sec_get_header(struct sframe_sec *sfsec)
>> +{
>> +	return sfsec ? &sfsec->header : NULL;
>> +}
>> +
>> +static int sframe_fre_copy(struct sframe_fre *dst,
>> +			   struct sframe_fre *src)
>> +{
>> +	if (dst == NULL || src == NULL)
>> +		return SFRAME_ERR;
>> +
>> +	memcpy(dst, src, sizeof(struct sframe_fre));
>> +	return 0;
>> +}
>> +
>> +static int sframe_decode_start_ip_offset(const char *fre_buf,
>> +					 uint32_t *start_ip_offset,
>> +					 unsigned int fre_type)
>> +{
>> +	uint32_t saddr = 0;
>> +
>> +	if (fre_type == SFRAME_FRE_TYPE_ADDR1) {
>> +		uint8_t *uc = (uint8_t *)fre_buf;
>> +		saddr = (uint32_t)*uc;
>> +	} else if (fre_type == SFRAME_FRE_TYPE_ADDR2) {
>> +		uint16_t *ust = (uint16_t *)fre_buf;
>> +		saddr = (uint32_t)*ust;
>> +	} else if (fre_type == SFRAME_FRE_TYPE_ADDR4) {
>> +		uint32_t *uit = (uint32_t *)fre_buf;
>> +		saddr = (uint32_t)*uit;
>> +	} else {
>> +		return SFRAME_ERR_INVAL;
>> +	}
>> +
>> +	*start_ip_offset = saddr;
>> +	return 0;
>> +}
>> +
>> +/* Get the total size in bytes for the stack offsets. */
>> +static size_t sframe_fre_stack_offsets_size(unsigned char fre_info)
>> +{
>> +	unsigned int offset_size, offset_cnt;
>> +
>> +	offset_size = sframe_fre_get_offset_size(fre_info);
>> +	offset_cnt = sframe_fre_get_offset_count(fre_info);
>> +
>> +	if (offset_size == SFRAME_FRE_OFFSET_2B
>> +	    || offset_size == SFRAME_FRE_OFFSET_4B)	/* 2 or 4 bytes. */
>> +		return (offset_cnt * (offset_size * 2));
>> +
>> +	return offset_cnt;
>> +}
>> +
>> +static size_t sframe_fre_get_start_address_tsize(unsigned int fre_type)
>> +{
>> +	/* Type size of the start_addr in an FRE. */
>> +	size_t saddr_tsize = 0;
>> +
>> +	switch (fre_type) {
>> +	case SFRAME_FRE_TYPE_ADDR1:
>> +		saddr_tsize = sizeof(uint8_t);
>> +		break;
>> +	case SFRAME_FRE_TYPE_ADDR2:
>> +		saddr_tsize = sizeof(uint16_t);
>> +		break;
>> +	case SFRAME_FRE_TYPE_ADDR4:
>> +		saddr_tsize = sizeof(uint32_t);
>> +		break;
>> +	default:
>> +		/* No other value is expected. */
>> +		break;
>> +	}
>> +	return saddr_tsize;
>> +}
>> +
>> +static size_t sframe_fre_vlen_size(struct sframe_fre *frep,
>> +				   unsigned int fre_type)
>> +{
>> +	unsigned char fre_info;
>> +	size_t ip_offset_tsize;
>> +
>> +	if (frep == NULL)
>> +		return 0;
>> +
>> +	fre_info = frep->fre_info;
>> +	ip_offset_tsize = sframe_fre_get_start_address_tsize(fre_type);
>> +
>> +	/*
>> +	 * An SFrame FRE is a variable length structure.  It includes the start
>> +	 * IP offset, FRE info field, and all trailing the stack offsets.
>> +	 */
>> +	return (ip_offset_tsize + sizeof(fre_info)
>> +		+ sframe_fre_stack_offsets_size(fre_info));
>> +}
>> +
>> +/*
>> + * Read an SFrame FRE which starts at location FRE_BUF.  The function
>> + * updates FRE_SIZE to the size of the FRE as stored in the binary format.
>> + *
>> + * Returns SFRAME_ERR if failure.
>> + */
>> +static int sframe_sec_read_fre(const char *fre_buf, struct sframe_fre *frep,
>> +			       unsigned int fre_type, size_t *fre_size)
>> +{
>> +	void *stack_offsets;
>> +	size_t stack_offsets_sz;
>> +	size_t ip_offset_tsize;
>> +	size_t esz;
>> +
>> +	if (fre_buf == NULL || frep == NULL || fre_size == NULL)
>> +		return SFRAME_ERR_INVAL;
>> +
>> +	/* Copy over the FRE start address. */
>> +	sframe_decode_start_ip_offset(fre_buf, &frep->start_ip_offset,
>> +				      fre_type);
>> +
>> +	ip_offset_tsize = sframe_fre_get_start_address_tsize(fre_type);
>> +	/* PS: Note how this API works closely with SFrame binary format. */
>> +	frep->fre_info = *(unsigned char *)(fre_buf + ip_offset_tsize);
>> +
>> +	memset(frep->fre_offsets, 0, MAX_STACK_OFFSET_NBYTES);
>> +	/* Get stack offsets. */
>> +	stack_offsets_sz = sframe_fre_stack_offsets_size(frep->fre_info);
>> +	stack_offsets = ((unsigned char *)fre_buf + ip_offset_tsize
>> +			 + sizeof(frep->fre_info));
>> +	memcpy(frep->fre_offsets, stack_offsets, stack_offsets_sz);
>> +
>> +	esz = sframe_fre_vlen_size(frep, fre_type);
>> +	*fre_size = esz;
>> +
>> +	return 0;
>> +}
>> +
>> +static struct sframe_func_desc_entry *
>> +sframe_sec_find_fde(struct sframe_sec *sfsec, int32_t addr, int *errp)
>> +{
>> +	struct sframe_header *header;
>> +	struct sframe_func_desc_entry *fde;
>> +	int low, high, cnt;
>> +
>> +	if (sfsec == NULL) {
>> +		sframe_set_errno(errp, SFRAME_ERR_INVAL);
>> +		return NULL;
>> +	}
>> +
>> +	header = sframe_sec_get_header(sfsec);
>> +	if (header == NULL || header->num_fdes == 0 || sfsec->fdes == NULL) {
>> +		sframe_set_errno(errp, SFRAME_ERR_INIT_INVAL);
>> +		return NULL;
>> +	}
>> +	/*
>> +	 * Skip binary search if FDE sub-section is not sorted on PCs.  GNU ld
>> +	 * sorts the FDEs on start PC by default though.
>> +	 */
>> +	if ((header->preamble.flags & SFRAME_F_FDE_SORTED) == 0) {
>> +		sframe_set_errno(errp, SFRAME_ERR_FDE_NOTSORTED);
>> +		return NULL;
>> +	}
>> +
>> +	/* Find the FDE that may contain the addr. */
>> +	fde = (struct sframe_func_desc_entry *)sfsec->fdes;
>> +	low = 0;
>> +	high = header->num_fdes;
>> +	cnt = high;
>> +	while (low <= high) {
>> +		int mid = low + (high - low) / 2;
>> +
>> +		if (fde[mid].func_start_address == addr)
>> +			return fde + mid;
>> +
>> +		if (fde[mid].func_start_address < addr) {
>> +			if (mid == (cnt - 1))
>> +				return fde + (cnt - 1);
>> +			else if (fde[mid+1].func_start_address > addr)
>> +				return fde + mid;
>> +			low = mid + 1;
>> +		} else
>> +			high = mid - 1;
>> +	}
>> +
>> +	sframe_set_errno(errp, SFRAME_ERR_FDE_NOTFOUND);
>> +	return NULL;
>> +}
>> +
>> +static int8_t sframe_sec_get_fixed_fp_offset(struct sframe_sec *sfsec)
>> +{
>> +	struct sframe_header *header = sframe_sec_get_header(sfsec);
>> +	return header->cfa_fixed_fp_offset;
>> +}
>> +
>> +static int8_t sframe_sec_get_fixed_ra_offset(struct sframe_sec *sfsec)
>> +{
>> +	struct sframe_header *header = sframe_sec_get_header(sfsec);
>> +	return header->cfa_fixed_ra_offset;
>> +}
>> +
>> +size_t sframe_sec_sizeof(void)
>> +{
>> +	return sizeof (struct sframe_sec);
>> +}
>> +
>> +int sframe_sec_init(struct sframe_sec *sfsec, const char *sf_buf,
>> +		    size_t sf_size)
>> +{
>> +	char *frame_buf;
>> +	const struct sframe_preamble *preamble;
>> +	struct sframe_header *header;
>> +
>> +	if ((sf_buf == NULL) || (sf_size < sizeof(struct sframe_header)))
>> +		return SFRAME_ERR_INVAL;
>> +
>> +	/* Check for foreign endianness. */
>> +	preamble = (const struct sframe_preamble *) sf_buf;
>> +	if (preamble->magic != SFRAME_MAGIC)
>> +		return SFRAME_ERR_INVAL;
>> +
>> +	/* Reset the SFrame section object. */
>> +	memset(sfsec, 0, sizeof(struct sframe_sec));
>> +
>> +	frame_buf = (char *)sf_buf;
>> +
>> +	/* Initialize the reference to the SFrame header. */
>> +	sfsec->header = *(struct sframe_header *) frame_buf;
>> +	header = &sfsec->header;
>> +	if (!sframe_header_sanity_check_p(header))
>> +		return SFRAME_ERR_INVAL;
>> +
>> +	/* Initialize the referece to the SFrame FDE section. */
>> +	frame_buf += sframe_sec_get_hdr_size(header);
>> +	sfsec->fdes = frame_buf;
>> +
>> +	/* Initialize the reference to the the SFrame FRE section. */
>> +	frame_buf += (header->num_fdes * sizeof(struct sframe_func_desc_entry));
>> +	sfsec->fres = frame_buf;
>> +
>> +	sfsec->fre_nbytes = header->fre_len;
>> +
>> +	return 0;
>> +}
>> +
>> +/*
>> + * Find the SFrame Frame Row Entry which contains the PC.
>> + * Returns error code if failure.
>> + */
>> +int sframe_sec_find_fre(struct sframe_sec *sfsec, int32_t pc,
>> +			struct sframe_fre *frep)
>> +{
>> +	struct sframe_func_desc_entry *fdep;
>> +	uint32_t start_address, i;
>> +	struct sframe_fre cur_fre, next_fre;
> 
> Although the sframe_fre structure is relatively small, I always get nervous
> when I see structures defined on the kernel stack.
> 
> Also, I personally prefer to keep items defined themselves. That is, one
> variable per line.
> 
>> +	unsigned char *fres;
>> +	unsigned int fre_type, fde_type;
>> +	size_t esz;
>> +	int err = 0;
>> +	size_t size = 0;
> 
> And to organize it in an "upside-down x-mas" tree fashion:
> 
> 	struct sframe_func_desc_entry *fdep;
> 	struct sframe_fre next_fre;
> 	struct sframe_fre cur_fre
> 	uint32_t start_address, i;
> 	unsigned int fre_type;
> 	unsigned int fde_type;
> 	unsigned char *fres;
> 	size_t size = 0;
> 	size_t esz;
> 	int err = 0;
> 
> Looks much nicer and easier to read that way ;-)
> 
> -- Steve
> 

OK. I can work these out. At some point I was running the 
script/checkpatch.pl but forgot about it over time. Will fix these issues.

Thanks