ifunc resolver is loosely based on the equivalent resolver for amd64.
Sponsored by: Arm Ltd
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Skipped - Unit
Tests Skipped - Build Status
Buildable 69762 Build 66645: arc lint + arc unit
Event Timeline
Comment Actions
I'm not sure if it is a good idea to copy the amd64 design pattern for ARM. We have ARCHLEVEL support there mainly to opt out of using SIMD, but it doesn't seem like that is salient point on AArch64.
I would strongly favour a refactor that does target selection just for this one function only, with no ARCHLEVEL framework.
You can write the resolve in C in a separate translation unit if needed (though it should be easy to do in assembly).
If a framework is desired, it should probably follow ARM architecture levels, so 8.0, 8.1, ..., 9.0, ... instead of individual extensions.
Please also document this change in simd(7), you can add a new key (maybe M) to the table to mean “function uses MOPS.”
| lib/libc/aarch64/string/aarch64_string_resolver.c | ||
|---|---|---|
| 28 ↗ | (On Diff #169315) | AFAIK we always set _IFUNC_ARG_HWCAP, so unless Linux compat is desired, we don't need to test for this. |
| sys/arm64/include/asm.h | ||
| 236 ↗ | (On Diff #169315) | We don't need CNAME anymore as ELF targets don't decorate C symbols. Please don't add it to platforms where it won't be needed. |
| lib/libc/aarch64/aarch64_archfuncs.h | ||
|---|---|---|
| 31 ↗ | (On Diff #169315) | Tabs for these |
| 47 ↗ | (On Diff #169315) | What's this needed for? (If needed, it always goes first, see style(9)) |
| 78 ↗ | (On Diff #169315) | The amd64 version puts an underscore between func and level for all these, is there a reason not to for arm64? |
| 88 ↗ | (On Diff #169315) | BTI_C? DTRACE_NOP at least is only for the kernel. Though why not just use LENTRY for the whole thing? |
| 91 ↗ | (On Diff #169315) | LEND, it's local not global (though in practice END is just defined to LEND) |
| lib/libc/aarch64/string/aarch64_string_resolver.c | ||
| 1 ↗ | (On Diff #169315) | Please read style(9) for how to organise includes |
| 10 ↗ | (On Diff #169315) | Tabs not spaces, but why isn't this using machine/ifunc.h's definitions for all these? |
| 21 ↗ | (On Diff #169315) | Wrapped arguments in definitions use a 4 space indent, rather than aligning with the previous line's |
| 35 ↗ | (On Diff #169315) | Exceeds 80 chars |
| lib/libc/aarch64/string/memcpy.S | ||
| 12 | Why are these needed?.. | |
| 17 | Why the double spacing? | |
| lib/libc/aarch64/string/memset.S | ||
| 5 | Tabs | |
| 16 | Tabs | |
| 19 | Tabs | |
| sys/arm64/include/asm.h | ||
| 236 ↗ | (On Diff #169315) | Tab after #define. Though can we not introduce CNAME? If you really want something there's the underused _C_LABEL, but it's pointless obfuscation for architectures that never have and never will do C symbol name mangling like Mach-O does. |
Comment Actions
This doesn't build for me:
lib/libc/aarch64/string/memcpy_resolver.c:11:6: error: use of undeclared identifier 'ifunc_arg'
11 | if (ifunc_arg->_hwcap2 & HWCAP2_MOPS)
| ^
1 error generated.
*** Error code 1Comment Actions
I think this is because our ifunc framework does not currently support fetching hwcap2 from the second argument and we have not yet decided how we want to support that (I dimly recall having discussed this with @jrtc27 at EuroBSDcon 2024). It might be easiest to grab hwcap2 using elf_aux_info() for now.
Comment Actions
No objections to this design from my side, though I can't test further on account of not having any HW with MOPS support (donations welcome :-)