clang-tidy / static analysis

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

clang-tidy / static analysis

Boris Kolpackov via cfe-dev
hello clang clan,

  I'm getting started with checkers and I have one in mind that I don't see a good way to implement.

The idea is to find all uses of putenv and replace them w/ setenv. This requires analyzing the argument to discover constant parts in it (setenv require a separate variable name). It also require to check that the argument is not modified (or such modifications need also to be replaces w/ setenvs).

Is there a way to use the fixit infrastructure together with the static analyzer?
If so, is it possible to make multiple changes part of the same fixit?
What would be the recommended way of proceeding in this case? any similar checks/analysis I can take a look at?

Thanks a lot,


       Maurizio

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: clang-tidy / static analysis

Boris Kolpackov via cfe-dev

Hi,

clang-tidy seems to be the correct point to start. It is made to rewrite source code and is easy to begin with.

Take a look into the clang-tools-extra repository and browse the code, there are a lot of examples. There is a dedicated tutorial in the docs to get started as well.


Am 12.11.2017 um 14:43 schrieb Maurizio Vitale via cfe-dev:
hello clang clan,

  I'm getting started with checkers and I have one in mind that I don't see a good way to implement.

The idea is to find all uses of putenv and replace them w/ setenv. This requires analyzing the argument to discover constant parts in it (setenv require a separate variable name). It also require to check that the argument is not modified (or such modifications need also to be replaces w/ setenvs).

Is there a way to use the fixit infrastructure together with the static analyzer?
If so, is it possible to make multiple changes part of the same fixit?
What would be the recommended way of proceeding in this case? any similar checks/analysis I can take a look at?

Thanks a lot,


       Maurizio


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: clang-tidy / static analysis

Boris Kolpackov via cfe-dev
Thanks. I didn't realize that some flow information is available in clang tidy. All the example I saw were local matches of the AST.
But I see now that checks like https://clang.llvm.org/extra/clang-tidy/checks/misc-use-after-move.html do require analysis not dissimilar from what I need, so I now have something to study.

On Sun, Nov 12, 2017 at 3:50 PM, Jonas Toth <[hidden email]> wrote:

Hi,

clang-tidy seems to be the correct point to start. It is made to rewrite source code and is easy to begin with.

Take a look into the clang-tools-extra repository and browse the code, there are a lot of examples. There is a dedicated tutorial in the docs to get started as well.


Am 12.11.2017 um 14:43 schrieb Maurizio Vitale via cfe-dev:
hello clang clan,

  I'm getting started with checkers and I have one in mind that I don't see a good way to implement.

The idea is to find all uses of putenv and replace them w/ setenv. This requires analyzing the argument to discover constant parts in it (setenv require a separate variable name). It also require to check that the argument is not modified (or such modifications need also to be replaces w/ setenvs).

Is there a way to use the fixit infrastructure together with the static analyzer?
If so, is it possible to make multiple changes part of the same fixit?
What would be the recommended way of proceeding in this case? any similar checks/analysis I can take a look at?

Thanks a lot,


       Maurizio


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev



_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: clang-tidy / static analysis

Boris Kolpackov via cfe-dev
In reply to this post by Boris Kolpackov via cfe-dev
[Please reply *only* to the list and do not include my email directly
in the To: or Cc: of your reply; otherwise I will not see your reply.
Thanks.]

In article <[hidden email]>,
    Maurizio Vitale via cfe-dev <[hidden email]> writes:

> The idea is to find all uses of putenv and replace them w/ setenv. This
> requires analyzing the argument to discover constant parts in it (setenv
> require a separate variable name). It also require to check that the
> argument is not modified (or such modifications need also to be replaces w/
> setenvs).

In addition to what's already been mentioned, I would recommend
implementing your check in stages:

1. Constant string arguments

    putenv("FOO=bar");

    =>

    setenv("FOO", "bar", 1);

2. Constant environment variable names

    char buff[256];
    sprintf(buff, "FOO=%d", value);
    putenv(buff);

    =>

    char buff[256];
    sprintf(buff, "%d", value);
    setenv("FOO", buff, 1);

3. Varying environment variable names

    .... you get the idea :)

Basically start with the simplest case and get that working and then
enhance for subsequent cases.  You can even submit incremental work
for review and incorporation into clang-tidy this way.  It is better
to have a working check that handles simple cases and does no harm on
complex cases than to wait for a check that covers 100% of everything.
--
"The Direct3D Graphics Pipeline" free book <http://tinyurl.com/d3d-pipeline>
            The Terminals Wiki <http://terminals-wiki.org>
     The Computer Graphics Museum <http://ComputerGraphicsMuseum.org>
  Legalize Adulthood! (my blog) <http://LegalizeAdulthood.wordpress.com>
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: clang-tidy / static analysis

