[cfe-dev] ASTMatcher for assert()

[Alex Horn]

> 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

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
>> >> >
>> >> >
>> >
>> >
>
>