This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH 1/2] Add private_function for private functions within glibc
On 06/22/2017 06:21 PM, H.J. Lu wrote:
> On Thu, Jun 22, 2017 at 7:41 AM, Florian Weimer <fweimer@redhat.com> wrote:
>> On 06/22/2017 04:38 PM, H.J. Lu wrote:
>>> On Thu, Jun 22, 2017 at 6:40 AM, Florian Weimer <fweimer@redhat.com> wrote:
>>>> On 06/17/2017 03:59 PM, H.J. Lu wrote:
>>>>> Here parameters are passed to _dl_init in registers. I want to minimize
>>>>> changes to avoid any potential issues.
>>>> Well, as a rule of thumb, if we do something that breaks our own code,
>>>> it is pretty much guaranteed to wreak havoc across the board (because
>>>> our test coverage is somewhat poor).
>>>>
>>>> I see a lot of use of regparm (3). For example:
>>>>
>>>> $ echo '#include <Qt/qchar.h>' | g++ -m32 -E -x c++ - | grep regparm
>>>> static Category __attribute__((regparm(3))) category(uint ucs4);
>>>> static Category __attribute__((regparm(3))) category(ushort ucs2);
>>>> static Direction __attribute__((regparm(3))) direction(uint ucs4);
>>>> static Direction __attribute__((regparm(3))) direction(ushort ucs2);
>>>> static Joining __attribute__((regparm(3))) joining(uint ucs4);
>>>> …
>>>>
>>>> I think these calls actually cross DSO boundaries.
>>> I don't think so. See "static ...".
>> It's C++ code, so it just means there's no this pointer.
>>
> How about this patch?
Yes, it addresses my concerns.
> +# The SHSTK compatible version.
> + .text
> + .globl _dl_runtime_resolve_shstk
> + .type _dl_runtime_resolve_shstk, @function
> + cfi_startproc
> + .align 16
> +_dl_runtime_resolve_shstk:
> + cfi_adjust_cfa_offset (8)
> + pushl %eax # Preserve registers otherwise clobbered.
> + cfi_adjust_cfa_offset (4)
> + pushl %edx
> + cfi_adjust_cfa_offset (4)
> + movl 12(%esp), %edx # Copy args pushed by PLT in register. Note
> + movl 8(%esp), %eax # that `fixup' takes its parameters in regs.
> + call _dl_fixup # Call resolver.
> + movl (%esp), %edx # Get register content back.
> + movl %eax, %ecx # Store the function address.
> + movl 4(%esp), %eax # Get register content back.
> + addl $16, %esp # Adjust stack: PLT1 + PLT2 + %eax + %edx
> + cfi_adjust_cfa_offset (-16)
> + jmp *%ecx # Jump to function address.
> + cfi_endproc
> + .size _dl_runtime_resolve_shstk, .-_dl_runtime_resolve_shstk
Are the CFI annotations correct? I think the offset after the push %eax
should be 8, not 12, as it currently appears to be. CFI annotations for
the spill slots of %eax and %edx would be nice. too.
It seems to me that _dl_fixup is called with an unaligned stack, either
in this implementation or the existing one. Don't we care about this
because we compile the dynamic linker without SSE support?
Thanks,
Florian