This is an archive of the discontinued LLVM Phabricator instance.

Differential D20347

Add support to clang-cl driver for /GS switch
ClosedPublic

Authored by etienneb on May 17 2016, 5:57 PM.

Details

Reviewers
hans
rnk
Commits
rGe28e7f234d6d: Add support to clang-cl driver for /GS switch
rC272832: Add support to clang-cl driver for /GS switch
rL272832: Add support to clang-cl driver for /GS switch
Summary

This patch is adding command-line support for the MSVC buffer security check.

The buffer security check is turned on with the '/GS' compiler switch.
https://msdn.microsoft.com/en-us/library/8dbf701c.aspx

The MSVC buffer security check in implemented here:
http://reviews.llvm.org/D20346

Diff Detail

Event Timeline

etienneb updated this revision to Diff 57545.May 17 2016, 5:57 PM
etienneb retitled this revision from to [draft] Prototyping GS on MSVC: plug the command-line switch..
etienneb updated this object.
etienneb updated this revision to Diff 57548.May 17 2016, 6:09 PM

nits

etienneb updated this revision to Diff 57550.May 17 2016, 6:12 PM

revert

etienneb retitled this revision from [draft] Prototyping GS on MSVC: plug the command-line switch. to Add support to clang-cl driver for /GS switch.May 18 2016, 9:39 AM
etienneb updated this object.
etienneb updated this object.
etienneb updated this revision to Diff 58460.May 25 2016, 11:10 AM
etienneb updated this object.

turn /GS on by default

etienneb added subscribers: thakis, hans, rnk.May 25 2016, 11:11 AM
hans added a comment.May 25 2016, 11:18 AM

The approach looks good, but please add tests (probably in test/Driver/cl-options.c).

include/clang/Driver/CLCompatOptions.td
81

