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 Thu, Jun 22, 2017 at 9:37 AM, Florian Weimer <fweimer@redhat.com> wrote:
> 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
There are 2 pushes for PLT and add push %eax. The offset is 12.
> the spill slots of %eax and %edx would be nice. too.
Don't we have
pushl %eax # Preserve registers otherwise clobbered.
cfi_adjust_cfa_offset (4)
pushl %edx
cfi_adjust_cfa_offset (4)
> 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?
>
Probably. But it is a separate issue.
--
H.J.