TEST COMMIT: Update OcCompilerIntrinsicsLib so that it can build the network stack #555
Conversation
mikebeaton
requested review from
vit9696,
mhaeuser,
savvamitrofanov and
MikhailKrichanov
| CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf | ||
|
|
There was a problem hiding this comment.
Were these lines ever needed? I hope I'm not missing something, but AFAICT not. (This commit is relative to the network boot PR, but these lines were present before (further up in the file), referring to the OcCompilerIntrinsicsLib, before the network boot commit.)
There was a problem hiding this comment.
Okay, based on feedback from #556, I believe there is indeed no point in including these lines for another package's library, as this is now - but it would have made sense to include them before, when they were our package's library.
|
|
||
| GLOBAL_REMOVE_IF_UNREFERENCED BOOLEAN gOcCompilerIntrinsicsLib; | ||
|
|
||
| #if defined (MDE_CPU_IA32) && defined (__clang__) && defined (__apple_build_version__) && __apple_build_version__ < 11000000 | ||
| #if defined (MDE_CPU_IA32) && defined (__clang__) && defined (__APPLE__) |
There was a problem hiding this comment.
I did need to remove the version test, __udivdi3 was getting generated by code in TlsDxe when compiling using Xcode 15.0.1
mikebeaton
force-pushed
the
network-boot
branch
from
5c94885
to
31464d1
Compare
mikebeaton
force-pushed
the
intrinsics-test
branch
from
2253496
to
7ff52a9
Compare
|
Closing this, as I believe the investigations done here more or less establish that 'the one' intrinsics lib to use would be the CryptoLib one, not an updated OcCompilerIntrinsicsLib. (Though this may end up meaning some update of the CryptoLib IntrinsicsLib, possibly providing ARM intrinsics too, if that gets produced in EDK 2.) |
@vit9696 - This is a test commit to show my current thinking on the intrinsics library.
This commit has the changes required to update our intrinsics library so that it can build the network stack. Note that we end up importing several things from the CryptoPkg IntrinsicLib (that is, the EDK 2 intrinsics library which I have used entirely instead of ours in the versions so far of the network boot PR).
This makes me wonder whether the CryptoPkg IntrinsicLib might not be the best one to base on (or rather just use as is, since it already builds our packages and the rest of EDK 2 fine) for a shared intrinsics lib, rather than ours, because the CryptoPkg library already covers the imported things and multiple other cases. (Albeit that, as you pointed out, the EDK 2 CryptoPkg IntrinsicLib has some bugs which we already had to fix.)
EDIT: Okay, it seems this commit has only the changes to our intrinsics library necessary to build the network stack on Xcode - as can be seen in the CI, we would need to import yet more of what is already in CryptoPkg (EDK 2) IntrinsicLib to update our intrinsics lib enough to build the network package on all the architectures. Which I think probably just confirms the point above.
My next step of reasoning, if the CryptoPkg IntrinsicLib is the best intrinsics lib to use, is that possibly moving it to MdePkg is not such a good idea. This is because it is already quite widely used as an intrinsics library all around EDK 2, despite by its name sounding as if it wouldn't be:
Another edit: I realised most of the above IntrinsicLib definitions are for use within CryptoPkg. :-/ But not all of them, as below. Which maybe allows me to still make the same point, but more weakly. i.e. it doesn't really seem worth moving the library out of CryptoPkg, if it's already at least somewhat normal to use it as an intrinsics lib outside that context. (But maybe I should be saying instead, it's been used outside twice - the fact that we want to use it outside a third time means it should be moved...?!) It's definitely that much easier to leave it where it is (assuming it's even the one we're going to use), but let me know.