Page MenuHomeFreeBSD

Process irelocs for statically linked binaries from crt1.
ClosedPublic

Authored by kib on Sep 30 2018, 12:28 PM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 9 2024, 2:25 PM
Unknown Object (File)
Feb 6 2024, 1:29 PM
Unknown Object (File)
Feb 1 2024, 11:57 PM
Unknown Object (File)
Feb 1 2024, 11:57 PM
Unknown Object (File)
Feb 1 2024, 11:56 PM
Unknown Object (File)
Feb 1 2024, 11:56 PM
Unknown Object (File)
Feb 1 2024, 11:55 PM
Unknown Object (File)
Feb 1 2024, 11:55 PM
Subscribers

Details

Summary

Only amd64 and i386 are handled at the moment, and only amd64 tested.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 19972

Event Timeline

Process irelocs before calling into libc and before executing constructors.

markj added inline comments.
lib/csu/amd64/reloc.c
4

I believe we can omit "All rights reserved" now.

lib/csu/common/ignore_init.c
82

should be "_SUPPRESS"

lib/csu/i386/reloc.c
60

If cpuid is supported, this has the side effect of leaving the ID bit inverted in EFLAGS. I guess that's not likely a problem in practice.

This revision is now accepted and ready to land.Oct 1 2018, 3:00 AM
kib marked 2 inline comments as done.Oct 1 2018, 12:54 PM
kib added inline comments.
lib/csu/amd64/reloc.c
4

This is what mandated by the contract I have signed with FF.

lib/csu/i386/reloc.c
60

The code is taken literally from the Intel App Note 485 CPU Identification. Code uses XOR, so any state of the EFLAGS.ID bit is fine for it. I do not believe that we need to care about anything else. The same code is present in x86/__vdso_tc.c and in i386/gen/getcontextx.c.

lib/csu/amd64/reloc.c
4

Eventually the Foundation contracts will be updated or amended, but the All rights reserved. text can indeed be removed from Foundation copyright templates.

Only amd64 and i386 are handled at the moment

The intent is to set CRT_IRELOC_SUPRESS for other architectures after review, for the committable version?

kib marked 3 inline comments as done.Oct 1 2018, 3:17 PM

Only amd64 and i386 are handled at the moment

The intent is to set CRT_IRELOC_SUPRESS for other architectures after review, for the committable version?

Exactly, I wanted each architecture to provide the explicit choice instead of doing nothing by default.

_SUPPRESS, tweak copyrights.

This revision now requires review to proceed.Oct 1 2018, 3:19 PM
This revision is now accepted and ready to land.Oct 1 2018, 3:30 PM

Do relocs before call to tls initialization.
Block rtld from using ifunc from libc for memset/bzero.

This revision now requires review to proceed.Oct 2 2018, 12:52 PM
This revision is now accepted and ready to land.Oct 2 2018, 2:28 PM

buildworld fails for me with:

===> libexec/rtld-elf (all)                                                              
/usr/bin/ld: error: duplicate symbol: bzero                                              
>>> defined at rtld.c:5599 (/usr/home/mjg/repos/freebsd/libexec/rtld-elf/rtld.c:5599)    
>>>            rtld.o:(bzero)                                                            
>>> defined at ifunc.c:17 (/usr/home/mjg/repos/freebsd/lib/libc/amd64/ifunc.c:17)        
>>>            ifunc.nossppico:(bzero_resolver) in archive /usr/obj/usr/home/mjg/repos/freebsd/amd64.amd64/lib/libc/libc_nossp_pic.a                                               
                                                                                         
/usr/bin/ld: error: duplicate symbol: memset                                             
>>> defined at rtld.c:5589 (/usr/home/mjg/repos/freebsd/libexec/rtld-elf/rtld.c:5589)    
>>>            rtld.o:(memset)                                                           
>>> defined at ifunc.c:8 (/usr/home/mjg/repos/freebsd/lib/libc/amd64/ifunc.c:8)          
>>>            ifunc.nossppico:(memset_resolver) in archive /usr/obj/usr/home/mjg/repos/freebsd/amd64.amd64/lib/libc/libc_nossp_pic.a                                              
cc: error: linker command failed with exit code 1 (use -v to see invocation)             
*** [ld-elf.so.1.full] Error code 1

