Summary: | [Regression] broken argv adjustment | ||
---|---|---|---|
Product: | glibc | Reporter: | John David Anglin <danglin> |
Component: | libc | Assignee: | Adhemerval Zanella <adhemerval.zanella> |
Status: | RESOLVED FIXED | ||
Severity: | normal | CC: | adhemerval.zanella, drepper.fsp, sam |
Priority: | P2 | ||
Version: | 2.36 | ||
Target Milestone: | 2.36 | ||
Host: | hppa*-*-linux* | Target: | hppa*-*-linux* |
Build: | hppa*-*-linux* | Last reconfirmed: |
Description
John David Anglin
2022-05-21 16:00:46 UTC
It seems hppa creates an unrelated stack frame that ld.so changes where it saves/restore both argc and argv. Does this fix the issue: diff --git a/sysdeps/hppa/dl-machine.h b/sysdeps/hppa/dl-machine.h index 8c0ca32fc6..9ce17a7e2f 100644 --- a/sysdeps/hppa/dl-machine.h +++ b/sysdeps/hppa/dl-machine.h @@ -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,14 @@ 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" \ +" addil LT'_dl_argc,%r19\n" \ +" ldw RT'_dl_argc(%r1),%r20\n" \ +" ldw 0(%r20),%r25\n" \ \ -" ldw -40(%sp),%r25\n" /* argc */ \ -" comib,= 0,%r20,.Lnofix\n" /* FIXME: Mispredicted branch */\ -" ldw -44(%sp),%r24\n" /* argv (delay slot) */ \ +" addil LT'_dl_argv,%r19\n" \ +" ldw RT'_dl_argv(%r1),%r20\n" \ +" ldw 0(%r20),%r24\n" \ \ -" sub %r25,%r20,%r25\n" \ -" stw %r25,-40(%sp)\n" \ -" sh2add %r20,%r24,%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" \ @@ -456,9 +446,6 @@ asm ( \ " bl _dl_init,%r2\n" \ " ldo 4(%r23),%r23\n" /* delay slot */ \ \ - /* Reload argc, argv to the registers start.S expects. */ \ -" ldw -40(%sp),%r25\n" \ -" ldw -44(%sp),%r24\n" \ \ /* _dl_fini is a local function in the loader, so we construct \ a false OPD here and pass this to the application. */ \ ? Thanks. I think we need to keep the instructions that save and restore r24 and r25. They will be clobbered by call to _dl_init. Will test. On 2022-05-24 4:26 p.m., adhemerval.zanella at linaro dot org wrote: > https://sourceware.org/bugzilla/show_bug.cgi?id=29165 > > --- Comment #1 from Adhemerval Zanella <adhemerval.zanella at linaro dot org> --- > It seems hppa creates an unrelated stack frame that ld.so changes where it > saves/restore both argc and argv. Does this fix the issue: > > diff --git a/sysdeps/hppa/dl-machine.h b/sysdeps/hppa/dl-machine.h > index 8c0ca32fc6..9ce17a7e2f 100644 > --- a/sysdeps/hppa/dl-machine.h > +++ b/sysdeps/hppa/dl-machine.h > @@ -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,14 @@ 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" \ > +" addil LT'_dl_argc,%r19\n" \ > +" ldw RT'_dl_argc(%r1),%r20\n" \ > +" ldw 0(%r20),%r25\n" \ > \ > -" ldw -40(%sp),%r25\n" /* argc */ \ > -" comib,= 0,%r20,.Lnofix\n" /* FIXME: Mispredicted branch */\ > -" ldw -44(%sp),%r24\n" /* argv (delay slot) */ \ > +" addil LT'_dl_argv,%r19\n" \ > +" ldw RT'_dl_argv(%r1),%r20\n" \ > +" ldw 0(%r20),%r24\n" \ > \ > -" sub %r25,%r20,%r25\n" \ > -" stw %r25,-40(%sp)\n" \ > -" sh2add %r20,%r24,%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" \ > @@ -456,9 +446,6 @@ asm ( > \ > " bl _dl_init,%r2\n" \ > " ldo 4(%r23),%r23\n" /* delay slot */ \ > \ > - /* Reload argc, argv to the registers start.S expects. */ \ > -" ldw -40(%sp),%r25\n" \ > -" ldw -44(%sp),%r24\n" \ > \ > /* _dl_fini is a local function in the loader, so we construct \ > a false OPD here and pass this to the application. */ \ > > > ? > I don't think it is required since _dl_fini does not have any arguments. (In reply to dave.anglin from comment #2) > Thanks. I think we need to keep the instructions that save and restore r24 > and r25. > They will be clobbered by call to _dl_init. > > Will test. But the comment says these are for (_start) in start.S. On 2022-05-25 7:57 a.m., adhemerval.zanella at linaro dot org wrote: > I don't think it is required since _dl_fini does not have any arguments. > > (In reply to dave.anglin from comment #2) >> Thanks. I think we need to keep the instructions that save and restore r24 >> and r25. >> They will be clobbered by call to _dl_init. >> >> Will test. But the start above will tail call _dl_start and _dl_start will call the user entrypoint that will eventually calls exit syscall. So I can't see why matter restore r24 and r25 at this point, specially since they will point to the ld.so ones. (In reply to dave.anglin from comment #4) > But the comment says these are for (_start) in start.S. > > On 2022-05-25 7:57 a.m., adhemerval.zanella at linaro dot org wrote: > > I don't think it is required since _dl_fini does not have any arguments. > > > > (In reply to dave.anglin from comment #2) > >> Thanks. I think we need to keep the instructions that save and restore r24 > >> and r25. > >> They will be clobbered by call to _dl_init. > >> > >> Will test. It looks to me as if _dl_start doesn't call the user entrypoint directly. It calls _start in start.S. _start calls __libc_start_main with the argument setup shown in the comment in sysdeps/hppa/start.S. On 2022-05-25 8:41 a.m., adhemerval.zanella at linaro dot org wrote: > https://sourceware.org/bugzilla/show_bug.cgi?id=29165 > > --- Comment #5 from Adhemerval Zanella <adhemerval.zanella at linaro dot org> --- > But the start above will tail call _dl_start and _dl_start will call the user > entrypoint that will eventually calls exit syscall. So I can't see why matter > restore r24 and r25 at this point, specially since they will point to the ld.so > ones. > > (In reply to dave.anglin from comment #4) >> But the comment says these are for (_start) in start.S. >> >> On 2022-05-25 7:57 a.m., adhemerval.zanella at linaro dot org wrote: >>> I don't think it is required since _dl_fini does not have any arguments. >>> >>> (In reply to dave.anglin from comment #2) >>>> Thanks. I think we need to keep the instructions that save and restore r24 >>>> and r25. >>>> They will be clobbered by call to _dl_init. >>>> >>>> Will test. My understanding is hppa has to create the synthetic frame layout and save the kernel passed argc/argv so the loader can proper obtain them on _dl_sysdep_parse_arguments. The call frame is: _start \_ _dl_start (void *arg) \_ _dl_start_final (void *arg) \_ _dl_sysdep_start (void *arg, ...) \_ _dl_sysdep_parse_arguments (void **start_argptr, ...) Once the loader setup _dl_argc and _dl_argc for the application, it does not matter the kernel passed argc/argv (there are already processed and should not be visible by the application). (In reply to dave.anglin from comment #6) > It looks to me as if _dl_start doesn't call the user entrypoint directly. > It calls _start in start.S. _start > calls __libc_start_main with the argument setup shown in the comment in > sysdeps/hppa/start.S. > > On 2022-05-25 8:41 a.m., adhemerval.zanella at linaro dot org wrote: > > https://sourceware.org/bugzilla/show_bug.cgi?id=29165 > > > > --- Comment #5 from Adhemerval Zanella <adhemerval.zanella at linaro dot org> --- > > But the start above will tail call _dl_start and _dl_start will call the user > > entrypoint that will eventually calls exit syscall. So I can't see why matter > > restore r24 and r25 at this point, specially since they will point to the ld.so > > ones. > > > > (In reply to dave.anglin from comment #4) > >> But the comment says these are for (_start) in start.S. > >> > >> On 2022-05-25 7:57 a.m., adhemerval.zanella at linaro dot org wrote: > >>> I don't think it is required since _dl_fini does not have any arguments. > >>> > >>> (In reply to dave.anglin from comment #2) > >>>> Thanks. I think we need to keep the instructions that save and restore r24 > >>>> and r25. > >>>> They will be clobbered by call to _dl_init. > >>>> > >>>> Will test. I see now what my fix is missing, let me fix it. (In reply to Adhemerval Zanella from comment #7) > My understanding is hppa has to create the synthetic frame layout and save > the kernel passed argc/argv so the loader can proper obtain them on > _dl_sysdep_parse_arguments. The call frame is: > > _start > \_ _dl_start (void *arg) > \_ _dl_start_final (void *arg) > \_ _dl_sysdep_start (void *arg, ...) > \_ _dl_sysdep_parse_arguments (void **start_argptr, ...) > > Once the loader setup _dl_argc and _dl_argc for the application, it does not > matter the kernel passed argc/argv (there are already processed and should > not be visible by the application). > > (In reply to dave.anglin from comment #6) > > It looks to me as if _dl_start doesn't call the user entrypoint directly. > > It calls _start in start.S. _start > > calls __libc_start_main with the argument setup shown in the comment in > > sysdeps/hppa/start.S. > > > > On 2022-05-25 8:41 a.m., adhemerval.zanella at linaro dot org wrote: > > > https://sourceware.org/bugzilla/show_bug.cgi?id=29165 > > > > > > --- Comment #5 from Adhemerval Zanella <adhemerval.zanella at linaro dot org> --- > > > But the start above will tail call _dl_start and _dl_start will call the user > > > entrypoint that will eventually calls exit syscall. So I can't see why matter > > > restore r24 and r25 at this point, specially since they will point to the ld.so > > > ones. > > > > > > (In reply to dave.anglin from comment #4) > > >> But the comment says these are for (_start) in start.S. > > >> > > >> On 2022-05-25 7:57 a.m., adhemerval.zanella at linaro dot org wrote: > > >>> I don't think it is required since _dl_fini does not have any arguments. > > >>> > > >>> (In reply to dave.anglin from comment #2) > > >>>> Thanks. I think we need to keep the instructions that save and restore r24 > > >>>> and r25. > > >>>> They will be clobbered by call to _dl_init. > > >>>> > > >>>> Will test. Fixed on master. *** Bug 29237 has been marked as a duplicate of this bug. *** |