On Fri, Jul 18, 2025 at 10:28:32AM +0200, Jens Remus wrote:
> On 17.07.2025 13:09, Jens Remus wrote:
> > On 17.07.2025 01:01, Josh Poimboeuf wrote:
> >> On Thu, Jul 10, 2025 at 06:35:13PM +0200, Jens Remus wrote:
> >>> +++ 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
