[DWARF5]: Added support for dumping strx forms in llvm-dwarfdump

[SouraVX]

Summary

This patch adds support for dumping DW_MACRO_define_strx, DW_MACRO_undef_strx in llvm-dwarfdump. These forms are currently supported only in debug_macro section.

• Added test containing multiple CUs(2 CU) contributing to macro section via DW_MACRO_define_strx form. llvm-dwarfdump is able to parse/dump contents correctly.

This patch is separated from D78500 based on feedback from @dblaikie , @ikudrin, Thanks for this!

Support for these forms in a debug_macro.dwo section will be done in a separate patch.

[SouraVX]

Rebase!

[SouraVX]

llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp

117

This check some how crept in while rebasing and separating. I'll remove this.

[SouraVX]

Removed un-necessary if condition left behind.

[probinson]

llvm/include/llvm/DebugInfo/DWARF/DWARFDebugMacro.h

12

Typo //, also please keep includes in correct alphabetical order.

llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp

92

It looks like StringExtractor should not be Optional?

[SouraVX]

llvm/include/llvm/DebugInfo/DWARF/DWARFDebugMacro.h

12

Ah, Sorry! I'll keep this in mind now onwards.
Thank You for this!

llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp

92

I marked it optional since, for cases of debug_macinfo[.dwo] . String Extractor is not needed since the macro info are represented in macinfo[.dwo] section itself.

[ikudrin]

The test should be reduced.

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

273

You do not need type units to parse the macro section. It is better to pass compile_units().

llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp

103

Not all units may reference the macro section. You should skip such units.

161–166

DWARFUnit provides a method for that, getStringOffsetSectionItem(). Please, use it.

By the way, the string extractor can also be obtained from DWARFUnit using the getStringExtractor() method.

[SouraVX]

Addressed @ikudrin comments thanks for this!

• replaced normal_units() with compile_units()
• Skipped the units which doesn't contribute to macro section.
• Removed getStringOffsetsExtractor from arguments and used the corresponding getStringOffsetSectionItem() function to retrieve contents.

[SouraVX]

llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp

161–166

By the way, the string extractor can also be obtained from DWARFUnit using the getStringExtractor() method.

There's small caveat here, consider a case of _strp forms. For this case we don't need any CU information to parse/dump the associated macro content.
So we if try this way --

E.MacroStr = GetContributionUnit()->getStringExtractor().getCStr(&StrOffset);

Then cases which doesn't have CU in first place will end up crashing the dwarfdump.
I tried this approach and ended with crashes for these test cases, as you may notice these test cases only contains the minimum needed information to dump _strp forms which is debug_str and debug_macro section --

Failing Tests (2):
  LLVM :: DebugInfo/X86/debug-macro-macinfo.s
  LLVM :: DebugInfo/X86/debug-macro-v5.s

Shall we continue with earlier approach(pass the String Extractor explicitly) just for the cases of _strp form ?

[SouraVX]

The test should be reduced.

Sorry I saw this a bit late. Just to be on same page WRT this.
I'm planning to reduce it more(previously removed language producer ... attributes) by removing following attributes - DW_AT_stmt_list, DW_AT_comp_dir and remove corresponding temporary labels.
I think rest of the stuff is needed, Please add to this if I'm still missing out.
Thanks a lot!

[SouraVX]

Reduced the test case.

[ikudrin]

Please, do not split definitions for sections in the test. It adds no value, just makes it harder to read.

llvm/include/llvm/DebugInfo/DWARF/DWARFDebugMacro.h

104–116

It starts looking that it is better to have two distinct methods in the public interface, parseMacinfo() and parseMacro(), than adding lots of the optional parameters. They can still share the implementation, though.

llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp

104

I would probably use try_emplace() instead of insert() here. It is the same but does not require curly brackets.

117

The lambda is simple and is used only in one place. Just move the code there.

119

• What if there is no record for that offset in the map?
• Why not just store a pointer to DWARFUnit in the map?

156–176

StringExtractor->getCStr(). But, please, check that StringExtractor has a value.

llvm/test/DebugInfo/X86/debug-macro-multi-cu-strx.s

68–70

Add comments for these values.

84

This second part of the .debug_abbrev section is not used.

[SouraVX]

Addressed @ikudrin comments, Thanks for this!