The description is not super informative, but it does match the MSVC description (except in the command-line options listing they say "Buffers security check".

Would it make more sense to call it something like stack canary, cookie, protector?

lib/Driver/Tools.cpp
9990 ↗(On Diff #58460)

Why is this needed?

etienneb updated this revision to Diff 58475.May 25 2016, 12:53 PM

fix hans@ comments

etienneb updated this revision to Diff 58483.May 25 2016, 1:01 PM

add unittests

etienneb added a comment.May 25 2016, 1:02 PM

added unittests. Should be fine now.

lib/Driver/Tools.cpp
9990 ↗(On Diff #58460)

copy paste bug!
Good catch.

hans added a comment.May 25 2016, 1:17 PM

Please also add a test in test/Driver/cl-options.c which tests that /GS causes the correct flag to be passed to the cc1 invocation, that it happens by default, and that /GS- makes it go away.

etienneb updated this revision to Diff 58497.May 25 2016, 1:46 PM

more tests

hans accepted this revision.May 25 2016, 1:50 PM
hans added a reviewer: hans.

lgtm with comments addressed

test/Driver/cl-options.c
62 ↗(On Diff #58497)

I'd suggest a test that checks that it's on by default too. You can reuse the same -check-prefix:

// RUN: %clang_cl -### -- %s 2>&1 | FileCheck -check-prefix=GS %s

Also, shouldn't the checks below be for "--stack-protector 2" or something, since we're enabling the strong level?

This revision is now accepted and ready to land.May 25 2016, 1:50 PM
etienneb edited edge metadata.May 25 2016, 1:54 PM
etienneb added a subscriber: cfe-commits.
etienneb updated this revision to Diff 58502.May 25 2016, 2:04 PM
etienneb marked an inline comment as done.

more tests

etienneb added a comment.May 25 2016, 2:05 PM

This patch needs land after http://reviews.llvm.org/D20346.
thx for the review.

etienneb added a subscriber: • chrisha.Jun 9 2016, 10:56 AM
hans added a comment.Jun 9 2016, 11:36 AM

Is this waiting for anything more now that http://reviews.llvm.org/D20346 has landed?

thakis added a comment.Jun 9 2016, 11:58 AM

probably at least the "the XOR with RSP/EBP/ESP" bit still (and maybe EH function upgrades instead of bailing)

lib/Driver/Tools.cpp
9990 ↗(On Diff #58475)

To pass on /GS- to the fallback compiler when needed, I suppose.

rnk accepted this revision.Jun 15 2016, 11:38 AM
rnk added a reviewer: rnk.

lgtm

Surely this is going to break something, but let's throw the switch and find out.

etienneb closed this revision.Jun 15 2016, 1:41 PM

Revision Contents

PathSize
include/
clang/
Driver/
6 lines

Diff 57545

include/clang/Driver/CLCompatOptions.td

Show First 20 Lines • Show All 71 Lines • ▼ Show 20 Linesdef _SLASH_fp_precise : CLFlag<"fp:precise">,
HelpText<"">, Alias<fno_fast_math>; HelpText<"">, Alias<fno_fast_math>;
def _SLASH_fp_strict : CLFlag<"fp:strict">, HelpText<"">, Alias<fno_fast_math>; def _SLASH_fp_strict : CLFlag<"fp:strict">, HelpText<"">, Alias<fno_fast_math>;
def _SLASH_GA : CLFlag<"GA">, Alias<ftlsmodel_EQ>, AliasArgs<["local-exec"]>, def _SLASH_GA : CLFlag<"GA">, Alias<ftlsmodel_EQ>, AliasArgs<["local-exec"]>,
HelpText<"Assume thread-local variables are defined in the executable">; HelpText<"Assume thread-local variables are defined in the executable">;
def _SLASH_GR : CLFlag<"GR">, HelpText<"Enable emission of RTTI data">; def _SLASH_GR : CLFlag<"GR">, HelpText<"Enable emission of RTTI data">;
def _SLASH_GR_ : CLFlag<"GR-">, HelpText<"Disable emission of RTTI data">; def _SLASH_GR_ : CLFlag<"GR-">, HelpText<"Disable emission of RTTI data">;
def _SLASH_GF_ : CLFlag<"GF-">, HelpText<"Disable string pooling">, def _SLASH_GF_ : CLFlag<"GF-">, HelpText<"Disable string pooling">,
Alias<fwritable_strings>; Alias<fwritable_strings>;
def _SLASH_GS : CLFlag<"GS">, HelpText<"Enable buffer security check">,
Alias<fstack_protector_strong>;
hansUnsubmitted
Not Done
ReplyInline Actions

The description is not super informative, but it does match the MSVC description (except in the command-line options listing they say "Buffers security check".

Would it make more sense to call it something like stack canary, cookie, protector?

hans: The description is not super informative, but it does match the MSVC description (except in the…
def _SLASH_GS_ : CLFlag<"GS-">, HelpText<"Disable buffer security check">,
Alias<fno_stack_protector>;
def _SLASH_Gs : CLJoined<"Gs">, HelpText<"Set stack probe size">, def _SLASH_Gs : CLJoined<"Gs">, HelpText<"Set stack probe size">,
Alias<mstack_probe_size>; Alias<mstack_probe_size>;
def _SLASH_Gy : CLFlag<"Gy">, HelpText<"Put each function in its own section">, def _SLASH_Gy : CLFlag<"Gy">, HelpText<"Put each function in its own section">,
Alias<ffunction_sections>; Alias<ffunction_sections>;
def _SLASH_Gy_ : CLFlag<"Gy-">, def _SLASH_Gy_ : CLFlag<"Gy-">,
HelpText<"Don't put each function in its own section">, HelpText<"Don't put each function in its own section">,
Alias<fno_function_sections>; Alias<fno_function_sections>;
def _SLASH_Gw : CLFlag<"Gw">, HelpText<"Put each data item in its own section">, def _SLASH_Gw : CLFlag<"Gw">, HelpText<"Put each data item in its own section">,
▲ Show 20 Lines • Show All 188 Lines • ▼ Show 20 Lines
def _SLASH_d2FastFail : CLIgnoredFlag<"d2FastFail">; def _SLASH_d2FastFail : CLIgnoredFlag<"d2FastFail">;
def _SLASH_d2Zi_PLUS : CLIgnoredFlag<"d2Zi+">; def _SLASH_d2Zi_PLUS : CLIgnoredFlag<"d2Zi+">;
def _SLASH_errorReport : CLIgnoredJoined<"errorReport">; def _SLASH_errorReport : CLIgnoredJoined<"errorReport">;
def _SLASH_Fd : CLIgnoredJoined<"Fd">; def _SLASH_Fd : CLIgnoredJoined<"Fd">;
def _SLASH_FC : CLIgnoredFlag<"FC">; def _SLASH_FC : CLIgnoredFlag<"FC">;
def _SLASH_FS : CLIgnoredFlag<"FS">, HelpText<"Force synchronous PDB writes">; def _SLASH_FS : CLIgnoredFlag<"FS">, HelpText<"Force synchronous PDB writes">;
def _SLASH_Gd : CLIgnoredFlag<"Gd">; def _SLASH_Gd : CLIgnoredFlag<"Gd">;
def _SLASH_GF : CLIgnoredFlag<"GF">; def _SLASH_GF : CLIgnoredFlag<"GF">;
def _SLASH_GS_ : CLIgnoredFlag<"GS-">;
def _SLASH_kernel_ : CLIgnoredFlag<"kernel-">; def _SLASH_kernel_ : CLIgnoredFlag<"kernel-">;
def _SLASH_nologo : CLIgnoredFlag<"nologo">; def _SLASH_nologo : CLIgnoredFlag<"nologo">;
def _SLASH_Ob1 : CLIgnoredFlag<"Ob1">; def _SLASH_Ob1 : CLIgnoredFlag<"Ob1">;
def _SLASH_Ob2 : CLIgnoredFlag<"Ob2">; def _SLASH_Ob2 : CLIgnoredFlag<"Ob2">;
def _SLASH_Og : CLIgnoredFlag<"Og">; def _SLASH_Og : CLIgnoredFlag<"Og">;
def _SLASH_openmp_ : CLIgnoredFlag<"openmp-">; def _SLASH_openmp_ : CLIgnoredFlag<"openmp-">;
def _SLASH_RTC : CLIgnoredJoined<"RTC">; def _SLASH_RTC : CLIgnoredJoined<"RTC">;
def _SLASH_sdl : CLIgnoredFlag<"sdl">; def _SLASH_sdl : CLIgnoredFlag<"sdl">;
Show All 27 Lines
def _SLASH_Ge : CLFlag<"Ge">; def _SLASH_Ge : CLFlag<"Ge">;
def _SLASH_Gh : CLFlag<"Gh">; def _SLASH_Gh : CLFlag<"Gh">;
def _SLASH_GH : CLFlag<"GH">; def _SLASH_GH : CLFlag<"GH">;
def _SLASH_GL : CLFlag<"GL">; def _SLASH_GL : CLFlag<"GL">;
def _SLASH_GL_ : CLFlag<"GL-">; def _SLASH_GL_ : CLFlag<"GL-">;
def _SLASH_Gm : CLFlag<"Gm">; def _SLASH_Gm : CLFlag<"Gm">;
def _SLASH_Gm_ : CLFlag<"Gm-">; def _SLASH_Gm_ : CLFlag<"Gm-">;
def _SLASH_Gr : CLFlag<"Gr">; def _SLASH_Gr : CLFlag<"Gr">;
def _SLASH_GS : CLFlag<"GS">;
def _SLASH_GT : CLFlag<"GT">; def _SLASH_GT : CLFlag<"GT">;
def _SLASH_Guard : CLJoined<"guard:">; def _SLASH_Guard : CLJoined<"guard:">;
def _SLASH_Gv : CLFlag<"Gv">; def _SLASH_Gv : CLFlag<"Gv">;
def _SLASH_Gz : CLFlag<"Gz">; def _SLASH_Gz : CLFlag<"Gz">;
def _SLASH_GZ : CLFlag<"GZ">; def _SLASH_GZ : CLFlag<"GZ">;
def _SLASH_H : CLFlag<"H">; def _SLASH_H : CLFlag<"H">;
def _SLASH_homeparams : CLFlag<"homeparams">; def _SLASH_homeparams : CLFlag<"homeparams">;
def _SLASH_hotpatch : CLFlag<"hotpatch">; def _SLASH_hotpatch : CLFlag<"hotpatch">;
Show All 22 Lines