[PATCH] D52217: [codeview] Emit S_FRAMEPROC and use S_DEFRANGE_FRAMEPOINTER_REL

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added inline comments.


================
Comment at: llvm/lib/DebugInfo/CodeView/SymbolRecordMapping.cpp:496
+    }
+    llvm_unreachable("bad encoding");
+  case CPUType::X64:
----------------
zturner wrote:
> rnk wrote:
> > zturner wrote:
> > > This should probably be an `assert(false)` as opposed to an `llvm_unreachable`.  This function operates on user input, so it can technically be any value that fits in the underlying type, and we want the program to at least work with bad user input.  If we use `llvm_unreachable` then entering this codepath would be undefined behavior.
> > Why would we ever prefer assert(false) to llvm_unreachable? The unreachable is usually preferred because more static analysis tools know what it means.
> > 
> > We also already assert that EncodedReg < 4 above, so this is already pretty trivially dead.
> > 
> > And, the places where we would get this from user data, it's extracted from a 2-bit bitfield. We've covered all the possible values it can be, unless there are bugs in our own code.
> If an unreachable path executes, it's basically undefined behavior.  We don't want undefined behavior executing conditionally based on the value of user input.  That should never happen.
If the switch can trigger an unhandled case, because of invalid user input, that case must be handled by code that is enabled in NDEBUG. If the we are just guarding against programming errors with an internal consistency check, then the unreachable is preferred over the assert. 


https://reviews.llvm.org/D52217