[PATCH] D72018: [attributes] [analyzer] Add an attribute to prevent checkers from modeling a function

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun created this revision.
xazax.hun added reviewers: aaron.ballman, NoQ, haowei.
xazax.hun added a project: clang.
Herald added subscribers: Charusso, gamesh411, dkrupp, donat.nagy, Szelethus, 
mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware.

While the static analyzer is doing a great job modeling functions most of the 
time, sometimes we have global dynamic invariants that might be infeasible to 
reason about statically (or in some cases just to complex to worth to implement 
it).

It would be great to have a way to tell the checkers that certain functions are 
not worth modeling. In case of fuchsia.HandleCheck, we could achieve the same 
by simply removing all the annotations. This is not always desired though. The 
annotations are useful on their own, they document the ownership semantics of 
the handles. So instead of removing the annotations for these functions, we 
think it might be better to introduce yet another annotation for this use case.

What do you think? Is this reasonable or do you have any alternative ideas?

For the curious the syscall I have problem with in Fuchsia is the 
`zx_channel_read`: 
https://fuchsia.dev/fuchsia-src/reference/syscalls/channel_read.md

The problem is mainly with the `handles` parameter.  We might not receive 
handles at all. And we might know that in advance that we will not receive 
handles, so we do not need to check the `actual_handles`. So assuming `handles` 
contains open handles will end up producing spurious handle leak errors.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D72018

Files:
  clang/include/clang/Basic/Attr.td
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
  clang/test/Analysis/fuchsia_handle.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test

Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -15,6 +15,7 @@
 // CHECK-NEXT: AllocSize (SubjectMatchRule_function)
 // CHECK-NEXT: AlwaysDestroy (SubjectMatchRule_variable)
 // CHECK-NEXT: AlwaysInline (SubjectMatchRule_function)
+// CHECK-NEXT: AnalyzerCheckerIgnore (SubjectMatchRule_function)
 // CHECK-NEXT: Annotate ()
 // CHECK-NEXT: AnyX86NoCfCheck (SubjectMatchRule_hasType_functionType)
 // CHECK-NEXT: ArcWeakrefUnavailable (SubjectMatchRule_objc_interface)
Index: clang/test/Analysis/fuchsia_handle.cpp
===
--- clang/test/Analysis/fuchsia_handle.cpp
+++ clang/test/Analysis/fuchsia_handle.cpp
@@ -12,10 +12,12 @@
 #define ZX_HANDLE_ACQUIRE  __attribute__((acquire_handle("Fuchsia")))
 #define ZX_HANDLE_RELEASE  __attribute__((release_handle("Fuchsia")))
 #define ZX_HANDLE_USE  __attribute__((use_handle("Fuchsia")))
+#define ZX_HANDLE_IGNORE __attribute__((analyzer_checker_ignore("fuchsia.HandleChecker")))
 #else
 #define ZX_HANDLE_ACQUIRE
 #define ZX_HANDLE_RELEASE
 #define ZX_HANDLE_USE
+#define ZX_HANDLE_IGNORE
 #endif
 
 zx_status_t zx_channel_create(
@@ -23,6 +25,12 @@
 zx_handle_t *out0 ZX_HANDLE_ACQUIRE,
 zx_handle_t *out1 ZX_HANDLE_ACQUIRE);
 
+ZX_HANDLE_IGNORE
+zx_status_t zx_channel_create_complicated(
+uint32_t options,
+zx_handle_t *out0 ZX_HANDLE_ACQUIRE,
+zx_handle_t *out1 ZX_HANDLE_ACQUIRE);
+
 zx_status_t zx_handle_close(
 zx_handle_t handle ZX_HANDLE_RELEASE);
 
@@ -327,3 +335,11 @@
 return;
   zx_handle_close(h2);
 } // *h should be escaped here. Right?
+
+// Ignored functions
+
+void ignore_function() {
+  zx_handle_t sa, sb;
+  if (zx_channel_create_complicated(0, &sa, &sb))
+return;
+}
Index: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
@@ -302,6 +302,12 @@
   if (!FuncDecl)
 return;
 
