<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Jul 3, 2014 at 3:02 PM, Alex Horn <span dir="ltr"><<a href="mailto:alex.horn@cs.ox.ac.uk" target="_blank">alex.horn@cs.ox.ac.uk</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Dear Manuel,<br>
<br>
I see what you mean :) and "change" is probably not the right word<br>
since, as you say, all the features are already there. To use these in<br>
the right way comes probably totally natural to everyone who works<br>
with Clang long enough --- and arguably that is all that matters in<br>
the end.<br>
<br>
As a newcomer to Clang, however, it can be challenging to decide when<br>
one "type of location" (spelling, expansion, plain-old getLocStart<br>
etc.) is the right thing to use and how they relate to each other.<br>
Your answers all helped me tremendously as I stumbled in the dark on<br>
this but (because of the way my brain works) it bothers me that I have<br>
written code that seems to do something useful but I don't understand<br>
why it works and when it doesn't. Since Clang source-to-source<br>
transformations must account for probably very obscure corner cases<br>
there may be no easy answer to all this but if you happen to know of<br>
an article that explains the design behind source locations, this<br>
might be a good way to end our discussion and it may also help others<br>
who might stumble across our discussion in the future.<br></blockquote><div><br></div><div>Ah, yes, I've planned to write such an article for a while, but alas, so far the priorities haven't panned out ...</div><div>
The best we currently have (afaik) is <a href="http://clang.llvm.org/docs/IntroductionToTheClangAST.html">http://clang.llvm.org/docs/IntroductionToTheClangAST.html</a> and the talk I gave at the euro llvm last year (the link has the video).</div>
<div><br></div><div>Cheers & thx for the feeback!,</div><div>/Manuel</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
Thanks for all your support and your generous interpretation of my questions.<br>
<br>
Best,<br>
Alex<br>
<div class=""><div class="h5"><br>
On 3 July 2014 09:36, Manuel Klimek <<a href="mailto:klimek@google.com">klimek@google.com</a>> wrote:<br>
> On Wed, Jul 2, 2014 at 10:15 PM, Alex Horn <<a href="mailto:alex.horn@cs.ox.ac.uk">alex.horn@cs.ox.ac.uk</a>> wrote:<br>
>><br>
>> Dear Manuel,<br>
>><br>
>> Thanks for those suggestions. If the mechanism for the refactoring<br>
>> inside macros ever changes and you happen to remember our little<br>
>> discussion don't hesitate to get in touch and I'd gladly try those<br>
>> changes out should that be helpful for the designers of the library.<br>
><br>
><br>
> Based on this discussion I'm not sure what needs to change :) In general,<br>
> source locations already model all expansions of macros, and we had good<br>
> success writing code transformations through that.<br>
><br>
> Cheers,<br>
> /Manuel<br>
><br>
>><br>
>><br>
>> All the best,<br>
>> Alex<br>
>><br>
>> On 10 June 2014 11:11, Manuel Klimek <<a href="mailto:klimek@google.com">klimek@google.com</a>> wrote:<br>
>> > On Fri, Jun 6, 2014 at 7:45 PM, Alex Horn <<a href="mailto:alex.horn@cs.ox.ac.uk">alex.horn@cs.ox.ac.uk</a>> wrote:<br>
>> >><br>
>> >> > Well, the macro has to be actually used ;) Even if it's a library,<br>
>> >> > I'd<br>
>> >> > hope you have at least a test that uses the macro... (same for<br>
>> >> > templates<br>
>> >> > btw).<br>
>> >><br>
>> >> Indeed, here's a small example:<br>
>> >><br>
>> >> #define foobar while(true) { }<br>
>> >> int main() { foobar }<br>
>> >><br>
>> >> The ASTMatchers (around 500 LOC) that are supposed to rewrite this<br>
>> >> kind of code are located here:<br>
>> >><br>
>> >> <a href="https://github.com/ahorn/native-symbolic-execution-clang" target="_blank">https://github.com/ahorn/native-symbolic-execution-clang</a><br>
>> ><br>
>> ><br>
>> > Ok, I think the problem here is (as opposed to the first example of<br>
>> > assert(true)) you now want to refactor *inside* the macro, so you have<br>
>> > to<br>
>> > use the spelling location.<br>
>> > Thus, you'll need to introduce the same line of code as in my first<br>
>> > reply on<br>
>> > this thread, but replace expansion with spelling:<br>
>> > SourceLocation SpellLocBegin = SM.getSpellingLoc(LocBegin);<br>
>> ><br>
>> > Generally, for any macro location, you have to decide what to do. What I<br>
>> > usually do is that I get a range from the AST node, and use<br>
>> > Lexer::makeFileCharRange to get the location range that captures the AST<br>
>> > range (if possible). This is pretty complex code, as macro expansion is<br>
>> > rather involved, so I wouldn't try to roll it on my own.<br>
>> ><br>
>> > Cheers,<br>
>> > /Manuel<br>
>> ><br>
>> >><br>
>> >> A few notes:<br>
>> >><br>
>> >> Each ASTMatcher checks whether<br>
>> >> Result.Context->getSourceManager().isInWrittenMainFile(); otherwise,<br>
>> >> the tool would try to rewrite system headers etc. But as a sanity<br>
>> >> check, I had also removed the isInWrittenMainFile() checks entirely.<br>
>> >> In either case, the<br>
>> >> "while" loop in the macro will not be rewritten. The case without<br>
>> >> macros works great and I highly suspect that I am doing something<br>
>> >> wrong that would be obvious to a Clang developer.<br>
>> >><br>
>> >> Best,<br>
>> >> Alex<br>
>> >><br>
>> >> On 6 June 2014 17:07, Manuel Klimek <<a href="mailto:klimek@google.com">klimek@google.com</a>> wrote:<br>
>> >> > On Fri, Jun 6, 2014 at 6:04 PM, Alex Horn <<a href="mailto:alex.horn@cs.ox.ac.uk">alex.horn@cs.ox.ac.uk</a>><br>
>> >> > wrote:<br>
>> >> >><br>
>> >> >> > [W]hat's the problem rewriting this inside the macro?<br>
>> >> >><br>
>> >> >> The problem I encountered was that Clang would not automatically<br>
>> >> >> invoke the ASTMatchers inside the macro definitions. The comments I<br>
>> >> >> found before I posted on this mailing list indicated that this is by<br>
>> >> >> design and that it can only be circumvented by rewriting macros<br>
>> >> >> beforehand, i.e. -rewrite-macros. But your previous remark gives me<br>
>> >> >> hope that the situation might have changed, or that I have just<br>
>> >> >> missed<br>
>> >> >> the right mailing list comments.<br>
>> >> ><br>
>> >> ><br>
>> >> > Well, the macro has to be actually used ;) Even if it's a library,<br>
>> >> > I'd<br>
>> >> > hope<br>
>> >> > you have at least a test that uses the macro... (same for templates<br>
>> >> > btw).<br>
>> >> ><br>
>> >> >><br>
>> >> >><br>
>> >> >> > [Use PPCallbacks to] collect information about the macros, and<br>
>> >> >> > then<br>
>> >> >> > use<br>
>> >> >> > that information to generate the replacements.<br>
>> >> >><br>
>> >> >> I'm afraid that with my limited expertise in Clang I would have to<br>
>> >> >> look at an example that illustrates the technique you've in mind. It<br>
>> >> >> may be even that I have not articulated clearly enough and that we<br>
>> >> >> are<br>
>> >> >> thinking of two different things here.<br>
>> >> ><br>
>> >> ><br>
>> >> > I'm, not sure you need that kind of information. See above. Code that<br>
>> >> > is<br>
>> >> > in<br>
>> >> > macros will be visited at every expansion location.<br>
>> >> ><br>
>> >> >><br>
>> >> >><br>
>> >> >> On 6 June 2014 16:24, Manuel Klimek <<a href="mailto:klimek@google.com">klimek@google.com</a>> wrote:<br>
>> >> >> > On Fri, Jun 6, 2014 at 4:48 PM, Alex Horn <<a href="mailto:alex.horn@cs.ox.ac.uk">alex.horn@cs.ox.ac.uk</a>><br>
>> >> >> > wrote:<br>
>> >> >> >><br>
>> >> >> >> > [...] Why not write all tools to work on the<br>
>> >> >> >> > non-macro-rewritten<br>
>> >> >> >> > code?<br>
>> >> >> >> > (you can get macros yourself via the PPCallbacks interface).<br>
>> >> >> >><br>
>> >> >> >> As far as I understand, PPCallbacks operate on a different level<br>
>> >> >> >> of<br>
>> >> >> >> abstraction than ASTMatchers. The RefactoringTool I have in mind<br>
>> >> >> >> rewrites two things: (1) "assert()" macros and (2) control-flow<br>
>> >> >> >> conditions in "if-then-else", "while" and "for" statements. The<br>
>> >> >> >> rewriting of both (1) and (2) is now implemented as ASTMatchers.<br>
>> >> >> >> Unfortunately, (2) does not work correctly unless macros are<br>
>> >> >> >> rewritten<br>
>> >> >> >> beforehand. To see this, consider "#define compare(a, b) do { if<br>
>> >> >> >> ((a)<br>
>> >> >> >> < (b)) swap(a, b) } while(0);".<br>
>> >> >> ><br>
>> >> >> ><br>
>> >> >> > I'm considering it :) So what's the problem rewriting this inside<br>
>> >> >> > the<br>
>> >> >> > macro?<br>
>> >> >> ><br>
>> >> >> >><br>
>> >> >> >> I presumed for now that it does not make sense, generally<br>
>> >> >> >> speaking,<br>
>> >> >> >> to<br>
>> >> >> >> invoke ASTMatchers from within PPCallbacks because the source<br>
>> >> >> >> code<br>
>> >> >> >> inside a macro definition may not be well-formed. This is why I<br>
>> >> >> >> was<br>
>> >> >> >> merely looking for a way to combine ASTMatchers and cc1 drivers.<br>
>> >> >> ><br>
>> >> >> ><br>
>> >> >> > No, my idea on how to use PPCallbacks was to just use them to<br>
>> >> >> > collect<br>
>> >> >> > information about the macros, and then use that information to<br>
>> >> >> > generate<br>
>> >> >> > the<br>
>> >> >> > replacements.<br>
>> >> >> ><br>
>> >> >> >><br>
>> >> >> >><br>
>> >> >> >> Best,<br>
>> >> >> >> Alex<br>
>> >> >> >><br>
>> >> >> >> On 6 June 2014 15:13, Manuel Klimek <<a href="mailto:klimek@google.com">klimek@google.com</a>> wrote:<br>
>> >> >> >> > On Fri, Jun 6, 2014 at 4:10 PM, Alex Horn<br>
>> >> >> >> > <<a href="mailto:alex.horn@cs.ox.ac.uk">alex.horn@cs.ox.ac.uk</a>><br>
>> >> >> >> > wrote:<br>
>> >> >> >> >><br>
>> >> >> >> >> Thanks for the effective response. This helped on the right<br>
>> >> >> >> >> track<br>
>> >> >> >> >> and<br>
>> >> >> >> >> I've successfully used<br>
>> >> >> >> >> SourceManager::getImmediateExpansionRange()<br>
>> >> >> >> >> to<br>
>> >> >> >> >> insert BAR and its surrounding parenthesis correctly.<br>
>> >> >> >> >><br>
>> >> >> >> >> As a directly related issue: The proposed technique of using<br>
>> >> >> >> >> the<br>
>> >> >> >> >> expansion range does the trick but of course requires that<br>
>> >> >> >> >> "-cc1<br>
>> >> >> >> >> -rewrite-macros" is _not_ invoked prior to rewriting<br>
>> >> >> >> >> "assert()"<br>
>> >> >> >> >> macros. However, this is problematic when other rewrites in<br>
>> >> >> >> >> the<br>
>> >> >> >> >> same<br>
>> >> >> >> >> RefactoringTool _do_ rely on macros having been rewritten.<br>
>> >> >> >> >> This<br>
>> >> >> >> >> poses<br>
>> >> >> >> >> the question whether these things can coexist in the same<br>
>> >> >> >> >> RefactoringTool. In particular, is it possible to call the<br>
>> >> >> >> >> RewriteMacros functionality only after certain matches have<br>
>> >> >> >> >> completed?<br>
>> >> >> >> >><br>
>> >> >> >> >> To ask the same thing differently, what is the preferred way<br>
>> >> >> >> >> to<br>
>> >> >> >> >> compose ASTMatchers and cc1 drivers such as RewriteMacros?<br>
>> >> >> >> ><br>
>> >> >> >> ><br>
>> >> >> >> > I wouldn't combine them. Why not write all tools to work on the<br>
>> >> >> >> > non-macro-rewritten code? (you can get macros yourself via the<br>
>> >> >> >> > PPCallbacks<br>
>> >> >> >> > interface).<br>
>> >> >> >> ><br>
>> >> >> >> >><br>
>> >> >> >> >><br>
>> >> >> >> >> On 6 June 2014 13:56, Manuel Klimek <<a href="mailto:klimek@google.com">klimek@google.com</a>> wrote:<br>
>> >> >> >> >> > On Tue, Jun 3, 2014 at 5:36 PM, Alex Horn<br>
>> >> >> >> >> > <<a href="mailto:alex.horn@cs.ox.ac.uk">alex.horn@cs.ox.ac.uk</a>><br>
>> >> >> >> >> > wrote:<br>
>> >> >> >> >> >><br>
>> >> >> >> >> >> Hello,<br>
>> >> >> >> >> >><br>
>> >> >> >> >> >> I am struggling to replace "assert(c)" macro calls by<br>
>> >> >> >> >> >> "BAR(c)"<br>
>> >> >> >> >> >> where<br>
>> >> >> >> >> >> c<br>
>> >> >> >> >> >> is some integer expression and BAR is a function.<br>
>> >> >> >> >> >><br>
>> >> >> >> >> >> The problem manifests itself in<br>
>> >> >> >> >> >> RefactoringTool::applyAllReplacements<br>
>> >> >> >> >> >> which returns false, thereby causing the tool to skip<br>
>> >> >> >> >> >> replacements.<br>
>> >> >> >> >> >><br>
>> >> >> >> >> >> I am using the following ASTMatcher:<br>
>> >> >> >> >> >><br>
>> >> >> >> >> >> StatementMatcher makeAssertMatcher() {<br>
>> >> >> >> >> >> return<br>
>> >> >> >> >> >><br>
>> >> >> >> >> >><br>
>> >> >> >> >> >><br>
>> >> >> >> >> >><br>
>> >> >> >> >> >><br>
>> >> >> >> >> >> callExpr(callee(functionDecl(hasName("__builtin_expect")))).bind("AssertBindId");<br>
>> >> >> >> >> >> }<br>
>> >> >> >> >> >><br>
>> >> >> >> >> >> Note: callExpr(callee(functionDecl(hasName("assert")))<br>
>> >> >> >> >> >> won't<br>
>> >> >> >> >> >> work<br>
>> >> >> >> >> >> here<br>
>> >> >> >> >> >> because assert() is a macro according to the assert.h and<br>
>> >> >> >> >> >> cassert<br>
>> >> >> >> >> >> headers.<br>
>> >> >> >> >> >><br>
>> >> >> >> >> >> Here's an example match using clang-query:<br>
>> >> >> >> >> >><br>
>> >> >> >> >> >> example.cpp:<br>
>> >> >> >> >> >><br>
>> >> >> >> >> >> #include <assert.h><br>
>> >> >> >> >> >> int main() {<br>
>> >> >> >> >> >> assert(false);<br>
>> >> >> >> >> >> return 0;<br>
>> >> >> >> >> >> }<br>
>> >> >> >> >> >><br>
>> >> >> >> >> >> $ clang-query> match<br>
>> >> >> >> >> >> callExpr(callee(functionDecl(hasName("__builtin_expect"))))<br>
>> >> >> >> >> >><br>
>> >> >> >> >> >> Match #1:<br>
>> >> >> >> >> >><br>
>> >> >> >> >> >> example.cpp:4:3: note: "AssertBindId" binds here<br>
>> >> >> >> >> >> assert(false);<br>
>> >> >> >> >> >> ^~~~~~~~~~~~~<br>
>> >> >> >> >> >> /usr/include/assert.h:93:6: note: expanded from macro<br>
>> >> >> >> >> >> 'assert'<br>
>> >> >> >> >> >> (__builtin_expect(!(e), 0) ? __assert_rtn(__func__,<br>
>> >> >> >> >> >> __FILE__,<br>
>> >> >> >> >> >> __LINE__, #e) : (void)0)<br>
>> >> >> >> >> >> ^~~~~~~~~~~~~~~~~~~~~~~~~<br>
>> >> >> >> >> >> 1 match.<br>
>> >> >> >> >> >><br>
>> >> >> >> >> >> But I cannot find the correct way to rewrite the match; the<br>
>> >> >> >> >> >> following<br>
>> >> >> >> >> >> replacement is skipped (i.e. see line 295 in<br>
>> >> >> >> >> >> Refactoring.cpp):<br>
>> >> >> >> >> >><br>
>> >> >> >> >> >> const CallExpr *E =<br>
>> >> >> >> >> >> Result.Nodes.getNodeAs<CallExpr>("AssertBindId");<br>
>> >> >> >> >> >> SourceManager &SM = *Result.SourceManager;<br>
>> >> >> >> >> >> SourceLocation LocBegin = E->getLocStart();<br>
>> >> >> >> >> >> Replace->insert(tooling::Replacement(SM, LocBegin,<br>
>> >> >> >> >> >> /* number of letters in assert */ 6, "BAR"));<br>
>> >> >> >> >> >><br>
>> >> >> >> >> >> What is the correct way (if any) to rewrite call<br>
>> >> >> >> >> >> expressions<br>
>> >> >> >> >> >> of<br>
>> >> >> >> >> >> macros<br>
>> >> >> >> >> >> such as "assert()"?<br>
>> >> >> >> >> ><br>
>> >> >> >> >> ><br>
>> >> >> >> >> > You need to get the expansion location:<br>
>> >> >> >> >> > SourceLocation ExpLocBegin = SM.getExpansionLoc(LocBegin);<br>
>> >> >> >> >> > Replace->insert(tooling::Replacement(SM, ExpLocBegin, 6,<br>
>> >> >> >> >> > "BAR"));<br>
>> >> >> >> >> ><br>
>> >> >> >> >> > Something like this should do the trick...<br>
>> >> >> >> >> > Cheers,<br>
>> >> >> >> >> > /Manuel<br>
>> >> >> >> ><br>
>> >> >> >> ><br>
>> >> >> ><br>
>> >> >> ><br>
>> >> ><br>
>> >> ><br>
>> ><br>
>> ><br>
><br>
><br>
</div></div></blockquote></div><br></div></div>