Boris Kolpackov via cfe-dev
Hello again,

I'm working on putenv->setenv rewrite as a way to get my feet wet with clang-tidy

I managed to get detection working fine. Now I'm starting working on recommending fixes and I'd like to reduce the matches to what I'm able to rewrite.
The first should capture a simple putenv("VAR=VALUE"). The following works in clang-query:

clang-query> let p callee(functionDecl(hasName("::putenv")))
clang-query> match callExpr(p, argumentCountIs(1), hasArgument(0, stringLiteral().bind("arg")))

Match #1:

/tmp/mav_clang/build/../third_party/clang-tools-extra/test/clang-tidy/misc-putenv.cpp:7:10: note: "arg" binds here
  putenv("VAR=VALUE");
         ^~~~~~~~~~~
/tmp/mav_clang/build/../third_party/clang-tools-extra/test/clang-tidy/misc-putenv.cpp:7:3: note: "root" binds here
  putenv("VAR=VALUE");
  ^~~~~~~~~~~~~~~~~~~
1 match.

But when I convert it to code in a clang-tidy check, as in:

  auto putenvCallee = callee(functionDecl(hasName("::putenv")));

  auto putenvConstantCallMatcher =
      callExpr(putenvCallee, argumentCountIs(1),
                   hasArgument(0, stringLiteral().bind("putenv_arg")))
          .bind("putenv_call");

  Finder->addMatcher(putenvConstantCallMatcher, this);

the match fails.

I'm a bit puzzled by clang-query behaving differently from the compiled checker.

If I replace the stringLiteral() matcher with expr(), then it does match, but it is too loose of a match.

I'm probably missing something obvious and I would appreciate any help.
[feel free to throw away my matchers and show me the proper way to match ::putenv("a string") and not ::putenv(a_char_ptr_var)]

Thanks a lot,

   Maurizio


On Mon, Nov 13, 2017 at 11:51 AM, Richard via cfe-dev <[hidden email]> wrote:
[Please reply *only* to the list and do not include my email directly
in the To: or Cc: of your reply; otherwise I will not see your reply.
Thanks.]

In article <[hidden email]>,
    Maurizio Vitale via cfe-dev <[hidden email]> writes:

> The idea is to find all uses of putenv and replace them w/ setenv. This
> requires analyzing the argument to discover constant parts in it (setenv
> require a separate variable name). It also require to check that the
> argument is not modified (or such modifications need also to be replaces w/
> setenvs).

In addition to what's already been mentioned, I would recommend
implementing your check in stages:

1. Constant string arguments

    putenv("FOO=bar");

    =>

    setenv("FOO", "bar", 1);

2. Constant environment variable names

    char buff[256];
    sprintf(buff, "FOO=%d", value);
    putenv(buff);

    =>

    char buff[256];
    sprintf(buff, "%d", value);
    setenv("FOO", buff, 1);

3. Varying environment variable names

    .... you get the idea :)

Basically start with the simplest case and get that working and then
enhance for subsequent cases.  You can even submit incremental work
for review and incorporation into clang-tidy this way.  It is better
to have a working check that handles simple cases and does no harm on
complex cases than to wait for a check that covers 100% of everything.
--
"The Direct3D Graphics Pipeline" free book <http://tinyurl.com/d3d-pipeline>
            The Terminals Wiki <http://terminals-wiki.org>
     The Computer Graphics Museum <http://ComputerGraphicsMuseum.org>
  Legalize Adulthood! (my blog) <http://LegalizeAdulthood.wordpress.com>
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: clang-tidy / static analysis

Boris Kolpackov via cfe-dev
I think you might miss a `allOf` in your callExpr matcher. 

Combining multiple conditions needs the logical operations (allOf, anyOf, none of, ...).

Am 22.11.2017 3:53 vorm. schrieb "Maurizio Vitale via cfe-dev" <[hidden email]>:
Hello again,

I'm working on putenv->setenv rewrite as a way to get my feet wet with clang-tidy

I managed to get detection working fine. Now I'm starting working on recommending fixes and I'd like to reduce the matches to what I'm able to rewrite.
The first should capture a simple putenv("VAR=VALUE"). The following works in clang-query:

clang-query> let p callee(functionDecl(hasName("::putenv")))
clang-query> match callExpr(p, argumentCountIs(1), hasArgument(0, stringLiteral().bind("arg")))

Match #1:

/tmp/mav_clang/build/../third_party/clang-tools-extra/test/clang-tidy/misc-putenv.cpp:7:10: note: "arg" binds here
  putenv("VAR=VALUE");
         ^~~~~~~~~~~
/tmp/mav_clang/build/../third_party/clang-tools-extra/test/clang-tidy/misc-putenv.cpp:7:3: note: "root" binds here
  putenv("VAR=VALUE");
  ^~~~~~~~~~~~~~~~~~~
