Page MenuHomeFreeBSD
Differential D54559

rtld-elf: Pass HWCAP flags to ifunc resolver functions
AcceptedPublic
Actions

Authored by sarah.walker2_arm.com on Tue, Jan 6, 3:41 PM.
Tags
None
Referenced Files
F141922984: D54559.diff
Mon, Jan 12, 2:13 PM
F141922977: D54559.diff
Mon, Jan 12, 2:12 PM
F141911544: D54559.id169313.diff
Mon, Jan 12, 10:11 AM
Unknown Object (File)
Sun, Jan 11, 5:04 AM
Unknown Object (File)
Sat, Jan 10, 4:44 PM
Unknown Object (File)
Sat, Jan 10, 3:56 PM
Unknown Object (File)
Sat, Jan 10, 5:38 AM
Unknown Object (File)
Fri, Jan 9, 6:48 PM
View All 17 Files
Subscribers
bdragon
emaste
imp
jrtc27

Details

Reviewers
andrew
kib
Group Reviewers
arm64
Summary

Function arguments are based on Section 9.4.1 "GNU C Library IFUNC interface"
from "System V ABI for the Arm 64-bit Architecture (AArch64)", 2025Q1.
(https://github.com/ARM-software/abi-aa/releases/download/2025Q1/sysvabi64.pdf)

Sponsored by: Arm Ltd

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 69763
Build 66646: arc lint + arc unit

Event Timeline

sarah.walker2_arm.com created this revision.Tue, Jan 6, 3:41 PM
Herald added a reviewer: andrew. · View Herald TranscriptTue, Jan 6, 3:41 PM
Herald added subscribers: bdragon, emaste, imp. · View Herald Transcript
sarah.walker2_arm.com requested review of this revision.Tue, Jan 6, 3:41 PM
Harbormaster completed remote builds in B69662: Diff 169189.Tue, Jan 6, 3:41 PM
sarah.walker2_arm.com added a parent revision: D54558: arm64: Enable MOPS in userspace.Tue, Jan 6, 3:41 PM
sarah.walker2_arm.com added a child revision: D54560: libc/aarch64: Use MOPS implementations of memcpy/memmove/memset where availble.
andrew added reviewers: arm64, kib.Tue, Jan 6, 3:46 PM
andrew added a comment.Tue, Jan 6, 3:49 PM

There should be a followup change to sys/arm64/include/ifunc.h to update DEFINE_UIFUNC. We can also use this to simplify the ifunc handlers in sys/crypto/sha2/sha512c.c and sys/crypto/sha2/sha256c.c

jrtc27 added a subscriber: jrtc27.Tue, Jan 6, 3:53 PM

Will you also be implementing this for csu for static binaries (often forgotten, including by me in the past)?

libexec/rtld-elf/aarch64/reloc.c
448

Why isn't this in a header?

454

Can this be NULL?

libexec/rtld-elf/aarch64/rtld_machdep.h
78

Shouldn't this live in a header somewhere? The SysV document also says glibc provides an __ifunc_arg_t typedef if you want to be fully compatible.

79–83

They're only uint64_t as placeholders for passing a 0 in a register.

sarah.walker2_arm.com added a comment.Tue, Jan 6, 4:28 PM
In D54559#1246501, @andrew wrote:

There should be a followup change to sys/arm64/include/ifunc.h to update DEFINE_UIFUNC. We can also use this to simplify the ifunc handlers in sys/crypto/sha2/sha512c.c and sys/crypto/sha2/sha256c.c

Can you give more detail on what you want here please?

andrew added a comment.Tue, Jan 6, 4:38 PM
In D54559#1246536, @sarah.walker2_arm.com wrote:
In D54559#1246501, @andrew wrote:

There should be a followup change to sys/arm64/include/ifunc.h to update DEFINE_UIFUNC. We can also use this to simplify the ifunc handlers in sys/crypto/sha2/sha512c.c and sys/crypto/sha2/sha256c.c

Can you give more detail on what you want here please?

ifunc.h has the DEFINE_UIFUNC macro where the first two arguments in the generated function signature will need an update. The other two files use DEFINE_UIFUNC and call elf_aux_info to read the HWCAPs, but could be updated after this change (and a similar change for static binaries)

sarah.walker2_arm.com updated this revision to Diff 169313.Thu, Jan 8, 10:52 AM

Address review comments.

Harbormaster completed remote builds in B69713: Diff 169313.Thu, Jan 8, 10:52 AM
sarah.walker2_arm.com marked 2 inline comments as done.Thu, Jan 8, 10:56 AM
In D54559#1246537, @andrew wrote:
In D54559#1246536, @sarah.walker2_arm.com wrote:
In D54559#1246501, @andrew wrote:

There should be a followup change to sys/arm64/include/ifunc.h to update DEFINE_UIFUNC. We can also use this to simplify the ifunc handlers in sys/crypto/sha2/sha512c.c and sys/crypto/sha2/sha256c.c

Can you give more detail on what you want here please?

ifunc.h has the DEFINE_UIFUNC macro where the first two arguments in the generated function signature will need an update. The other two files use DEFINE_UIFUNC and call elf_aux_info to read the HWCAPs, but could be updated after this change (and a similar change for static binaries)

Addressed in https://reviews.freebsd.org/D54598 and https://reviews.freebsd.org/D54600

libexec/rtld-elf/aarch64/reloc.c
448

Moved to header - see https://reviews.freebsd.org/D54598

libexec/rtld-elf/aarch64/rtld_machdep.h
78

See https://reviews.freebsd.org/D54598

kib added inline comments.Thu, Jan 8, 11:10 AM
libexec/rtld-elf/aarch64/reloc.c
454–461

Can this be done statically?

455
sarah.walker2_arm.com updated this revision to Diff 169317.Thu, Jan 8, 11:57 AM

Address review comments.

Harbormaster completed remote builds in B69717: Diff 169317.Thu, Jan 8, 11:57 AM
sarah.walker2_arm.com added a comment.Thu, Jan 8, 12:06 PM
In D54559#1246503, @jrtc27 wrote:

Will you also be implementing this for csu for static binaries (often forgotten, including by me in the past)?

See https://reviews.freebsd.org/D54599

jrtc27 added inline comments.Thu, Jan 8, 3:57 PM
libexec/rtld-elf/aarch64/rtld_machdep.h
80–83

Both these lines now exceed 80 chars (I suggest using tools/build/checkstyle9.pl prior to submitting patches)

jrtc27 added inline comments.Thu, Jan 8, 3:58 PM
libexec/rtld-elf/aarch64/reloc.c
454–461

Exceeds 80 chars

455

This, and the hwcap3/4 lines, are indented using spaces not tabs

sarah.walker2_arm.com updated this revision to Diff 169399.Fri, Jan 9, 4:03 PM

Address style issues.

Harbormaster completed remote builds in B69760: Diff 169399.Fri, Jan 9, 4:03 PM
kib added inline comments.Fri, Jan 9, 4:14 PM
libexec/rtld-elf/aarch64/reloc.c
455

Continuation lines should use indent +4 spaces.

libexec/rtld-elf/aarch64/rtld_machdep.h
83

No need to additionally indent, it is constant +4 spaces relatively to the start of the split line.

sarah.walker2_arm.com updated this revision to Diff 169402.Fri, Jan 9, 4:18 PM

Fix indentation issues.

Harbormaster completed remote builds in B69763: Diff 169402.Fri, Jan 9, 4:19 PM
kib accepted this revision.Sat, Jan 10, 3:42 AM
This revision is now accepted and ready to land.Sat, Jan 10, 3:42 AM
kib added inline comments.Sat, Jan 10, 3:44 AM
libexec/rtld-elf/aarch64/reloc.c
447

As a pure cosmetic, I do not think that we need to put __ifunc_arg into the private namespace. This is rtld, which is freestanding program, and which symbols are exported in controlled way. So there would be no harm from removing the '__' prefix.

sarah.walker2_arm.com updated this revision to Diff 169509.Mon, Jan 12, 9:42 AM

Move ifunc_arg from private namespace and fix const-ness.

This revision now requires review to proceed.Mon, Jan 12, 9:42 AM
Harbormaster completed remote builds in B69817: Diff 169509.Mon, Jan 12, 9:43 AM
kib accepted this revision.Mon, Jan 12, 10:42 AM
This revision is now accepted and ready to land.Mon, Jan 12, 10:42 AM
andrew accepted this revision.Mon, Jan 12, 10:54 AM

Revision Contents
Changeset List

PathSize
libexec/
rtld-elf/
aarch64/
15 lines
10 lines

Diff 169402

View Options

libexec/rtld-elf/aarch64/reloc.c

Loading...
View Options

libexec/rtld-elf/aarch64/rtld_machdep.h

Loading...