Switch DWARFDie::getAttributeValueAsString() to return an llvm::Optional<const char *>.

[clayborg]

Summary

I will be changing all DWARFDie::getAttributeValueAsXXX() over to return llvm::Optional one by one. Starting with string values.

[aprantl]

I'm all for replacing the Value getAttribute(Attr attr, Value default) interfaces with Optional<Value> getAttribute(Attr attr) versions, and perhaps provide the default-value mechanism in a generic way. However, in the concrete case of a const char * return value I think LLVM typically returns a nullptr to signal failure.

[clayborg]

Ok, then string access is the way it should be. I'll make patches for the integers.

[aprantl]

I thought you still wanted to get rid of the default value? It's just that Optional<PointerType> is redundant because pointers are already implicitly optional (via nullptr).

include/llvm/DebugInfo/DWARF/DWARFDie.h

 //===-- DWARFDie.h --------------------------------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_LIB_DEBUGINFO_DWARFDIE_H
 #define LLVM_LIB_DEBUGINFO_DWARFDIE_H
 
+#include "llvm/ADT/Optional.h"
 #include "llvm/DebugInfo/DWARF/DWARFDebugInfoEntry.h"
 
 namespace llvm {
     
 class DWARFUnit;
 class DWARFDebugInfoEntry;
 class raw_ostream;
   
public:
   /// DW_AT_abstract_origin referenced DIEs.
   ///
   /// \param Attr the attribute to extract.
   /// \param FailValue the value to return if this DIE doesn't have this
   /// attribute.
   /// \returns the NULL terminated C string value owned by the DWARF section
   /// that contains the string or FailValue if the attribute doesn't exist or
   /// if the attribute's form isn't a form that describes an string.
-  const char *getAttributeValueAsString(dwarf::Attribute Attr,
-                                        const char *FailValue) const;
+  Optional<const char *> getAttributeValueAsString(dwarf::Attribute Attr) const;
   
   /// Extract the specified attribute from this DIE as an address.
   ///
   /// Extract an attribute value from this DIE only. This call doesn't look
   /// for the attribute value in any DW_AT_specification or
   /// DW_AT_abstract_origin referenced DIEs.
   ///
   /// \param Attr the attribute to extract.

lib/DebugInfo/DWARF/DWARFDie.cpp

bool DWARFDie::getAttributeValue(dwarf::Attribute Attr,
   if (!U)
     return false;
   auto AbbrevDecl = getAbbreviationDeclarationPtr();
   if (AbbrevDecl)
     return AbbrevDecl->getAttributeValue(getOffset(), Attr, *U, FormValue);
   return false;
 }
 
-const char *DWARFDie::getAttributeValueAsString(dwarf::Attribute Attr,
-                                                const char *FailValue) const {
+Optional<const char *>
+DWARFDie::getAttributeValueAsString(dwarf::Attribute Attr) const {
   DWARFFormValue FormValue;
   if (!getAttributeValue(Attr, FormValue))
-    return FailValue;
-  Optional<const char *> Result = FormValue.getAsCString();
-  return Result.hasValue() ? Result.getValue() : FailValue;
+    return Optional<const char *>();
+  return FormValue.getAsCString();
 }
 
 uint64_t DWARFDie::getAttributeValueAsAddress(dwarf::Attribute Attr,
                                               uint64_t FailValue) const {
   DWARFFormValue FormValue;
   if (!getAttributeValue(Attr, FormValue))
     return FailValue;
   Optional<uint64_t> Result = FormValue.getAsAddress();
if (!isSubroutineDIE())
     return nullptr;
   return getName(Kind);
 }
 
 const char *
 DWARFDie::getName(DINameKind Kind) const {
   if (!isValid() || Kind == DINameKind::None)
     return nullptr;
-  const char *name = nullptr;
+  Optional<const char *> name;
   // Try to get mangled name only if it was asked for.
   if (Kind == DINameKind::LinkageName) {
-    if ((name = getAttributeValueAsString(DW_AT_MIPS_linkage_name, nullptr)))
-      return name;
-    if ((name = getAttributeValueAsString(DW_AT_linkage_name, nullptr)))
-      return name;
+    if ((name = getAttributeValueAsString(DW_AT_MIPS_linkage_name)))
+      return *name;
+    if ((name = getAttributeValueAsString(DW_AT_linkage_name)))
+      return *name;
   }
-  if ((name = getAttributeValueAsString(DW_AT_name, nullptr)))
-    return name;
+  if ((name = getAttributeValueAsString(DW_AT_name)))
+    return *name;
   // Try to get name from specification DIE.
   DWARFDie SpecDie = getAttributeValueAsReferencedDie(DW_AT_specification);
   if (SpecDie && (name = SpecDie.getName(Kind)))
-    return name;
+    return *name;
   // Try to get name from abstract origin DIE.
   DWARFDie AbsDie = getAttributeValueAsReferencedDie(DW_AT_abstract_origin);
   if (AbsDie && (name = AbsDie.getName(Kind)))
-    return name;
+    return *name;
   return nullptr;
 }
 
 void DWARFDie::getCallerFrame(uint32_t &CallFile, uint32_t &CallLine,
                               uint32_t &CallColumn) const {
   CallFile = getAttributeValueAsUnsignedConstant(DW_AT_call_file, 0);
   CallLine = getAttributeValueAsUnsignedConstant(DW_AT_call_line, 0);
   CallColumn = getAttributeValueAsUnsignedConstant(DW_AT_call_column, 0);

lib/DebugInfo/DWARF/DWARFUnit.cpp

void DWARFUnit::clear() {
   BaseAddr = 0;
   RangeSectionBase = 0;
   AddrOffsetSectionBase = 0;
   clearDIEs(false);
   DWO.reset();
 }
 
 const char *DWARFUnit::getCompilationDir() {
-  return getUnitDIE().getAttributeValueAsString(DW_AT_comp_dir, nullptr);
+  auto UnitDie = getUnitDIE();
+  if (!UnitDie)
+    return nullptr;
+  auto CompDir = UnitDie.getAttributeValueAsString(DW_AT_comp_dir);
+  if (CompDir)
+    return *CompDir;
+  return nullptr;
 }
 
 uint64_t DWARFUnit::getDWOId() {
   return getUnitDIE().getAttributeValueAsUnsignedConstant(DW_AT_GNU_dwo_id,
                                                           -1ULL);
 }
 
 void DWARFUnit::setDIERelations() {

 bool DWARFUnit::parseDWO() {
   if (isDWO)
     return false;
   if (DWO.get())
     return false;
   DWARFDie UnitDie = getUnitDIE();
   if (!UnitDie)
     return false;
-  const char *DWOFileName =
-      UnitDie.getAttributeValueAsString(DW_AT_GNU_dwo_name, nullptr);
+  auto DWOFileName = UnitDie.getAttributeValueAsString(DW_AT_GNU_dwo_name);
   if (!DWOFileName)
     return false;
-  const char *CompilationDir =
-      UnitDie.getAttributeValueAsString(DW_AT_comp_dir, nullptr);
   SmallString<16> AbsolutePath;
-  if (sys::path::is_relative(DWOFileName) && CompilationDir != nullptr) {
-    sys::path::append(AbsolutePath, CompilationDir);
+  if (sys::path::is_relative(*DWOFileName)) {
+    if (auto CompilationDir = UnitDie.getAttributeValueAsString(DW_AT_comp_dir))
+      sys::path::append(AbsolutePath, *CompilationDir);
   }
-  sys::path::append(AbsolutePath, DWOFileName);
+  sys::path::append(AbsolutePath, *DWOFileName);
   DWO = llvm::make_unique<DWOHolder>(AbsolutePath);
   DWARFUnit *DWOCU = DWO->getUnit();
   // Verify that compile unit in .dwo file is valid.
   if (!DWOCU || DWOCU->getDWOId() != getDWOId()) {
     DWO.reset();
     return false;
   }
   // Share .debug_addr and .debug_ranges section with compile unit in .dwo

tools/dsymutil/DwarfLinker.cpp

static bool analyzeContextInfo(const DWARFDie &DIE,
   //
   // This does not extend to the types inside the modules:
   //  "[I]n C, this implies that if two structs are defined in
   //   different submodules with the same name, those two types are
   //   distinct types (but may be compatible types if their
   //   definitions match)."
   //
   // We treat non-C++ modules like namespaces for this reason.
-  if (DIE.getTag() == dwarf::DW_TAG_module && ParentIdx == 0 &&
-      DIE.getAttributeValueAsString(dwarf::DW_AT_name,
-                                    "") != CU.getClangModuleName()) {
-    InImportedModule = true;
+  if (DIE.getTag() == dwarf::DW_TAG_module && ParentIdx == 0) {
+    if (auto Name = DIE.getAttributeValueAsString(dwarf::DW_AT_name))
+      InImportedModule = *Name != CU.getClangModuleName();
   }
 
   Info.ParentIdx = ParentIdx;
   bool InClangModule = CU.isClangModule() || InImportedModule;
   if (CU.hasODR() || InClangModule) {
     if (CurrentDeclContext) {
       auto PtrInvalidPair = Contexts.getChildDeclContext(
           *CurrentDeclContext, DIE, CU, StringPool, InClangModule);
if (!DwoId)
     DwoId = CUDie.getAttributeValueAsUnsignedConstant(dwarf::DW_AT_GNU_dwo_id,
                                                       0);
   return DwoId;
 }
 
 bool DwarfLinker::registerModuleReference(
     const DWARFDie &CUDie, const DWARFUnit &Unit,
     DebugMap &ModuleMap, unsigned Indent) {
-  std::string PCMfile =
-      CUDie.getAttributeValueAsString(dwarf::DW_AT_dwo_name, "");
-  if (PCMfile.empty())
-    PCMfile =
-        CUDie.getAttributeValueAsString(dwarf::DW_AT_GNU_dwo_name, "");
-  if (PCMfile.empty())
+  auto PCMfile = CUDie.getAttributeValueAsString(dwarf::DW_AT_dwo_name);
+  if (!PCMfile)
+    PCMfile = CUDie.getAttributeValueAsString(dwarf::DW_AT_GNU_dwo_name);
+  if (!PCMfile)
     return false;
 
   // Clang module DWARF skeleton CUs abuse this for the path to the module.
-  std::string PCMpath =
-      CUDie.getAttributeValueAsString(dwarf::DW_AT_comp_dir, "");
+  std::string PCMpath;
+  if (auto CompDir = CUDie.getAttributeValueAsString(dwarf::DW_AT_comp_dir))
+    PCMpath = *CompDir;
+    
   uint64_t DwoId = getDwoId(CUDie, Unit);
 
-  std::string Name =
-      CUDie.getAttributeValueAsString(dwarf::DW_AT_name, "");
-  if (Name.empty()) {
-    reportWarning("Anonymous module skeleton CU for " + PCMfile);
+  auto Name = CUDie.getAttributeValueAsString(dwarf::DW_AT_name);
+  if (!Name) {
+    reportWarning(Twine("Anonymous module skeleton CU for ") + *PCMfile);
     return true;
   }
 
   if (Options.Verbose) {
     outs().indent(Indent);
-    outs() << "Found clang module reference " << PCMfile;
+    outs() << "Found clang module reference " << *PCMfile;
   }
 
-  auto Cached = ClangModules.find(PCMfile);
+  auto Cached = ClangModules.find(*PCMfile);
   if (Cached != ClangModules.end()) {
     // FIXME: Until PR27449 (https://llvm.org/bugs/show_bug.cgi?id=27449) is
     // fixed in clang, only warn about DWO_id mismatches in verbose mode.
     // ASTFileSignatures will change randomly when a module is rebuilt.
     if (Options.Verbose && (Cached->second != DwoId))
       reportWarning(Twine("hash mismatch: this object file was built against a "
-                          "different version of the module ") + PCMfile);
+                          "different version of the module ") + *PCMfile);
     if (Options.Verbose)
       outs() << " [cached].\n";
     return true;
   }
   if (Options.Verbose)
     outs() << " ...\n";
 
   // Cyclic dependencies are disallowed by Clang, but we still
   // shouldn't run into an infinite loop, so mark it as processed now.
-  ClangModules.insert({PCMfile, DwoId});
-  loadClangModule(PCMfile, PCMpath, Name, DwoId, ModuleMap, Indent + 2);
+  ClangModules.insert({*PCMfile, DwoId});
+  loadClangModule(*PCMfile, PCMpath, *Name, DwoId, ModuleMap, Indent + 2);
   return true;
 }
 
 ErrorOr<const object::ObjectFile &>
 DwarfLinker::loadObject(BinaryHolder &BinaryHolder, DebugMapObject &Obj,
                         const DebugMap &Map) {
   auto ErrOrObjs =
       BinaryHolder.GetObjectFiles(Obj.getObjectFilename(), Obj.getTimestamp());

unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp

EXPECT_EQ(
       Data4);
   EXPECT_EQ(
       DieDG.getAttributeValueAsUnsignedConstant(Attr_DW_FORM_data8, 0),
       Data8);
 
   //----------------------------------------------------------------------
   // Test string forms
   //----------------------------------------------------------------------
-  const char *ExtractedStringValue =
-      DieDG.getAttributeValueAsString(Attr_DW_FORM_string, nullptr);
-  EXPECT_TRUE(ExtractedStringValue != nullptr);
-  EXPECT_TRUE(strcmp(StringValue, ExtractedStringValue) == 0);
-
-  const char *ExtractedStrpValue =
-      DieDG.getAttributeValueAsString(Attr_DW_FORM_strp, nullptr);
-  EXPECT_TRUE(ExtractedStrpValue != nullptr);
-  EXPECT_TRUE(strcmp(StrpValue, ExtractedStrpValue) == 0);
+  auto ExtractedStringValue =
+      DieDG.getAttributeValueAsString(Attr_DW_FORM_string);
+  EXPECT_TRUE((bool)ExtractedStringValue);
+  EXPECT_TRUE(strcmp(StringValue, *ExtractedStringValue) == 0);
+
+  auto ExtractedStrpValue = DieDG.getAttributeValueAsString(Attr_DW_FORM_strp);
+  EXPECT_TRUE((bool)ExtractedStrpValue);
+  EXPECT_TRUE(strcmp(StrpValue, *ExtractedStrpValue) == 0);
 
   //----------------------------------------------------------------------
   // Test reference forms
   //----------------------------------------------------------------------
   EXPECT_EQ(DieDG.getAttributeValueAsReference(Attr_DW_FORM_ref_addr, 0),
             RefAddr);
   EXPECT_EQ(DieDG.getAttributeValueAsReference(Attr_DW_FORM_ref1, 0),
             Data1);