1 match.

But when I convert it to code in a clang-tidy check, as in:

  auto putenvCallee = callee(functionDecl(hasName("::putenv")));

  auto putenvConstantCallMatcher =
      callExpr(putenvCallee, argumentCountIs(1),
                   hasArgument(0, stringLiteral().bind("putenv_arg")))
          .bind("putenv_call");

  Finder->addMatcher(putenvConstantCallMatcher, this);

the match fails.

I'm a bit puzzled by clang-query behaving differently from the compiled checker.

If I replace the stringLiteral() matcher with expr(), then it does match, but it is too loose of a match.

I'm probably missing something obvious and I would appreciate any help.
[feel free to throw away my matchers and show me the proper way to match ::putenv("a string") and not ::putenv(a_char_ptr_var)]

Thanks a lot,

   Maurizio


On Mon, Nov 13, 2017 at 11:51 AM, Richard via cfe-dev <[hidden email]> wrote:
[Please reply *only* to the list and do not include my email directly
in the To: or Cc: of your reply; otherwise I will not see your reply.
Thanks.]

In article <[hidden email]>,
    Maurizio Vitale via cfe-dev <[hidden email]> writes:

> The idea is to find all uses of putenv and replace them w/ setenv. This
> requires analyzing the argument to discover constant parts in it (setenv
> require a separate variable name). It also require to check that the
> argument is not modified (or such modifications need also to be replaces w/
> setenvs).

In addition to what's already been mentioned, I would recommend
implementing your check in stages:

1. Constant string arguments

    putenv("FOO=bar");

    =>

    setenv("FOO", "bar", 1);

2. Constant environment variable names

    char buff[256];
    sprintf(buff, "FOO=%d", value);
    putenv(buff);

    =>

    char buff[256];
    sprintf(buff, "%d", value);
    setenv("FOO", buff, 1);

3. Varying environment variable names

    .... you get the idea :)

Basically start with the simplest case and get that working and then
enhance for subsequent cases.  You can even submit incremental work
for review and incorporation into clang-tidy this way.  It is better
to have a working check that handles simple cases and does no harm on
complex cases than to wait for a check that covers 100% of everything.
--
"The Direct3D Graphics Pipeline" free book <http://tinyurl.com/d3d-pipeline>
            The Terminals Wiki <http://terminals-wiki.org>
     The Computer Graphics Museum <http://ComputerGraphicsMuseum.org>
  Legalize Adulthood! (my blog) <http://LegalizeAdulthood.wordpress.com>
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: clang-tidy / static analysis

Boris Kolpackov via cfe-dev
Thanks, but I don't think that's the reason. 

The documentation says "For convenience, all node matchers take an arbitrary number of arguments and implicitly act as allOf matchers." and I already had multiple matchers working fine.
But it seems like stringLiteral doesn't match there, even though it does in clang-query and expr() does work in its stead. Dumping the AST shows an implicitCastExpr there, but even adding it around stringLiteral doesn't work.

I'm not sure how to debug, but I guess it is time to learn how to unleash gdb on clang-tidy.

On Wed, Nov 22, 2017 at 8:22 AM, Jonas Toth <[hidden email]> wrote:
I think you might miss a `allOf` in your callExpr matcher. 

Combining multiple conditions needs the logical operations (allOf, anyOf, none of, ...).

Am 22.11.2017 3:53 vorm. schrieb "Maurizio Vitale via cfe-dev" <[hidden email]>:
Hello again,

I'm working on putenv->setenv rewrite as a way to get my feet wet with clang-tidy

I managed to get detection working fine. Now I'm starting working on recommending fixes and I'd like to reduce the matches to what I'm able to rewrite.
The first should capture a simple putenv("VAR=VALUE"). The following works in clang-query:

clang-query> let p callee(functionDecl(hasName("::putenv")))
clang-query> match callExpr(p, argumentCountIs(1), hasArgument(0, stringLiteral().bind("arg")))

Match #1:

/tmp/mav_clang/build/../third_party/clang-tools-extra/test/clang-tidy/misc-putenv.cpp:7:10: note: "arg" binds here
  putenv("VAR=VALUE");
         ^~~~~~~~~~~
/tmp/mav_clang/build/../third_party/clang-tools-extra/test/clang-tidy/misc-putenv.cpp:7:3: note: "root" binds here
  putenv("VAR=VALUE");
  ^~~~~~~~~~~~~~~~~~~
1 match.

But when I convert it to code in a clang-tidy check, as in:

  auto putenvCallee = callee(functionDecl(hasName("::putenv")));

  auto putenvConstantCallMatcher =
      callExpr(putenvCallee, argumentCountIs(1),
                   hasArgument(0, stringLiteral().bind("putenv_arg")))
          .bind("putenv_call");

  Finder->addMatcher(putenvConstantCallMatcher, this);

