This is an archive of the discontinued LLVM Phabricator instance.

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

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

Details

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 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

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 ↗(On Diff #58460)

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

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

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

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?

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