[PATCH] D106823: [analyzer][solver] Iterate to a fixpoint during symbol simplification with constants

Fri Nov 5 09:35:49 PDT 2021

steakhal accepted this revision.
steakhal added a comment.
This revision is now accepted and ready to land.

In D106823#3109469 <https://reviews.llvm.org/D106823#3109469>, @martong wrote:

>> As of diff 5, line 1767 and all the code in the block at line 2184 are uncovered by the tests you provided.
>
> Thanks, I've added new tests that cover the re-assume logic (the block of line 2184).
>
> However, I was unable to add a test that covers  the case when the simplification of a trivial symbol in the disequality info results an infeasible state (line 1767). Here is how I tried: I had changed the line to an assertion and then initiated the static analysis of the following opensource projects: memcached,tmux,curl,twin,redis,vim,openssl,sqlite,ffmpeg,postgresql,tinyxml2,libwebm,xerces,bitcoin,protobuf. My idea had been, if the assertion would fired then I would use creduce to the create the test case (btw, this is how I added the other infeasible state test case). However, it did not fired.
>
> IMHO, having a defensive check at L1767 is correct b/c there is a slight chance of reaching an infeasible state there. Although the chance is minimal, I cannot prove that is 0.

It's fine by me. Thanks for the investigation.

In D106823#3111110 <https://reviews.llvm.org/D106823#3111110>, @martong wrote:

> There are no runtime or peak memory usage growth with this patch (actually, the runtime decreases with a few %).
> I am attaching the measurements results of the latest Diff.F20089096: stats.html <https://reviews.llvm.org/F20089096>

Great!

The tests refer to `reg_$0` by spelling the id number, which is unfortunate, but I suspect these tests will break for the slightest changes in the solver anyway so I'm not too bothered with this.

Please wait a week for the rest of the members of the community to have a look before committing.
@NoQ @Szelethus @ASDenysPetrov

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106823/new/

https://reviews.llvm.org/D106823