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

[Reid Kleckner via Phabricator via llvm-commits]

rnk added inline comments.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:108
+  default:
+    report_fatal_error("target architecture doesn't map to a CodeView CPUType");
+  }
----------------
zturner wrote:
> Similarly here, shouldn't this just be `assert(false)`?
Why? Assertions are for internal errors. We want to report an error even if assertions are disabled if a user somehow manages to generate code for ppc64-windows-msvc and enable codeview. That's not an internal error, and there's no checking up until this point that will error out for it.


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


https://reviews.llvm.org/D52217