During function validation, objtool maintains a per-instruction state,
in particular to track call frame information. When tracing validation,
print any instruction state changes.

Signed-off-by: Alexandre Chartre <alexandre.chartre@oracle.com>
---
 tools/objtool/check.c                 | 116 +++++++++++++++++++++++++-
 tools/objtool/disas.c                 |  27 ++++++
 tools/objtool/include/objtool/check.h |   1 +
 3 files changed, 143 insertions(+), 1 deletion(-)

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 40eaac4b5756..050d34930372 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -111,6 +111,111 @@ static void vtrace_insn(struct instruction *insn, const char *format, ...)
 		free((char *)addr_str);
 }
 
+/*
+ * Macros to trace CFI state attributes changes.
+ */
+
+#define VTRACE_CFI_ATTR(attr, prev, next, fmt, ...)			\
+	do {								\
+		if ((prev)->attr != (next)->attr)			\
+			VTRACE_PRINTF("%s=" fmt " ", #attr, __VA_ARGS__); \
+	} while (0)
+
+#define VTRACE_CFI_ATTR_BOOL(attr, prev, next)				\
+	VTRACE_CFI_ATTR(attr, prev, next,				\
+			"%s", (next)->attr ? "true" : "false")
+
+#define VTRACE_CFI_ATTR_NUM(attr, prev, next, fmt)			\
+	VTRACE_CFI_ATTR(attr, prev, next, fmt, (next)->attr)
+
+/*
+ * Functions and macros to trace CFI registers changes.
+ */
+
+static void vtrace_cfi_register(const char *prefix, int reg, const char *fmt,
+				int base_prev, int offset_prev,
+				int base_next, int offset_next)
+{
+	const char *rname;
+
+	if (base_prev == base_next && offset_prev == offset_next)
+		return;
+
+	if (prefix)
+		VTRACE_PRINTF("%s:", prefix);
+
+	rname = register_name(reg);
+
+	if (base_next == CFI_UNDEFINED) {
+		VTRACE_PRINTF("%1$s=<undef> ", rname);
+	} else {
+		VTRACE_PRINTF(fmt, rname,
+			      register_name(base_next), offset_next);
+	}
+}
+
+static void vtrace_cfi_reg_value(const char *prefix, int reg,
+				 int base_prev, int offset_prev,
+				 int base_next, int offset_next)
+{
+	vtrace_cfi_register(prefix, reg, "%1$s=%2$s%3$+d ",
+			    base_prev, offset_prev, base_next, offset_next);
+}
+
+static void vtrace_cfi_reg_reference(const char *prefix, int reg,
+				     int base_prev, int offset_prev,
+				     int base_next, int offset_next)
+{
+	vtrace_cfi_register(prefix, reg, "%1$s=(%2$s%3$+d) ",
+			    base_prev, offset_prev, base_next, offset_next);
+}
+
+#define VTRACE_CFI_REG_VAL(reg, prev, next)				\
+	vtrace_cfi_reg_value(NULL, reg, prev.base, prev.offset,		\
+			     next.base, next.offset)
+
+#define VTRACE_CFI_REG_REF(reg, prev, next)				\
+	vtrace_cfi_reg_reference(NULL, reg, prev.base, prev.offset,	\
+				 next.base, next.offset)
+
+static void vtrace_insn_state(struct instruction *insn,
+			      struct insn_state *sprev,
+			      struct insn_state *snext)
+{
+	struct cfi_state *cprev, *cnext;
+	int i;
+
+	if (memcmp(sprev, snext, sizeof(struct insn_state)) == 0)
+		return;
+
+	cprev = &sprev->cfi;
+	cnext = &snext->cfi;
+
+	vtrace_insn(insn, NULL);
+	VTRACE_PRINTF(" - state: ");
+
+	/* print registers changes */
+	VTRACE_CFI_REG_VAL(CFI_CFA, cprev->cfa, cnext->cfa);
+	for (i = 0; i < CFI_NUM_REGS; i++) {
+		VTRACE_CFI_REG_VAL(i, cprev->vals[i], cnext->vals[i]);
+		VTRACE_CFI_REG_REF(i, cprev->regs[i], cnext->regs[i]);
+	}
+
+	/* print attributes changes */
+	VTRACE_CFI_ATTR_NUM(stack_size, cprev, cnext, "%d");
+	VTRACE_CFI_ATTR_BOOL(drap, cprev, cnext);
+	if (cnext->drap) {
+		vtrace_cfi_reg_value("drap", cnext->drap_reg,
+				     cprev->drap_reg, cprev->drap_offset,
+				     cnext->drap_reg, cnext->drap_offset);
+	}
+	VTRACE_CFI_ATTR_BOOL(bp_scratch, cprev, cnext);
+	VTRACE_CFI_ATTR_NUM(instr, sprev, snext, "%d");
+	VTRACE_CFI_ATTR_NUM(uaccess_stack, sprev, snext, "%u");
+
+	VTRACE_PRINTF("\n");
+}
+
 struct instruction *find_insn(struct objtool_file *file,
 			      struct section *sec, unsigned long offset)
 {
@@ -3698,6 +3803,7 @@ static int validate_insn(struct objtool_file *file, struct symbol *func,
 			 struct instruction *prev_insn, struct instruction *next_insn,
 			 bool *validate_nextp)
 {
+	struct insn_state state_prev;
 	struct alternative *alt;
 	u8 visited;
 	int ret;
@@ -3814,7 +3920,15 @@ static int validate_insn(struct objtool_file *file, struct symbol *func,
 	if (skip_alt_group(insn))
 		return 0;
 
-	if (handle_insn_ops(insn, next_insn, statep))
+	if (vtrace)
+		state_prev = *statep;
+
+	ret = handle_insn_ops(insn, next_insn, statep);
+
+	if (vtrace)
+		vtrace_insn_state(insn, &state_prev, statep);
+
+	if (ret)
 		return 1;
 
 	switch (insn->type) {
diff --git a/tools/objtool/disas.c b/tools/objtool/disas.c
index 1e198d5f9205..4326c608f925 100644
--- a/tools/objtool/disas.c
+++ b/tools/objtool/disas.c
@@ -29,6 +29,33 @@ struct disas_context {
 	((*(dinfo)->fprintf_func)((dinfo)->stream, __VA_ARGS__))
 
 
+#define REGISTER_NAME_MAXLEN   16
+
+/*
+ * Return the name of a register. Note that the same static buffer
+ * is returned if the name is dynamically generated.
+ */
+const char *register_name(unsigned int reg)
+{
+	static char rname_buffer[REGISTER_NAME_MAXLEN];
+
+	switch (reg) {
+	case CFI_UNDEFINED:
+		return "<undefined>";
+	case CFI_CFA:
+		return "cfa";
+	case CFI_SP_INDIRECT:
+		return "(sp)";
+	case CFI_BP_INDIRECT:
+		return "(bp)";
+	}
+
+	if (snprintf(rname_buffer, REGISTER_NAME_MAXLEN, "r%d", reg) == 1)
+		return NULL;
+
+	return (const char *)rname_buffer;
+}
+
 static int dbuffer_init(struct dbuffer *dbuf, size_t size)
 {
 	dbuf->used = 0;
diff --git a/tools/objtool/include/objtool/check.h b/tools/objtool/include/objtool/check.h
index 1b9b399578ea..137d20963921 100644
--- a/tools/objtool/include/objtool/check.h
+++ b/tools/objtool/include/objtool/check.h
@@ -152,5 +152,6 @@ int disas_info_init(struct disassemble_info *dinfo,
 size_t disas_insn(struct disas_context *dctx, struct instruction *insn);
 char *disas_result(struct disas_context *dctx);
 const char *objtool_disas_insn(struct instruction *insn);
+const char *register_name(unsigned int reg);
 
 #endif /* _CHECK_H */
-- 
2.43.5

On Fri, Jun 06, 2025 at 05:34:37PM +0200, Alexandre Chartre wrote:
> +++ b/tools/objtool/check.c
> @@ -111,6 +111,111 @@ static void vtrace_insn(struct instruction *insn, const char *format, ...)
>  		free((char *)addr_str);
>  }
>  
> +/*
> + * Macros to trace CFI state attributes changes.
> + */
> +
> +#define VTRACE_CFI_ATTR(attr, prev, next, fmt, ...)			\
> +	do {								\
> +		if ((prev)->attr != (next)->attr)			\
> +			VTRACE_PRINTF("%s=" fmt " ", #attr, __VA_ARGS__); \
> +	} while (0)
> +
> +#define VTRACE_CFI_ATTR_BOOL(attr, prev, next)				\
> +	VTRACE_CFI_ATTR(attr, prev, next,				\
> +			"%s", (next)->attr ? "true" : "false")
> +
> +#define VTRACE_CFI_ATTR_NUM(attr, prev, next, fmt)			\
> +	VTRACE_CFI_ATTR(attr, prev, next, fmt, (next)->attr)
> +
> +/*
> + * Functions and macros to trace CFI registers changes.
> + */
> +
> +static void vtrace_cfi_register(const char *prefix, int reg, const char *fmt,
> +				int base_prev, int offset_prev,
> +				int base_next, int offset_next)
> +{

In keeping with the namespace of the other vtrace_cfi_reg_*()
functions/macros, I'd suggest calling this vtrace_cfi_reg()

(or trace_cfi_reg() if we go with my previous suggestion to remove the "v"
everywhere)

> +	const char *rname;
> +
> +	if (base_prev == base_next && offset_prev == offset_next)
> +		return;
> +
> +	if (prefix)
> +		VTRACE_PRINTF("%s:", prefix);
> +
> +	rname = register_name(reg);

For similar reasons, "register_name()" -> "reg_name()".

> +
> +	if (base_next == CFI_UNDEFINED) {
> +		VTRACE_PRINTF("%1$s=<undef> ", rname);
> +	} else {
> +		VTRACE_PRINTF(fmt, rname,
> +			      register_name(base_next), offset_next);
> +	}
> +}
> +
> +static void vtrace_cfi_reg_value(const char *prefix, int reg,
> +				 int base_prev, int offset_prev,
> +				 int base_next, int offset_next)
> +{
> +	vtrace_cfi_register(prefix, reg, "%1$s=%2$s%3$+d ",
> +			    base_prev, offset_prev, base_next, offset_next);
> +}
> +
> +static void vtrace_cfi_reg_reference(const char *prefix, int reg,
> +				     int base_prev, int offset_prev,
> +				     int base_next, int offset_next)
> +{
> +	vtrace_cfi_register(prefix, reg, "%1$s=(%2$s%3$+d) ",
> +			    base_prev, offset_prev, base_next, offset_next);
> +}
> +
> +#define VTRACE_CFI_REG_VAL(reg, prev, next)				\
> +	vtrace_cfi_reg_value(NULL, reg, prev.base, prev.offset,		\
> +			     next.base, next.offset)
> +
> +#define VTRACE_CFI_REG_REF(reg, prev, next)				\
> +	vtrace_cfi_reg_reference(NULL, reg, prev.base, prev.offset,	\
> +				 next.base, next.offset)

For similar reasons, "*_val()" and "*_ref()".

> +
> +static void vtrace_insn_state(struct instruction *insn,
> +			      struct insn_state *sprev,
> +			      struct insn_state *snext)
> +{
> +	struct cfi_state *cprev, *cnext;
> +	int i;
> +
> +	if (memcmp(sprev, snext, sizeof(struct insn_state)) == 0)
> +		return;

"!memcmp()"

> @@ -3698,6 +3803,7 @@ static int validate_insn(struct objtool_file *file, struct symbol *func,
>  			 struct instruction *prev_insn, struct instruction *next_insn,
>  			 bool *validate_nextp)
>  {
> +	struct insn_state state_prev;

Can we call it prev_state for consistency with prev_insn?

>  	struct alternative *alt;
>  	u8 visited;
>  	int ret;
> @@ -3814,7 +3920,15 @@ static int validate_insn(struct objtool_file *file, struct symbol *func,
>  	if (skip_alt_group(insn))
>  		return 0;
>  
> -	if (handle_insn_ops(insn, next_insn, statep))
> +	if (vtrace)
> +		state_prev = *statep;
> +
> +	ret = handle_insn_ops(insn, next_insn, statep);
> +
> +	if (vtrace)
> +		vtrace_insn_state(insn, &state_prev, statep);

For consistency with the other tracing, can the trace statement be a
capitalized macro, with the "vtrace" check hidden?

	state_prev = *statep;
	ret = handle_insn_ops(insn, next_insn, statep);
	TRACE_INSN_STATE(insn, &state_prev, statep);

> +++ b/tools/objtool/disas.c
> @@ -29,6 +29,33 @@ struct disas_context {
>  	((*(dinfo)->fprintf_func)((dinfo)->stream, __VA_ARGS__))
>  
>  
> +#define REGISTER_NAME_MAXLEN   16
> +
> +/*
> + * Return the name of a register. Note that the same static buffer
> + * is returned if the name is dynamically generated.
> + */
> +const char *register_name(unsigned int reg)
> +{
> +	static char rname_buffer[REGISTER_NAME_MAXLEN];
> +
> +	switch (reg) {
> +	case CFI_UNDEFINED:
> +		return "<undefined>";
> +	case CFI_CFA:
> +		return "cfa";
> +	case CFI_SP_INDIRECT:
> +		return "(sp)";
> +	case CFI_BP_INDIRECT:
> +		return "(bp)";
> +	}
> +
> +	if (snprintf(rname_buffer, REGISTER_NAME_MAXLEN, "r%d", reg) == 1)
> +		return NULL;
> +
> +	return (const char *)rname_buffer;
> +}
> +

Would this not be a better fit as a static function in check.c?  Its
only callers are there.  And it references the CFI stuff.

Or we might also want to consider moving all the trace_*() functions to
their own file, trace.c or so.

-- 
Josh