This is an archive of the discontinued LLVM Phabricator instance.

[clang][MinGW] Add `-mguard=cf` and `-mguard=cf-nochecks`
ClosedPublic

Authored by alvinhochun on Aug 28 2022, 10:19 AM.

Details

Summary

This option can be used to enable Control Flow Guard checks and
generation of address-taken function table. They are equivalent to
/guard:cf and /guard:cf,nochecks in clang-cl. Passing this flag to
the Clang driver will also pass --guard-cf to the MinGW linker.

This feature is disabled by default. The option -mguard=none is also
available to explicitly disable this feature.

Depends on D132808

Diff Detail

Event Timeline

alvinhochun created this revision.Aug 28 2022, 10:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 28 2022, 10:19 AM
Herald added a subscriber: mstorsjo. · View Herald Transcript
alvinhochun published this revision for review.Aug 28 2022, 10:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 28 2022, 10:42 AM

Looks reasonable to me, but I'd appreciate input from people more familiar with adding new options to the GCC style driver about option naming.

rnk added a comment.Aug 29 2022, 10:12 AM

+@MaskRay, should we use -mguard= as the GCC spelling of the MSVC /GUARD: flag? Short flags are nice, but it seems there may be a risk of GCC using -mguard= for something else at some point. I lean towards using -mguard= as is.

rnk added a comment.Aug 29 2022, 10:12 AM

(I added @MaskRay based on the new code ownership structure proposed here: https://discourse.llvm.org/t/rfc-proposed-changes-to-clangs-code-ownership/64813)

+@MaskRay, should we use -mguard= as the GCC spelling of the MSVC /GUARD: flag? Short flags are nice, but it seems there may be a risk of GCC using -mguard= for something else at some point. I lean towards using -mguard= as is.

@MaskRay Can you follow up here?

The risk conflicting with a GCC option is probably quite low. If there is something, the GCC option will likely be -mguard-*= instead of -mguard= (IMHO confusing).

clang/lib/Driver/ToolChains/MinGW.cpp
626

If this is Clang specific, is it necessary to use insensitive option names?

clang/test/Driver/mingw-cfguard.c
2

Use --target= for new tests (avoid legacy -target ).

Omit unneeded -v.

4

{{$}} is unneeded

// NO_CF:     "-cc1"
// NO_CF-NOT: "-cfguard"
// NO_CF-NOT: "-cfguard-no-checks"
// NO_CF-NEXT: ld"
// NO_CF-NOT: "--guard-cf"
// DEFAULT-NOT: "--no-guard-cf"
// GUARD_NONE-SAME: "--no-guard-cf"

Applied suggestions. Thanks for the comment.

mstorsjo added inline comments.Sep 6 2022, 11:38 PM
clang/test/Driver/mingw-cfguard.c
2

The comment about preferring --target= over -target for new tests seems to be unaddressed here.

Fixed --target= option for test

rnk accepted this revision.Sep 7 2022, 8:58 AM

lgtm

This revision is now accepted and ready to land.Sep 7 2022, 8:58 AM
This revision was landed with ongoing or failed builds.Sep 8 2022, 11:56 PM
This revision was automatically updated to reflect the committed changes.