[DebugInfo] Add error-handling to DWARFAbbreviationDeclarationSet

[bulbazord]

Summary

This commit aims to improve error handling in the
DWARFAbbreviationDeclarationSet class. Specifically, we change the return type
of DWARFAbbreviationDeclarationSet::extract to an llvm::Error. In doing
so, we propagate the error from DWARFAbbreviationDeclaration::extract
another layer upward.

I have built on the previous unittest for DWARFDebugAbbrev that I
wrote a few days prior.
Namely, I am verifying that the following should give an error:

• An invalid tag following a non-null code
• An invalid attribute with a valid form
• A valid attribute with an invalid form
• An incorrectly terminated DWARFAbbreviationDeclaration

Additionally, I uncovered some invalid DWARF in an unrelated dsymutil
test. Namely the last Abbreviation Decl was missing a code.
This test has been updated accordingly. However, this commit does
not fix the underlying issue: llvm-dwarfdump does not correctly
verify the debug abbreviation section to catch these kinds of
mistakes. I have updated DWARFVerifier to not dereference a
pointer without first checking it and left a FIXME for future
contributors.

[fdeazeve]

llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp

304

I am a bit confused by this: isn't NumErrors always 0 here?

312

Same questions about NumErrors here

[fdeazeve]

llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp

304

Oh nvm, I see that the previous code was already doing the same

[bulbazord]

llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp

304

I can move the declaration down and return 0 directly here. That might make it less confusing.

[aprantl]

LGTM if the other reviewers are happy!

[jhenderson]

llvm/include/llvm/DebugInfo/DWARF/DWARFDebugAbbrev.h

37

llvm:: seems unnecessary?

llvm/lib/DebugInfo/DWARF/DWARFDebugAbbrev.cpp

28

Ditto.

36

I know this isn't something you changed in this patch, but it might be worth removing the unnecessary qualifier here too, whilst you're in the immediate area.

53

I feel like it's stated somewhere, although I don't know where, but the preferred way of returning a success state is to do return Error::success();

127

Is there a particular reason you're using auto rather than Error here and in the similar code below? It doesn't seem to improve readability to me.

129

Unnecessary llvm:: qualifier?

169

Ditto.

llvm/unittests/DebugInfo/DWARF/DWARFDebugAbbrevTest.cpp

56–57

This is the canonical way of testing errors. There's an equivalent invocation for testing Expected and also failure states too.

88–89

See above.

130–134

Use EXPECT_THAT_ERROR with FailedWithMessage to check the error has a specific message. EXPECT version should be preferred, since there's no further code in this test.

159–164

As above.

189–194

As above.

215–220

As above.

[bulbazord]

llvm/lib/DebugInfo/DWARF/DWARFDebugAbbrev.cpp

53

Ah yes, I took some time to read the LLVM Programmer's Manual and they suggest doing the same: https://llvm.org/docs/ProgrammersManual.html#recoverable-errors

127

In general, I use auto when I feel I can convey the type information through other means (the expression or name of variable), but I hold my opinion on the use of auto very weakly. I'll switch it over to use Error.

llvm/unittests/DebugInfo/DWARF/DWARFDebugAbbrevTest.cpp

56–57

Thanks for pointing that out! I'm still trying to understand LLVM's best practices with respect to testing.

[bulbazord]

Address feedback from @jhenderson

[fdeazeve]

llvm/unittests/DebugInfo/DWARF/DWARFDebugAbbrevTest.cpp

24

This is likely irrelevant for the patch itself, but I missed the boat on the original review: aren't we missing the compile unit terminator for this in most tests?

We have this structure:

First code
  DW_TAG_compile_unit
    Has Children = true
    Name, strp
    0, 0   // terminates list of attributes

    DW_TAG_subprogram   // children of compile unit
      Has children = false
      Name, strp
      0, 0   // terminates list of attributes

This is all the function writes. Sometimes, the callees write an extra zero, which is terminating the DW_TAG_compile_unit children list. (Maybe we should write that one here instead? The extra entries we write manually write in each test would simply become siblings of the TAG_compile_unit, which is fine)

But even with that extra zero, we're missing an extra zero terminating the abbreviation table itself (when multiple abbreviation tables are stitched together, they are separated by this terminator, see page 207 line 15 of the D5 spec)

Anyhow, I am pointing all of this out because it's curious that the DWARFAbbreviationDeclarationSet::extract method seems to happily ignore the absence of this?

[fdeazeve]

LGTM! Thanks for driving this

llvm/unittests/DebugInfo/DWARF/DWARFDebugAbbrevTest.cpp

24

Ahhhhhh nvm, I am making the same mistake I made months ago when I first looked into this:

Sometimes, the callees write an extra zero, which is terminating the DW_TAG_compile_unit children list

This is wrong. The extra zero terminating the children lists are part of the debug info section, not the debug abbrev section. The zero you write is indeed terminating the abbrev table itself.

[jhenderson]

LGMT. There are a couple of pre-merge tests that failed that have something to do with DWARF. It's unlikely to be related to this patch, but it might be worth double-checking before landing this.

llvm/lib/DebugInfo/DWARF/DWARFDebugAbbrev.cpp

127

It's worth noting that LLVM has policies on auto usage documented in the coding standards document. I think it largely boils down to "use auto for iterators and where the type can be inferred from the right-hand-side of the expression", although there are some nuances to it beyond that.

[bulbazord]

In D151353#4380119, @jhenderson wrote:

LGMT. There are a couple of pre-merge tests that failed that have something to do with DWARF. It's unlikely to be related to this patch, but it might be worth double-checking before landing this.

Yeah, I saw that as well. I was going to see if I could reproduce the issue locally before landing. If not, I'll watch out for failures on the bot and go from there. Thank you for the review!

llvm/lib/DebugInfo/DWARF/DWARFDebugAbbrev.cpp

127

I'll take a look, thanks for pointing that out.

[bulbazord]

