Page MenuHomePhabricator

[Clang] Implement function attribute no_stack_protector.
ClosedPublic

Authored by manojgupta on Apr 30 2018, 10:34 PM.

Details

Summary

This attribute tells clang to skip this function from stack protector
when -stack-protector option is passed.
GCC option for this is:
attribute((optimize("no-stack-protector"))) and the
equivalent clang syntax would be: attribute((no_stack_protector))

This is used in Linux kernel to selectively disable stack protector
in certain functions.

Diff Detail

Repository
rC Clang

Event Timeline

manojgupta created this revision.Apr 30 2018, 10:34 PM

Looks fine. Please wait for a more experienced person to review this, however. Thanks for adding this feature.

aaron.ballman requested changes to this revision.May 1 2018, 6:35 PM

Missing Sema tests that the attribute only appertains to functions, accepts no arguments, etc.

include/clang/Basic/Attr.td
1499

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")?

1501

No new undocumented attributes, please.

This revision now requires changes to proceed.May 1 2018, 6:35 PM

Added docs, Sema test case for the attribute.

manojgupta added inline comments.May 7 2018, 12:39 PM
include/clang/Basic/Attr.td
1499

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)

aaron.ballman edited reviewers, added: probinson; removed: void.May 7 2018, 1:38 PM
aaron.ballman added a subscriber: probinson.

Adding in @probinson as he originally added the optnone attribute. Paul, do you recall why you opted (haha, pun totally intended) to implement optnone rather than optimize("O0") from GCC?

include/clang/Basic/Attr.td
1499

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.

rnk added inline comments.May 8 2018, 5:18 PM
include/clang/Basic/Attr.td
1499

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.

aaron.ballman added inline comments.May 9 2018, 5:26 AM
include/clang/Basic/Attr.td
1499

Thank you, @rnk, that makes sense to me and seems like a solid reason for this approach.

include/clang/Basic/AttrDocs.td
2746

to disable -> which disables the

2747

functions -> function

2748

disabling stack -> disabling the stack

2749

options -> option

Backticks around the compiler flag.

2751

disables stack -> disables the stack

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

2752

with stack -> with the stack
with -fstack-protector -> with the -fstack-protector

Backticks around the compiler flag.

2757

C requires parameters to be named; alternatively, you can use the C++ spelling of the attribute.

2759

with stack -> with the stack

test/Sema/no_stack_protector.c
5

Can you also add a test to ensure the attribute accepts no arguments?

Updated test case for error msg with arguments.
Updated the documentation.

This revision is now accepted and ready to land.May 9 2018, 1:43 PM
This revision was automatically updated to reflect the committed changes.