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

IntrinsicLib: Move from CryptoPkg to MdePkg #67

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

Conversation

mikebeaton
Copy link
Contributor

@mikebeaton mikebeaton commented Sep 7, 2024

This PR suggests moving IntrinsicLib from CryptoPkg to MdePkg.

This library is already used by two clients in audk other than CryptoPkg: OvmfPkg/Library/BhyveFwCtlLib (not added by us, present in EDK 2) and UefiCpuPkg/Library/CpuPageTableLib (added by us, not present in EDK 2).

It should be noted that OpenCorePkg and OpenDuetPkg can be built and run successfully using CryptoPkg IntrinsicLib entirely instead of OcCompilerIntrinsicsLib. On the other hand OcCompilerIntrinsicsLib is quite far from being able to compile and run CryptoPkg, as shown here (it would basically require importing large chunks of CryptoPkg IntrinsicLib before it could do so).

So the purpose of this move is to end up with one shared IntrinsicLib, in a reasonable shared place such as MdePkg, which can be used to build the EDK 2 network stack and OpenCore/OpenDuet.

EDIT: The reason for starting on this change in the first place is that our OcCompilerIntrinsicsLib is linked globally in OpenCorePkg, but defines some of the same functions as CryptoPkg IntrinsicLib. Therefore if we want to compile TlsDxe (which uses CryptoPkg IntrinsicsLib) for https support in the network stack, we get a conflict between the two libraries. I haven't looked in detail at why we need an intrinsic library globally, although removing it gives linking problems in debug print calls, which obviously are used globally; I haven't investigated further yet. From a brief further investigation, the intrinsics provided by our smaller intrinsics lib (such as memset) seem reasonable, and not easily (or at all?) avoidable. They end up used by our debug library (which is obviously enough to make sense of the global linkage already), but definitely also other places. Some libraries in EDK 2 also provide their own (worth looking at that link for how they handle the same conflict; some other packages provide it without handling the conflict, presumably they don't also use CryptoPkg). But there doesn't seem to be any problem, in terms of correct functionality, with us using the larger CryptoPkg (or MdePkg) IntrinsicLib instead.


Some reasons to be somewhat wary of making this move are:

If we do make this change (or similar) then I assume we would eventually completely remove OcCompilerIntrinsicsLib, if it is no longer being used then I am not sure if we would want to keep OcCompilerIntrinsicsLib, even though we are not using it. Or whether we would want to still use it for OpenDuetPkg, even if we stop using it for OpenCorePkg.

@@ -373,7 +373,6 @@
CryptoPkg/Library/MbedTlsLib/MbedTlsLib.inf
CryptoPkg/Library/MbedTlsLib/MbedTlsLibFull.inf
CryptoPkg/Library/BaseCryptLibNull/BaseCryptLibNull.inf
CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf
Copy link
Contributor Author

@mikebeaton mikebeaton Sep 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved to MdePkg. (Note that assumed purpose of libraries in Components section, like this - even though packages also build fine without these lines - is something like guaranteeing a test build of the library, whether or not it's consumed by the package which provides it; see comments of closed PR acidanthera/OpenCorePkg#556.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

1 participant