<div dir="ltr"><div>Hi Artem,</div><div><br></div><div>thanks for your well thought-out and useful reply. It sounds like clang-tidy will be the sweet spot between usefulness and effort to implement.</div><div><br></div><div>I have a few other responses down below.</div><div><br></div><div>Regards,</div><div>Billy.<br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, 3 Jul 2019 at 23:23, Artem Dergachev <<a href="mailto:noqnoqneo@gmail.com">noqnoqneo@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div bgcolor="#FFFFFF">
Hi,<br>
<br>
It depends on how strict do you want the checking be and on the
details of the rule. If you're designing a new API from scratch and
stuck with gcc forever, i wouldn't mind using the gcc
__attribute__((cleanup())) for your purpose.<br>
<br></div></blockquote><div>I didn't know about that gcc attrib. I need to read the gcc manual attrib section! I should've added that while we will be developing on gcc the code should be compilable on other toolchains/OSs also, so we are avoiding any gcc extensions (e.g. gcc has extensions for thread local storage but we are not using those for the same reason). <br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div bgcolor="#FFFFFF">
The rule you described should be reasonably easy to implement with
the Static Analyzer. The good side of it is that you get a lot of
semantic modeling for free. For instance, if the developer copies
`dev` into a local variable and then uses that local variable
outside of api_enter..api_exit, the tool will be able to handle
transparently, as it deals with values rather than with variables.</div></blockquote><div>That is really cool.</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div bgcolor="#FFFFFF">
Also it will probably be the easiest tool for your problem. The
downside would be that it's not guaranteed to find all bugs; it'll
inevitably give up on complicated code with high cyclomatic
complexity :) So if you want strict/paranoid enforcement of rules,
the Static Analyzer is not the right tool. But if you want to simply
find some bugs for free, it's the right tool.<br>
<br>
It sounds as if your problem is not inter-procedural. Let me
double-check this: would you have another api_enter..api_exit pair
in the body of your tweak() function? Or is just one
api_enter..api_exit enough? Or is it a bug to call api_enter twice
without an api_exit in between?<br></div></blockquote><div>Yes in this case tweak() could be another public fn of the api that would also have an enter/exit pair. But we are using recursive mutexes (a thread can acquire the same mutex N times (ie call api_enter) so long as it also releases N times) so that will be okay. <br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div bgcolor="#FFFFFF">
<br>
If you have to write api_enter..api_exit in *every* function that
deals with devices, then the problem is not inter-procedural, which
makes it much easier. In particular, you should be able to come up
with a purely syntactic analysis ("every function that accesses a
device_t must start with api_enter() and must end in exactly one
spot with api_exit()").</div></blockquote><div>We will only insist on enter/exit in public API functions. Which are the only ones a client application can call. Internal private functions we won't have locking (as they can only be called ultimately from a public function so the device will be locked.) We are going to allow private fns to call back into the api via the public i/face. But we will have the public functions in specific files and have a specific naming prefix.<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div> Such analysis should be easily do-able in
clang-tidy as long as you're satisfied with this level of
aggressiveness. In particular, you'll have to be willing to
sacrifice code like this:<br>
<br>
void foo(device_t *dev) {<br>
if (flip_a_coin()) {<br>
api_enter(dev);<br>
...<br>
api_exit(dev);<br>
}<br>
}<br>
<br>
But it may be perfectly fine if you seriously want to enforce a
strict structure on all your functions that deal with devices.</div></blockquote><div><br></div><div>So is it the case that clang-tidy kind of passes info to the checker-extension in a syntactic code-parsing order. Whereas the static analyzer passes information to the checker in a simulated run-time order?</div><div><br></div><div>E.g in your foo() above my proposed checker gets fed 1) theres a function called foo, 2) theres an if with a call to flip_a_coin 3) in the true case there is a call to enter then exit 4) in the else there is nothing 5) there is a return (at which point my checker would need to be pretty smart and hold a lot of state to figure out something was wrong) . And to compare for the static analyzer it's more like 1) there is fn foo 2.1) there is a code path through foo with enter/exit 2.2) there is a code path with just return (at which point my reasonably simple checker would raise an error).<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div>
<br>
I think the truly-truly right tool for your problem would be to come
up with a custom analysis over Clang CFG.</div></blockquote><div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div bgcolor="#FFFFFF">.<br></div></blockquote><div>By CFG you mean the Clang Static Analyzer? </div>
<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div> It would be harder to
implement, but it would allow you to express things like "every
execution path within a function that accesses `dev` must have a
api_enter before it and an api_exit after it; you are not allowed to
copy `dev` around". This would strictly enforce the rule.</div></blockquote><div>Yes that would be great. But I think just using clang-tidy from what you are saying would get us a long way. And there are heaps of simpler checks we would like to implement also.<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div> At the
same time it'll allow you to lift the requirement of exactly one
return point - you would still be able to ensure that all accesses
are covered. If you need to allow to copy `dev` around, it should
still be doable, but it will be significantly more difficult to
implement<br></div></blockquote><div>Does 'copy around' include passing to my private fns such as tweak()?. We don't need to copy dev anywhere within the public fns but we do need it to pass it to private fns.<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div bgcolor="#FFFFFF">
<br>
<div class="gmail-m_2793145245148744982moz-cite-prefix">On 7/2/19 12:13 PM, Billy O'Mahony via
cfe-dev wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">
<div>Hello,</div>
<div><br>
</div>
<div>I'd like to write a rule for either clang-tidy or static
analyzer to help catch some potential errors in a project I'm
working on.</div>
<div><br>
</div>
<div>My questions are: <br>
</div>
<div> a) is only one or the other will be able to do what I
want to do?<br>
</div>
<div> b) if both are feasible which would have the simpler
implementation?</div>
<div><br>
</div>
<div>The project involves writing an API that will run in a
multi-threaded application and is responsible for serializing
all access to a device structure. Therefore the first thing in
every function in the API must be to call api_enter (which
will among other things acquire a mutex on the device) and the
last thing before returning must be to call api_exit. Also I
want to enforce single exit point from every API function - or
certainly if there are any return points that bypass the
api_exit call.<br>
</div>
<div><br>
</div>
<div>So here is an example function with errors I want to catch
highlighted.</div>
<div><br>
</div>
<div>int api_foo(device_t *dev) {</div>
<div> int ret_val = 0;<br>
</div>
<div><br>
</div>
<div> bar(); // fn calls & decls before api_enter is ok-
just don't access dev.</div>
<div> dev->bla = 1; // NO! device access before
api_enter() called</div>
<div> api_enter(dev); // error if this call is not present
exactly once</div>
<div><br>
</div>
<div> if (dev->bla)</div>
<div> return; // NO! didn't call api_exit before rtn.
Also two return points</div>
<div><br>
</div>
<div> if (dev->ma) {</div>
<div> ret_val = 1;<br>
</div>
<div> goto cleanup;</div>
<div> }</div>
<div> tweak(dev);<br>
</div>
<div><br>
</div>
<div>cleanup:</div>
<div> api_exit(dev); // error if this is not present exactly
once<br>
</div>
<div> dev->bla = 1; //NO! device access after api_exit()<br>
</div>
<div> return ret_val;<br>
</div>
<div>}</div>
<div><br>
</div>
<div>I don't think it matters but the project is C compiled with
gcc.<br>
</div>
<div><br>
</div>
<div>Also if both are feasible any other pointers, tips or good
resources would be appreciated. E.g is there a totally
different methodology I'm not considering - e.g. would using
something like pycparser be a lot easier - though I'd prefer
to keep it in clang as we plan to use tidy & static
analyzer in any case for standard QA.<br>
</div>
<div><br>
</div>
<div>Thanks for reading,</div>
<div>Billy.<br>
</div>
</div>
<br>
<fieldset class="gmail-m_2793145245148744982mimeAttachmentHeader"></fieldset>
<pre class="gmail-m_2793145245148744982moz-quote-pre">_______________________________________________
cfe-dev mailing list
<a class="gmail-m_2793145245148744982moz-txt-link-abbreviated" href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a>
<a class="gmail-m_2793145245148744982moz-txt-link-freetext" href="https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev</a>
</pre>
</blockquote>
<br>
</div>
</blockquote></div></div>