Ok, the pre-submission checks failing are actually due to this change. Specifically, parsing the debug_abbrev section of the objects produced by these tests leads to an infinite loop. It goes as follows:

• We fail to get an ULEB128 value for the code because the contents of the debug_abbrev are invalid. The DataExtractor thus returns 0, signaling to the DWARFAbbreviationDeclaration parsing code that were done with this DWARFAbbreviationDeclaration.
• The DWARFAbbreviationDeclarationSet sees it's done and returns an empty Set to the DWARFDebugAbbrev parsing code. The offset is still at 0, so it's still technically a valid offset for the DataExtractor. So we continue on...

I believe DWARFDebugAbbrev::parse was relying on knowing when it's done by checking if the offset has moved. Otherwise we end up in an infinite loop. I'm going to add some additional safety checks, but it shouldn't be significant. I'll request another review after adding it.

[bulbazord]

I've identified the root cause and I aim to address it in a separate change before this change goes in: https://reviews.llvm.org/D151755

Afterwards I will revisit and adjust this change if needed. If this change needs no adjustments, I will land it shortly after that change.

[bulbazord]

Updating to match changes from D151755

[bulbazord]

I didn't change anything except the tests, could I get a pair of eyes on the tests one last time? Thanks!

[jhenderson]

Latest changes look fine, but there are a number of failures in the pre-merge check. Is this patch based on another pending commit? If so, please make sure to update the parent/child relationship that Phabricator knows about by selecting the Edit Related Revisions option in the UI and updating accordingly. You could also put in the child patch description "Depends on D......" (where D...... is the relevant patch ID).

[bulbazord]

In D151353#4402741, @jhenderson wrote:

Latest changes look fine, but there are a number of failures in the pre-merge check. Is this patch based on another pending commit? If so, please make sure to update the parent/child relationship that Phabricator knows about by selecting the Edit Related Revisions option in the UI and updating accordingly. You could also put in the child patch description "Depends on D......" (where D...... is the relevant patch ID).

This patch is not based on another pending commit, it looks like these these are a result of this change. Normally I run the test suite before submitting a patch but I don't have a memory of doing that again here, so I'm assuming that I didn't. I'll dig into it, thanks for the help.

[bulbazord]

Update tests with incorrect debug_abbrev data.
Modified one dsymutil test to generate an object instead of checking it in.

[jhenderson]

LGTM.

[fdeazeve]

LGTM! Some of those invalid tests must have been painful to track, thanks for looking into them

llvm/include/llvm/DebugInfo/DWARF/DWARFDebugAbbrev.h

class DWARFAbbreviationDeclarationSet {
   using const_iterator =
       std::vector<DWARFAbbreviationDeclaration>::const_iterator;
 
 public:
   DWARFAbbreviationDeclarationSet();
 
   uint64_t getOffset() const { return Offset; }
   void dump(raw_ostream &OS) const;
-  bool extract(DataExtractor Data, uint64_t *OffsetPtr);
+  llvm::Error extract(DataExtractor Data, uint64_t *OffsetPtr);
 
   const DWARFAbbreviationDeclaration *
   getAbbreviationDeclaration(uint32_t AbbrCode) const;
 
   const_iterator begin() const {
     return Decls.begin();
   }
 

llvm/lib/DebugInfo/DWARF/DWARFDebugAbbrev.cpp

 }
 
 void DWARFAbbreviationDeclarationSet::clear() {
   Offset = 0;
   FirstAbbrCode = 0;
   Decls.clear();
 }
 