• Addressed the case when there are no records in map. By asserting it before we parsing the _strx form.
• Addressed test case related comments. Thanks for this, now test is more reduced and more readeable.
• lambda removal, using DWARFUnit, using try_emplace.

[SouraVX]

It starts looking that it is better to have two distinct methods in the public interface, parseMacinfo() and parseMacro(), than adding lots of the optional parameters. They can still share the implementation, though.

Yes, I'm also noticing it. But this can be tricky(since we want code sharing too) these macro and macinfo is using 4 common forms. I'll be separately investigating this to cleanly refactor it so that the public interface should be more clean/intuitive. I think a private member function parseImpl to be called by public parseMacinfo/Macro would be the best fit?

• This is just an abstract idea, have to dig-in more for implementation. May be 1/2 other patches ?

[SouraVX]

llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp

163

Ah, Small typo! this should be MacroContributionOffset

[SouraVX]

Removed a typo! + Rebase!

[ikudrin]

In D78736#2005497, @SouraVX wrote:

It starts looking that it is better to have two distinct methods in the public interface, parseMacinfo() and parseMacro(), than adding lots of the optional parameters. They can still share the implementation, though.

Yes, I'm also noticing it. But this can be tricky(since we want code sharing too) these macro and macinfo is using 4 common forms. I'll be separately investigating this to cleanly refactor it so that the public interface should be more clean/intuitive. I think a private member function parseImpl to be called by public parseMacinfo/Macro would be the best fit?

• This is just an abstract idea, have to dig-in more for implementation. May be 1/2 other patches ?

We can go with a private parseImpl() for now. It may be improved later.

[ikudrin]

llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp

156–176

assert should not be used for the cases when malicious user input may trigger it.

164

The same. Please review other lines in this block to avoid runtime errors/crashes on unexpected input.

llvm/test/DebugInfo/X86/debug-macro-multi-cu-strx.s

83

The definitions for both CUs should lay under one .section directive. In that case, a reader can easily see that there are two CUs in the test. If there are several .section directives for one section, it makes reading the test more difficult.

The same for other sections.

[SouraVX]

In D78736#2007450, @ikudrin wrote:

In D78736#2005497, @SouraVX wrote:

It starts looking that it is better to have two distinct methods in the public interface, parseMacinfo() and parseMacro(), than adding lots of the optional parameters. They can still share the implementation, though.

Yes, I'm also noticing it. But this can be tricky(since we want code sharing too) these macro and macinfo is using 4 common forms. I'll be separately investigating this to cleanly refactor it so that the public interface should be more clean/intuitive. I think a private member function parseImpl to be called by public parseMacinfo/Macro would be the best fit?

• This is just an abstract idea, have to dig-in more for implementation. May be 1/2 other patches ?

We can go with a private parseImpl() for now. It may be improved later.

Thanks for sharing your thoughts @ikudrin!
Should we do this refactoring after these 4 patches are in and macro infrastructure is all set ? Or should we delay these patches and focus on refactoring first ? Advantage of the former approach would be that we'll have all ingredients(macro macinfo split & non-split) already and then doing the refactoring.
Could you please share your thoughts on this too! Thanks!

[SouraVX]

llvm/test/DebugInfo/X86/debug-macro-multi-cu-strx.s

83

Thanks for clearing this!, I will make sure to follow this practice in all test cases and revision.

[SouraVX]

Addressed review comments by @ikudrin . Thanks for this!
Following changes have been made to this revision-

• Refactoring front based on comment by @ikudrin -
  • In an effort to simplify the public interfacing of macro/macinfo,( which is currently used by DWARFContext). public member function parse and private member function parseImpl(to do actual parsing) are introduced in class DWARFDebugMacro. This approach simplifies the interfacing to DWARFContext to some extent(atleast parameter passing and readeability).
  • Also included a FIXME, as a hint that we should introduce a better abstraction.

On testing front -

• Previously we're asserting for a malformed user provided input, that has been switched to error handling. As a result 2 test cases were also added to verify these error messages.
• Furthermore test case is further reduced based on @ikudrin suggestion. Thanks for this, it helped a lot, now test case is more readable asm then previous.

• Lastly, I'll be revising other patches specifically D78500, After we reach consensus on the changes in this revision.

[SouraVX]

llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp

156–176

