[PATCH] D37187: [Analyzer] Fix Bug 25609 - Assertion UNREACHABLE: 'Unexpected ProgramPoint' with widen-loops=true

[Henry Wong via Phabricator via cfe-commits]

MTC added a comment.

ping?



================
Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:651-652
+  } else if (StoreSite->getLocation().getAs<BlockEntrance>()) {
+    os << "Reach the max loop limit.";
+    os << " Assigning a conjured symbol";
+    if (R->canPrintPretty()) {
----------------
zaks.anna wrote:
> xazax.hun wrote:
> > zaks.anna wrote:
> > > MTC wrote:
> > > > NoQ wrote:
> > > > > This is user-facing text, and users shouldn't know about conjured symbols, and "max" shouldn't be shortened, and i'm not sure what else. I'd probably suggest something along the lines of "Contents of <...> are wiped", but this is still not good enough.
> > > > > 
> > > > > Also could you add a test that displays this note? I.e. with `-analyzer-output=text`.
> > > > Thanks for your review. 
> > > > 
> > > > You are right, whether this information should be displayed to the user is a question worth discussing.
> > > I am not convinced that we need to print this information to the user. The problem here is that it leaks internal implementation details about the analyzer. The users should not know about "loop limits" and "invalidation" and most of the users would not even know what this means. I can see how this is useful to the analyzer developers for debugging the analyzer, but not to the end user.
> > > 
> > While we might not want to expose this to the user this can be really useful to understand what the analyzer is doing when we debugging a false positive finding. Maybe it would be worth to introduce a developer mode or verbose mode for those purposes. What do you think?
> I'd be fine with that in theory, though the downside is that it would pollute the code a bit. One trick that's often used to better understand a report when debugging is to remove the path note pruning (by passing a flag). I am not sure if that approach can be extended for this case. Ex: maybe we could have special markers on the notes saying that they are for debug purposes only and have the pruning remove them.
> 
> By the way, is this change related to the other change from this patch?
Thank you for your review!
Agree with you, and Artem also recommends not exposing too much internal implementation to the user.


================
Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:651-652
+  } else if (StoreSite->getLocation().getAs<BlockEntrance>()) {
+    os << "Reach the max loop limit.";
+    os << " Assigning a conjured symbol";
+    if (R->canPrintPretty()) {
----------------
MTC wrote:
> zaks.anna wrote:
> > xazax.hun wrote:
> > > zaks.anna wrote:
> > > > MTC wrote:
> > > > > NoQ wrote:
> > > > > > This is user-facing text, and users shouldn't know about conjured symbols, and "max" shouldn't be shortened, and i'm not sure what else. I'd probably suggest something along the lines of "Contents of <...> are wiped", but this is still not good enough.
> > > > > > 
> > > > > > Also could you add a test that displays this note? I.e. with `-analyzer-output=text`.
> > > > > Thanks for your review. 
> > > > > 
> > > > > You are right, whether this information should be displayed to the user is a question worth discussing.
> > > > I am not convinced that we need to print this information to the user. The problem here is that it leaks internal implementation details about the analyzer. The users should not know about "loop limits" and "invalidation" and most of the users would not even know what this means. I can see how this is useful to the analyzer developers for debugging the analyzer, but not to the end user.
> > > > 
> > > While we might not want to expose this to the user this can be really useful to understand what the analyzer is doing when we debugging a false positive finding. Maybe it would be worth to introduce a developer mode or verbose mode for those purposes. What do you think?
> > I'd be fine with that in theory, though the downside is that it would pollute the code a bit. One trick that's often used to better understand a report when debugging is to remove the path note pruning (by passing a flag). I am not sure if that approach can be extended for this case. Ex: maybe we could have special markers on the notes saying that they are for debug purposes only and have the pruning remove them.
> > 
> > By the way, is this change related to the other change from this patch?
> Thank you for your review!
> Agree with you, and Artem also recommends not exposing too much internal implementation to the user.
Thank you, Gábor!
That's a good point. I think this idea requires  a little bit of work to be done,  and it should be done by someone more familiar with BugReport or PathDiagnostic, :). 



================
Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:651-652
+  } else if (StoreSite->getLocation().getAs<BlockEntrance>()) {
+    os << "Reach the max loop limit.";
+    os << " Assigning a conjured symbol";
+    if (R->canPrintPretty()) {
----------------
MTC wrote:
> MTC wrote:
> > zaks.anna wrote:
> > > xazax.hun wrote:
> > > > zaks.anna wrote:
> > > > > MTC wrote:
> > > > > > NoQ wrote:
> > > > > > > This is user-facing text, and users shouldn't know about conjured symbols, and "max" shouldn't be shortened, and i'm not sure what else. I'd probably suggest something along the lines of "Contents of <...> are wiped", but this is still not good enough.
> > > > > > > 
> > > > > > > Also could you add a test that displays this note? I.e. with `-analyzer-output=text`.
> > > > > > Thanks for your review. 
> > > > > > 
> > > > > > You are right, whether this information should be displayed to the user is a question worth discussing.
> > > > > I am not convinced that we need to print this information to the user. The problem here is that it leaks internal implementation details about the analyzer. The users should not know about "loop limits" and "invalidation" and most of the users would not even know what this means. I can see how this is useful to the analyzer developers for debugging the analyzer, but not to the end user.
> > > > > 
> > > > While we might not want to expose this to the user this can be really useful to understand what the analyzer is doing when we debugging a false positive finding. Maybe it would be worth to introduce a developer mode or verbose mode for those purposes. What do you think?
> > > I'd be fine with that in theory, though the downside is that it would pollute the code a bit. One trick that's often used to better understand a report when debugging is to remove the path note pruning (by passing a flag). I am not sure if that approach can be extended for this case. Ex: maybe we could have special markers on the notes saying that they are for debug purposes only and have the pruning remove them.
> > > 
> > > By the way, is this change related to the other change from this patch?
> > Thank you for your review!
> > Agree with you, and Artem also recommends not exposing too much internal implementation to the user.
> Thank you, Gábor!
> That's a good point. I think this idea requires  a little bit of work to be done,  and it should be done by someone more familiar with BugReport or PathDiagnostic, :). 
> 
Thanks, anna! 
The output here is just to make it better for the user to figure out what's going on here. Delete the output information does not affect the other part, but the output information is changed from "**Reach the maximum loop limit. Invalidate the value of 'num'**" becomes the original output information "**value assigned to 'p'**".



================
Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:651-652
+  } else if (StoreSite->getLocation().getAs<BlockEntrance>()) {
+    os << "Reach the max loop limit.";
+    os << " Assigning a conjured symbol";
+    if (R->canPrintPretty()) {
----------------
xazax.hun wrote:
> MTC wrote:
> > MTC wrote:
> > > MTC wrote:
> > > > zaks.anna wrote:
> > > > > xazax.hun wrote:
> > > > > > zaks.anna wrote:
> > > > > > > MTC wrote:
> > > > > > > > NoQ wrote:
> > > > > > > > > This is user-facing text, and users shouldn't know about conjured symbols, and "max" shouldn't be shortened, and i'm not sure what else. I'd probably suggest something along the lines of "Contents of <...> are wiped", but this is still not good enough.
> > > > > > > > > 
> > > > > > > > > Also could you add a test that displays this note? I.e. with `-analyzer-output=text`.
> > > > > > > > Thanks for your review. 
> > > > > > > > 
> > > > > > > > You are right, whether this information should be displayed to the user is a question worth discussing.
> > > > > > > I am not convinced that we need to print this information to the user. The problem here is that it leaks internal implementation details about the analyzer. The users should not know about "loop limits" and "invalidation" and most of the users would not even know what this means. I can see how this is useful to the analyzer developers for debugging the analyzer, but not to the end user.
> > > > > > > 
> > > > > > While we might not want to expose this to the user this can be really useful to understand what the analyzer is doing when we debugging a false positive finding. Maybe it would be worth to introduce a developer mode or verbose mode for those purposes. What do you think?
> > > > > I'd be fine with that in theory, though the downside is that it would pollute the code a bit. One trick that's often used to better understand a report when debugging is to remove the path note pruning (by passing a flag). I am not sure if that approach can be extended for this case. Ex: maybe we could have special markers on the notes saying that they are for debug purposes only and have the pruning remove them.
> > > > > 
> > > > > By the way, is this change related to the other change from this patch?
> > > > Thank you for your review!
> > > > Agree with you, and Artem also recommends not exposing too much internal implementation to the user.
> > > Thank you, Gábor!
> > > That's a good point. I think this idea requires  a little bit of work to be done,  and it should be done by someone more familiar with BugReport or PathDiagnostic, :). 
> > > 
> > Thanks, anna! 
> > The output here is just to make it better for the user to figure out what's going on here. Delete the output information does not affect the other part, but the output information is changed from "**Reach the maximum loop limit. Invalidate the value of 'num'**" becomes the original output information "**value assigned to 'p'**".
> > 
> I agree that we should just commit this without the message to fix the assert. The "debug message" could be addressed in a separate patch. 
Done!


https://reviews.llvm.org/D37187