[Analyzer] Better unreachable message in enumeration

[george.karpenkov]

Summary

@dcoughlin I'm curious whether you'd like such a change: in general, I think it is much better when the assert failure tells the developer _what_ value is failing, rather than saying "oops we are dead".
I would personally even go further with providing a templated function from dump-able object to a string, which could be used to avoid such a cumbersome construction [which I have seen in a few places in Clang]

[danielmarjamaki]

I think it is much better when the assert failure tells the developer _what_ value is failing, rather than saying "oops we are dead".

yes of course, more informative assert messages is better.

[dcoughlin]

What is the workflow where this is needed? Is it when an end-user is running a build with assertions and can't provide a reproducer but can provide the console output?

Does llvm_unreachable() guarantee that the string construction code is completely removed from release builds?

[george.karpenkov]

@dcoughlin

Is it when an end-user is running a build with assertions and can't provide a reproducer but can provide the console output?

Yes, or just for developer staring at the crash for the first time, or for the crashers in CI.

Does llvm_unreachable() guarantee that the string construction code is completely removed from release builds?

I am not sure what do you mean, the string construction code would only be encountered when we are in the default branch, which would mean that we are crashing already. Or do you worry about the code size being slightly larger?

[NoQ]

Does llvm_unreachable() guarantee that the string construction code is completely removed from release builds?

http://llvm.org/docs/CodingStandards.html#assert-liberally:

When assertions are disabled (i.e. in release builds), llvm_unreachable becomes a hint to compilers to skip generating code for this branch. If the compiler does not support this, it will fall back to the “abort” implementation.

So i think it's fine. Not sure how much time it saves compared to writing these dumps (as a *lot* of places would need those).

[george.karpenkov]

@dcoughlin @NoQ so can this be committed?

[dcoughlin]

Personally, I don't think this is worth it and I find it unpleasant to add untestable code -- especially if that code is going to stick around in release builds.

[george.karpenkov]

@dcoughlin OK, I guess you could theoretically come up with a scenario where an error in this code would lead to crashing-while-crashing which would obscure the original error.

lib/StaticAnalyzer/Core/SimpleConstraintManager.cpp

ProgramStateRef SimpleConstraintManager::assumeAux(ProgramStateRef State,
   if (!canReasonAbout(Cond)) {
     // Just add the constraint to the expression without trying to simplify.
     SymbolRef Sym = Cond.getAsSymExpr();
     assert(Sym);
     return assumeSymUnsupported(State, Sym, Assumption);
   }
 
   switch (Cond.getSubKind()) {
-  default:
-    llvm_unreachable("'Assume' not implemented for this NonLoc");
+  default: {
+    SmallString<64> buf;
+    llvm::raw_svector_ostream out(buf);
+    out << "'Assume' not implemented for this NonLoc: " << Cond << "\n";
+    llvm_unreachable(out.str().data());
+  }
 
   case nonloc::SymbolValKind: {
     nonloc::SymbolVal SV = Cond.castAs<nonloc::SymbolVal>();
     SymbolRef Sym = SV.getSymbol();
     assert(Sym);
     return assumeSym(State, Sym, Assumption);
   }