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

[Zachary Turner via Phabricator via llvm-commits]

zturner accepted this revision.
zturner added inline comments.
This revision is now accepted and ready to land.


================
Comment at: llvm/lib/DebugInfo/CodeView/SymbolRecordMapping.cpp:496
+    }
+    llvm_unreachable("bad encoding");
+  case CPUType::X64:
----------------
rnk wrote:
> zturner wrote:
> > rnk wrote:
> > > aprantl wrote:
> > > > 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. 
> > > > 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.
> > > 
> > > I think it's just a consequence of current implementation details that llvm_unreachable is immediate UB and a failed assert is... probably-UB-soon. I think if it was easy for us to say, `if (!assert_cond) __builtin_unreachable();` in the implementation of assert.h, we would do it.
> > > 
> > > I think the important thing is whether the condition is triggerable by user input changes or not. It's currently impossible. All the callers do `Input & 3U` before calling here, so invalid values are impossible. If that changes, that'd be a bug and we'll find it with assertions like we always do.
> > This code gets run in `llvm-pdbutil` when we dump.  If we get an invalid PDB or one with a frame pointer value which was valid but we just didn't know about, we would crash here.  We should print something sane in that case, like "<unknown>" or "<none>".
> > 
> > But the point is that this is all in generic library code which is intended to be consumed by debuggers and the like.  And they may have to deal with bad debug info which could have changed in various ways across different compiler versions.
> > 
> > We could maybe return an `Expected<RegisterId>` here, or even an `Optional<RegisterId>` if `Expected` is too cumbersome.
> I've explained my position and I don't intend to make this change. This program point is in fact unreachable, it's implied by the assertion in this same function.
Hmm, I somehow missed that it’s asserted on earlier :-/


https://reviews.llvm.org/D52217