-bool DWARFAbbreviationDeclarationSet::extract(DataExtractor Data,
+llvm::Error DWARFAbbreviationDeclarationSet::extract(DataExtractor Data,
                                                      uint64_t *OffsetPtr) {
   clear();
   const uint64_t BeginOffset = *OffsetPtr;
   Offset = BeginOffset;
   DWARFAbbreviationDeclaration AbbrDecl;
   uint32_t PrevAbbrCode = 0;
   while (true) {
     llvm::Expected<DWARFAbbreviationDeclaration::ExtractState> ES =
         AbbrDecl.extract(Data, OffsetPtr);
-    if (!ES) {
-      // FIXME: We should propagate the error upwards.
-      llvm::consumeError(ES.takeError());
-      break;
-    }
+    if (!ES)
+      return ES.takeError();
 
     if (*ES == DWARFAbbreviationDeclaration::ExtractState::Complete)
       break;
 
     if (FirstAbbrCode == 0) {
       FirstAbbrCode = AbbrDecl.getCode();
     } else if (PrevAbbrCode + 1 != AbbrDecl.getCode()) {
       // Codes are not consecutive, can't do O(1) lookups.
       FirstAbbrCode = UINT32_MAX;
     }
     PrevAbbrCode = AbbrDecl.getCode();
     Decls.push_back(std::move(AbbrDecl));
   }
-  return BeginOffset != *OffsetPtr;
+  return llvm::ErrorSuccess();
 }
 
 void DWARFAbbreviationDeclarationSet::dump(raw_ostream &OS) const {
   for (const auto &Decl : Decls)
     Decl.dump(OS);
 }
 
 const DWARFAbbreviationDeclaration *
if (!Data)
     return;
   uint64_t Offset = 0;
   auto I = AbbrDeclSets.begin();
   while (Data->isValidOffset(Offset)) {
     while (I != AbbrDeclSets.end() && I->first < Offset)
       ++I;
     uint64_t CUAbbrOffset = Offset;
     DWARFAbbreviationDeclarationSet AbbrDecls;
-    if (!AbbrDecls.extract(*Data, &Offset))
+    if (auto Err = AbbrDecls.extract(*Data, &Offset)) {
+      // FIXME: We should propagate the error upwards.
+      llvm::consumeError(std::move(Err));
       break;
+    }
     AbbrDeclSets.insert(I, std::make_pair(CUAbbrOffset, std::move(AbbrDecls)));
   }
   Data = std::nullopt;
 }
 
 void DWARFDebugAbbrev::dump(raw_ostream &OS) const {
   parse();
 
DWARFDebugAbbrev::getAbbreviationDeclarationSet(uint64_t CUAbbrOffset) const {
   if (Pos != End) {
     PrevAbbrOffsetPos = Pos;
     return &(Pos->second);
   }
 
   if (Data && CUAbbrOffset < Data->getData().size()) {
     uint64_t Offset = CUAbbrOffset;
     DWARFAbbreviationDeclarationSet AbbrDecls;
-    if (!AbbrDecls.extract(*Data, &Offset))
+    if (auto Err = AbbrDecls.extract(*Data, &Offset)) {
+      // FIXME: We should propagate the error upwards.
+      llvm::consumeError(std::move(Err));
       return nullptr;
+    }
     PrevAbbrOffsetPos =
         AbbrDeclSets.insert(std::make_pair(CUAbbrOffset, std::move(AbbrDecls)))
             .first;
     return &PrevAbbrOffsetPos->second;
   }
 
   return nullptr;
 }

llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp

if (!CallAttr) {
     return 1;
   }
 
   return 0;
 }
 
 unsigned DWARFVerifier::verifyAbbrevSection(const DWARFDebugAbbrev *Abbrev) {
   unsigned NumErrors = 0;
-  if (Abbrev) {
-    const DWARFAbbreviationDeclarationSet *AbbrDecls =
-        Abbrev->getAbbreviationDeclarationSet(0);
-    for (auto AbbrDecl : *AbbrDecls) {
-      SmallDenseSet<uint16_t> AttributeSet;
-      for (auto Attribute : AbbrDecl.attributes()) {
-        auto Result = AttributeSet.insert(Attribute.Attr);
-        if (!Result.second) {
-          error() << "Abbreviation declaration contains multiple "
-                  << AttributeString(Attribute.Attr) << " attributes.\n";
-          AbbrDecl.dump(OS);
-          ++NumErrors;
-        }
-      }
-    }
-  }
+  if (!Abbrev)
+    return NumErrors;
+
+  // FIXME: If we failed to get a DWARFAbbreviationDeclarationSet, it's possible
+  // that there are errors. We need to propagate the error from
+  // getAbbreviationDeclarationSet.
+  if (!AbbrDecls)
+    return NumErrors;
+
   return NumErrors;
 }
 
 bool DWARFVerifier::handleDebugAbbrev() {
   OS << "Verifying .debug_abbrev...\n";
 
   const DWARFObject &DObj = DCtx.getDWARFObj();
   unsigned NumErrors = 0;

llvm/test/tools/dsymutil/Inputs/swift-interface.s

 ##; Generated from the following manually stripped empty Swift program:
 ##;
 ##; target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
 ##; target triple = "x86_64-apple-macosx10.9.0"
-##;  
+##;
 ##; @__swift_reflection_version = linkonce_odr hidden constant i16 3
 ##; @llvm.used = appending global [1 x i8*] [i8* bitcast (i16* @__swift_reflection_version to i8*)], section "llvm.metadata", align 8
-##;  
+##;
 ##; define i32 @main(i32, i8**) !dbg !29 {
 ##; entry:
 ##;   %2 = bitcast i8** %1 to i8*
 ##;   ret i32 0, !dbg !35
 ##; }
-##;  
+##;
 ##; !llvm.dbg.cu = !{!0}
 ##; !swift.module.flags = !{!14}
 ##; !llvm.module.flags = !{!20, !21, !24}
 ##;  
 ##; !0 = distinct !DICompileUnit(language: DW_LANG_Swift, file: !1, isOptimized: false, runtimeVersion: 5, emissionKind: FullDebug, enums: !2, imports: !3, sysroot: "/SDK")
 ##; !1 = !DIFile(filename: "ParseableInterfaceImports.swift", directory: "/")
 ##; !2 = !{}
 ##; !3 = !{!4, !6, !8}
 ##; !4 = !DIImportedEntity(tag: DW_TAG_imported_module, scope: !1, entity: !5, file: !1)
 ##; !5 = !DIModule(scope: null, name: "Foo", includePath: "/Foo/x86_64.swiftinterface")
 ##; !6 = !DIImportedEntity(tag: DW_TAG_imported_module, scope: !1, entity: !7, file: !1)
-##; !7 = !DIModule(scope: null, name: "Swift", includePath: "/SDK/Swift.swiftmodule/x86_64.swiftinterface", sysroot: "/SDK")
+##; !7 = !DIModule(scope: null, name: "Swift", includePath: "/SDK/Swift.swiftmodule/x86_64.swiftinterface")
 ##; !8 = !DIImportedEntity(tag: DW_TAG_imported_module, scope: !1, entity: !7, file: !1)
-##; !9 = !DIModule(scope: null, name: "Foundation", includePath: "/SDK/Foundation.swiftmodu
+##; !9 = !DIModule(scope: null, name: "Foundation", includePath: "/SDK/Foundation.swiftmodule")
 ##; !14 = !{!"standard-library", i1 false}
 ##; !20 = !{i32 2, !"Dwarf Version", i32 4}
 ##; !21 = !{i32 2, !"Debug Info Version", i32 3}
 ##; !24 = !{i32 1, !"Swift Version", i32 7}
 ##; !29 = distinct !DISubprogram(name: "main", linkageName: "main", scope: !5, file: !1, line: 1, type: !30, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !2)
 ##; !30 = !DISubroutineType(types: !31)
 ##; !31 = !{}
 ##; !35 = !DILocation(line: 0, scope: !36)
 ##; !36 = !DILexicalBlockFile(scope: !29, file: !37, discriminator: 0)
 ##; !37 = !DIFile(filename: "<compiler-generated>", directory: "")
-        .section  __TEXT,__text,regular,pure_instructions
+  .section  __TEXT,__text,regular,pure_instructions
   .macosx_version_min 10, 9
-  .globl  _main                   ## -- Begin function main
+  .globl  _main                           ## -- Begin function main
   .p2align  4, 0x90
 _main:                                  ## @main
 Lfunc_begin0:
-  .file 1 "/ParseableInterfaceImports.swift"
-  .loc  1 0 0                   ## ParseableInterfaceImports.swift:0:0
+  .file 1 "/" "ParseableInterfaceImports.swift"
+  .loc  1 0 0                           ## ParseableInterfaceImports.swift:0:0
   .cfi_startproc
 ## %bb.0:                               ## %entry
   xorl  %eax, %eax
   retq
 Ltmp0:
 Lfunc_end0:
   .cfi_endproc
                                         ## -- End function
   .private_extern ___swift_reflection_version ## @__swift_reflection_version
   .section  __TEXT,__const
   .globl  ___swift_reflection_version
   .weak_definition  ___swift_reflection_version
-  .p2align  1
+  .p2align  1, 0x0
 ___swift_reflection_version:
-  .short  3                       ## 0x3
+  .short  3                               ## 0x3
 
   .no_dead_strip  ___swift_reflection_version
-  .section  __DWARF,__debug_str,regular,debug
-Linfo_string:
-  .byte 0                       ## string offset=0
-  .asciz  "ParseableInterfaceImports.swift" ## string offset=1
-  .asciz  "/"                     ## string offset=33
-  .asciz  "Foo"                   ## string offset=35
-  .asciz  "/Foo/x86_64.swiftinterface" ## string offset=39
-  .asciz  "Swift"                 ## string offset=66
-  .asciz  "/SDK/Swift.swiftmodule/x86_64.swiftinterface" ## string offset=72
-  .asciz  "/SDK"                  ## string offset=117
-  .asciz  "main"                  ## string offset=122
-  .asciz  "Foundation"            ## string offset=127
-        .asciz  "/SDK/Foundation.swiftmodule/x86_64.swiftinterface" ## string offset=138
   .section  __DWARF,__debug_abbrev,regular,debug
 Lsection_abbrev:
-  .byte 1                       ## Abbreviation Code
-  .byte 17                      ## DW_TAG_compile_unit
-  .byte 1                       ## DW_CHILDREN_yes
-  .byte 37                      ## DW_AT_producer
-  .byte 14                      ## DW_FORM_strp
-  .byte 19                      ## DW_AT_language
-  .byte 5                       ## DW_FORM_data2
-  .byte 3                       ## DW_AT_name
-  .byte 14                      ## DW_FORM_strp
-  .ascii  "\202|"                 ## DW_AT_LLVM_sysroot
-  .byte 14                      ## DW_FORM_strp
-  .byte 16                      ## DW_AT_stmt_list
-  .byte 23                      ## DW_FORM_sec_offset
-  .byte 27                      ## DW_AT_comp_dir
-  .byte 14                      ## DW_FORM_strp
-  .ascii  "\345\177"              ## DW_AT_APPLE_major_runtime_vers
-  .byte 11                      ## DW_FORM_data1
-  .byte 17                      ## DW_AT_low_pc
-  .byte 1                       ## DW_FORM_addr
-  .byte 18                      ## DW_AT_high_pc
-  .byte 6                       ## DW_FORM_data4
-  .byte 0                       ## EOM(1)
-  .byte 0                       ## EOM(2)
-  .byte 2                       ## Abbreviation Code
-  .byte 30                      ## DW_TAG_module
-  .byte 1                       ## DW_CHILDREN_yes
-  .byte 3                       ## DW_AT_name
-  .byte 14                      ## DW_FORM_strp
-  .ascii  "\200|"                 ## DW_AT_LLVM_include_path
-  .byte 14                      ## DW_FORM_strp
-  .byte 0                       ## EOM(1)
-  .byte 0                       ## EOM(2)
-  .byte 3                       ## Abbreviation Code
-  .byte 46                      ## DW_TAG_subprogram
-  .byte 0                       ## DW_CHILDREN_no
-  .byte 17                      ## DW_AT_low_pc
-  .byte 1                       ## DW_FORM_addr
-  .byte 18                      ## DW_AT_high_pc
-  .byte 6                       ## DW_FORM_data4
-  .ascii  "\347\177"              ## DW_AT_APPLE_omit_frame_ptr
-  .byte 25                      ## DW_FORM_flag_present
-  .byte 64                      ## DW_AT_frame_base
-  .byte 24                      ## DW_FORM_exprloc
-  .byte 110                     ## DW_AT_linkage_name
-  .byte 14                      ## DW_FORM_strp
-  .byte 3                       ## DW_AT_name
-  .byte 14                      ## DW_FORM_strp
-  .byte 58                      ## DW_AT_decl_file
-  .byte 11                      ## DW_FORM_data1
-  .byte 59                      ## DW_AT_decl_line
-  .byte 11                      ## DW_FORM_data1
-  .byte 63                      ## DW_AT_external
-  .byte 25                      ## DW_FORM_flag_present
-  .byte 0                       ## EOM(1)
-  .byte 0                       ## EOM(2)
-  .byte 4                       ## Abbreviation Code
-  .byte 58                      ## DW_TAG_imported_module
-  .byte 0                       ## DW_CHILDREN_no
-  .byte 24                      ## DW_AT_import
-  .byte 19                      ## DW_FORM_ref4
-  .byte 0                       ## EOM(1)
-  .byte 0                       ## EOM(2)
-  .byte 5                       ## Abbreviation Code
-  .byte 30                      ## DW_TAG_module
-  .byte 0                       ## DW_CHILDREN_no
-  .byte 3                       ## DW_AT_name
-  .byte 14                      ## DW_FORM_strp
-  .ascii  "\200|"                 ## DW_AT_LLVM_include_path
-  .byte 14                      ## DW_FORM_strp
-  .ascii  "\202|"                 ## DW_AT_LLVM_sysroot
-  .byte 14                      ## DW_FORM_strp
-  .byte 0                       ## EOM(1)
-  .byte 0                       ## EOM(2)
-  .byte 30                      ## DW_TAG_module
-  .byte 1                       ## DW_CHILDREN_no
-  .byte 3                       ## DW_AT_name
-  .byte 14                      ## DW_FORM_strp
-  .ascii  "\200|"                 ## DW_AT_LLVM_include_path
-  .byte 14                      ## DW_FORM_strp
-  .byte 0                       ## EOM(1)
-  .byte 0                       ## EOM(2)
-  .byte 0                       ## EOM(3)
+  .byte 1                               ## Abbreviation Code
+  .byte 17                              ## DW_TAG_compile_unit
+  .byte 1                               ## DW_CHILDREN_yes
+  .byte 37                              ## DW_AT_producer
+  .byte 14                              ## DW_FORM_strp
+  .byte 19                              ## DW_AT_language
+  .byte 5                               ## DW_FORM_data2
+  .byte 3                               ## DW_AT_name
+  .byte 14                              ## DW_FORM_strp
+  .ascii  "\202|"                         ## DW_AT_LLVM_sysroot
+  .byte 14                              ## DW_FORM_strp
+  .byte 16                              ## DW_AT_stmt_list
+  .byte 23                              ## DW_FORM_sec_offset
+  .byte 27                              ## DW_AT_comp_dir
+  .byte 14                              ## DW_FORM_strp
+  .ascii  "\345\177"                      ## DW_AT_APPLE_major_runtime_vers
+  .byte 11                              ## DW_FORM_data1
+  .byte 17                              ## DW_AT_low_pc
+  .byte 1                               ## DW_FORM_addr
+  .byte 18                              ## DW_AT_high_pc
+  .byte 6                               ## DW_FORM_data4
+  .byte 0                               ## EOM(1)
+  .byte 0                               ## EOM(2)
+  .byte 2                               ## Abbreviation Code
+  .byte 30                              ## DW_TAG_module
+  .byte 1                               ## DW_CHILDREN_yes
+  .byte 3                               ## DW_AT_name
+  .byte 14                              ## DW_FORM_strp
+  .ascii  "\200|"                         ## DW_AT_LLVM_include_path
+  .byte 14                              ## DW_FORM_strp
+  .byte 0                               ## EOM(1)
+  .byte 0                               ## EOM(2)
+  .byte 3                               ## Abbreviation Code
+  .byte 46                              ## DW_TAG_subprogram
+  .byte 0                               ## DW_CHILDREN_no
+  .byte 17                              ## DW_AT_low_pc
+  .byte 1                               ## DW_FORM_addr
+  .byte 18                              ## DW_AT_high_pc
+  .byte 6                               ## DW_FORM_data4
+  .ascii  "\347\177"                      ## DW_AT_APPLE_omit_frame_ptr
+  .byte 25                              ## DW_FORM_flag_present
+  .byte 64                              ## DW_AT_frame_base
+  .byte 24                              ## DW_FORM_exprloc
+  .byte 110                             ## DW_AT_linkage_name
+  .byte 14                              ## DW_FORM_strp
+  .byte 3                               ## DW_AT_name
+  .byte 14                              ## DW_FORM_strp
+  .byte 58                              ## DW_AT_decl_file
+  .byte 11                              ## DW_FORM_data1
+  .byte 59                              ## DW_AT_decl_line
+  .byte 11                              ## DW_FORM_data1
+  .byte 63                              ## DW_AT_external
+  .byte 25                              ## DW_FORM_flag_present
+  .byte 0                               ## EOM(1)
+  .byte 0                               ## EOM(2)
+  .byte 4                               ## Abbreviation Code
+  .byte 58                              ## DW_TAG_imported_module
+  .byte 0                               ## DW_CHILDREN_no
+  .byte 24                              ## DW_AT_import
+  .byte 19                              ## DW_FORM_ref4
+  .byte 0                               ## EOM(1)
+  .byte 0                               ## EOM(2)
+  .byte 5                               ## Abbreviation Code
+  .byte 30                              ## DW_TAG_module
+  .byte 0                               ## DW_CHILDREN_no
+  .byte 3                               ## DW_AT_name
+  .byte 14                              ## DW_FORM_strp
+  .ascii  "\200|"                         ## DW_AT_LLVM_include_path
+  .byte 14                              ## DW_FORM_strp
+  .byte 0                               ## EOM(1)
+  .byte 0                               ## EOM(2)
+  .byte 0                               ## EOM(3)
   .section  __DWARF,__debug_info,regular,debug
 Lsection_info:
 Lcu_begin0:
 .set Lset0, Ldebug_info_end0-Ldebug_info_start0 ## Length of Unit
   .long Lset0
 Ldebug_info_start0:
-  .short  4                       ## DWARF version number
+  .short  4                               ## DWARF version number
 .set Lset1, Lsection_abbrev-Lsection_abbrev ## Offset Into Abbrev. Section
   .long Lset1
-  .byte 8                       ## Address Size (in bytes)
-  .byte 1                       ## Abbrev [1] 0xb:0x5b DW_TAG_compile_unit
-  .long 0                       ## DW_AT_producer
-  .short  30                      ## DW_AT_language
-  .long 1                       ## DW_AT_name
-        .long  117                     ## DW_AT_name
+  .byte 8                               ## Address Size (in bytes)
+  .byte 1                               ## Abbrev [1] 0xb:0x60 DW_TAG_compile_unit
+  .long 0                               ## DW_AT_producer
+  .short  30                              ## DW_AT_language
+  .long 1                               ## DW_AT_name
+  .long  33                              ## DW_AT_LLVM_sysroot
 .set Lset2, Lline_table_start0-Lsection_line ## DW_AT_stmt_list
   .long Lset2
-  .long 33                      ## DW_AT_comp_dir
-  .byte 5                       ## DW_AT_APPLE_major_runtime_vers
-  .quad Lfunc_begin0            ## DW_AT_low_pc
+  .long 38                              ## DW_AT_comp_dir
+  .byte 5                               ## DW_AT_APPLE_major_runtime_vers
+  .quad Lfunc_begin0                    ## DW_AT_low_pc
 .set Lset3, Lfunc_end0-Lfunc_begin0     ## DW_AT_high_pc
   .long Lset3
-  .byte 2                       ## Abbrev [2] 0x2b:0x23 DW_TAG_module
-  .long 35                      ## DW_AT_name
-  .long 39                      ## DW_AT_LLVM_include_path
-  .byte 3                       ## Abbrev [3] 0x34:0x19 DW_TAG_subprogram
-  .quad Lfunc_begin0            ## DW_AT_low_pc
+  .byte 2                               ## Abbrev [2] 0x2f:0x23 DW_TAG_module
+  .long 40                              ## DW_AT_name
+  .long 44                              ## DW_AT_LLVM_include_path
+  .byte 3                               ## Abbrev [3] 0x38:0x19 DW_TAG_subprogram
+  .quad Lfunc_begin0                    ## DW_AT_low_pc
 .set Lset4, Lfunc_end0-Lfunc_begin0     ## DW_AT_high_pc
   .long Lset4
                                         ## DW_AT_APPLE_omit_frame_ptr
-  .byte 1                       ## DW_AT_frame_base
+  .byte 1                               ## DW_AT_frame_base
   .byte 87
-  .long 122                     ## DW_AT_linkage_name
-  .long 122                     ## DW_AT_name
-  .byte 1                       ## DW_AT_decl_file
-  .byte 1                       ## DW_AT_decl_line
+  .long 122                             ## DW_AT_linkage_name
+  .long 122                             ## DW_AT_name
+  .byte 1                               ## DW_AT_decl_file
+  .byte 1                               ## DW_AT_decl_line
                                         ## DW_AT_external
-  .byte 0                       ## End Of Children Mark
-  .byte 4                       ## Abbrev [4] 0x4e:0x5 DW_TAG_imported_module
-  .long 47                      ## DW_AT_import
-  .byte 5                       ## Abbrev [5] 0x53:0xd DW_TAG_module
-  .long 66                      ## DW_AT_name
-  .long 72                      ## DW_AT_LLVM_include_path
-  .long 117                     ## DW_AT_LLVM_sysroot
-  .byte 4                       ## Abbrev [4] 0x60:0x5 DW_TAG_imported_module
-  .long 105                      ## DW_AT_import
-  .byte 2                       ## Abbrev [2] 0x2b:0x23 DW_TAG_module
-  .long 127                     ## DW_AT_name
-  .long 138                     ## DW_AT_LLVM_include_path
-  .byte 0                       ## End Of Children Mark
-  .byte 0                       ## End Of Children Mark
+  .byte 0                               ## End Of Children Mark
+  .byte 4                               ## Abbrev [4] 0x52:0x5 DW_TAG_imported_module
+  .long 47                              ## DW_AT_import
+  .byte 5                               ## Abbrev [5] 0x57:0x9 DW_TAG_module
+  .long 71                              ## DW_AT_name
+  .long 77                              ## DW_AT_LLVM_include_path
+  .byte 4                               ## Abbrev [4] 0x60:0x5 DW_TAG_imported_module
+  .long 87                              ## DW_AT_import
+  .byte  4                               ## Abbrev [4] 0x65:0x5 DW_TAG_imported_module
+  .long 87                              ## DW_AT_import
+  .byte 0                               ## End Of Children Mark
 Ldebug_info_end0:
+  .section  __DWARF,__debug_str,regular,debug
+Linfo_string:
+  .byte 0                               ## string offset=0
+  .asciz  "ParseableInterfaceImports.swift" ## string offset=1
+  .asciz  "/SDK"                          ## string offset=33
+  .asciz  "/"                             ## string offset=38
+  .asciz  "Foo"                           ## string offset=40
+  .asciz  "/Foo/x86_64.swiftinterface"    ## string offset=44
+  .asciz  "Swift"                         ## string offset=71
+  .asciz  "/SDK/Swift.swiftmodule/x86_64.swiftinterface" ## string offset=77
+  .asciz  "main"                          ## string offset=122
+  .section  __DWARF,__apple_names,regular,debug
+Lnames_begin:
+  .long 1212240712                      ## Header Magic
+  .short  1                               ## Header Version
+  .short  0                               ## Header Hash Function
+  .long 1                               ## Header Bucket Count
+  .long 1                               ## Header Hash Count
+  .long 12                              ## Header Data Length
+  .long 0                               ## HeaderData Die Offset Base
+  .long 1                               ## HeaderData Atom Count
+  .short  1                               ## DW_ATOM_die_offset
+  .short  6                               ## DW_FORM_data4
+  .long 0                               ## Bucket 0
+  .long 2090499946                      ## Hash in Bucket 0
+.set Lset5, LNames0-Lnames_begin        ## Offset in Bucket 0
+  .long Lset5
+LNames0:
+  .long 122                             ## main
+  .long 1                               ## Num DIEs
+  .long 56
+  .long 0
+  .section  __DWARF,__apple_objc,regular,debug
+Lobjc_begin:
+  .long 1212240712                      ## Header Magic
+  .short  1                               ## Header Version
+  .short  0                               ## Header Hash Function
+  .long 1                               ## Header Bucket Count
+  .long 0                               ## Header Hash Count
+  .long 12                              ## Header Data Length
+  .long 0                               ## HeaderData Die Offset Base
+  .long 1                               ## HeaderData Atom Count
+  .short  1                               ## DW_ATOM_die_offset
+  .short  6                               ## DW_FORM_data4
+  .long -1                              ## Bucket 0
+  .section  __DWARF,__apple_namespac,regular,debug
+Lnamespac_begin:
+  .long 1212240712                      ## Header Magic
+  .short  1                               ## Header Version
+  .short  0                               ## Header Hash Function
+  .long 1                               ## Header Bucket Count
+  .long 0                               ## Header Hash Count
+  .long 12                              ## Header Data Length
+  .long 0                               ## HeaderData Die Offset Base
+  .long 1                               ## HeaderData Atom Count
+  .short  1                               ## DW_ATOM_die_offset
+  .short  6                               ## DW_FORM_data4
+  .long -1                              ## Bucket 0
+  .section  __DWARF,__apple_types,regular,debug
+Ltypes_begin:
+  .long 1212240712                      ## Header Magic
+  .short  1                               ## Header Version
+  .short  0                               ## Header Hash Function
+  .long 1                               ## Header Bucket Count
+  .long 0                               ## Header Hash Count
+  .long 20                              ## Header Data Length
+  .long 0                               ## HeaderData Die Offset Base
+  .long 3                               ## HeaderData Atom Count
+  .short  1                               ## DW_ATOM_die_offset
+  .short  6                               ## DW_FORM_data4
+  .short  3                               ## DW_ATOM_die_tag
+  .short  5                               ## DW_FORM_data2
+  .short  4                               ## DW_ATOM_type_flags
+  .short  11                              ## DW_FORM_data1
+  .long -1                              ## Bucket 0
 .subsections_via_symbols
   .section  __DWARF,__debug_line,regular,debug
 Lsection_line:
 Lline_table_start0:

llvm/unittests/DebugInfo/DWARF/DWARFDebugAbbrevTest.cpp

 #include "llvm/Testing/Support/Error.h"
 #include "gtest/gtest.h"
 
 using namespace llvm;
 using namespace dwarf;
 
 enum OrderKind : bool { InOrder, OutOfOrder };
 
-void writeAbbreviationDeclarations(raw_ostream &OS, uint32_t FirstCode,
+void writeValidAbbreviationDeclarations(raw_ostream &OS, uint32_t FirstCode,
                                         OrderKind Order) {
   encodeULEB128(FirstCode, OS);
   encodeULEB128(DW_TAG_compile_unit, OS);
   OS << static_cast<uint8_t>(DW_CHILDREN_yes);
   encodeULEB128(DW_AT_name, OS);
   encodeULEB128(DW_FORM_strp, OS);
   encodeULEB128(0, OS);
   encodeULEB128(0, OS);
 
void writeValidAbbreviationDeclarations(raw_ostream &OS, uint32_t FirstCode,
   encodeULEB128(0, OS);
 }
 
 TEST(DWARFDebugAbbrevTest, DWARFAbbrevDeclSetExtractSuccess) {
   SmallString<64> RawData;
   raw_svector_ostream OS(RawData);
   uint32_t FirstCode = 5;
 
-  writeAbbreviationDeclarations(OS, FirstCode, InOrder);
+  writeValidAbbreviationDeclarations(OS, FirstCode, InOrder);
   encodeULEB128(0, OS);
 
   uint64_t Offset = 0;
   DataExtractor Data(RawData, sys::IsLittleEndianHost, sizeof(uint64_t));
   DWARFAbbreviationDeclarationSet AbbrevSet;
-  const bool DataWasExtracted = AbbrevSet.extract(Data, &Offset);
-  EXPECT_TRUE(DataWasExtracted);
+  llvm::Error Err = AbbrevSet.extract(Data, &Offset);
+  ASSERT_FALSE(bool(Err));
   // The Abbreviation Declarations are in order and contiguous, so we want to
   // make sure that FirstAbbrCode was correctly set.
   EXPECT_EQ(AbbrevSet.getFirstAbbrCode(), FirstCode);
 
   const DWARFAbbreviationDeclaration *Abbrev5 =
       AbbrevSet.getAbbreviationDeclaration(FirstCode);
   ASSERT_TRUE(Abbrev5);
   EXPECT_EQ(Abbrev5->getTag(), DW_TAG_compile_unit);
   EXPECT_TRUE(Abbrev5->hasChildren());
   EXPECT_EQ(Abbrev5->getNumAttributes(), 1u);
 
   const DWARFAbbreviationDeclaration *Abbrev6 =
       AbbrevSet.getAbbreviationDeclaration(FirstCode + 1);
   ASSERT_TRUE(Abbrev6);
   EXPECT_EQ(Abbrev6->getTag(), DW_TAG_subprogram);
   EXPECT_FALSE(Abbrev6->hasChildren());
   EXPECT_EQ(Abbrev6->getNumAttributes(), 1u);
 }
 
 TEST(DWARFDebugAbbrevTest, DWARFAbbrevDeclSetExtractSuccessOutOfOrder) {
   SmallString<64> RawData;
   raw_svector_ostream OS(RawData);
   uint32_t FirstCode = 2;
 
-  writeAbbreviationDeclarations(OS, FirstCode, OutOfOrder);
+  writeValidAbbreviationDeclarations(OS, FirstCode, OutOfOrder);
   encodeULEB128(0, OS);
 
   uint64_t Offset = 0;
   DataExtractor Data(RawData, sys::IsLittleEndianHost, sizeof(uint64_t));
   DWARFAbbreviationDeclarationSet AbbrevSet;
-  const bool DataWasExtracted = AbbrevSet.extract(Data, &Offset);
-  EXPECT_TRUE(DataWasExtracted);
+  llvm::Error Err = AbbrevSet.extract(Data, &Offset);
+  ASSERT_FALSE(bool(Err));
   // The declarations are out of order, ensure that FirstAbbrCode is UINT32_MAX.
   EXPECT_EQ(AbbrevSet.getFirstAbbrCode(), UINT32_MAX);
 
   const DWARFAbbreviationDeclaration *Abbrev2 =
       AbbrevSet.getAbbreviationDeclaration(FirstCode);
   ASSERT_TRUE(Abbrev2);
   EXPECT_EQ(Abbrev2->getTag(), DW_TAG_compile_unit);
   EXPECT_TRUE(Abbrev2->hasChildren());
   EXPECT_EQ(Abbrev2->getNumAttributes(), 1u);
 
   const DWARFAbbreviationDeclaration *Abbrev1 =
       AbbrevSet.getAbbreviationDeclaration(FirstCode - 1);
   ASSERT_TRUE(Abbrev1);
   EXPECT_EQ(Abbrev1->getTag(), DW_TAG_subprogram);
   EXPECT_FALSE(Abbrev1->hasChildren());
   EXPECT_EQ(Abbrev1->getNumAttributes(), 1u);
 }
+
+TEST(DWARFDebugAbbrevTest, DWARFAbbrevDeclSetInvalidTag) {
+  SmallString<64> RawData;
+  raw_svector_ostream OS(RawData);
+  uint32_t FirstCode = 1;
+  // First, we're going to manually add good data.
+  writeValidAbbreviationDeclarations(OS, FirstCode, InOrder);
+
+  // Afterwards, we're going to write an Abbreviation Decl manually with an
+  // invalid tag.
+  encodeULEB128(FirstCode + 2, OS);
+  encodeULEB128(0, OS); // Invalid Tag
+  OS << static_cast<uint8_t>(DW_CHILDREN_no);
+  encodeULEB128(DW_AT_name, OS);
+  encodeULEB128(DW_FORM_strp, OS);
+  encodeULEB128(0, OS);
+  encodeULEB128(0, OS);
+
+  encodeULEB128(0, OS);
+
+  uint64_t Offset = 0;
+  DataExtractor Data(RawData, sys::IsLittleEndianHost, sizeof(uint64_t));
+  DWARFAbbreviationDeclarationSet AbbrevSet;
+  llvm::Error Err = AbbrevSet.extract(Data, &Offset);
+  // Verify that we got an error and it is the one we expected.
+  ASSERT_TRUE(bool(Err));
+  EXPECT_EQ("abbreviation declaration requires a non-null tag",
+            llvm::toString(std::move(Err)));
+}
+
+TEST(DWARFDebugAbbrevTest, DWARFAbbrevDeclSetInvalidAttrValidForm) {
+  SmallString<64> RawData;
+  raw_svector_ostream OS(RawData);
+  uint32_t FirstCode = 120;
+  // First, we're going to manually add good data.
+  writeValidAbbreviationDeclarations(OS, FirstCode, InOrder);
+
+  // Afterwards, we're going to write an Abbreviation Decl manually with an
+  // invalid attribute but valid form.
+  encodeULEB128(FirstCode - 5, OS);
+  encodeULEB128(DW_TAG_compile_unit, OS);
+  OS << static_cast<uint8_t>(DW_CHILDREN_no);
+  encodeULEB128(0, OS); // Invalid attribute followed by an invalid form.
+  encodeULEB128(DW_FORM_strp, OS);
+  encodeULEB128(0, OS);
+  encodeULEB128(0, OS);
+
+  encodeULEB128(0, OS);
+
+  uint64_t Offset = 0;
+  DataExtractor Data(RawData, sys::IsLittleEndianHost, sizeof(uint64_t));
+  DWARFAbbreviationDeclarationSet AbbrevSet;
+  llvm::Error Err = AbbrevSet.extract(Data, &Offset);
+  // Verify that we got an error and it is the one we expected.
+  ASSERT_TRUE(bool(Err));
+  EXPECT_EQ("malformed abbreviation declaration attribute. Either the "
+            "attribute or the form is zero while the other is not",
+            llvm::toString(std::move(Err)));
+}
+
+TEST(DWARFDebugAbbrevTest, DWARFAbbrevDeclSetValidAttrInvalidForm) {
+  SmallString<64> RawData;
+  raw_svector_ostream OS(RawData);
+  uint32_t FirstCode = 120;
+  // First, we're going to manually add good data.
+  writeValidAbbreviationDeclarations(OS, FirstCode, InOrder);
+
+  // Afterwards, we're going to write an Abbreviation Decl manually with a
+  // valid attribute but invalid form.
+  encodeULEB128(FirstCode - 5, OS);
+  encodeULEB128(DW_TAG_compile_unit, OS);
+  OS << static_cast<uint8_t>(DW_CHILDREN_no);
+  encodeULEB128(DW_AT_name, OS);
+  encodeULEB128(0, OS); // Invalid form after a valid attribute.
+  encodeULEB128(0, OS);
+  encodeULEB128(0, OS);
+
+  encodeULEB128(0, OS);
+
+  uint64_t Offset = 0;
+  DataExtractor Data(RawData, sys::IsLittleEndianHost, sizeof(uint64_t));
+  DWARFAbbreviationDeclarationSet AbbrevSet;
+  llvm::Error Err = AbbrevSet.extract(Data, &Offset);
+  // Verify that we got an error and it is the one we expected.
+  ASSERT_TRUE(bool(Err));
+  EXPECT_EQ("malformed abbreviation declaration attribute. Either the "
+            "attribute or the form is zero while the other is not",
+            llvm::toString(std::move(Err)));
+}
+
+TEST(DWARFDebugAbbrevTest, DWARFAbbrevDeclSetMissingTerminator) {
+  SmallString<64> RawData;
+  raw_svector_ostream OS(RawData);
+  uint32_t FirstCode = 120;
+  // First, we're going to manually add good data.
+  writeValidAbbreviationDeclarations(OS, FirstCode, InOrder);
+
+  // Afterwards, we're going to write an Abbreviation Decl manually without a
+  // termintating sequence.
+  encodeULEB128(FirstCode + 7, OS);
+  encodeULEB128(DW_TAG_compile_unit, OS);
+  OS << static_cast<uint8_t>(DW_CHILDREN_no);
+  encodeULEB128(DW_AT_name, OS);
+  encodeULEB128(DW_FORM_strp, OS);
+
+  uint64_t Offset = 0;
+  DataExtractor Data(RawData, sys::IsLittleEndianHost, sizeof(uint64_t));
+  DWARFAbbreviationDeclarationSet AbbrevSet;
+  llvm::Error Err = AbbrevSet.extract(Data, &Offset);
+  // Verify that we got an error and it is the one we expected.
+  ASSERT_TRUE(bool(Err));
+  EXPECT_EQ("abbreviation declaration attribute list was not terminated with a "
+            "null entry",
+            llvm::toString(std::move(Err)));
+}