Page MenuHomePhabricator

[clang-tidy] Optimize query in bugprone-exception-escape
ClosedPublic

Authored by baloghadamsoftware on Oct 12 2018, 1:56 AM.

Details

Summary

Checking whether a functions throws indirectly may be very expensive because it needs to visit its whole call graph. Therefore we should first check whether the function is forbidden to throw and only check whether it throws afterward. This also seems to solve bug bug 39167 where the execution time is so long that it seems to hang.

Diff Detail

Repository
rL LLVM

Event Timeline

This looks reasonable to me but could you please explain a bit what the issue was in the bugreport? So a very deep hierarchy causing the problem makes sense, but why was "ignoring first" the difference maker?
Would it make sense to add a warning in the documentation that this check can be very expensive? And are there measures to speed the process up somehow?

I think that with this optimization it is not so expensive anymore. I do not think it was an endless loop in the bugreport but it was insufferable execution time. Maybe we could speed it up a little more by changing it totally to a width-first CFG visitor. Then we could apply your solution as well (not removing visited function from the call stack) so the algorithm would visit every called function (for every function that should not throw) only once along the shortest path. This would not introduce new false positives neither would it lose true positives.

I think that with this optimization it is not so expensive anymore. I do not think it was an endless loop in the bugreport but it was insufferable execution time. Maybe we could speed it up a little more by changing it totally to a width-first CFG visitor. Then we could apply your solution as well (not removing visited function from the call stack) so the algorithm would visit every called function (for every function that should not throw) only once along the shortest path. This would not introduce new false positives neither would it lose true positives.

In the debugging if have seen some functions analyzed thousands of times so I think this would really make a difference. Caching the result might work too?
If you have time for this I would really appreciate if you took a look into!

I think that with this optimization it is not so expensive anymore. I do not think it was an endless loop in the bugreport but it was insufferable execution time. Maybe we could speed it up a little more by changing it totally to a width-first CFG visitor. Then we could apply your solution as well (not removing visited function from the call stack) so the algorithm would visit every called function (for every function that should not throw) only once along the shortest path. This would not introduce new false positives neither would it lose true positives.

In the debugging if have seen some functions analyzed thousands of times so I think this would really make a difference. Caching the result might work too?

Thousands? After the query optimization the max was 173, and that only for a single function. The next number was 64.

Thousands? After the query optimization the max was 173, and that only for a single function. The next number was 64.

See https://bugs.llvm.org/attachment.cgi?id=20963
I did not run again with your patch. But even 173 and 64 seem like a lot and might be worth optimizing.

Warning added to the docs.

I think further optimization steps should be done is separate patches. However, this is the biggest step.

JonasToth accepted this revision.Oct 13 2018, 1:46 AM

I think further optimization steps should be done is separate patches. However, this is the biggest step.

Agreed.

LGTM.

This revision is now accepted and ready to land.Oct 13 2018, 1:46 AM
This revision was automatically updated to reflect the committed changes.
MTC added a subscriber: MTC.Oct 15 2018, 1:21 AM