[cfe-dev] ASTMatcher for assert()

Thu Jul 3 06:16:11 PDT 2014

On Thu, Jul 3, 2014 at 3:02 PM, Alex Horn <alex.horn at cs.ox.ac.uk> wrote:

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

Ah, yes, I've planned to write such an article for a while, but alas, so
far the priorities haven't panned out ...
The best we currently have (afaik) is
http://clang.llvm.org/docs/IntroductionToTheClangAST.html and the talk I
gave at the euro llvm last year (the link has the video).

Cheers & thx for the feeback!,
/Manuel

>
> 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
> >> >> >> >> >
> >> >> >> >> >
> >> >> >> >
> >> >> >> >
> >> >> >
> >> >> >
> >> >
> >> >
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20140703/fe47a20f/attachment.html>