[prev in list] [next in list] [prev in thread] [next in thread]
List: cfe-dev
Subject: Re: [cfe-dev] Order in which matchers are invoked
From: Billy O'Mahony via cfe-dev <cfe-dev () lists ! llvm ! org>
Date: 2019-11-25 15:15:18
Message-ID: CAEHc-tW5s6e8sAjd4n8qBpiueckH_oZRjwz9BhyO=Vbgmwus8A () mail ! gmail ! com
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
Hi Whisperity,
thanks for the tips. Some follow-ups below..
On Mon, 25 Nov 2019 at 13:36, Whisperity <whisperity@gmail.com> wrote:
> RecursiveASTVisitor has an overridable which enables post-order visitation
> instead of pre-order.
>
From the context of my clang-tidy check's registerMatchers(MatchFinder
*Finder) is this possible?
> Another approach you could take is to match the functions directly, and in
> the callback fire another matcher - you could manually call
> clang::ast_matchers::match(MATCHER, Node); (several other overloads exist)
> which is essentially "fetch me the matched nodes and give me back a data
> structure on the subtree started by 'Node'." Creating custom matchers local
> to your code, you can use the "static const auto M =
> declarative(matcher(syntax()))", or the AST_MATCHER macro, in case your
> matching requires manually investigating the node via directly calling C++
> functions on the node, or calculating some result.
>
So in my existing clang-tidy ::check(const MatchFinder::MatchResult
&Result) I can get the function's Node from Result.Context? Can I register
several matchers here and ensure post-order visitation? Without those two
features the matcher-in-callback strategy would have the same out-of-order
issue.
On the other hand maybe I should forget about clang-tidy? (and do a
stand-alone tool with RecursiveASTVisitor like you suggest). As far as I
know it has drawbacks in that in order to distribute the project-specific
test i have to distribute a full and entire clang-tidy - I can't just
distribute an .so and plug that into an existing clang-tidy install at
run-time.
> I don't know how much of this could reasonably be channeled from the
> checker's code to Clang-Tidy and the execution, or more importantly, how
> much it could mess up potential other checkers running on the same
> invocation...
>
> Another option you could go for is using your check() only as a "data
> collection" callback and build some (reasonably small!) data structure, and
> using "void onEndOfTranslationUnit()" at the end to read your data
> structure, calculate some reasonable result from it, and emit your
> diagnostics.
>
That's my current plan for a work around. Good to learn about EndOfTu
though.
> Billy O'Mahony via cfe-dev <cfe-dev@lists.llvm.org> ezt írta (időpont:
> 2019. nov. 25., H, 11:48):
>
>> Hi,
>>
>> I've been writing a project specific clang-tidy check (thanks to Artem
>> and Steve for their help). I've matchers written and working but was seeing
>> some strange things about the order in which my matcher callbacks were
>> being called.
>>
>> I noticed that in the documentation for MatchFinder it says " The order
>> of matches is guaranteed to be equivalent to doing a pre-order traversal on
>> the AST, and applying the matchers in the order in which they were added to
>> the MatchFinder
>> <https://clang.llvm.org/doxygen/classclang_1_1ast__matchers_1_1MatchFinder.html>
>> ."
>>
>> As my checker is checking that certain functions follow certain steps in
>> order (check the "device" arg ptr for NULL, lock the device, do NOT return
>> while the device is locked, unlock the device) I was expecting (depending
>> on) in AST-order of the match callbacks. Not all the device-ptr-check
>> matches first, then all the return matches, then all the etc etc.matches.
>>
>>
>> So two questions:
>> * Can this call-back order be changed somehow? - worth a shot ;)
>>
>> * Failing this I can maintain information about the function and line
>> number of all the steps in all the functions and figure out the AST-order
>> from that. But I notice that using srcLocation often returns the same value
>> for several statements (this is because in the code the Lock is actually
>> done in a macro which locks the device but also returns if the lock fails -
>> so I end up with a lock and a return both reporting the same srcLocation -
>> is there a way to determine which comes before which. (I think I came
>> across a way of matching on macros too but I had already written working
>> AST matchers at that point).
>>
>> Thanks for reading,
>>
>> Billy.
>>
>>
>> --------------
>>
>>
>> Here's an example of one of the type of function I want to check with
>> some examples of errors in the code order:
>>
>> int api_foo(device_t *dev) {
>> int ret_val = 0;
>>
>> bar(); // fn calls & decls before api_enter is ok- just don't access
>> dev.
>> dev->bla = 1; // NO! device access before api_enter() called
>> api_enter(dev); // error if this call is not present exactly once
>>
>> if (dev->bla)
>> return; // NO! didn't call api_exit before rtn. Also two return
>> points
>>
>> if (dev->ma) {
>> ret_val = 1;
>> goto cleanup;
>> }
>> tweak(dev);
>>
>> cleanup:
>> api_exit(dev); // error if this is not present exactly once
>> dev->bla = 1; //NO! device access after api_exit()
>> return ret_val;
>> }
>> _______________________________________________
>> cfe-dev mailing list
>> cfe-dev@lists.llvm.org
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>>
>
[Attachment #5 (text/html)]
<div dir="ltr"><div>Hi Whisperity,</div><div><br></div><div>thanks for the tips. Some \
follow-ups below..<br></div><br><div class="gmail_quote"><div dir="ltr" \
class="gmail_attr">On Mon, 25 Nov 2019 at 13:36, Whisperity <<a \
href="mailto:whisperity@gmail.com">whisperity@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 \
dir="ltr"><div>RecursiveASTVisitor has an overridable which enables post-order \
visitation instead of pre-order. </div></div></blockquote><div><br></div><div>From \
the context of my clang-tidy check's registerMatchers(MatchFinder *Finder) is \
this possible? <br></div><div> </div><blockquote class="gmail_quote" \
style="margin:0px 0px 0px 0.8ex;border-left:1px solid \
rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div>Another approach you could \
take is to match the functions directly, and in the callback fire another matcher - \
you could manually call clang::ast_matchers::match(MATCHER, Node); (several other \
overloads exist) which is essentially "fetch me the matched nodes and give me \
back a data structure on the subtree started by 'Node'." Creating custom \
matchers local to your code, you can use the "static const auto M = \
declarative(matcher(syntax()))", or the AST_MATCHER macro, in case your matching \
requires manually investigating the node via directly calling C++ functions on the \
node, or calculating some result.</div></div></blockquote><div><br></div><div>So in \
my existing clang-tidy ::check(const MatchFinder::MatchResult &Result) I can get \
the function's Node from Result.Context? Can I register several matchers here and \
ensure post-order visitation? Without those two features the matcher-in-callback \
strategy would have the same out-of-order issue.<br></div><div><br></div><div>On the \
other hand maybe I should forget about clang-tidy? (and do a stand-alone tool with \
RecursiveASTVisitor like you suggest). As far as I know it has drawbacks in that \
in order to distribute the project-specific test i have to distribute a full and \
entire clang-tidy - I can't just distribute an .so and plug that into an existing \
clang-tidy install at run-time.<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 dir="ltr"><div><br></div><div>I don't \
know how much of this could reasonably be channeled from the checker's code to \
Clang-Tidy and the execution, or more importantly, how much it could mess up \
potential other checkers running on the same \
invocation...</div><div><br></div><div>Another option you could go for is using your \
check() only as a "data collection" callback and build some (reasonably \
small!) data structure, and using "void onEndOfTranslationUnit()" at the \
end to read your data structure, calculate some reasonable result from it, and emit \
your diagnostics.</div></div></blockquote><div><br></div><div>That's my current \
plan for a work around. Good to learn about EndOfTu \
though.<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"><br><div \
class="gmail_quote"><div dir="ltr" class="gmail_attr">Billy O'Mahony via cfe-dev \
<<a href="mailto:cfe-dev@lists.llvm.org" \
target="_blank">cfe-dev@lists.llvm.org</a>> ezt írta (időpont: 2019. nov. 25., \
H, 11:48):<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 \
dir="ltr"><div>Hi,</div><div><br></div><div>I've been writing a project specific \
clang-tidy check (thanks to Artem and Steve for their help). I've matchers \
written and working but was seeing some strange things about the order in which my \
matcher callbacks were being called.<br></div><div><br></div><div>I noticed that in \
the documentation for MatchFinder it says " The order of matches is guaranteed \
to be equivalent to doing a pre-order traversal on the AST, and applying the \
matchers in the order in which they were added to the <a \
href="https://clang.llvm.org/doxygen/classclang_1_1ast__matchers_1_1MatchFinder.html" \
title="A class to allow finding matches over the Clang AST." \
target="_blank">MatchFinder</a>."</div><div><br></div><div>As my checker is \
checking that certain functions follow certain steps in order (check the \
"device" arg ptr for NULL, lock the device, do NOT return while the device \
is locked, unlock the device) I was expecting (depending on) in AST-order of the \
match callbacks. Not all the device-ptr-check matches first, then all the return \
matches, then all the etc etc.matches.</div><div><br></div><div><br></div><div>So two \
questions:<br></div><div>* Can this call-back order be changed somehow? - worth a \
shot ;)</div><div><br></div><div>* Failing this I can maintain information about the \
function and line number of all the steps in all the functions and figure out the \
AST-order from that. But I notice that using srcLocation often returns the same value \
for several statements (this is because in the code the Lock is actually done in a \
macro which locks the device but also returns if the lock fails - so I end up with a \
lock and a return both reporting the same srcLocation - is there a way to determine \
which comes before which. (I think I came across a way of matching on macros too but \
I had already written working AST matchers at that \
point).<br></div><div><br></div><div>Thanks for \
reading,</div><div><br></div><div>Billy.<br></div><div><br></div><div><br></div><div>--------------</div><div><br></div><div><br></div><div>
<div>Here's an example of one of the type of function I want to check with some \
examples of errors in the code order:</div><div><br></div><div>int api_foo(device_t \
*<span>dev</span>) {</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 \
<span>dev</span>.</div><div> <span>dev</span>->bla = 1; // NO! device access \
before api_enter() called</div><div> api_enter(<span>dev</span>); // error \
if this call is not present exactly once</div><div><br></div><div> if \
(<span>dev</span>->bla)</div><div> return; // NO! didn't call \
api_exit before rtn. Also two return points</div><div><br></div><div> if \
(<span>dev</span>->ma) {</div><div> ret_val = 1;<br></div><div> \
goto cleanup;</div><div> }</div><div> \
tweak(<span>dev</span>);<br></div><div><br></div><div>cleanup:</div><div> \
api_exit(<span>dev</span>); // error if this is not present exactly \
once<br></div><div> <span>dev</span>->bla = 1; //NO! device access after \
api_exit()<br></div><div> return ret_val;<br></div><div>}</div>
</div></div>
_______________________________________________<br>
cfe-dev mailing list<br>
<a href="mailto:cfe-dev@lists.llvm.org" \
target="_blank">cfe-dev@lists.llvm.org</a><br> <a \
href="https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev" rel="noreferrer" \
target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev</a><br> \
</blockquote></div> </blockquote></div></div>
[Attachment #6 (text/plain)]
_______________________________________________
cfe-dev mailing list
cfe-dev@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic