Stack Protectors

Clang Frontend
#1

Hi,

Here's a patch to implement stack protector support for Clang. This doesn't include test cases, which I will create before checking it. Could someone please review this?

Thanks!
-bw

clang-ssp.patch (7.31 KB)

#2

// Blocks default to on for 10.6 (darwin10) and beyond.
    // As does nonfragile-abi for 64bit mode
- if (Maj > 9)
+ if (Maj > 9) {
      Opts.Blocks = 1;
+ Opts.StackProtector = 1;
+ }

Might want to update the comment, replace it with a list or something :slight_smile:

Otherwise looks pretty good to me. Not that I'm a clang reviewer or anything though :slight_smile:

-eric

#3

<clang-ssp.patch>

  // Blocks default to on for 10.6 (darwin10) and beyond.
  // As does nonfragile-abi for 64bit mode
- if (Maj > 9)
+ if (Maj > 9) {
    Opts.Blocks = 1;
+ Opts.StackProtector = 1;
+ }

Might want to update the comment, replace it with a list or something :slight_smile:

I updated them to this:

   // Blocks and stack protectors default to on for 10.6 (darwin10) and beyond.
   if (Maj > 9) {
     Opts.Blocks = 1;
     Opts.StackProtector = 1;
   }

   // Non-fragile ABI (in 64-bit mode) default to on for 10.5 (darwin9) and
   // beyond.
   if (Maj >= 9 && Opts.ObjC1 && !strncmp(Triple, "x86_64", 6))
     Opts.ObjCNonFragileABI = 1;

Otherwise looks pretty good to me. Not that I'm a clang reviewer or anything though :slight_smile:

Thanks for the initial review at least! :slight_smile:

-bw

#4

Hi,

Here's a patch to implement stack protector support for Clang. This doesn't
include test cases, which I will create before checking it. Could someone
please review this?

Review below:

Index: include/clang/Basic/TargetInfo.h

--- include/clang/Basic/TargetInfo.h (revision 74401)
+++ include/clang/Basic/TargetInfo.h (working copy)
@@ -52,6 +52,7 @@
   const char *UserLabelPrefix;
   const llvm::fltSemantics *FloatFormat, *DoubleFormat, *LongDoubleFormat;
   unsigned char RegParmMax, SSERegParmMax;
+ unsigned char StackProtector;

   // TargetInfo Constructor. Default initializes all fields.
   TargetInfo(const std::string &T);

This variable is unused; please get rid of it and the related code.

Index: lib/Driver/Tools.cpp

--- lib/Driver/Tools.cpp (revision 74401)
+++ lib/Driver/Tools.cpp (working copy)
@@ -498,6 +498,14 @@
   Args.AddLastArg(CmdArgs, options::OPT_fvisibility_EQ);
   Args.AddLastArg(CmdArgs, options::OPT_fwritable_strings);

+ // Forward stack protector flags.
+ if (Args.hasArg(options::OPT_fno_stack_protector))
+ CmdArgs.push_back("--stack-protector=0");
+ else if (Args.hasArg(options::OPT_fstack_protector))
+ CmdArgs.push_back("--stack-protector=1");
+ else if (Args.hasArg(options::OPT_fstack_protector_all))
+ CmdArgs.push_back("--stack-protector=2");
+
   // Forward -f options with positive and negative forms; we translate
   // these by hand.

This doesn't match gcc's behavior; please add a new form of
ArgList::getLastArg which takes three options as arguments and use it
here.

Besides those issues, the patch looks fine.

-Eli

#5

Hi Eli,

I made those changes. Thanks for the review! Checked in as r74405.

-bw