On 5/21/21 11:11 AM, Mark Brown wrote:

On Sat, May 15, 2021 at 11:00:17PM -0500, madvenka@linux.microsoft.com wrote:

quoted

Other reliability checks will be added in the future.

...

quoted

+	frame->reliable = true;
+

All these checks are good checks but as you say there's more stuff that
we need to add (like your patch 2 here) so I'm slightly nervous about
actually setting the reliable flag here without even a comment.  Equally
well there's no actual use of this until arch_stack_walk_reliable() gets
implemented so it's not like it's causing any problems and it gives us
the structure to start building up the rest of the checks.

OK. So how about changing the field from a flag to an enum that says exactly
what happened with the frame?

enum {
	FRAME_NORMAL = 0,
	FRAME_UNALIGNED,
	FRAME_NOT_ACCESSIBLE,
	FRAME_RECURSION,
	FRAME_GRAPH_ERROR,
	FRAME_INVALID_TEXT_ADDRESS,
	FRAME_UNRELIABLE_FUNCTION,
	FRAME_NUM_STATUS,
} frame_status;

struct stackframe {
	...
	enum frame_status status;
};

unwind_frame()
{
	frame->status = FRAME_NORMAL;

	Then, for each situation, change the status appropriately.
}

Eventually, arch_stack_walk_reliable() could just declare the stack trace
as unreliable if status != FRAME_NORMAL.

Also, the caller can get an exact idea of why the stack trace failed.

Is that acceptable?

The other thing I guess is the question of if we want to bother flagging
frames as unrelaible when we return an error; I don't see an issue with
it and it may turn out to make it easier to do something in the future
so I'm fine with that

Initially, I thought that there is no need to flag it for errors. But Josh
had a comment that the stack trace is indeed unreliable on errors. Again, the
word unreliable is the one causing the problem.

The above enum-based solution addresses Josh's comment as well.

Let me know if this is good.

Thanks!

Madhavan

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel