[05/17] hppa: Remove _dl_skip_args usage (BZ# 29165)
Checks
| Context |
Check |
Description |
| dj/TryBot-apply_patch |
success
|
Patch applied to master at the time it was sent
|
Commit Message
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.
---
sysdeps/hppa/dl-machine.h | 28 +++++++++++-----------------
1 file changed, 11 insertions(+), 17 deletions(-)
Comments
* 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.
Thanks,
Florian
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.
* Adhemerval Zanella:
> 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.
Sorry, I missed that. 8-( Commit subject is sufficient.
Thanks for fixing it.
Florian
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.
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. */ \
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. */
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.
Dave
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.
@@ -415,10 +415,8 @@ asm ( \
So, obviously, we can't just pass %sp to _dl_start. That's \
okay, argv-4 will do just fine. \
\
- The pleasant part of this is that if we need to skip \
- arguments we can just decrement argc and move argv, because \
- the stack pointer is utterly unrelated to the location of \
- the environment and argument vectors. */ \
+ 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. */\
\
/* This is always within range so we'll be okay. */ \
" bl _dl_start,%rp\n" \
@@ -430,22 +428,18 @@ asm ( \
/* Save the entry point in %r3. */ \
" copy %ret0,%r3\n" \
\
- /* See if we were called as a command with the executable file \
- name as an extra leading argument. */ \
-" addil LT'_dl_skip_args,%r19\n" \
-" ldw RT'_dl_skip_args(%r1),%r20\n" \
-" ldw 0(%r20),%r20\n" \
- \
-" ldw -40(%sp),%r25\n" /* argc */ \
-" comib,= 0,%r20,.Lnofix\n" /* FIXME: Mispredicted branch */\
-" ldw -44(%sp),%r24\n" /* argv (delay slot) */ \
- \
-" sub %r25,%r20,%r25\n" \
+ /* Load argc and store the argc if the loader changes it. */ \
+" addil LT'_dl_argc,%r19\n" \
+" ldw RT'_dl_argc(%r1),%r20\n" \
+" ldw 0(%r20),%r25\n" \
" stw %r25,-40(%sp)\n" \
-" sh2add %r20,%r24,%r24\n" \
+ \
+ /* Same for argv. */ \
+" addil LT'_dl_argv,%r19\n" \
+" ldw RT'_dl_argv(%r1),%r20\n" \
+" ldw 0(%r20),%r24\n" \
" stw %r24,-44(%sp)\n" \
\
-".Lnofix:\n" \
/* Call _dl_init(main_map, argc, argv, envp). */ \
" addil LT'_rtld_local,%r19\n" \
" ldw RT'_rtld_local(%r1),%r26\n" \