Re: [PATCH 05/17] hppa: Remove _dl_skip_args usage (BZ# 29165)

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

On 26/05/2022 21:01, John David Anglin wrote:
> On 2022-05-26 7:10 p.m., Adhemerval Zanella wrote:
>>
>> On 26/05/2022 15:58, John David Anglin wrote:
>>> On 2022-05-26 2:23 p.m., Adhemerval Zanella wrote:
>>>> On 26/05/2022 15:14, Florian Weimer wrote:
>>>>> * Adhemerval Zanella via Libc-alpha:
>>>>>
>>>>>> Different than other architectures, hppa creates an unrelated stack
>>>>>> frame where ld.so argc/argv adjustments done by ad43cac44a6860eaefc
>>>>>> is not done on the argc/argv saved/restore by _dl_start_user.
>>>>>>
>>>>>> Instead load _dl_argc and _dl_argv directlty instead of adjust them
>>>>>> using _dl_skip_args value.
>>>>>>
>>>>>> Checked on hppa-linux-gnu.
>>>>> Does this fix bug 29165?  If yes, it should say so on the commit
>>>>> message.
>>>> It does, I assumed the title was suffice. I will add a note in the
>>>> commit itself.
>>> This comment could be improved:
>>>
>>>> +      It also mean that to get the correct argc and argv if the    \
>>>> +      program is ld.so it requires to read _dl_argc and _dl_argv. */\
>>> "mean" should be "means" and the wording is somewhat stilted.
>>>
>> Indeed it sounds strange, I have changed to:
>>
>>           Also, the loader will adjust argc, argv, env, and aux vectors \
>>           directly on the stack to remove any arguments used for direct \
>>           loader invocation.  So it requires to use _dl_argc and           \
>>      _dl_argv instead of reload them from previous saved area.  */ \
> This comment is improved but I would move it before the instructions to load _dl_argc
> and _dl_argc in _dl_start_user.
> 
> It is better to use present tense:
> 
>     The loader adjusts argc, argv, env, and the aux vectors \
>     directly on the stack to remove any arguments used for direct \
>     loader invocation.  Thus, argc and argv must be reloaded from \
>     from _dl_argc and _dl_argv.  */

Ack.

> 
> In looking at the code, I think we can remove the following instructions:
> 
>>         /* Save the relevant arguments (yes, those are the correct      \
>>            registers, the kernel is weird) in their stack slots. */     \
>> "       stw     %r25,-40(%sp)\n" /* argc */                             \
>> "       stw     %r24,-44(%sp)\n" /* argv */                             \
> 
> The saved values are no longer used as far as I can tell.

Indeed, I removed them.