Note the build host runs without any of the patches.

Here is the change I was testing with, it creates all expected symbols (memmove, memcpy, bcopy, bzero, memset):
https://people.freebsd.org/~mjg/libc-ifunc.diff

In D17363#370833, @mjg wrote:

buildworld fails for me with:

===> libexec/rtld-elf (all)                                                              
/usr/bin/ld: error: duplicate symbol: bzero                                              
>>> defined at rtld.c:5599 (/usr/home/mjg/repos/freebsd/libexec/rtld-elf/rtld.c:5599)    
>>>            rtld.o:(bzero)                                                            
>>> defined at ifunc.c:17 (/usr/home/mjg/repos/freebsd/lib/libc/amd64/ifunc.c:17)        
>>>            ifunc.nossppico:(bzero_resolver) in archive /usr/obj/usr/home/mjg/repos/freebsd/amd64.amd64/lib/libc/libc_nossp_pic.a                                               
                                                                                         
/usr/bin/ld: error: duplicate symbol: memset                                             
>>> defined at rtld.c:5589 (/usr/home/mjg/repos/freebsd/libexec/rtld-elf/rtld.c:5589)    
>>>            rtld.o:(memset)                                                           
>>> defined at ifunc.c:8 (/usr/home/mjg/repos/freebsd/lib/libc/amd64/ifunc.c:8)          
>>>            ifunc.nossppico:(memset_resolver) in archive /usr/obj/usr/home/mjg/repos/freebsd/amd64.amd64/lib/libc/libc_nossp_pic.a                                              
cc: error: linker command failed with exit code 1 (use -v to see invocation)             
*** [ld-elf.so.1.full] Error code 1

Note the build host runs without any of the patches.

Here is the change I was testing with, it creates all expected symbols (memmove, memcpy, bcopy, bzero, memset):
https://people.freebsd.org/~mjg/libc-ifunc.diff

Right, this is due to your change defining resolvers for all functions in one .o file. You need to

  1. add remaining overrides to rtld.c
  2. split ifunc,c into per-function ifunc.

And, until ld.lld and strip bugs are not fixed, static binaries will be broken. Workaround is to not strip them on install.

Ok, I confirm it works fine for dynamic and static binaries (not stripped) for the aforementioned routines on amd64. Will test i386 some time later.

This revision now requires review to proceed.Oct 4 2018, 5:53 PM
lib/csu/amd64/reloc.c
60

How can this work with a debugger that wishes to find the resolved address of an ifunc? I peeked at glibc and they seem to avoid passing arguments to the resolver somehow.

kib marked an inline comment as done.Oct 8 2018, 6:54 PM
kib added inline comments.
lib/csu/amd64/reloc.c
60

Why would debugger require that ? What matter is the address of the function which is written into the call instruction, not the return value from the ifunc resolver.

If really wanted, debugger can reconstruct the args.

As discussed elsewhere there are a few toolchain issues with static binaries with ifuncs - one fix is in https://github.com/emaste/freebsd/commit/36131f9c4c9aa343ea2679c18b3d1d621bb837f1

Add suppression for !x86 arches.

lib/csu/common/ignore_init.c
54

I'm slightly surprised to find this in ignore_init.c

kib marked an inline comment as done.Oct 12 2018, 5:20 PM
kib added inline comments.
lib/csu/common/ignore_init.c
54

I may rename it after this change lands.

Commit candidate, passed tinderbox.

This revision is now accepted and ready to land.Oct 13 2018, 2:09 PM
emaste added inline comments.
lib/csu/common/ignore_init.c
54

Should we perhaps combine crtbrand.c and ignore_init.c into one?

kib marked an inline comment as done.Oct 13 2018, 6:30 PM
kib added inline comments.
lib/csu/common/ignore_init.c
54

We can. I do not have a strong opinion. I slightly prefer a split.

lib/csu/common/ignore_init.c
54

Just seems strange to me to combine crtbrand and my upcoming feature flags into one, and ignore_init and ifuncs into another. But either way I agree this can be a change after this patch.

This revision was automatically updated to reflect the committed changes.