-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Move StackCheckLib To a Defined Library Class #6201
base: master
Are you sure you want to change the base?
Conversation
⚠ 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: Attn Admins: Admin Instructions:
|
@ardbiesheuvel @lgao4, I took the recently merged stack cookies PR and realized why the original commits did not use MdeLibs.dsc.inc to host the null implementation, because we cannot override it with a non-null implementation, as it is a NULL library class. Can you please review this? |
Is it possible to use a PCD and #if statement in MdePkgLibs.dsc to conditionally add the NULL lib class? If disabled, it would allow platform DSC to provide an alternate NULL lib instance. |
@mdkinney I don't think we can use a PCD here, based on my tests. When I introduce a PCD to MdePkg.dec and add
The build fails because MdeLibs.dsc.inc does not have the PCD set inside of it, looks like the failure comes from here:
Using a macro does work:
When the endpoint DSC does not set this value, the null version is used, if it does include it, then this is not included and the endpoint DSC must include a version of the StackCheckLib. To be honest, I would like to avoid adding more custom macros to the build system as it adds complexity and we might hit other weird scenarios. But if you prefer this scenario, we can go this route. |
Did you use a Feature PCD? How was the PCD declared in MdePkg.dec? Here is an example of using a PCD in a DSC file: !if gMinPlatformPkgTokenSpaceGuid.PcdTpm2Enable == TRUE |
@mdkinney I originally used a FixedAtBuild PCD, but I see the same behavior when I use a FeatureFlag PCD. Here is how I defined the PCD in MdePkg.dec:
And then if I try to use it in MdeLibs.dsc.inc:
The build fails with:
And only if I add this to MdeLibs.dsc.inc does that error go away:
But then that is setting the PCD and including that lib, which doesn't let it be overridden. I can, in the endpoint DSC (I'm using FatPkg.dsc for my testing) do a conditional with the PCD and not have it defined in the DSC (like in your example). Seemingly with the dsc.inc BaseTools is taking a different path and complains when the PCD is not defined in the dsc.inc itself. If I set the PCD in the endpoint DSC (FatPkg.dsc) I still get the build error that MdeLibs.dsc.inc does not include the PCD. So, from this I think there are only three options:
Unless you have any other ideas. I can push up a test branch to illustrate the issue with the PCD if you want. |
Thanks for the detailed investigation. My concern with the current patch is that it requires updates to all downstream DSC files that consume edk2 repo. If we can get the settings into MdeLibs.dsc.inc that preserve the existing behavior, then no downstream DSC files need updates. Only platforms that want to enable new behavior would enable the new features. |
It also seems that use of a NULL lib instance makes this harder to manage because it can not be overridden in a platform DSC file. Was a formal lib class considered? Could all the entry point lib instances be updated to depend on a new lib class so the lib dependency is only required for all module types other than SEC? |
This went through a few iterations in Project Mu and I do believe it started as a formal library class. I don't know the full history of why the change was made, I'll follow up with @spbrogan to see if there was an inherent issue with a formal library class. It may just have been trying to not be as high touch as requiring the entry point libs to need a new library class. I agree that having this as a NULL library class adds challenges. My initial testing of using a formal library class tied to the entry point libs looks promising, my simple FatPkg test went fine and the more complex MdePkg and OvmfPkg IA32,X64 builds succeeded. I only tried on GCC, let me code that up more formally and push that up, let's see if all CI passes. My initial concern was on building libraries in a DSC (say MdePkg CI build), since I was assuming that they would not link against the entry point lib until actually included in a PEIM/Driver, but that doesn't seem to be the case, as the build is passing. |
Commit tianocore@ac43bba added StackCheckLibNull to MdeLibs.dsc.inc per review requests on the PR: tianocore#5957 (comment) tianocore#5957 (comment) The PR was adapted to move specifying StackCheckLibNull in every DSC to MdeLibs.dsc.inc. However, while this works, it does not allow for a platform to use one of the other StackCheckLibs (such as StackCheckLibStaticInit) because we get a linker error by having the compiler defined stack cookie variables defined more than once (once from StackCheckLibNull in MdeLibs.dsc.inc and the other in the actual StackCheckLib implementation). Every platform must include MdeLibs.dsc.inc and there is no way to override a NULL library class. In order to avoid build breaks, this PR updates all DSCs and relevant dsc.incs to remove StackCheckLibNull for the CI build. It also changes it to the StackCheckLib library class in MdeLibs.dsc.inc. StackCheckLib is added as a LibraryClass dependency to each of the Entry Point libs. This has been tested that StackCheckLibStaticInit can be used in a package's CI instead of the null version now. Continuous-integration-options: PatchCheck.ignore-multi-package Signed-off-by: Oliver Smith-Denny <[email protected]>
443d8a6
to
6e686d9
Compare
⚠ 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: Attn Admins: Admin Instructions:
|
A formal library class and a dependency added to EntryPointLibs for this seems like a bad design. The library class doesn't have a complete API, is compiler specific, and is controlled not by dependency and usage within code but by compiler flags. Additionally, just leveraging EntryPointLib with the hope that this will catch all modules dependency is an unreliable solution. While the NULL| in MdeLibs.dsc.inc does have the unfortunate/unacceptable property of not being overridable I don't think anything with dsc.inc design should influence module and library design as the dsc.inc design has so many unfortunate flaws/confusion inducing challenges for real platforms that it shouldn't be used for anything other than a CI crutch. |
Description
Commit ac43bba
added StackCheckLibNull to MdeLibs.dsc.inc per review requests on the PR:
#5957 (comment)
#5957 (comment)
The PR was adapted to move specifying StackCheckLibNull in every DSC
to MdeLibs.dsc.inc. However, while this works, it does not allow for
a platform to use one of the other StackCheckLibs (such as
StackCheckLibStaticInit) because we get a linker error by having the
compiler defined stack cookie variables defined more than once (once
from StackCheckLibNull in MdeLibs.dsc.inc and the other in the actual
StackCheckLib implementation). Every platform must include MdeLibs.dsc.inc
and there is no way to override a NULL library class.
In order to avoid build breaks, this PR updates all DSCs and relevant
dsc.incs to remove StackCheckLibNull for the CI build. It also changes
it to the StackCheckLib library class in MdeLibs.dsc.inc. StackCheckLib
is added as a LibraryClass dependency to each of the Entry Point libs.
This has been tested that StackCheckLibStaticInit can be used in a
package's CI instead of the null version now.
Continuous-integration-options: PatchCheck.ignore-multi-package
How This Was Tested
This has been tested that StackCheckLibStaticInit can be used in a package's CI instead of the null version now.
Integration Instructions
If a platform would like to include a real version of the StackCheckLib, it should include:
For the current details, see https://github.com/tianocore/edk2/blob/HEAD/MdePkg/Library/StackCheckLib/Readme.md.