[cfe-dev] Stack Protectors

[Eli Friedman]

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