[codeview] make DbgVariableLocation::extractFromMachineInstruction use Optional

[inglorion]

Summary

DbgVariableLocation::extractFromMachineInstruction originally
returned a boolean indicating success. This change makes it return
an Optional<DbgVariableLocation> so we cannot try to access the fields
of the struct if they aren't valid.

[rnk]

If we don't have any consumers that can use the error messages, I would rather return Optional<DbgVariableLocation>.

[inglorion]

rewritten using Optional instead of Expected

[rnk]

lgtm

llvm/lib/CodeGen/AsmPrinter/DebugHandlerBase.cpp

33–35

I think the idiomatic thing to return here is None

[inglorion]

Use None instead of its overly verbose equivalent.

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp

void CodeViewDebug::calculateRanges(
 
   // calculate the definition ranges.
   for (auto I = Ranges.begin(), E = Ranges.end(); I != E; ++I) {
     const InsnRange &Range = *I;
     const MachineInstr *DVInst = Range.first;
     assert(DVInst->isDebugValue() && "Invalid History entry");
     // FIXME: Find a way to represent constant variables, since they are
     // relatively common.
-    DbgVariableLocation Location;
-    bool Supported =
-        DbgVariableLocation::extractFromMachineInstruction(Location, *DVInst);
-    // If not Supported, don't even look at Location because it's invalid.
-    if (!Supported)
+    Expected<DbgVariableLocation> Location =
+        DbgVariableLocation::extractFromMachineInstruction(*DVInst);
+    if (!Location) {
+      consumeError(Location.takeError());
       continue;
+    }
 
     // Because we cannot express DW_OP_deref in CodeView directly,
     // we use a trick: we encode the type as a reference to the
     // real type.
     if (Var.Deref) {
       // When we're encoding the type as a reference to the original type,
       // we need to remove a level of indirection from incoming locations.
       // E.g. [RSP+8] with DW_OP_deref becomes [RSP+8],
       // and [RCX+0] without DW_OP_deref becomes RCX.
-      if (!Location.Deref) {
-        if (Location.InMemory)
-          Location.InMemory = false;
+      if (!Location->Deref) {
+        if (Location->InMemory)
+          Location->InMemory = false;
         else
-          Supported = false;
+          continue;
       }
-    } else if (Location.Deref) {
+    } else if (Location->Deref) {
       // We've encountered a Deref range when we had not applied the
       // reference encoding. Start over using reference encoding.
       Var.Deref = true;
       Var.DefRanges.clear();
       calculateRanges(Var, Ranges);
       return;
     }
 
     // If we don't know how to handle this range, skip past it.
-    if (!Supported || Location.Register == 0 ||
-        (Location.Offset && !Location.InMemory))
+    if (Location->Register == 0 || (Location->Offset && !Location->InMemory))
       continue;
 
     // Handle the two cases we can handle: indirect in memory and in register.
     {
       LocalVarDefRange DR;
-      DR.CVRegister = TRI->getCodeViewRegNum(Location.Register);
-      DR.InMemory = Location.InMemory;
-      DR.DataOffset = Location.Offset;
-      if (Location.FragmentInfo) {
+      DR.CVRegister = TRI->getCodeViewRegNum(Location->Register);
+      DR.InMemory = Location->InMemory;
+      DR.DataOffset = Location->Offset;
+      if (Location->FragmentInfo) {
         DR.IsSubfield = true;
-        DR.StructOffset = Location.FragmentInfo->OffsetInBits / 8;
+        DR.StructOffset = Location->FragmentInfo->OffsetInBits / 8;
       } else {
         DR.IsSubfield = false;
         DR.StructOffset = 0;
       }
 
       if (Var.DefRanges.empty() ||
           Var.DefRanges.back().isDifferentLocation(DR)) {
         Var.DefRanges.emplace_back(std::move(DR));

llvm/lib/CodeGen/AsmPrinter/DebugHandlerBase.h

 #define LLVM_LIB_CODEGEN_ASMPRINTER_DEBUGHANDLERBASE_H
 
 #include "AsmPrinterHandler.h"
 #include "DbgValueHistoryCalculator.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/CodeGen/LexicalScopes.h"
 #include "llvm/CodeGen/MachineInstr.h"
 #include "llvm/IR/DebugInfoMetadata.h"
+#include "llvm/Support/Error.h"
 
 namespace llvm {
 
 class AsmPrinter;
 class MachineInstr;
 class MachineModuleInfo;
 
 /// Represents the location at which a variable is stored.
struct DbgVariableLocation {
 
   /// If false, the location holds the variable's value.
   /// If true, the location holds the variable's address.
   unsigned Deref : 1;
 
   /// Present if the location is part of a larger variable.
   llvm::Optional<llvm::DIExpression::FragmentInfo> FragmentInfo;
 
-  /// Extract a VariableLocation from a MachineInstr.  The struct passed in as
-  /// Location is populated.  The MachineInstr must be a debug value
-  /// instruction.
-  /// @return true if successful and false if not.
-  static bool extractFromMachineInstruction(DbgVariableLocation &Location,
-                                            const MachineInstr &Instruction);
+  /// Extract a VariableLocation from a MachineInstr.
+  /// This will only work if Instruction is a debug value instruction
+  /// and the associated DIExpression is in one of the supported forms.
+  /// If these requirements are not met, this function will return an
+  /// Error value.
+  static Expected<DbgVariableLocation>
+  extractFromMachineInstruction(const MachineInstr &Instruction);
 };
 
 /// Base class for debug information backends. Common functionality related to
 /// tracking which variables and scopes are alive at a given PC live here.
 class DebugHandlerBase : public AsmPrinterHandler {
 protected:
   DebugHandlerBase(AsmPrinter *A);
 

llvm/lib/CodeGen/AsmPrinter/DebugHandlerBase.cpp

 //===-- llvm/lib/CodeGen/AsmPrinter/DebugHandlerBase.cpp -------*- C++ -*--===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // Common functionality for different debug information format backends.
 // LLVM currently supports DWARF and CodeView.
 //
 //===----------------------------------------------------------------------===//
 
 #include "DebugHandlerBase.h"
+#include "llvm/ADT/Twine.h"
 #include "llvm/CodeGen/AsmPrinter.h"
 #include "llvm/CodeGen/MachineFunction.h"
 #include "llvm/CodeGen/MachineInstr.h"
 #include "llvm/CodeGen/MachineModuleInfo.h"
 #include "llvm/IR/DebugInfo.h"
 #include "llvm/MC/MCStreamer.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Target/TargetSubtargetInfo.h"
 
 using namespace llvm;
 
-bool DbgVariableLocation::extractFromMachineInstruction(
-    DbgVariableLocation &Location, const MachineInstr &Instruction) {
+Expected<DbgVariableLocation>
+DbgVariableLocation::extractFromMachineInstruction(
+    const MachineInstr &Instruction) {
+  DbgVariableLocation Location;
   if (!Instruction.isDebugValue())
-    return false;
+    return make_error<StringError>("extractFromMachineInstruction instruction "
+                                   "is not a debug value instruction",
+                                   inconvertibleErrorCode());
   if (!Instruction.getOperand(0).isReg())
-    return false;
+    return make_error<StringError>(
+        "extractFromMachineInstruction requires first operand to be a register",
+        inconvertibleErrorCode());
   Location.Register = Instruction.getOperand(0).getReg();
   Location.InMemory = Instruction.getOperand(1).isImm();
   Location.Deref = false;
   Location.FragmentInfo.reset();
   // We only handle expressions generated by DIExpression::appendOffset,
   // which doesn't require a full stack machine.
   int64_t Offset = 0;
   const DIExpression *DIExpr = Instruction.getDebugExpression();
case dwarf::DW_OP_plus_uconst:
       break;
     case dwarf::DW_OP_LLVM_fragment:
       Location.FragmentInfo = {Op->getArg(1), Op->getArg(0)};
       break;
     case dwarf::DW_OP_deref:
       Location.Deref = true;
       break;
     default:
-      return false;
+      return make_error<StringError>("unsupported DW_OP " + Twine(Op->getOp()) +
+                                         " in extractFromMachineInstruction",
+                                     inconvertibleErrorCode());
     }
     ++Op;
   }
 
   Location.Offset = Offset;
-  return true;
+  return Location;
 }
 
 DebugHandlerBase::DebugHandlerBase(AsmPrinter *A) : Asm(A), MMI(Asm->MMI) {}
 
 // Each LexicalScope has first instruction and last instruction to mark
 // beginning and end of a scope respectively. Create an inverse map that list
 // scopes starts (and ends) with an instruction. One instruction may start (or
 // end) multiple scopes. Ignore scopes that are not reachable.