Skip to content
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

Bug xxx crypto pkg openssl clang asm file fix #10634

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mdkinney
Copy link
Member

@mdkinney mdkinney commented Jan 16, 2025

Description

Update the CryptoPkg to support CLANGPDB and CLANGDWARF from
both Windows and Linux host environments.

  • Add PcdOpensslLibAssemblySourceStyleNasm to select the correct
    optimized assembly source style for OpensslLib for IA32/X64.
    NASM style is for MSFT and CLANGPDB. GAS style is for GCC.
    Use this PCD in OpensslLibAccel.inf and OpensslLibFullAccel.inf
    to select between .nasm and .S files.
  • Add missing intrinsic functions required by CLANG IA32/X64 builds.
    • __ashlti3
    • __lshrdi3
  • Disable warning -Wno-error=unused-function for CLANG build
    compatibility
  • Set -D OPENSSL_NO_INLINE_ASM for CLANG build compatibility
  • Update TestBaseCryptLib to split out the implementation of
    main() into its own C file that is only use for host-based
    unit tests. This is due to CLANGPDB for host environments
    injecting a __main() call that can only be resolved in host
    based builds that link against host libraries.
  • Update Configure.py to update IA32/X64 [Sources] sections
    with feature flag expressions using the new PCD
    PcdOpensslLibAssemblySourceStyleNasm.

Align CLANG and GCC linker scripts to not discard COMMON
sections for CLANG builds. If COMMON sections are discarded,
then global variables from optimized OpensslLib assembly
source files are removed and results in symbol not found
errors.

  • Breaking change?
    • Breaking change - Does this PR cause a break in build or boot behavior?
    • Examples: Platform DSC files that use OpesslLibAccel.inf or OpensslLibFullAccel.inf must add the following statement to select the correct asm source files:
  !include CryptoPkg\CryptoPkgFeatureFlagPcds.dsc.inc
  • Impacts security?
    • Security - Does this PR have a direct security impact?
    • Examples: Crypto algorithm change or buffer overflow fix.
  • Includes tests?
    • Tests - Does this PR include any explicit test code?
    • Examples: Unit tests or integration tests.

How This Was Tested

Build CryptoPkg on Windows using VS2022, CLANGPDB and CLANGDWARF for IA32/X64 for DEBUG/RELEASE/NOOPT
Build CryptoPkg on Linux using GCC, GCC5, CLANGPDB and CLANGDWARF for IA32/X64 for DEBUG/RELEASE/NOOPT

Integration Instructions

Platform DSC file should add the following to support all OpensslLib instances

  !include CryptoPkg\CryptoPkgFeatureFlagPcds.dsc.inc

@github-actions github-actions bot added the impact:breaking-change This change breaks existing APIs impacting build or boot. label Jan 16, 2025
@mdkinney mdkinney requested review from liyi77 and jyao1 January 16, 2025 03:15
@mdkinney mdkinney force-pushed the Bug_xxx_CryptoPkg_Openssl_Clang_AsmFile_Fix branch from a85ae47 to 8ff353d Compare January 16, 2025 05:29
@mdkinney mdkinney requested a review from lgao4 January 16, 2025 05:55
@mdkinney mdkinney force-pushed the Bug_xxx_CryptoPkg_Openssl_Clang_AsmFile_Fix branch 3 times, most recently from 009ca2a to 93f4874 Compare January 21, 2025 16:22
@mdkinney mdkinney marked this pull request as ready for review January 21, 2025 17:55
@jyao1
Copy link
Contributor

jyao1 commented Mar 3, 2025

would you please fix the conflict?

@mdkinney mdkinney force-pushed the Bug_xxx_CryptoPkg_Openssl_Clang_AsmFile_Fix branch from 93f4874 to b6bd181 Compare March 3, 2025 16:41
@mdkinney
Copy link
Member Author

mdkinney commented Mar 3, 2025

PR #10687 was merged that consolidated the CLANG and GCC linker scripts to the GccBase.lds that was not discarding the COMMON sections. The commit in this PR to prevent COMMON section removal has been removed and this PR has been rebased.

@mdkinney mdkinney requested a review from nate-desimone March 3, 2025 16:46
Update the CryptoPkg to support CLANGPDB and CLANGDWARF from
both Windows and Linux host environments.

* Add PcdOpensslLibAssemblySourceStyleNasm to select the correct
  optimized assembly source style for OpensslLib for IA32/X64.
  NASM style is for MSFT and CLANGPDB. GAS style is for GCC.
  Use this PCD in OpensslLibAccel.inf and OpensslLibFullAccel.inf
  to select between .nasm and .S files.
* Add intrinsic functions required by CLANG IA32/X64 builds.
  * __ashlti3
  * __lshrdi3
* Disable warning -Wno-error=unused-function for CLANG build
  compatibility
* Set -D OPENSSL_NO_INLINE_ASM for CLANG build compatibility
* Update TestBaseCryptLib to split out the implementation of
  main() into its own C file that is only use for host-based
  unit tests. This is due to CLANGPDB for host environments
  injecting a __main() call that can only be resolved in host
  based builds that link against host libraries.
* Update Configure.py to update IA32/X64 [Sources] sections
  with feature flag expressions using the new PCD
  PcdOpensslLibAssemblySourceStyleNasm.

Signed-off-by: Michael D Kinney <[email protected]>
@mdkinney mdkinney force-pushed the Bug_xxx_CryptoPkg_Openssl_Clang_AsmFile_Fix branch from b6bd181 to 9818368 Compare March 4, 2025 05:56
@mdkinney
Copy link
Member Author

mdkinney commented Mar 4, 2025

@jyao1 @liyi77 I pushed one more update that was required to build CryptoPkg unit tests for CLANGPDB NOOPT on both Windows and Linux. The file CryptoPkg/Test/UnitTest/Library/BaseCryptLib/UnitTestMain.c containes main() to support host based unit test builds. This function is unused when building target based unit tests. However, when CLANGPDB NOOPT profile is used, unused functions are not optimized away, and the clang compiler has special treatment for main(). It always injects a call to __main() that is a function implemented in the clang library that handles global constructors. That causes an unresolved external when building target based unit tests.

The fix was to move main() into a new files called UnitTestMainHost.c so the clang compiler does not see a function called main() when building target based unit tests.

@liyi77
Copy link
Contributor

liyi77 commented Mar 4, 2025

This change looks fine to me, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact:breaking-change This change breaks existing APIs impacting build or boot.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants