[PATCH] D24507: Add attribute for return values that shouldn't be cast to bool

[Anton Urusov via cfe-commits]

urusant updated this revision to Diff 71921.
urusant added a comment.

In https://reviews.llvm.org/D24507#546380, @aaron.ballman wrote:

> We try to keep our tests segregated by functionality. e.g., tests relating to the way the attribute is handled (what it appertains to, args, etc) should live in Sema, tests relating to the static analyzer behavior should live in test/Analysis, etc.
>
> Tests that are still missing are: applying to a non-function type, applying to a member function, applying to an Obj-C method. For member functions, what should happen if the function is virtual? What if the overriders do not specify the attribute? What if an override specifies the attribute but the base does not?


I have added the test cases about member functions.
As for ObjC methods, I didn't pay much attention to them while developing the check as ObjC wasn't the primary target. I tried to make a test case for it, and it turned out that it is OK to put an attribute on ObjC method, but you wouldn't get neither compiler warning nor StaticAnalyzer report. That is why I removed ObjC methods from the attribute subjects and replaced the ObjC test case with another one that shows that you cannot apply the attribute to ObjC methods (not sure if it is still necessary, because it seems not very different from applying the attribute to a non-function variable - in both cases we get the same warning). Do you think it's worth digging into how to make it work with ObjC? In this case I might need some help because I don't really speak Objective C.

> > > Have you considered making this a type attribute on the return type of the function rather than a declaration attribute on the function declaration?

> 

> > 

> 

> > 

> 

> > No, I hadn't. On a quick look though, I couldn't find a way to simplify my solution using this idea, because as far as I understand, the type attribute isn't inherited, so, for example, if I have something like `int r = X509_verify_cert(...)` and the function `X509_verify_cert` has a return type with attribute, `r` won't have the attribute. If that is correct, we still need to backtrace the value to the function declaration. Is there something I am missing?

> 

> 

> I was thinking it would be diagnosed if you attempted to assign from your attributed type to a type that is not compatible. However, that may still be problematic because it raises other questions (can you SFINAE on it? Overload? etc).


This might also make the check itself easier (as we don't need path-sensitive analysis), however, it would make the use more complicated as all the users of the dangerous function would have to change their code (even if they are using it correctly). For example, if we refer to the original motivation, annotating dangerous OpenSSL functions would allow us to protect dozens of codebases using them without changing every one of them.


Repository:
  rL LLVM

https://reviews.llvm.org/D24507

Files:
  include/clang/Basic/Attr.td
  include/clang/Basic/AttrDocs.td
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/StaticAnalyzer/Checkers/Checkers.td
  lib/Sema/SemaChecking.cpp
  lib/Sema/SemaDeclAttr.cpp
  lib/StaticAnalyzer/Checkers/CMakeLists.txt
  lib/StaticAnalyzer/Checkers/ReturnNonBoolChecker.cpp
  test/Analysis/ReturnNonBoolTest.c
  test/Analysis/ReturnNonBoolTest.cpp
  test/SemaCXX/ReturnNonBoolTestCompileTime.cpp
  test/SemaObjC/ReturnNonBoolTestCompileTime.m

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D24507.71921.patch
Type: text/x-patch
Size: 21250 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160920/9b34ac06/attachment-0001.bin>