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:
>> 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.
Ok. Makes sense.
>> 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
jre...@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/
