Opened 11 years ago
Closed 11 years ago
#10800 closed bug (fixed)
UEFI: assembly files need to take calling convention into account
Reported by: | jessicah | Owned by: | axeld |
---|---|---|---|
Priority: | normal | Milestone: | R1 |
Component: | System/libroot.so | Version: | R1/Development |
Keywords: | Cc: | ||
Blocked By: | Blocking: | ||
Platform: | All |
Description
For UEFI, the calling conventions on x86_64 follow the Microsoft calling conventions, which are not compatible with Haiku's calling conventions.
As a result, existing functions written in assembly fail to run as expected. E.g. byteorder.S currently returns 0 for all of the byteswap routines, which are used elsewhere, such as the definitions for the B_BENDIAN_xyz macros, and the ntohs function.
References: Wikipedia, OSDev Wiki
Attachments (2)
Change History (13)
comment:1 by , 11 years ago
comment:2 by , 11 years ago
The best solution would be to use inline assembly in C code and let GCC take care of calling convention. Moreover, many of such asm functions should be inlined wherever possible since they usually contain only few instructions. Use of GCC builtins also should be considered as it would allow some compile time optimizations, e.g. when changing byte order of the value that is known at compile time.
comment:3 by , 11 years ago
Hey,
I am not very much in favor of GCC inlining. I consider C files to be pretty "platform-agnostic", however adding ASM inline in those files breaks this. I am also not comfortable with the inlining syntax as it is really messy in my opinion.
Letting gcc take care of the calling convention is however very appealing... What is your take on this jessicah?
comment:4 by , 11 years ago
My vote goes for using the gcc builtins if available. They are listed here (at tne bottom of the page): http://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html
There's nothing for float and doubles, however.
comment:5 by , 11 years ago
I'm happy for the builtins to be used. I also see no problem with using inline assembly. If it means less duplicated code and/or conditional code, then that, in my view, is a win.
comment:6 by , 11 years ago
When I looked into this I wanted MS calling conventions to be used for UEFI calls only. We set GNU_EFI_USE_MS_ABI and that makes HAVE_USE_MS_ABI 1 to be set in efibind.h. So if we use MS calling conventions all the time I think we are doing something wrong. Note that if our main function, called from start_func.S isn't marked as EFIAPI we might need to fix the asm or mark it EFIAPI.
So that might be worth investigating.
#if defined(GNU_EFI_USE_MS_ABI) #if defined(__GNUC__) && (__GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 7)) #define HAVE_USE_MS_ABI 1 #else #error Compiler is too old for GNU_EFI_USE_MS_ABI #endif #endif
#ifndef EFIAPI // Forces EFI calling conventions reguardless of compiler options #ifdef _MSC_EXTENSIONS #define EFIAPI __cdecl // Force C calling convention for Microsoft C compiler #elif defined(HAVE_USE_MS_ABI) // Force amd64/ms calling conventions. #define EFIAPI __attribute__((ms_abi)) #else #define EFIAPI // Substitute expresion to force C calling convention #endif #endif
by , 11 years ago
Attachment: | 0001-Replace-arch-dependent-integer-byteswap-routines-wit.patch added |
---|
comment:7 by , 11 years ago
I'm not sure if this is the right approach to take. Can we safely assume GCC has support for these on all of our supported/targeted platforms? And is this the right place to do that? At the very least, they now become single inlined instructions with this change rather than function calls, and resolves the issue I was seeing with the EFI bootloader.
Of course, it'll need to be cleaned up to also remove the platform dependent code where necessary and simplify affected Jamfiles.
comment:8 by , 11 years ago
I think this would be a good question on the haiku-dev mailing list. As I wrote in my previous post I still think that approach should work, however I havn't had the time to look at the build-flags yet and what needs to be done.
by , 11 years ago
Attachment: | 0001-Use-GCC-builtins-for-byte-swapping.-Fixes-10800.patch added |
---|
comment:9 by , 11 years ago
patch: | 0 → 1 |
---|
comment:11 by , 11 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
I've filed this, and hoping it can be a task for choupy to undertake.