-
Notifications
You must be signed in to change notification settings - Fork 104
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update MathLib.c to fix DisplayEngineDxe not compiling on ARM #351
Conversation
Make it so it can compile on 32-bit ARM architectures.
@@ -218,7 +218,7 @@ sqrt_d ( | |||
IN CONST double input | |||
) | |||
{ | |||
UINT64 firstGuess = (UINT64)input; | |||
UINT32 firstGuess = (UINT32)input; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be missing something on this, but wouldn't this cast cause the upper 32 bit getting truncated as a double should be 64bit in width?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is exactly why I asked for a review, while this fixed building, it's not the best solution, hence I need someone with experience to look at this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sonic011gamer, looking at your build log __aeabi_ul2d
looks like a compiler built-in function that is not defined. We define those for Arm an AARCH64 in https://github.com/microsoft/mu_basecore/tree/release/202302/MdePkg/Library/CompilerIntrinsicsLib.
That usually gets linked to every module by applying it like this in a DSC:
NULL|MdePkg/Library/CompilerIntrinsicsLib/ArmCompilerIntrinsicsLib.inf
Could you try defining that function there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like there is a definition in ArmSoftFloatLib
in https://github.com/microsoft/mu_silicon_arm_tiano/tree/release/202302/ArmPkg/Library/ArmSoftFloatLib. Are you linking that against this module?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like there is a definition in
ArmSoftFloatLib
in https://github.com/microsoft/mu_silicon_arm_tiano/tree/release/202302/ArmPkg/Library/ArmSoftFloatLib. Are you linking that against this module?
yes, I am building with this module and the one above mentioned included in the .dsc
This PR has been automatically marked as stale because it has not had activity in 60 days. It will be closed if no further activity occurs within 7 days. Thank you for your contributions. |
This pull request has been automatically been closed because it did not have any activity in 60 days and no follow up within 7 days after being marked stale. Thank you for your contributions. |
Description
This change was made for fixing DisplayEngineDxe giving a linking error on 32-bit ARM architecture because of sqrt_d in MathLib. It fixes issue #350
flow, or firmware?
validation improvement, ...
in build or boot behavior?
a function in a new library class in a pre-existing module, ...
outside direct code modifications (and comments)?
on an a separate Web page, ...
How This Was Tested
<Please describe the test(s) that were run to verify the changes.>
Integration Instructions
N/A