[PATCH] D33537: [clang-tidy] Exception Escape Checker

[Jennifer Yu via Phabricator via cfe-commits]

jyu2 added a comment.

In https://reviews.llvm.org/D33537#771274, @aaron.ballman wrote:

> In https://reviews.llvm.org/D33537#771159, @baloghadamsoftware wrote:
>
> > In https://reviews.llvm.org/D33537#770264, @aaron.ballman wrote:
> >
> > > I think we should try to get as much of this functionality in https://reviews.llvm.org/D33333 as possible; there is a considerable amount of overlap between that functionality and this functionality. This check can then cover only the parts that are not reasonably handled by the frontend check instead of duplicating diagnostics the user already receives.
> >
> >
> > I suppose that the frontend check will not travarse the call-graph just check direct throws. Should we only check indirect throws then?
>
>
> The check in https://reviews.llvm.org/D33333 is using a CFG, not just checking direct throws.




In https://reviews.llvm.org/D33537#807997, @erichkeane wrote:

> In https://reviews.llvm.org/D33537#801889, @baloghadamsoftware wrote:
>
> > In https://reviews.llvm.org/D33537#771274, @aaron.ballman wrote:
> >
> > > The check in https://reviews.llvm.org/D33333 is using a CFG, not just checking direct throws.
> >
> >
> > I tested the latest revision (the fronted patch already included) on my test file. Disregarding of the not so important parameters (`EnabledFunctions` and `IgnoredExceptions`) I do not get warnings for any indirect throws (`indirect_implicit()` and `indirect_explicit()`). So for me it does not seem to be using the CFG. Furthermore, I do not get warning for `throw_and_catch_some()` where `1.1` is a `double` thus `catch(int &)` should not catch it. The same happens in `throw_catch_rethrow_the_rest()`, where `catch(int &)` should not catch `1.1`, but `catch(...)` should catch and rethrow it. This latter may be a bug.
>
>
> Jen Yu can clarify, but I believe it was decided to put the implementation in the CFG, but not do a full analysis (leave that for a later implementation).  It is not doing 'catch' analysis, and I don't believe they decided to do analysis on the function call itself, since the false-positive rate is massive thanks to the C headers.


The catch  analys

In https://reviews.llvm.org/D33537#807997, @erichkeane wrote:

> In https://reviews.llvm.org/D33537#801889, @baloghadamsoftware wrote:
>
> > In https://reviews.llvm.org/D33537#771274, @aaron.ballman wrote:
> >
> > > The check in https://reviews.llvm.org/D33333 is using a CFG, not just checking direct throws.
> >
> >
> > I tested the latest revision (the fronted patch already included) on my test file. Disregarding of the not so important parameters (`EnabledFunctions` and `IgnoredExceptions`) I do not get warnings for any indirect throws (`indirect_implicit()` and `indirect_explicit()`). So for me it does not seem to be using the CFG. Furthermore, I do not get warning for `throw_and_catch_some()` where `1.1` is a `double` thus `catch(int &)` should not catch it. The same happens in `throw_catch_rethrow_the_rest()`, where `catch(int &)` should not catch `1.1`, but `catch(...)` should catch and rethrow it. This latter may be a bug.
>
>
> Jen Yu can clarify, but I believe it was decided to put the implementation in the CFG, but not do a full analysis (leave that for a later implementation).  It is not doing 'catch' analysis, and I don't believe they decided to do analysis on the function call itself, since the false-positive rate is massive thanks to the C headers.




In https://reviews.llvm.org/D33537#801889, @baloghadamsoftware wrote:

> In https://reviews.llvm.org/D33537#771274, @aaron.ballman wrote:
>
> > The check in https://reviews.llvm.org/D33333 is using a CFG, not just checking direct throws.
>
>
> I tested the latest revision (the fronted patch already included) on my test file. Disregarding of the not so important parameters (`EnabledFunctions` and `IgnoredExceptions`) I do not get warnings for any indirect throws (`indirect_implicit()` and `indirect_explicit()`). So for me it does not seem to be using the CFG. Furthermore, I do not get warning for `throw_and_catch_some()` where `1.1` is a `double` thus `catch(int &)` should not catch it. The same happens in `throw_catch_rethrow_the_rest()`, where `catch(int &)` should not catch `1.1`, but `catch(...)` should catch and rethrow it. This latter may be a bug.




In https://reviews.llvm.org/D33537#807997, @erichkeane wrote:

> In https://reviews.llvm.org/D33537#801889, @baloghadamsoftware wrote:
>
> > In https://reviews.llvm.org/D33537#771274, @aaron.ballman wrote:
> >
> > > The check in https://reviews.llvm.org/D33333 is using a CFG, not just checking direct throws.
> >
> >
> > I tested the latest revision (the fronted patch already included) on my test file. Disregarding of the not so important parameters (`EnabledFunctions` and `IgnoredExceptions`) I do not get warnings for any indirect throws (`indirect_implicit()` and `indirect_explicit()`). So for me it does not seem to be using the CFG. Furthermore, I do not get warning for `throw_and_catch_some()` where `1.1` is a `double` thus `catch(int &)` should not catch it. The same happens in `throw_catch_rethrow_the_rest()`, where `catch(int &)` should not catch `1.1`, but `catch(...)` should catch and rethrow it. This latter may be a bug.
>
>
> Jen Yu can clarify, but I believe it was decided to put the implementation in the CFG, but not do a full analysis (leave that for a later implementation).  It is not doing 'catch' analysis, and I don't believe they decided to do analysis on the function call itself, since the false-positive rate is massive thanks to the C headers.




>>>> The patch do not do indirect function call check, since that would cause false-positive.  
>>>> The tests of throw_and_catch_some and throw_catch_rethrow_the_rest no warning emit due the "throw 1.1" is unreachable code after "throw 1", it is not the bug.  I am not sure clang has some option to emit unreachable code.


https://reviews.llvm.org/D33537