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

Fix CFG on our static-lib-only DLL projects #16056

Merged
merged 1 commit into from
Sep 29, 2023

Conversation

DHowett
Copy link
Member

@DHowett DHowett commented Sep 28, 2023

Control Flow Guard requires both linker and compiler flags.

It turns out that the MSVC build rules determine whether to link with CFG based on whether it compiled anything with CFG.

It also turns out that when you don't compile anything (such as in our DLL projects that only consume a static library!), the build rules can't guess whether to link with CFG.

Whoops.
We need to force it.

(cherry picked from commit 447841463bfda0f5dc4476bfe0ae324e1f5c54b1)
@@ -101,6 +101,13 @@
<PreferredToolArchitecture>x64</PreferredToolArchitecture>
<EnableHybridCRT Condition="'$(EnableHybridCRT)'=='' and '$(ConfigurationSupportsHybridCRT)'=='true'">true</EnableHybridCRT>
<UseCrtSDKReferenceStaticWarning Condition="'$(EnableHybridCRT)'=='true'">false</UseCrtSDKReferenceStaticWarning>
<!--
We must set this to True for projects that do not contain any ClCompile entries.
The Link task guesses whether to enable CFG based on ClCompile->ControlFlowGuard.

Choose a reason for hiding this comment

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

Does it use the metadata of items in the Sources parameter for that? From the Link task documentation, I'd have expected the ObjectFiles parameter instead, but that one has type String[] and thus cannot carry any metadata.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, it uses the ClCompile item metadata ☹️

Microsoft\VC\v170\Microsoft.CppCommon.targets
1091:      <LinkControlFlowGuard Condition="'$(LinkControlFlowGuard)' == '' and '@(ClCompile)' != '' and '@(ClCompile->AnyHaveMetadataValue('ControlFlowGuard', 'Guard'))' == 'true'">true</LinkControlFlowGuard>

Choose a reason for hiding this comment

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

Oh that's not the Link task doing the guessing then.

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right of course, and you caught me out on describing it improperly :)

@DHowett DHowett merged commit 1b143e3 into main Sep 29, 2023
@DHowett DHowett deleted the dev/duhowett/fix-some-more-build-stuff branch September 29, 2023 18:25
DHowett added a commit that referenced this pull request Sep 29, 2023
Control Flow Guard requires both linker and compiler flags.

It turns out that the MSVC build rules determine whether to _link_ with
CFG based on whether it compiled anything with CFG.

It also turns out that when you don't compile anything (such as in our
DLL projects that only consume a static library!), the build rules can't
guess whether to link with CFG.

Whoops.
We need to force it.

(cherry picked from commit 1b143e3)
Service-Card-Id: 90688104
Service-Version: 1.18
DHowett added a commit that referenced this pull request Sep 29, 2023
Control Flow Guard requires both linker and compiler flags.

It turns out that the MSVC build rules determine whether to _link_ with
CFG based on whether it compiled anything with CFG.

It also turns out that when you don't compile anything (such as in our
DLL projects that only consume a static library!), the build rules can't
guess whether to link with CFG.

Whoops.
We need to force it.

(cherry picked from commit 1b143e3)
Service-Card-Id: 90688105
Service-Version: 1.19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

4 participants