edk2 icon indicating copy to clipboard operation
edk2 copied to clipboard

MdePkg: Move CompilerIntrinsicsLib and ArmSoftFloatLib from ArmPkg

Open os-d opened this issue 1 year ago • 4 comments

Description

As per the emailed RFC in https://edk2.groups.io/g/devel/topic/rfc_move/107675828, this patch moves CompilerIntrinsicsLib and ArmSoftFloatLib from ArmPkg to MdePkg as these libraries provide industry standard compiler intrinsics and ARM floating point functionality, respectively.

This aligns with the goal of integrating ArmPkg into existing packages: https://bugzilla.tianocore.org/show_bug.cgi?id=4121.

The newly placed libs are added to MdeLibs.dsc.inc as every DSC that builds ARM/AARCH64 needs CompilerIntrinsicsLib and every module that builds pieces of OpenSSL for ARM needs SoftFloatLib. The old location is removed from every DSC in edk2 in this commit also to not break bisectability with minimal hoop jumping.

Continuous-integration-options: PatchCheck.ignore-multi-package

  • [x] Breaking change?
    • Breaking change - Does this PR cause a break in build or boot behavior?
    • Examples: Does it add a new library class or move a module to a different repo.
  • [ ] 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

No functional change.

Integration Instructions

Any DSCs referencing ArmSoftFloatLib or CompilerIntrinsicsLib should remove those lines and use the version specified in MdeLibs.dsc.inc.

os-d avatar Aug 05 '24 18:08 os-d

WARNING: Cannot add some reviewers: A user specified as a reviewer for this PR is not a collaborator of the repository. Please add them as a collaborator to the repository so they can be requested in the future.

Non-collaborators requested:

  • @javagedes
  • @Zclarkwilliams

Attn Admins:

  • @jljusten
  • @lgao4
  • @mdkinney
  • @vincent-j-zimmer
  • @jkmathews
  • @miki-intel-work

Admin Instructions:

  • Add the non-collaborators as collaborators to the appropriate team(s) listed in teams
  • If they are no longer needed as reviewers, remove them from Maintainers.txt

@mdkinney @leiflindholm @ardbiesheuvel following up from the RFC email I have put up this PR, please review when you have a chance. I did all of the moves in single commits for simplicity and not breaking bisectability. If you feel there is a better approach, please let me know.

os-d avatar Aug 07 '24 19:08 os-d

When adding a compiler intrinsics library to MdePkg (which makes sense to me) we might move over the x86 bits in CryptoPkg/Library/IntrinsicLib too.

kraxel avatar Aug 21 '24 13:08 kraxel

When adding a compiler intrinsics library to MdePkg (which makes sense to me) we might move over the x86 bits in CryptoPkg/Library/IntrinsicLib too.

That makes sense to me, too, although we may not want to make those intrinsics widely available. My understanding of the CryptoPkg InstrinsicLib is that is just to support OpenSSL, which obviously can't control the use of patterns that result in compiler intrinsics. For ARM/AARCH64, we do not have any way to avoid the compiler adding intrinsics, hence making this widely available. I'll let @mdkinney, @leiflindholm, and @ardbiesheuvel speak to that, though. If we do move over IntrinsicLib, I would propose to do that in a follow on patch, to not make this more complicated.

os-d avatar Aug 21 '24 17:08 os-d

WARNING: Cannot add some reviewers: A user specified as a reviewer for this PR is not a collaborator of the repository. Please add them as a collaborator to the repository so they can be requested in the future.

Non-collaborators requested:

  • @javagedes

Attn Admins:

  • @jljusten
  • @lgao4
  • @mdkinney
  • @vincent-j-zimmer
  • @jkmathews
  • @miki-intel-work

Admin Instructions:

  • Add the non-collaborators as collaborators to the appropriate team(s) listed in teams
  • If they are no longer needed as reviewers, remove them from Maintainers.txt

PR can not be merged due to conflict. Please rebase and resubmit

mergify[bot] avatar Aug 30 '24 08:08 mergify[bot]

@ardbiesheuvel @leiflindholm @mdkinney , I have dropped moving ArmSoftFloatLib in this PR (and updated the title and description) as Ard is removing the submodule entirely. Please review when you have a chance.

os-d avatar Sep 05 '24 16:09 os-d

@lgao4 , I made your requested change, can you please re-review?

os-d avatar Sep 08 '24 15:09 os-d

@mergify requeue

ardbiesheuvel avatar Sep 12 '24 13:09 ardbiesheuvel

requeue

☑️ This pull request is already queued

mergify[bot] avatar Sep 12 '24 13:09 mergify[bot]