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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
