Skip to content

Add Control Flow Guard support #304

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

Merged
merged 4 commits into from
Oct 1, 2022

Conversation

alvinhochun
Copy link
Contributor

Adds the option to build with Control Flow Guard support. This is disabled by default and currently only enabled for the scheduled GitHub Actions build because it requires latest LLVM 16 and mingw-w64. We should change the default to enabled for the next release.

Closes #301

Copy link
Owner

@mstorsjo mstorsjo left a comment

Choose a reason for hiding this comment

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

Looking extremely good overall, thanks! I've only got a couple minor details to discuss.

@alvinhochun alvinhochun force-pushed the alvin/cfguard-scheduled-only branch from 3c06b8f to 584ddde Compare September 30, 2022 12:03
@alvinhochun alvinhochun force-pushed the alvin/cfguard-scheduled-only branch from 584ddde to fb92119 Compare September 30, 2022 12:06
@alvinhochun
Copy link
Contributor Author

Thanks, I have addressed the comments. Let's wait and see how the actions https://github.com/alvinhochun/llvm-mingw/actions/runs/3158626818 and https://github.com/alvinhochun/llvm-mingw/actions/runs/3158634041 go.

@mstorsjo
Copy link
Owner

Thanks, I have addressed the comments. Let's wait and see how the actions https://github.com/alvinhochun/llvm-mingw/actions/runs/3158626818 and https://github.com/alvinhochun/llvm-mingw/actions/runs/3158634041 go.

Thanks! Can you kick off another actions run, with a branch on top of this which bumps llvm/mingw to versions that support it, and which enables it by default even for non-scheduled runs, to see it in action?

@alvinhochun
Copy link
Contributor Author

Thanks, I have addressed the comments. Let's wait and see how the actions https://github.com/alvinhochun/llvm-mingw/actions/runs/3158626818 and https://github.com/alvinhochun/llvm-mingw/actions/runs/3158634041 go.

Thanks! Can you kick off another actions run, with a branch on top of this which bumps llvm/mingw to versions that support it, and which enables it by default even for non-scheduled runs, to see it in action?

https://github.com/alvinhochun/llvm-mingw/actions/runs/3158634041 does use the latest versions of llvm and mingw-w64 (the get-versions version check is enabled) and has cfguard enabled.

@mstorsjo
Copy link
Owner

Thanks, I have addressed the comments. Let's wait and see how the actions https://github.com/alvinhochun/llvm-mingw/actions/runs/3158626818 and https://github.com/alvinhochun/llvm-mingw/actions/runs/3158634041 go.

Thanks! Can you kick off another actions run, with a branch on top of this which bumps llvm/mingw to versions that support it, and which enables it by default even for non-scheduled runs, to see it in action?

https://github.com/alvinhochun/llvm-mingw/actions/runs/3158634041 does use the latest versions of llvm and mingw-w64 (the get-versions version check is enabled) and has cfguard enabled.

These test runs seem to have run successfully - so, I guess there's nothing left stopping this from getting merged then?

@alvinhochun
Copy link
Contributor Author

These test runs seem to have run successfully - so, I guess there's nothing left stopping this from getting merged then?

Not from me!

@mstorsjo
Copy link
Owner

mstorsjo commented Oct 1, 2022

These test runs seem to have run successfully - so, I guess there's nothing left stopping this from getting merged then?

Not from me!

Great! I did some extra test runs with Wine (with run-tests.sh tweaked to actually run the failure tests there too), and it worked as expected (check_enabled indicated that it wasn't enabled and skipped the rest), so this looks good to go to me. Thanks!

@mstorsjo mstorsjo merged commit e76e935 into mstorsjo:master Oct 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Control Flow Guard (CFG/CFGuard) on Windows
2 participants