[cfe-dev] Stack Protectors

[Bill Wendling]

Hi Eli,

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

-bw

On Jun 27, 2009, at 6:44 PM, Eli Friedman wrote:

> On Sat, Jun 27, 2009 at 6:16 PM, Bill Wendling<wendling at apple.com>  
> wrote:
>> 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