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

[Zachary Turner via Phabricator via llvm-commits]

zturner added inline comments.


================
Comment at: llvm/lib/DebugInfo/CodeView/SymbolRecordMapping.cpp:496
+    }
+    llvm_unreachable("bad encoding");
+  case CPUType::X64:
----------------
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.


https://reviews.llvm.org/D52217