[PATCH] D46300: [Clang] Implement function attribute no_stack_protector.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: include/clang/Basic/Attr.td:1494
+def NoStackProtector : InheritableAttr {
+  let Spellings = [GCC<"no_stack_protector">];
+  let Subjects = SubjectList<[Function]>;
----------------
rnk wrote:
> aaron.ballman wrote:
> > manojgupta wrote:
> > > aaron.ballman wrote:
> > > > This is not a GCC attribute, so this should use the Clang spelling.
> > > > 
> > > > However, why spell the attribute this way rather than use the GCC spelling (`optimize("no-stack-protector")`?
> > > Thanks, I have changed it to use Clang spelling.
> > > 
> > > Regarding __attribute__((optimize("..."))), it is a generic facility in GCC that works for many optimizer flags.
> > > Clang currently does not support this syntax currently instead preferring its own version for some options e.g. -O0. 
> > > e.g.  
> > > ```
> > > __attribute__((optimize("O0")))  // clang version is __attribute__((optnone)) 
> > > ```
> > > If we want to support the GCC syntax, future expectation would be support more flags under this syntax. Is that the path we want to take (I do not know the history related to previous syntax decisions but better GCC compatibility will be a nice thing to have) 
> > The history of `optnone` predates my involvement with Clang and I've not been able to find the original review thread (I did find the one where I gave my LGTM on the original patch, but that was a resubmission after the original design was signed off).
> > 
> > I'm not keen on attributes that have the same semantics but differ in spelling from attributes supported by GCC unless there's a good reason to deviate. Given that we've already deviated, I'd like to understand why better -- I don't want to push you to implement something we've already decided was a poor design, but I also don't want to accept code if we can instead use syntax that is compatible with GCC.
> I do not think we want to pursue supporting generic optimization flags with `__attribute__((optimize("...")))`.
> 
> Part of the reason we only support `optnone` is that LLVM's pass pipeline is global to the entire module. It's not trivial to enable optimizations for a single function, but it is much easier to have optimization passes skip functions marked `optnone`.
Thank you, @rnk, that makes sense to me and seems like a solid reason for this approach.


================
Comment at: include/clang/Basic/AttrDocs.td:2746
+  let Content = [{
+Clang supports the ``__attribute__((no_stack_protector))`` attribute to disable
+stack protector on the specified functions. This attribute is useful for
----------------
to disable -> which disables the


================
Comment at: include/clang/Basic/AttrDocs.td:2747
+Clang supports the ``__attribute__((no_stack_protector))`` attribute to disable
+stack protector on the specified functions. This attribute is useful for
+selectively disabling stack protector on some functions when building with
----------------
functions -> function


================
Comment at: include/clang/Basic/AttrDocs.td:2748
+stack protector on the specified functions. This attribute is useful for
+selectively disabling stack protector on some functions when building with
+-fstack-protector compiler options.
----------------
disabling stack -> disabling the stack


================
Comment at: include/clang/Basic/AttrDocs.td:2749
+selectively disabling stack protector on some functions when building with
+-fstack-protector compiler options.
+
----------------
options -> option

Backticks around the compiler flag.


================
Comment at: include/clang/Basic/AttrDocs.td:2751
+
+For example, it disables stack protector for the function foo but function bar
+will still be built with stack protector with -fstack-protector option.
----------------
disables stack -> disables the stack

Please put backticks around `foo` and `bar` so they highlight as code.


================
Comment at: include/clang/Basic/AttrDocs.td:2752
+For example, it disables stack protector for the function foo but function bar
+will still be built with stack protector with -fstack-protector option.
+
----------------
with stack -> with the stack
with -fstack-protector -> with the -fstack-protector

Backticks around the compiler flag.


================
Comment at: include/clang/Basic/AttrDocs.td:2757
+    int __attribute__((no_stack_protector))
+    foo (int); // stack protection will be disabled for foo.
+
----------------
C requires parameters to be named; alternatively, you can use the C++ spelling of the attribute.


================
Comment at: include/clang/Basic/AttrDocs.td:2759
+
+    int bar(int a); // bar can be built with stack protector.
+
----------------
with stack -> with the stack


================
Comment at: test/Sema/no_stack_protector.c:4
+void __attribute__((no_stack_protector)) foo() {}
+int __attribute__((no_stack_protector)) var; // expected-warning {{'no_stack_protector' attribute only applies to functions}}
----------------
Can you also add a test to ensure the attribute accepts no arguments?


Repository:
  rC Clang

https://reviews.llvm.org/D46300