zfs
zfs copied to clipboard
openzfs
Unify linux assembler files
Make both Linux and Windows use the same assembler files.
This is only the Linux part of the work, the Windows changes are here: https://github.com/openzfsonwindows/openzfs/pull/188/commits
Motivation and Context
Avoiding code-duplication is sometimes desirable, the question is, is this one of those times?
Description
Turns out clang (gnu) has a way to tell C to call an assembler function using sysv_abi (and handle the register parameter order, and pushing on-volatile registers on the stack) on Windows, thereby, making it possible to use the same Unix style assembler files.
Linux will have to accept some changes, like always using ENTRY_NP and similar macros.
I believe macOS could also be done, as I consider Windows platform to be the most "foreign" to the Unix way.
It's not complete, this only handles x86_64. But in future, ARM64 will probably also
attempted. ASMABI is #define-d as empty initially, for all the non-x86 platforms, and
changed inside #ifdef x86. The other way to do it, is to list all platforms in an #ifdef each.
I moved asm_linkage.h from module/icp to include/spl purely for lua/setjmp (the only .S file outside icp). By far the hardest part of this PR. But that could be beneficial for future .S work.
Please advise if this is desirable at all.
I don't care about the macro names, so specify what is better (name space?)
How Has This Been Tested?
Types of changes
- [ ] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [x] Performance enhancement (non-breaking change which improves efficiency)
- [x] Code cleanup (non-breaking change which makes code smaller or more readable)
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
- [ ] Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
- [ ] Documentation (a change to man pages or other documentation)
Checklist:
- [x] My code follows the OpenZFS code style requirements.
- [ ] I have updated the documentation accordingly.
- [x] I have read the contributing document.
- [ ] I have added tests to cover my changes.
- [x] I have run the ZFS Test Suite with this change applied.
- [x] All commit messages are properly formatted and contain
Signed-off-by.
Sheesh getting the includes to work after moving the header file is a pain. It's mostly because there is 1 set of assembler files (setjmp) in ZFS.
Please change the commit message to say "assembly files". We do not call our C files "compiler files", so saying "assembler files" is inconsistent.
Somewhat interesting linguistically, when C64 came to Europe, we are started calling it assembler for everything, to the point where C64-wiki as to start with Assembly language, also incorrectly referred to as assembler. But that's probably mostly down to ESOL. It's certainly stuck for me.
This seems to have not given any attention to our ARM and POWER assembly. Was that intentional?
Yep - just doing Intel, with the idea that we will just add parallel includes to ia32/asm_linkage.h in future. I may also to aarch64 if I can get the M1 to assemble "anything at all". Beyond that, I have no other archs for Windows/macOS.
Please change the commit message to say "assembly files". We do not call our C files "compiler files", so saying "assembler files" is inconsistent.
Somewhat interesting linguistically, when C64 came to Europe, we are started calling it assembler for everything, to the point where
C64-wikias to start with Assembly language, also incorrectly referred to as assembler. But that's probably mostly down to ESOL. It's certainly stuck for me.
That explains why I heard that the translation into Polish was literally "assembler".
Does the test
Test: /usr/share/zfs/zfs-tests/tests/functional/pyzfs/pyzfs_unittest (run as root) [18:46] [KILLED]
have anything to do with crypto, is it something to look into?
It looks like a previous test failure prevented pyzfs_unittest from running properly. It doesn't look like it had anything to do with crypto, I'll resubmit the Ubuntu 20.04 build.
Yeah, this is a good point. We could take align out of ENTRY_NP() and keep it separate, or as you suggest, add it to macro. But since it's an illumos macro, perhaps separate is better. One annoying thing is, sometimes it's literal "16" and sometimes its bit shift "4" - but perhaps the macro can handle that. But it does need to be a macro, since macos takes 2 args:
.align 6, 0x90
I'll whip up your idea and see how it looks
We could take align out of ENTRY_NP() and keep it separate
ENTRY_NP() is used in other places as well IIRC, my suggestion was to do something like
#define ENTRY_NP_WITH_ALIGN(x,a) \
.text; \
.align a; \
.globl x; \
x:
and use it in places where the alignment isn't 16.
One annoying thing is, sometimes it's literal "16" and sometimes its bit shift "4"
GAS has two directives, .align <bytes> and .p2align <power of two>. IIRC llvm only has .p2align, so maybe it would be better to use .p2align. Both take two additional arguments, the first of which is the fill value. In the text segment this would be NOP per default, and the opcode of NOP is 0x90. So .align 16 and .align 16, 0x90 are equivalent.
.align 6, 0x90
This syntax is new to me, did you mean .p2align 6, 0x90 by any chance? Or is this something Windows specific?
Well, actually .align 6 is llvm syntax and would translate to .align 64 in GAS. The good new is that llvm also has a .p2align directive, so the Mac port would profit from using .p2align everywhere.
macOS has the 2 argument align, you specify the value you want in there, 0x90 is NOP or something. https://github.com/openzfsonosx/openzfs/blob/development/module/icp/asm-x86_64/os/macos/aes/aes_amd64.S#L696
but I see I've left the align problem alone for now, when this PR lands, I'll do a PR for macOS, to handle the align macro.
If one of align, or p2align, works on macOS, I'll just change it to that, otherwise a new MACRO
OK so, I don't see anything using MCOUNT, whatever that is. I think maybe it was something mdb related for illumos kernels? Either way, we are not using it. (ok, GPROF)
https://github.com/illumos/illumos-gate/blob/9ecd05bdc59e4a1091c51ce68cce2028d5ba6fd1/usr/src/uts/sparc/sys/asm_linkage.h#L66-L76
So if we are moving away from illumos macros, I just went with ENTRY_ALIGN() macro. Lemme know if anyone have strong opinions on names.
@lundman When you do macOS you could try .balign first. It has the least impact (same syntax as .align on Linux) and should work fine wit one argument on macOS. I tried it with GAS and clang on Linux, and they behave identical, padding with nops inside the .text segment and with zeros inside the .data segment .
@lundman @AttilaFueloep thanks for the additional review and refactoring! I'll get this pulled in.