Hello Josh,
thank you very much for your valuable feedback!
On 18.07.2025 18:59, Josh Poimboeuf wrote:
On Fri, Jul 18, 2025 at 10:28:32AM +0200, Jens Remus wrote:
quoted
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.
Ok. Makes sense.
quoted
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.
Ok. I was hesitating to add it to ptrace.h. Given it falls into the
same category as user_stack_pointer() and frame_pointer() I was
astonished it did not yet exist. But given unwind user would be the
single user I agree that it is better to do as you suggest.
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?
The terminology in DWARF and SFrame is return address (RA) register.
AArch64 uses link register (LR). s390 uses RA register. x86 uses return
address. While I am open to use your suggestion, I wonder what others
would prefer.
In fact the whole option is not required, if using '#define fname fname'
instead (that you suggest not to down below). user_unwind.c would then
contain the following as default for architectures that do not define
their custom implementation (or link_reg instead of ra_reg):
#ifndef unwind_user_get_ra_reg
int unwind_user_get_ra_reg(unsigned long *val)
{
WARN_ON_ONCE(1);
return -EINVAL;
}
#define unwind_user_get_ra_reg unwind_user_get_ra_reg
#endif
The commit subject should better be changed to one of the following
(with the commit message reworded accordingly):
unwind_user: Enable archs that have a RA register
or
unwind_user: Enable archs that have a link register
Similarly, CONFIG_HAVE_UNWIND_USER_LOC_REG isn't that descriptive, how
about CONFIG_HAVE_UNWIND_USER_LINK_REG_SPILL?
Are you perhaps mixing up two of the new config options introduced by
this and the subsequent patch?
HAVE_USER_RA_REG (introduced by this patch): Indicates the architecture
has/uses a RA/LR register. I would be fine to rename this to
"CONFIG_HAVE_UNWIND_USER_LINK_REG" (as you suggest), provided "link
register" is the terminology we can all agree on, although DWARF and
SFrame use "return address register". Otherwise
"CONFIG_HAVE_UNWIND_USER_RA_REG" or even omit, if there is no need for
the option at all.
CONFIG_HAVE_UNWIND_USER_LOC_REG (introduced by the subsequent patch):
Indicates that the architecture may save registers in other registers.
Those support UNWIND_USER_LOC_REG (location register) in addition to
UNWIND_USER_LOC_STACK (location stack). Note that this applies to
both FP and RA.
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
Is a config option actually required (see above)? The reason I added
them was to perform checks, that you suggest to omit. Your below code
suggestion would no longer required the config options at all. What
is preferable? The config option would ensure a build error if an
architecture enables the option without providing the arch-specific
implementations.
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;
...
Thanks and regards,
Jens
--
Jens Remus
Linux on Z Development (D3303)
+49-7031-16-1128 Office
jremus@de.ibm.com
IBM
IBM Deutschland Research & Development GmbH; Vorsitzender des Aufsichtsrats: Wolfgang Wendt; Geschäftsführung: David Faller; Sitz der Gesellschaft: Böblingen; Registergericht: Amtsgericht Stuttgart, HRB 243294
IBM Data Privacy Statement: https://www.ibm.com/privacy/