This String Extractor is innocuous in a sense, it is passed as parameters by functions up in the stack. So I left this `assert as it is. ? Are you okay with this.
Other asserts, I've replaced with error handling.

[SouraVX]

Ping! @all reviewers!

[ikudrin]

llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp

156–176

What if a malformed .debug_macinfo section contains a DW_MACRO_define_strp entry? Does the code have any protection against that?

[SouraVX]

llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp

156–176

Ah, thanks for noting that. I think we should consider assert for these cases DW_MACRO_define_strp and DW_MACRO_define_strx since these forms belongs and should only be present when section is debug_macro. + Test cases to test these cases.
Does this sounds fair to you ?

[SouraVX]

Addressed @ikudrin comments, Thanks for this!

• Added assert while parsing _strp/_strx forms. Since DW_MACRO_define_strp and DW_MACRO_define_strx forms are only allowed in macro and macro[.dwo] sections respectively.

[SouraVX]

llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp

150

This different indentation for both assert statements is after running git-clang-format )

[ikudrin]

llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp

156–176

As I have already said, asserts should not be used to handle invalid user input.

[SouraVX]

Addressed @ikudrin comments, Thanks for this!

• Replaced asserts with error handling and added 2 test cases to test these error creation and dumping.

[ikudrin]

llvm/include/llvm/DebugInfo/DWARF/DWARFDebugMacro.h

108

The method looks weird. It would be better to add two separate methods to parse macro and macinfo sections respectively.

llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp

169

This probably should be a recoverable error. It would be good to report the offset of the macro table.

171

You've already found the record, no need to search again.

176

This also should be a recoverable error. It would be good to report the problematic offset.

[SouraVX]

llvm/include/llvm/DebugInfo/DWARF/DWARFDebugMacro.h

108

I thought we already had a discussion/agreement on this:
https://reviews.llvm.org/D78736#2007450

llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp

169

I guess it should be non-recoverable error since:

• Once an Macro Offset is not found, rest of the macro unit or a particular contribution of the CU(containing strx forms) becomes non-parseable(Since we cannot establish the mapping b/w macro contribution to String contribution).

This is based on premises that the contribution is homogeneous, in a sense, contribution contains single type(in this case strx) form to represent the macro information. This is expected since most producers don't use multiple forms(strp strx or pre-v5 forms such as define ) when producing macro info for a CU.
This also holds true for the 2nd case/comment where a String Contribution is not found.
This seems recoverable error in a mixed form(strp or DW_MACRO_define) contribution scenario: but this is hand crafted case.

debug_macro: 
Macro Header
DW_MACRO_define_strx
....
DW_MACRO_define

[ikudrin]

llvm/include/llvm/DebugInfo/DWARF/DWARFDebugMacro.h

108

It looks like I was not clear enough there. I meant to move the implementation into a private method parseImpl() and add two public methods parseMacinfo() and parseMacro(). This will make the public interface of the class clear while the implementation may be improved later.

llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp

151

For consistency, the actions should be the same as in the default case. From the perspective of MACINFO sections, all these encodings are unknown.

169

In general, if an invalid record can be safely skipped, we prefer to do that, generate a recoverable error, and continue parsing. Otherwise, recoverable errors would not exist at all.

Anyway, that is not very critical for this patch. Handling error cases may be improved later when/if someone needs that.

llvm/test/DebugInfo/X86/debug-macro-multi-cu-strx.s

103

That is a bit weird that units with 3 and 2 string offsets both have the same unit length.

[SouraVX]

Addressed @ikudrin comments, Thanks for this!

• Simplified the public interface further, as per suggestion. Now parseMacro and parseMacinfo is the public interfacing of parsing macro and macinfo respectively.
• For the case of strp or strx forms found in macinfo[.dwo] section, halt parsing(default behavior since these forms are invalid WRT debug_macinfo[.dwo] sections.)
• Corrected the unit length in test case.

[ikudrin]

LGTM with a nit. Please wait a couple of days in case other reviewers have anything to say,

llvm/test/DebugInfo/X86/debug-macro-empty-str-offset.s

3

Remove trailing whitespace in this line.

[SouraVX]

In D78736#2026656, @ikudrin wrote:

LGTM with a nit. Please wait a couple of days in case other reviewers have anything to say,

Thank You @ikudrin for investing time in reviewing this. I'll wait for a while(till Monday) to have other reviewers(including @dblaikie , @aprantl and others) opinion on this.
I'm planning to revise rest of the revisions after this one lands.

[dblaikie]

llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp

169

+1 here - while I don't feel super invested in the error handling here, I'd like us to stick to consistent habits here - please at least provide a follow-up patch in short-order after this one lands that changes this to a non-fatal error & tests that.

170–173

This variable name and error message seem a bit off. This isn't about the fact that the "macro contribution" for "the unit" couldn't be found - but somewhat the opposite "No compile unit found for this macro contribution (needed for discovering the str_offsets contribution when decoding a strx form in the macro contribution)" or something like that, perhaps.

177

Prefer just the straight boolean test:

if (!StrOffset)

183

No need for the extra () here, just:

&*StrOffset

[dblaikie]

(the usual convention is to address outstanding comments that are made when a patch is approved (such as the naming and parentheses I mentioned) before committing (this is done to save an extra feedback loop when the changes seem sufficiently obvious to not need another check to ensure they were done as requested) - so if you could do those in a follow-up commit (no need to send it for review, but please follow-up here and mention the commit hashes of the commits that address the feedback))

[SouraVX]

In D78736#2030799, @dblaikie wrote:

(the usual convention is to address outstanding comments that are made when a patch is approved (such as the naming and parentheses I mentioned) before committing (this is done to save an extra feedback loop when the changes seem sufficiently obvious to not need another check to ensure they were done as requested) - so if you could do those in a follow-up commit (no need to send it for review, but please follow-up here and mention the commit hashes of the commits that address the feedback))

Ah Sorry! But I've addressed/incorporated if (!StrOffset) and (&(*StrOffset) related comments Correctly right?.
Only the Variable renaming and Error message is remaining which I plan to address separately(Since making the error recoverable won't be trivial because, in case of error(MacroContribution missing or StrOffsets missing) we have to correctly skip the entire CU macro contribution and start parsing the next CU if it exist.).
Right now I'm rebasing rest of the 3 revision on top of this.

[dblaikie]

In D78736#2030810, @SouraVX wrote:

In D78736#2030799, @dblaikie wrote:

(the usual convention is to address outstanding comments that are made when a patch is approved (such as the naming and parentheses I mentioned) before committing (this is done to save an extra feedback loop when the changes seem sufficiently obvious to not need another check to ensure they were done as requested) - so if you could do those in a follow-up commit (no need to send it for review, but please follow-up here and mention the commit hashes of the commits that address the feedback))

Ah Sorry! But I've addressed/incorporated if (!StrOffset) and (&(*StrOffset) related comments Correctly right?.

Oh, right, so you did! My mistake! (I thought I'd checked the Phab close email & thought it didn't mention any further changes - but it did! totally my mistake)

Only the Variable renaming and Error message is remaining which I plan to address separately(Since making the error recoverable won't be trivial because, in case of error(MacroContribution missing or StrOffsets missing) we have to correctly skip the entire CU macro contribution and start parsing the next CU if it exist.).
Right now I'm rebasing rest of the 3 revision on top of this.

*nod* Sounds good.

llvm/include/llvm/DebugInfo/DWARF/DWARFDebugMacro.h

 //===- DWARFDebugMacro.h ----------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_DEBUGINFO_DWARF_DWARFDEBUGMACRO_H
 #define LLVM_DEBUGINFO_DWARF_DWARFDEBUGMACRO_H
 
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/DebugInfo/DWARF/DWARFDataExtractor.h"
+#include "llvm/DebugInfo/DWARF/DWARFUnit.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/WithColor.h"
 #include <cstdint>
 
 namespace llvm {
 
 class raw_ostream;
#include "llvm/BinaryFormat/Dwarf.def"
   /// A list of all the macro entries in the debug_macinfo section.
   std::vector<MacroList> MacroLists;
 
 public:
   DWARFDebugMacro() = default;
 
   /// Print the macro list found within the debug_macinfo/debug_macro section.
   void dump(raw_ostream &OS) const;
 
-  /// Parse the debug_macinfo/debug_macro section accessible via the 'Data'
-  /// parameter.
-  Error parse(DataExtractor StringExtractor, DWARFDataExtractor Data,
-              bool IsMacro);
+  /// Helper function for abstracting the parsing of debug_macinfo/debug_macro
+  /// section.
+  // FIXME: We should provide a better abstraction.
+  Error parse(DWARFUnitVector::iterator_range Units,
+              DataExtractor StringExtractor, DWARFDataExtractor MacroData,
+              bool IsMacro) {
+    if (IsMacro)
+      return parseImpl(Units, StringExtractor, MacroData, IsMacro);
+    else
+      return parseImpl(None, None, MacroData, IsMacro);
+    return Error::success();
+  }
 
   /// Return whether the section has any entries.
   bool empty() const { return MacroLists.empty(); }
+
+private:
+  /// Parse the debug_macinfo/debug_macro section accessible via the 'MacroData'
+  /// parameter.
+  Error parseImpl(Optional<DWARFUnitVector::iterator_range> Units,
+                  Optional<DataExtractor> StringExtractor,
+                  DWARFDataExtractor Data, bool IsMacro);
 };
 
 } // end namespace llvm
 
 #endif // LLVM_DEBUGINFO_DWARF_DWARFDEBUGMACRO_H

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

while (rnglistData.isValidOffset(Offset)) {
     }
   }
 }
 
 std::unique_ptr<DWARFDebugMacro>
 DWARFContext::parseMacroOrMacinfo(MacroSecType SectionType) {
   auto Macro = std::make_unique<DWARFDebugMacro>();
   auto ParseAndDump = [&](DWARFDataExtractor &Data, bool IsMacro) {
-    if (Error Err = Macro->parse(getStringExtractor(), Data, IsMacro)) {
+    // FIXME: Add support for debug_macro.dwo section.
+    if (Error Err = Macro->parse(compile_units(), getStringExtractor(), Data,
+                                 IsMacro)) {
       RecoverableErrorHandler(std::move(Err));
       Macro = nullptr;
     }
   };
   switch (SectionType) {
   case MacinfoSection: {
     DWARFDataExtractor Data(DObj->getMacinfoSection(), isLittleEndian(), 0);
     ParseAndDump(Data, /*IsMacro=*/false);
     break;
   }
   case MacinfoDwoSection: {
     DWARFDataExtractor Data(DObj->getMacinfoDWOSection(), isLittleEndian(), 0);
     ParseAndDump(Data, /*IsMacro=*/false);

llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp

for (const Entry &E : Macros.Macros) {
         // DW_MACRO_undef      == DW_MACINFO_undef
         // DW_MACRO_start_file == DW_MACINFO_start_file
         // DW_MACRO_end_file   == DW_MACINFO_end_file
         // For readability/uniformity we are using DW_MACRO_*.
       case DW_MACRO_define:
       case DW_MACRO_undef:
       case DW_MACRO_define_strp:
       case DW_MACRO_undef_strp:
+      case DW_MACRO_define_strx:
+      case DW_MACRO_undef_strx:
         OS << " - lineno: " << E.Line;
         OS << " macro: " << E.MacroStr;
         break;
       case DW_MACRO_start_file:
         OS << " - lineno: " << E.Line;
         OS << " filenum: " << E.File;
         break;
       case DW_MACRO_import:
         OS << format(" - import offset: 0x%08" PRIx64, E.ImportOffset);
         break;
       case DW_MACRO_end_file:
         break;
       case DW_MACINFO_vendor_ext:
         OS << " - constant: " << E.ExtConstant;
         OS << " string: " << E.ExtStr;
         break;
       }
       OS << "\n";
     }
   }
 }
 
-Error DWARFDebugMacro::parse(DataExtractor StringExtractor,
-                             DWARFDataExtractor Data, bool IsMacro) {
+Error DWARFDebugMacro::parseImpl(
+    Optional<DWARFUnitVector::iterator_range> Units,
+    Optional<DataExtractor> StringExtractor, DWARFDataExtractor Data,
+    bool IsMacro) {
   uint64_t Offset = 0;
   MacroList *M = nullptr;
+  using MacroToUnitsMap = DenseMap<uint64_t, DWARFUnit *>;
+  MacroToUnitsMap MacroToUnits;
+  if (IsMacro && Data.isValidOffset(Offset)) {
+    // Keep a mapping from Macro contribution to CUs, this will
+    // be needed while retrieving macro from DW_MACRO_define_strx form.
+    for (const auto &U : Units.getValue())
+      if (auto CUDIE = U->getUnitDIE())
+        // Skip units which does not contibutes to macro section.
+        if (auto MacroOffset = toSectionOffset(CUDIE.find(DW_AT_macros)))
+          MacroToUnits.try_emplace(*MacroOffset, U.get());
+  }
   while (Data.isValidOffset(Offset)) {
     if (!M) {
       MacroLists.emplace_back();
       M = &MacroLists.back();
       M->Offset = Offset;
       if (IsMacro) {
         auto Err = M->Header.parseMacroHeader(Data, &Offset);
         if (Err)
           return Err;
       }
     }
     // A macro list entry consists of:
     M->Macros.emplace_back();
     Entry &E = M->Macros.back();
     // 1. Macinfo type
     E.Type = Data.getULEB128(&Offset);
 
     if (E.Type == 0) {
       // Reached end of a ".debug_macinfo/debug_macro" section contribution.
       M = nullptr;
       continue;
     }
while (Data.isValidOffset(Offset)) {
     case DW_MACRO_undef:
       // 2. Source line
       E.Line = Data.getULEB128(&Offset);
       // 3. Macro string
       E.MacroStr = Data.getCStr(&Offset);
       break;
     case DW_MACRO_define_strp:
     case DW_MACRO_undef_strp: {
       uint64_t StrOffset = 0;
       // 2. Source line
       E.Line = Data.getULEB128(&Offset);
       // 3. Macro string
       // FIXME: Add support for DWARF64
       StrOffset = Data.getRelocatedValue(/*OffsetSize=*/4, &Offset);
-      E.MacroStr = StringExtractor.getCStr(&StrOffset);
+      assert(StringExtractor && "String Extractor not found");
+      E.MacroStr = StringExtractor->getCStr(&StrOffset);
+      break;
+    }
+    case DW_MACRO_define_strx:
+    case DW_MACRO_undef_strx: {
+      E.Line = Data.getULEB128(&Offset);
+      auto MacroContributionOffset = MacroToUnits.find(M->Offset);
+      if (MacroContributionOffset == MacroToUnits.end())
+        return createStringError(errc::invalid_argument,
+                                 "Macro contribution of the unit not found");
+      auto ContributionUnit = MacroToUnits.find(M->Offset)->second;
+      Optional<uint64_t> StrOffset =
+          ContributionUnit->getStringOffsetSectionItem(
+              Data.getULEB128(&Offset));
+      if (!StrOffset.hasValue())
+        return createStringError(
+            errc::invalid_argument,
+            "String offsets contribution of the unit not found");
+      E.MacroStr =
+          ContributionUnit->getStringExtractor().getCStr(&(*StrOffset));
       break;
     }
     case DW_MACRO_start_file:
       // 2. Source line
       E.Line = Data.getULEB128(&Offset);
       // 3. Source file id
       E.File = Data.getULEB128(&Offset);
       break;
     case DW_MACRO_end_file:
       break;
     case DW_MACRO_import:
       // FIXME: Add support for DWARF64
       E.ImportOffset = Data.getRelocatedValue(/*OffsetSize=*/4, &Offset);
       break;
     case DW_MACINFO_vendor_ext:

llvm/test/DebugInfo/X86/debug-macro-empty-macro-offset.s

+## This test checks that llvm-dwarfdump produces error message
+## while parsing an *_strx form, if units contribution to 
+## debug_macro[.dwo] section is missing.
+
+# RUN: llvm-mc -triple x86_64-unknown-linux -filetype=obj %s -o -| \
+# RUN:   not llvm-dwarfdump -debug-macro - /dev/null 2>&1 | FileCheck %s
+
+# CHECK: error: Macro contribution of the unit not found
+       .section        .debug_abbrev,"",@progbits
+       .byte   1                       # Abbreviation Code
+       .byte   17                      # DW_TAG_compile_unit
+       .byte   0                       # DW_CHILDREN_no
+       .byte   0                       # EOM(1)
+       .byte   0                       # EOM(2)
+       .byte   0                       # EOM(3)
+
+       .section        .debug_info,"",@progbits
+.Lcu_begin0:
+       .long   .Ldebug_info_end0-.Ldebug_info_start0 # Length of Unit
+.Ldebug_info_start0:
+       .short  5                      # DWARF version number
+       .byte   1                       # DWARF Unit Type
+       .byte   8                       # Address Size (in bytes)
+       .long   .debug_abbrev           # Offset Into Abbrev. Section
+       .byte   1                       # Abbrev [1] 0xc:0x12 DW_TAG_compile_unit
+.Ldebug_info_end0:
+
+       .section        .debug_macro,"",@progbits
+.Lcu_macro_begin0:
+       .short  5                      # Macro information version
+       .byte   0                       # Flags: 32 bit
+       .byte   11                      # DW_MACRO_define_strx

llvm/test/DebugInfo/X86/debug-macro-empty-str-offset.s

+## This test checks that llvm-dwarfdump produces error message
+## while parsing an *_strx form, if units contribution to 
+## debug_str_offsets[.dwo] section is missing.
+
+# RUN: llvm-mc -triple x86_64-unknown-linux -filetype=obj %s -o -| \
+# RUN:   not llvm-dwarfdump -debug-macro - /dev/null 2>&1 | FileCheck %s
+
+# CHECK: error: String offsets contribution of the unit not found
+
+       .section        .debug_abbrev,"",@progbits
+       .byte   1                       # Abbreviation Code
+       .byte   17                      # DW_TAG_compile_unit
+       .byte   0                       # DW_CHILDREN_no
+       .byte   121                     # DW_AT_macros
+       .byte   23                      # DW_FORM_sec_offset
+       .byte   0                       # EOM(1)
+       .byte   0                       # EOM(2)
+       .byte   0                       # EOM(3)
+
+       .section        .debug_info,"",@progbits
+.Lcu_begin0:
+       .long   .Ldebug_info_end0-.Ldebug_info_start0 # Length of Unit
+.Ldebug_info_start0:
+       .short  5                      # DWARF version number
+       .byte   1                       # DWARF Unit Type
+       .byte   8                       # Address Size (in bytes)
+       .long   .debug_abbrev           # Offset Into Abbrev. Section
+       .byte   1                       # Abbrev [1] 0xc:0x12 DW_TAG_compile_unit
+       .long   .Lcu_macro_begin0       # DW_AT_macros
+.Ldebug_info_end0:
+
+       .section        .debug_macro,"",@progbits
+.Lcu_macro_begin0:
+       .short  5                      # Macro information version
+       .byte   0                       # Flags: 32 bit
+       .byte   11                      # DW_MACRO_define_strx

llvm/test/DebugInfo/X86/debug-macro-multi-cu-strx.s

+## This test checks that llvm-dwarfdump can dump debug_macro
+## section containing contributions from multiple CU's represented
+## using DW_MACRO_define_strx form.
+
+# RUN: llvm-mc -triple x86_64-unknown-linux -filetype=obj %s -o -| \
+# RUN:   llvm-dwarfdump -debug-macro - | FileCheck -strict-whitespace -match-full-lines %s
+
+#      CHECK:.debug_macro contents:
+# CHECK-NEXT:0x00000000:
+# CHECK-NEXT:macro header: version = 0x0005, flags = 0x02, debug_line_offset = 0x0000
+# CHECK-NEXT:DW_MACRO_start_file - lineno: 0 filenum: 0
+# CHECK-NEXT:  DW_MACRO_define_strx - lineno: 1 macro: DWARF_VERSION 5
+# CHECK-NEXT:  DW_MACRO_define_strx - lineno: 2 macro: COMPILE_UNIT 1
+# CHECK-NEXT:  DW_MACRO_undef_strx - lineno: 3 macro: COMPILE_UNIT
+# CHECK-NEXT:DW_MACRO_end_file
+
+#      CHECK:0x00000015:
+# CHECK-NEXT:macro header: version = 0x0005, flags = 0x02, debug_line_offset = 0x0000
+# CHECK-NEXT:DW_MACRO_start_file - lineno: 1 filenum: 3
+# CHECK-NEXT:  DW_MACRO_define_strx - lineno: 2 macro: COMPILE_UNIT 2
+# CHECK-NEXT:  DW_MACRO_undef_strx - lineno: 3 macro: COMPILE_UNIT
+# CHECK-NEXT:DW_MACRO_end_file
+
+       .section        .debug_abbrev,"",@progbits
+       .byte   1                       # Abbreviation Code
+       .byte   17                      # DW_TAG_compile_unit
+       .byte   0                       # DW_CHILDREN_no
+       .byte   114                     # DW_AT_str_offsets_base
+       .byte   23                      # DW_FORM_sec_offset
+       .byte   121                     # DW_AT_macros
+       .byte   23                      # DW_FORM_sec_offset
+       .byte   0                       # EOM(1)
+       .byte   0                       # EOM(2)
+       .byte   0                       # EOM(3)
+
+       .section        .debug_info,"",@progbits
+.Lcu_begin0:
+       .long   .Ldebug_info_end0-.Ldebug_info_start0 # Length of Unit
+.Ldebug_info_start0:
+       .short  5                      # DWARF version number
+       .byte   1                       # DWARF Unit Type
+       .byte   8                       # Address Size (in bytes)
+       .long   .debug_abbrev           # Offset Into Abbrev. Section
+       .byte   1                       # Abbrev [1] 0xc:0x12 DW_TAG_compile_unit
+       .long   .Lstr_offsets_base0     # DW_AT_str_offsets_base
+       .long   .Lcu_macro_begin0       # DW_AT_macros
+.Ldebug_info_end0:
+.Lcu_begin1:
+       .long   .Ldebug_info_end1-.Ldebug_info_start1 # Length of Unit
+.Ldebug_info_start1:
+       .short  5                      # DWARF version number
+       .byte   1                       # DWARF Unit Type
+       .byte   8                       # Address Size (in bytes)
+       .long   .debug_abbrev           # Offset Into Abbrev. Section
+       .byte   1                       # Abbrev [1] 0xc:0x12 DW_TAG_compile_unit
+       .long   .Lstr_offsets_base1     # DW_AT_str_offsets_base
+       .long   .Lcu_macro_begin1       # DW_AT_macros
+.Ldebug_info_end1:
+
+       .section        .debug_macro,"",@progbits
+.Lcu_macro_begin0:
+       .short  5                      # Macro information version
+       .byte   2                       # Flags: 32 bit, debug_line_offset present
+       .long   0                       # debug_line_offset
+       .byte   3                       # DW_MACRO_start_file
+       .byte   0                       # Line Number
+       .byte   0                       # File Number
+       .byte   11                      # DW_MACRO_define_strx
+       .byte   1                       # Line Number
+       .byte   0                       # Macro String
+       .byte   11                      # DW_MACRO_define_strx
+       .byte   2                       # Line Number
+       .byte   1                       # Macro String
+       .byte   12                      # DW_MACRO_undef_strx
+       .byte   3                       # Line Number
+       .byte   2                       # Macro String
+       .byte   4                       # DW_MACRO_end_file
+       .byte   0                       # End Of Macro List Mark
+.Lcu_macro_begin1:
+       .short  5                      # Macro information version
+       .byte   2                       # Flags: 32 bit, debug_line_offset present
+       .long   0                       # debug_line_offset
+       .byte   3                       # DW_MACRO_start_file
+       .byte   1                       # Line Number
+       .byte   3                       # File Number
+       .byte   11                      # DW_MACRO_define_strx
+       .byte   2                       # Line Number
+       .byte   0                       # Macro String
+       .byte   12                      # DW_MACRO_undef_strx
+       .byte   3                       # Line Number
+       .byte   1                       # Macro String
+       .byte   4                       # DW_MACRO_end_file
+
+       .section        .debug_str_offsets,"",@progbits
+       .long   12                      # Unit length
+       .short  5                       # Version
+       .short  0                       # Padding
+.Lstr_offsets_base0:
+       .long   .Linfo_string0
+       .long   .Linfo_string1
+       .long   .Linfo_string2
+       .long   12                      # Unit length
+       .short  5                       # Version
+       .short  0                       # Padding
+.Lstr_offsets_base1:
+       .long   .Linfo_string3
+       .long   .Linfo_string4
+
+       .section        .debug_str,"MS",@progbits,1
+.Linfo_string0:
+       .asciz  "DWARF_VERSION 5"
+.Linfo_string1:
+       .asciz  "COMPILE_UNIT 1"
+.Linfo_string2:
+       .asciz  "COMPILE_UNIT"
+.Linfo_string3:
+       .asciz  "COMPILE_UNIT 2"
+.Linfo_string4:
+       .asciz  "COMPILE_UNIT"