Page MenuHomePhabricator

Adding nocf_check attribute for cf-protection fine tuning
ClosedPublic

Authored by oren_ben_simhon on Jan 9 2018, 12:48 PM.

Details

Summary

The patch adds nocf_check target independent attribute for disabling checks that were enabled by cf-protection flag.
The attribute can be appertained to functions and function pointers.
Attribute name follows GCC's similar attribute name.

Please see the following for more information:
https://reviews.llvm.org/D41879

Diff Detail

Repository
rL LLVM

Event Timeline

craig.topper added inline comments.Feb 13 2018, 11:54 AM
lib/Sema/SemaDeclAttr.cpp
2007 ↗(On Diff #129147)

Wy did this get renamed?

2016 ↗(On Diff #129147)

Why did this get renamed?

aaron.ballman added a subscriber: aaron.ballman.
aaron.ballman added inline comments.
include/clang/Basic/Attr.td
2089 ↗(On Diff #129147)

This attribute doesn't appear to be supported by GCC. Did you mean to use the Clang spelling instead?

include/clang/Basic/AttrDocs.td
2876 ↗(On Diff #129147)

tampering with addresses

2878 ↗(On Diff #129147)

bytes in the binary

lib/Sema/SemaDeclAttr.cpp
1990 ↗(On Diff #129147)

attr doesn't follow the proper naming conventions.

1991–1992 ↗(On Diff #129147)

Why do you need to manually check the attribute has no args -- doesn't the common handling in handleCommonAttributeFeatures() already cover that?

1994–2002 ↗(On Diff #129147)

I think all of this can be replaced by setting the proper subject on the attribute (FunctionLike) in Attr.td.

test/Sema/attr-nocf_check.c
7–9 ↗(On Diff #129147)

How is this mismatched?

oren_ben_simhon marked 6 inline comments as done.Feb 14 2018, 1:45 AM
oren_ben_simhon added inline comments.
include/clang/Basic/Attr.td
2089 ↗(On Diff #129147)
lib/Sema/SemaDeclAttr.cpp
2007 ↗(On Diff #129147)

To reuse existing code. The same check is performed in several attributes so instead of writing this block for every attribute i only call this function.

2016 ↗(On Diff #129147)

To remove multiple code fragments. Same as CheckAttrNoArgs.

Implemented comments posted until 2/14 (Thanks Aaron and Craig)

One thing I notice is that GCC trunk requires passing the -fcf-protection flag in order to enable the attribute. Do we wish to do the same?

include/clang/Basic/Attr.td
2089 ↗(On Diff #129147)

Ah, it was hiding in there -- my apologies!

lib/Sema/SemaDeclAttr.cpp
1990 ↗(On Diff #129147)

Please don't name the parameter variable after a type -- that can confuse some editors.

2007 ↗(On Diff #129147)

I think this may be the wrong approach -- this code seems like it should be handled automatically in handleCommonAttributeFeatures(). We already check whether the attribute appertains to its subjects and other table-gennable predicate checks, and this seems like it would be another instance of that kind of situation.

I think you need to retain this function (because it's called for type attributes, which don't have automated checking yet), but call it from handleCommonAttributeFeatures() rather than call it manually.

1968–1969 ↗(On Diff #134175)

I think this can be removed entirely -- it should be handled automatically by the common handler (double-check that there's test coverage for it).

1983 ↗(On Diff #134175)

I think you can drop the CheckAttrNoArgs() call from here as well. And once CheckAttrTarget() is moved to the common handler, this function can also be replaced by a call to handleSimpleAttribute<>()

1990 ↗(On Diff #134175)

Once you move the target check into the common predicate function, this function can go away and you can use handleSimpleAttribute<>() instead.

test/Sema/attr-nocf_check.c
18–20 ↗(On Diff #134175)

These are an error in GCC and I think we should match that behavior. https://godbolt.org/g/r3pf4X

oren_ben_simhon marked 5 inline comments as done.Feb 15 2018, 5:14 AM
oren_ben_simhon added inline comments.
lib/Sema/SemaDeclAttr.cpp
1990 ↗(On Diff #129147)

I am following the same convention that other functions are using.

test/Sema/attr-nocf_check.c
18–20 ↗(On Diff #134175)

I will create a warning however in LLVM we don't create an error upon incompatible pointer due to function attribute types.

Implemented comments posted until 02/15 (Thanks Aaron)

aaron.ballman added inline comments.Feb 15 2018, 5:57 AM
lib/Sema/SemaDeclAttr.cpp
1990 ↗(On Diff #129147)

I am following the same convention that other functions are using.

They're doing it wrong. I can clean those up in a separate patch.

test/Sema/attr-nocf_check.c
18–20 ↗(On Diff #134175)

It should be an error -- Clang does error on this sort of thing when appropriate (which I believe it is, here). For instance, calling convention attributes do this: https://godbolt.org/g/mkTGLg

oren_ben_simhon marked an inline comment as done.Feb 15 2018, 7:13 AM
oren_ben_simhon added inline comments.
lib/Sema/SemaDeclAttr.cpp
1990 ↗(On Diff #129147)

Its OK i can make the change.

test/Sema/attr-nocf_check.c
18–20 ↗(On Diff #134175)

In Clang there is Sema::IncompatiblePointer in case to pointers are not compatible. This flag emits warning message. In the time i check for pointer incompatibility (checkPointerTypesForAssignment()), i don;t have a handle to the attributes. Any suggestion how to implement the exception for nocf_check attribute?

aaron.ballman added inline comments.Feb 15 2018, 8:50 AM
lib/Sema/SemaDeclAttr.cpp
1990 ↗(On Diff #129147)

I've commit a cleanup in r325256 that fixes the identifiers in the file (along with several other coding standard issues).

test/Sema/attr-nocf_check.c
18–20 ↗(On Diff #134175)

I believe this is handled in ASTContext::mergeFunctionType(). See:

// Compatible functions must have compatible calling conventions
if (lbaseInfo.getCC() != rbaseInfo.getCC())
  return QualType();

Somewhere around there is likely where you should be.

oren_ben_simhon marked an inline comment as done.Feb 15 2018, 2:14 PM
oren_ben_simhon added inline comments.
test/Sema/attr-nocf_check.c
18–20 ↗(On Diff #134175)

I already added there getnocfcheck.

After double checking, I see that nocf_check behavior is identical to other function attributes.
For some reason in the clang tests they give warning but in godbolt it gives an error.
I am not sure what is the difference between the flags in godbolt and in my test but this is what causing the warning/error message difference.

So basically my behavior is identical to other function type attributes (e.g. no_caller_saved_registers). I believe it is also identical to GCC but i can't prove it because i don't know the flags that godbolt is using.

aaron.ballman added inline comments.Feb 16 2018, 6:17 AM
test/Sema/attr-nocf_check.c
18–20 ↗(On Diff #134175)

You can see the flags being passed in godbolt by passing -v on the command line. FWIW, I get the same error behavior elsewhere as well:

http://coliru.stacked-crooked.com/a/d28234385fa68374
https://wandbox.org/permlink/SRLM82l2uJ8q3o1Q

I think you should do some more investigation into what's going on there. Ultimately, I want to avoid clang accepting the nocf_check attribute (even with a warning) in cases where GCC doesn't accept it, because that leads to incompatibilities when switching between the two compilers. We should accept what GCC accepts and reject what GCC rejects unless there's a good reason to deviate.

aaron.ballman added inline comments.Feb 16 2018, 7:31 AM
test/Sema/attr-nocf_check.c
18–20 ↗(On Diff #134175)

Ah, I think the distinction here is C++ vs C code. In C, this code should warn, in C++ this code should err. I'm guessing that if you add a C++ test case, the behavior will be to err on this without any other code changes.

Implemented comments posted until 02/22 (Thanks Aaron).

oren_ben_simhon marked 2 inline comments as done.Feb 22 2018, 3:36 AM

I added a comment ignoring nocf_check attribute in case -fcf-protection is not set.
Now LLVM is identical to GCC.

test/Sema/attr-nocf_check.c
18–20 ↗(On Diff #134175)

Indeed the difference is C++ Vs C. I added a new test that checks for C++ as well.

aaron.ballman added inline comments.Feb 22 2018, 7:18 AM
include/clang/Basic/DiagnosticSemaKinds.td
2691 ↗(On Diff #135386)

Diagnostics are not complete sentences, so the punctuation and capitalization should go away. Typo in the flag spelling. The attribute name should be quoted. I'd go with: "'nocf_check' attribute ignored; use -fcf-protection to enable the attribute"

include/clang/CodeGen/CGFunctionInfo.h
519 ↗(On Diff #135386)

This is unfortunate -- it bumps the bit-field over 32 bits. Can the bit be stolen from elsewhere?

include/clang/Sema/Sema.h
3331–3332 ↗(On Diff #135386)

Please don't name the parameter after a type.

lib/Frontend/CompilerInvocation.cpp
1999 ↗(On Diff #135386)

Can elide some parens.

lib/Sema/SemaDeclAttr.cpp
1979–1980 ↗(On Diff #135386)

Can you use the LangOpts field to specify this in Attr.td? Then you can go back to the simple handler.

1985 ↗(On Diff #135386)

Don't name the parameter after a type.

test/Sema/attr-nocf_check.cpp
1 ↗(On Diff #135386)

For better test coverage, you can switch to the [[gnu::nocf_check]] spelling in this file and pass -std=c++11

aaron.ballman added inline comments.Feb 23 2018, 11:30 AM
test/Sema/attr-nocf_check.c
1 ↗(On Diff #135386)

You likely need to specify an explicit triple here, or some of the bots fail this test because the attribute isn't supported on that architecture. You should also add a test that verifies the attribute isn't supported on non-x86 platforms.

test/Sema/attr-nocf_check.cpp
1 ↗(On Diff #135386)

This one also likely needs an explicit triple.

oren_ben_simhon marked 7 inline comments as done.Feb 26 2018, 8:34 AM
oren_ben_simhon added inline comments.
include/clang/CodeGen/CGFunctionInfo.h
519 ↗(On Diff #135386)

The field is orthogonal to the other fields moreover i think that double meaning of the same field will lead to future bugs. The class is not a compact packed structure, so i don't feel it worth the confusion.

lib/Sema/SemaDeclAttr.cpp
1979–1980 ↗(On Diff #135386)

When using LangOpts field in Attr.td, the diagnostic warning will not be descriptive as i use here (use -fcf-protection flag...).

test/Sema/attr-nocf_check.cpp
1 ↗(On Diff #135386)

Since it doesn't test the functionality of this specific attribute, I believe it is an overkill to switch to [[gnu::nocf_check]] spelling.

Implemented comments posted until 02/26 (Thanks Aaron)

aaron.ballman added inline comments.Feb 26 2018, 2:22 PM
include/clang/CodeGen/CGFunctionInfo.h
519 ↗(On Diff #135386)

The class packs its fields for space savings because it's used fairly frequently; if we can keep the size from having to use another allocation unit, that's better. I wasn't suggesting we make a bit have double meaning, I was wondering if we could reduce the size of one of the other fields. For instance, I'd be surprised if we need all 8 bits for calling conventions, so we might be able to reduce that to a 7-bit field.

lib/Sema/SemaDeclAttr.cpp
1979–1980 ↗(On Diff #135386)

That's true, and this code is fine for now. However, it does suggest that the declarative handler could be improved to support this sort of thing -- the same issue is present with *all* attributes gated on a language option.

test/Sema/attr-nocf_check.cpp
1 ↗(On Diff #135386)

Since it doesn't test the functionality of this specific attribute, I believe it is an overkill to switch to [[gnu::nocf_check]] spelling.

C++ attribute spellings have slightly different code paths than GNU attribute spellings, so the test isn't overkill. Also, this is a C++ test file that already uses C++11, so adding the C++11 spelling is an improvement over using the GNU spelling (that's already covered with C tests).

It's not critical, but it's still a strict improvement.

erichkeane added inline comments.Feb 26 2018, 2:43 PM
include/clang/CodeGen/CGFunctionInfo.h
519 ↗(On Diff #135386)

Note that LLVM::CallingConv (http://llvm.org/doxygen/namespacellvm_1_1CallingConv.html) only uses up to 96, yet has a 'MaxID' of 1023 for some reason. It seems that backing this down to 255 would be more than reasonable and would prevent issues here, but wouldn't save any bits here. We could possibly make it '127' in LLVM, but I fear that would limit the values pretty harshly.

HOWEVER, clang::CallingConv (https://clang.llvm.org/doxygen/Specifiers_8h_source.html#l00233) only has 17 items. Because of this, 7 bits is 1 more than necessary. We could save a bit from ASTCallingConvention, and still permit nearly doubling calling conventions.

oren_ben_simhon marked 3 inline comments as done.Feb 27 2018, 2:19 AM
oren_ben_simhon added inline comments.
lib/Sema/SemaDeclAttr.cpp
1979–1980 ↗(On Diff #135386)

I agree. I believe such a change is out of the scope of the review.

Implemented comments posted until 02/27 (Thanks Aaron and Erich)

aaron.ballman accepted this revision.Feb 27 2018, 8:20 AM
aaron.ballman added a reviewer: rsmith.

Aside from some minor testing nits, I think this LGTM. However, you should wait to see if Richard has comments as well before committing.

test/Sema/attr-nocf_check.cpp
13 ↗(On Diff #136053)

Can you slide the attribute after i so it applies to the parameter (should still diagnose as not appertaining to that)?

test/Sema/nocf_check_attr_not_allowed.c
2 ↗(On Diff #136053)

Can you add another RUN line for when -fcf-protection is not supplied so that we have test coverage for that custom diagnostic?

This revision is now accepted and ready to land.Feb 27 2018, 8:20 AM
oren_ben_simhon marked 2 inline comments as done.Feb 27 2018, 11:48 PM

Implemented commented posted by Aaron (Thanks)

This revision was automatically updated to reflect the committed changes.