+  if (const auto *Attr = FuncDecl->getAttr())
+if (llvm::any_of(Attr->checkers(), [this](StringRef Checker) {
+  return Checker == getCheckerName();
+}))
+  return;
+
   ProgramStateRef State = C.getState();
 
   std::vector> Notes;
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -2065,6 +2065,25 @@
   D->addAttr(::new (S.Context) AnalyzerNoReturnAttr(S.Context, AL));
 }
 
+static void HandleAnalyzerCheckerIgnoreAttr(Sema &S, Decl *D,
+const ParsedAttr &AL) {
+  if (!checkAttributeAtLeastNumArgs(S, AL, 1))
+return;
+
+  SmallVector Checkers;
+  for (unsigned I = 0, E = AL.getNumArgs(); I != E; ++I) {
+StringRef CheckerName;
+
+if (!S.checkStringLiteral

[PATCH] D72018: [attributes] [analyzer] Add an attribute to prevent checkers from modeling a function

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

> What do you think? Is this reasonable or do you have any alternative ideas?

This seems like a very specialized attribute for the static analyzer and I'm 
not certain how much utility it adds, but that may be because I don't 
understand the analyzer requirements sufficiently yet. It seems as though this 
attribute is intended to suppress diagnostics within a certain function for a 
certain check, much like `gsl::suppress` does for the C++ Core Guidelines. Is 
that correct? If so, perhaps we should just reuse that attribute (but with a 
different spelling)?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72018/new/

https://reviews.llvm.org/D72018



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72018: [attributes] [analyzer] Add an attribute to prevent checkers from modeling a function

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

In D72018#1800018 , @aaron.ballman 
wrote:

> This seems like a very specialized attribute for the static analyzer and I'm 
> not certain how much utility it adds, but that may be because I don't 
> understand the analyzer requirements sufficiently yet. It seems as though 
> this attribute is intended to suppress diagnostics within a certain function 
> for a certain check, much like `gsl::suppress` does for the C++ Core 
> Guidelines. Is that correct? If so, perhaps we should just reuse that 
> attribute (but with a different spelling)?


In some sense they are similar, but not entirely. When I annotate a function 
with `gsl::suppress`, I would expect to suppress diagnostics inside the 
function. When I annotate a function with `analyzer_checker_ignore`, that would 
suppress diagnostics in the callers of the function. So while the 
`gsl::suppress` is merely a way to suppress diagnostics, this attribute does 
change how the analysis is carried out.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72018/new/

https://reviews.llvm.org/D72018



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72018: [attributes] [analyzer] Add an attribute to prevent checkers from modeling a function

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D72018#1801450 , @xazax.hun wrote:

> In D72018#1800018 , @aaron.ballman 
> wrote:
>
> > This seems like a very specialized attribute for the static analyzer and 
> > I'm not certain how much utility it adds, but that may be because I don't 
> > understand the analyzer requirements sufficiently yet. It seems as though 
> > this attribute is intended to suppress diagnostics within a certain 
> > function for a certain check, much like `gsl::suppress` does for the C++ 
> > Core Guidelines. Is that correct? If so, perhaps we should just reuse that 
> > attribute (but with a different spelling)?
>
>
> In some sense they are similar, but not entirely. When I annotate a function 
> with `gsl::suppress`, I would expect to suppress diagnostics inside the 
> function. When I annotate a function with `analyzer_checker_ignore`, that 
> would suppress diagnostics in the callers of the function. So while the 
> `gsl::suppress` is merely a way to suppress diagnostics, this attribute does 
> change how the analysis is carried out.


Thank you for the explanation, that makes sense, but I'm still a bit 
uncomfortable. In this case, it seems like the author of the API needs to 1) 
know that users will be analyzing the code with clang's static analyzer, 2) and 
that a particular function will cause problems for a specific check. It seems 
like the API author won't be in a position to add the attribute to the correct 
APIs, and what we really need is something for a *user* to write on an existing 
declaration they may not control or have the ability to modify the declaration 
of. I am not certain how much I like this idea, but perhaps we could allow the 
attribute to be written on a redeclaration and apply it to the canonical 
declaration so that users could add the attribute instead of relying on the 
library author to do it for them?

Also, does this mean that changing the name of a check in the static analyzer 
will now have the potential to break code? e.g., if a check move from the alpha 
group to the core group, the check name changes, and thus code needs to be 
updated. I presume one of the things this attribute will do is diagnose when an 
unknown check name is given? This has the potential to introduce diagnostics 
where a library is written for a newer clang but being compiled within an older 
clang, so we'd probably need a warning flag or something else to control the 
diagnostic.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72018/new/

https://reviews.llvm.org/D72018



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72018: [attributes] [analyzer] Add an attribute to prevent checkers from modeling a function

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

In D72018#1801725 , @aaron.ballman 
wrote:

> Thank you for the explanation, that makes sense, but I'm still a bit 
> uncomfortable. In this case, it seems like the author of the API needs to 1) 
> know that users will be analyzing the code with clang's static analyzer, 2) 
> and that a particular function will cause problems for a specific check. It 
> seems like the API author won't be in a position to add the attribute to the 
> correct APIs, and what we really need is something for a *user* to write on 
> an existing declaration they may not control or have the ability to modify 
> the declaration of. I am not certain how much I like this idea, but perhaps 
> we could allow the attribute to be written on a redeclaration and apply it to 
> the canonical declaration so that users could add the attribute instead of 
> relying on the library author to do it for them?


This is a valid concern. Currently, this is a common practice in the static 
analyzer to exclude certain functions from the checks, but until now, we 
usually hard coded the name/signature of the function rather than relying on an 
attribute. This did make sense, since it is an implementation detail that 
belongs to the analyzer. The main reason why we was thinking about making this 
an annotation, because if there is an annotation, when the API changes, we do 
not need to update the analyzer, and do not need to make sure that the analyzer 
version and the API matches.

> Also, does this mean that changing the name of a check in the static analyzer 
> will now have the potential to break code? e.g., if a check move from the 
> alpha group to the core group, the check name changes, and thus code needs to 
> be updated. I presume one of the things this attribute will do is diagnose 
> when an unknown check name is given? This has the potential to introduce 
> diagnostics where a library is written for a newer clang but being compiled 
> within an older clang, so we'd probably need a warning flag or something else 
> to control the diagnostic.

This is correct. If we want to diagnose non-existent checker names we 
definitely would need a diagnostic flag and probably we want to have the check 
off by default. The reason other reason (apart from backward compatibility) is 
that, some users might load non-upstreamed checkers as plugins for some builds 
(but not for others).

I think if there are many problems with this concept, I will fall back to hard 
code this information in the checker instead of using an annotation.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72018/new/

https://reviews.llvm.org/D72018



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72018: [attributes] [analyzer] Add an attribute to prevent checkers from modeling a function

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Would changing the literal in the attribute have the same effect? I.e., 
`acquire_handle("Fuchsia_But_Please_Ignore_Me")`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72018/new/

https://reviews.llvm.org/D72018



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72018: [attributes] [analyzer] Add an attribute to prevent checkers from modeling a function

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D72018#1802636 , @NoQ wrote:

> Would changing the literal in the attribute have the same effect? I.e., 
> `acquire_handle("Fuchsia_But_Please_Ignore_Me")`.


It should, but doesn't currently because we don't have any checking that the 
string literal matches a name in the static analyzer or clang-tidy for that 
attribute.

In D72018#1801739 , @xazax.hun wrote:

> I think if there are many problems with this concept, I will fall back to 
> hard code this information in the checker instead of using an annotation.


How much hard coded information would this remove? How often do you find that 
users want to add to the list of hard coded functions themselves but can't? 
Maybe the benefits to the attribute outweigh the downside and it is worth 
exploring ways to resolve some of these concerns -- I don't have a good feeling 
for the problem space though.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72018/new/

https://reviews.llvm.org/D72018



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72018: [attributes] [analyzer] Add an attribute to prevent checkers from modeling a function

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

In D72018#1803012 , @aaron.ballman 
wrote:

> In D72018#1802636 , @NoQ wrote:
>
> > Would changing the literal in the attribute have the same effect? I.e., 
> > `acquire_handle("Fuchsia_But_Please_Ignore_Me")`.
>
>
> It should, but doesn't currently because we don't have any checking that the 
> string literal matches a name in the static analyzer or clang-tidy for that 
> attribute.


Actually, the static analyzer checker does check for the literal, so this could 
work.

> 
> 
> In D72018#1801739 , @xazax.hun wrote:
> 
>> I think if there are many problems with this concept, I will fall back to 
>> hard code this information in the checker instead of using an annotation.
> 
> 
> How much hard coded information would this remove? How often do you find that 
> users want to add to the list of hard coded functions themselves but can't? 
> Maybe the benefits to the attribute outweigh the downside and it is worth 
> exploring ways to resolve some of these concerns -- I don't have a good 
> feeling for the problem space though.

I expect not to have too many functions that need such exclusions. It was more 
about making it easier to do some exclusions without touching the analyzer. I 
will ask around what are the preferences of the team, maybe Artem's proposal 
will work for us.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72018/new/

https://reviews.llvm.org/D72018



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72018: [attributes] [analyzer] Add an attribute to prevent checkers from modeling a function

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D72018#1803144 , @xazax.hun wrote:

> In D72018#1803012 , @aaron.ballman 
> wrote:
>
> > In D72018#1802636 , @NoQ wrote:
> >
> > > Would changing the literal in the attribute have the same effect? I.e., 
> > > `acquire_handle("Fuchsia_But_Please_Ignore_Me")`.
> >
> >
> > It should, but doesn't currently because we don't have any checking that 
> > the string literal matches a name in the static analyzer or clang-tidy for 
> > that attribute.
>
>
> Actually, the static analyzer checker does check for the literal, so this 
> could work.


Huh, that's interesting. I would expect that check to happen in more than just 
the static analyzer though -- for instance, clang-tidy checks may want to use 
those attributes, or even a less-heavy CFG analysis than the static analyzer 
might.

>> 
>> 
>> In D72018#1801739 , @xazax.hun 
>> wrote:
>> 
>>> I think if there are many problems with this concept, I will fall back to 
>>> hard code this information in the checker instead of using an annotation.
>> 
>> 
>> How much hard coded information would this remove? How often do you find 
>> that users want to add to the list of hard coded functions themselves but 
>> can't? Maybe the benefits to the attribute outweigh the downside and it is 
>> worth exploring ways to resolve some of these concerns -- I don't have a 
>> good feeling for the problem space though.
> 
> I expect not to have too many functions that need such exclusions. It was 
> more about making it easier to do some exclusions without touching the 
> analyzer. I will ask around what are the preferences of the team, maybe 
> Artem's proposal will work for us.

Okay, that makes sense to me, thank you!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72018/new/

https://reviews.llvm.org/D72018



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72018: [attributes] [analyzer] Add an attribute to prevent checkers from modeling a function

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D72018#1803144 , @xazax.hun wrote:

> I expect not to have too many functions that need such exclusions. It was 
> more about making it easier to do some exclusions without touching the 
> analyzer. I will ask around what are the preferences of the team, maybe 
> Artem's proposal will work for us.


I think it ultimately makes sense to introduce analyzer IPA hints ("please 
inline this function", "please evaluate this one conservatively", "please don't 
model effects of this function on checker state", etc.), given that otherwise 
our set of heuristics on this subject is incomprehensible.

But if you have a chance to hardcode a small list of legacy functions with 
weird behavior (that cannot be expressed with annotations; maybe add explicit 
modeling for such functions) and use the analyzer //enforce// the lack of other 
exceptional functions (so that everything else was expressible with 
annotations), that should be the way to go simply because it makes your 
function surfaces much easier to reason about for the programmer.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72018/new/

https://reviews.llvm.org/D72018



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits