Re: [RFC PATCH v1 07/16] unwind_user: Enable archs that do not necessarily save RA

On Fri, Jul 18, 2025 at 10:28:32AM +0200, Jens Remus wrote:

On 17.07.2025 13:09, Jens Remus wrote:

quoted

On 17.07.2025 01:01, Josh Poimboeuf wrote:

quoted

On Thu, Jul 10, 2025 at 06:35:13PM +0200, Jens Remus wrote:

quoted

+++ b/arch/Kconfig

@@ -450,6 +450,11 @@ config HAVE_UNWIND_USER_SFRAME
 	bool
 	select UNWIND_USER
 
+config HAVE_USER_RA_REG
+	bool
+	help
+	  The arch passes the return address (RA) in user space in a register.

How about "HAVE_UNWIND_USER_RA_REG" so it matches the existing
namespace?

Ok.  I am open to any improvements.

Thinking about this again I realized that the config option actually
serves two purposes:

1. Enable code (e.g. unwind user) to determine the presence of the new
   user_return_address().  That is where I derived the name from.
2. Enable unwind user (sframe) to behave differently, if an architecture
   has/uses a RA register (unlike x86, which solely uses the stack).

The sframe CONFIG_HAVE_USER_RA_REG check is redundant with the
unwind_user one, no?  I'm thinking it's better for sframe to just decode
the entry as it is, and then let unwind_user validate things.

I think the primary notion is that an architecture has/uses a register
for the return address and thus provides user_return_address().  What
consumers such as unwind user do with that info is secondary.

Thoughts?

user_return_address() only has the single user, and is not all that
generically useful anyway (e.g., it warns on x86), so let's keep it
encapsulated in include/linux/unwind_user.h and give it the
"unwind_user" prefix.

Also, "RA_REG" is a bit ambiguous, it sounds almost like that other
option which spills RA to another register.  Conceptually, it's a link
register, so can we rename that to CONFIG_HAVE_UNWIND_USER_LINK_REG and
unwind_user_get_link_reg() or so?

Similarly, CONFIG_HAVE_UNWIND_USER_LOC_REG isn't that descriptive, how
about CONFIG_HAVE_UNWIND_USER_LINK_REG_SPILL?

Also we can get rid of the '#define func_name func_name' things and just
guard those functions with their corresponding CONFIG options in
inclide/linux/unwind_user.h.

Also those two functions should have similar naming and prototypes.

For example, in include/linux/unwind_user.h:

#ifndef CONFIG_HAVE_UNWIND_USER_LINK_REG
int unwind_user_get_link_reg(unsigned long *val)
{
	WARN_ON_ONCE(1);
	return -EINVAL;
}
#endif

#ifndef CONFIG_HAVE_UNWIND_USER_LINK_REG_SPILL
int unwind_user_get_reg(unsigned long *val, unsigned int regnum)
{
	WARN_ON_ONCE(1);
	return -EINVAL;
}
#endif

Then the code can be simplified (assuming no topmost checks):

	/* Get the Return Address (RA) */
	switch (frame->ra.loc) {
	case UNWIND_USER_LOC_NONE:
		if (unwind_user_get_link_reg(&ra))
			goto done;
		break;
	...
	case UNWIND_USER_LOC_REG:
		if (unwind_user_get_reg(&ra, frame->ra.regnum))
			goto done;
		break;
	...

-- 
Josh