[cfe-dev] ASTMatcher for assert()

[Alex Horn]

Dear Manuel,

I see what you mean :) and "change" is probably not the right word
since, as you say, all the features are already there. To use these in
the right way comes probably totally natural to everyone who works
with Clang long enough --- and arguably that is all that matters in
the end.

As a newcomer to Clang, however, it can be challenging to decide when
one "type of location" (spelling, expansion, plain-old getLocStart
etc.) is the right thing to use and how they relate to each other.
Your answers all helped me tremendously as I stumbled in the dark on
this but (because of the way my brain works) it bothers me that I have
written code that seems to do something useful but I don't understand
why it works and when it doesn't. Since Clang source-to-source
transformations must account for probably very obscure corner cases
there may be no easy answer to all this but if you happen to know of
an article that explains the design behind source locations, this
might be a good way to end our discussion and it may also help others
who might stumble across our discussion in the future.

Thanks for all your support and your generous interpretation of my questions.

Best,
Alex

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