the match fails.

I'm a bit puzzled by clang-query behaving differently from the compiled checker.

If I replace the stringLiteral() matcher with expr(), then it does match, but it is too loose of a match.

I'm probably missing something obvious and I would appreciate any help.
[feel free to throw away my matchers and show me the proper way to match ::putenv("a string") and not ::putenv(a_char_ptr_var)]

Thanks a lot,

   Maurizio


On Mon, Nov 13, 2017 at 11:51 AM, Richard via cfe-dev <[hidden email]> wrote:
[Please reply *only* to the list and do not include my email directly
in the To: or Cc: of your reply; otherwise I will not see your reply.
Thanks.]

In article <[hidden email]>,
    Maurizio Vitale via cfe-dev <[hidden email]> writes:

> The idea is to find all uses of putenv and replace them w/ setenv. This
> requires analyzing the argument to discover constant parts in it (setenv
> require a separate variable name). It also require to check that the
> argument is not modified (or such modifications need also to be replaces w/
> setenvs).

In addition to what's already been mentioned, I would recommend
implementing your check in stages:

1. Constant string arguments

    putenv("FOO=bar");

    =>

    setenv("FOO", "bar", 1);

2. Constant environment variable names

    char buff[256];
    sprintf(buff, "FOO=%d", value);
    putenv(buff);

    =>

    char buff[256];
    sprintf(buff, "%d", value);
    setenv("FOO", buff, 1);

3. Varying environment variable names

    .... you get the idea :)

Basically start with the simplest case and get that working and then
enhance for subsequent cases.  You can even submit incremental work
for review and incorporation into clang-tidy this way.  It is better
to have a working check that handles simple cases and does no harm on
complex cases than to wait for a check that covers 100% of everything.
--
"The Direct3D Graphics Pipeline" free book <http://tinyurl.com/d3d-pipeline>
            The Terminals Wiki <http://terminals-wiki.org>
     The Computer Graphics Museum <http://ComputerGraphicsMuseum.org>
  Legalize Adulthood! (my blog) <http://LegalizeAdulthood.wordpress.com>
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev



_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: clang-tidy / static analysis

Boris Kolpackov via cfe-dev
[Please reply *only* to the list and do not include my email directly
in the To: or Cc: of your reply; otherwise I will not see your reply.
Thanks.]

In article <CAAeLbQLgNGRUwaxE4rwRSE74dm6X-KmM6GtZB2q2V+iacep1=[hidden email]>,
    Maurizio Vitale via cfe-dev <[hidden email]> writes:

> [...] Dumping the AST shows an
> implicitCastExpr there, but even adding it around stringLiteral doesn't
> work.

I don't see anything wrong with your approach; it's the one I've used
successfully in the past.  However, I haven't attempted any clang-tidy
hacking in quite some time so something in the infrastructure might
have changed in the meantime.

I've generally gone through cycles of using clang-query, dumping the
parsed AST and trying things in clang-tidy.
--
"The Direct3D Graphics Pipeline" free book <http://tinyurl.com/d3d-pipeline>
            The Terminals Wiki <http://terminals-wiki.org>
     The Computer Graphics Museum <http://ComputerGraphicsMuseum.org>
  Legalize Adulthood! (my blog) <http://LegalizeAdulthood.wordpress.com>
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: clang-tidy / static analysis

Boris Kolpackov via cfe-dev
If that works do it :)

Maybe a ignoreImplicitCasts is worth a try. 
I usually brute force matchers until they work. If someone knows better ways please teach me :)

Am 22.11.2017 5:52 nachm. schrieb "Richard via cfe-dev" <[hidden email]>:
[Please reply *only* to the list and do not include my email directly
in the To: or Cc: of your reply; otherwise I will not see your reply.
Thanks.]

In article <CAAeLbQLgNGRUwaxE4rwRSE74dm6X-KmM6GtZB2q2V+iacep1=[hidden email]>,
    Maurizio Vitale via cfe-dev <[hidden email]> writes:

> [...] Dumping the AST shows an
> implicitCastExpr there, but even adding it around stringLiteral doesn't
> work.

I don't see anything wrong with your approach; it's the one I've used
successfully in the past.  However, I haven't attempted any clang-tidy
hacking in quite some time so something in the infrastructure might
have changed in the meantime.

I've generally gone through cycles of using clang-query, dumping the
parsed AST and trying things in clang-tidy.
--
"The Direct3D Graphics Pipeline" free book <http://tinyurl.com/d3d-pipeline>
            The Terminals Wiki <http://terminals-wiki.org>
     The Computer Graphics Museum <http://ComputerGraphicsMuseum.org>
  Legalize Adulthood! (my blog) <http://LegalizeAdulthood.wordpress.com>
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev