Re: [PATCH 1/2] Add private_function for private functions within glibc

["H.J. Lu" <hjl dot tools at gmail dot com>]

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.