[DWARF5] Added support for debug_macro section parsing and dumping in llvm-dwarfdump.

[SouraVX]

Summary

Summary:
This patch adds parsing and dumping DWARFv5 macro section in llvm-dwarfdump, it does not introduce any new switch. Existing "--debug-macro" can be used to dump macinfo/macro section.

[jhenderson]

llvm/test/DebugInfo/X86/debug-macro-v5.s

1 ↗

(On Diff #249372)

Same comments apply in this test as in the previous one.

Also, use single or double dashes consistently for the entire llvm-mc call.

[SouraVX]

Something you may wish to consider is writing the tests as unit tests rather than using dwarfdump to test them, since the code is in a library. It may require a little extra legwork to set up initially (writing dwarf generator code, like there already is for debug info and debug line), but I think there is a decent payoff in return. It'll allow you to avoid the noise that some of these tests have.

Nice suggestion thanks!, but I think that should be taken as a separate exercise. what's your take on this?

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

455

Thanks! for the suggestion here. I've partially tried to address concern related to this also in this inline comment.

These three functions all look very similar..

850

Since these are mostly 5 liner functions handling their cases cleanly as needed, introducing one more function just to reduce 2-3 LOC doesn't sound good idea(personal opinion).
Plus problem in this entire patch(due to code shared by macro/macinfo) is that, you flip one thing and you'll endup modifying everything (handling macro/macinfo, parsing/dumping) just to make sure that existing functionality is not broken.

llvm/test/DebugInfo/X86/debug-macro-macinfo.s

8 ↗

(On Diff #249372)

emission of DW_FORM_define_strp form is getting checked here. Is this what you're asking ?

30–39 ↗

(On Diff #249372)

Yes, If Flags in Macro header indicate debug_line_offset is present then it is needed. That's why here.

45 ↗

(On Diff #249372)

Yes, macinfo section represent this way, If accidentally removed would result in assmbler error.

55–63 ↗

(On Diff #249372)

Sorry this one is not needed, removed thanks!

[SouraVX]

Addressed review comments by @jhenderson. Thanks a lot for this!

[SouraVX]

llvm/test/DebugInfo/X86/debug-macro-v5.s

11 ↗

(On Diff #249591)

I tried multiple times, but seems like this line is causing the test case to fail when using --strict-whitespace with FileCheck Even though indentation is correct(2 spaces) checked through normal dumping on stdout and by redirecting to a file.
So I temporarily removed --strict-whitespace option. Still digging will update here once done.

[SouraVX]

llvm/test/DebugInfo/X86/debug-macro-macinfo.s

49 ↗

(On Diff #249372)

In editor it was looking normal *No extra space/character*. I'll check once more.

[SouraVX]

Added descriptive comments in test cases.

[SouraVX]

Addressed @jhenderson comments.
Thanks for this!

[SouraVX]

llvm/test/DebugInfo/X86/debug-macro-v5.s

11 ↗

(On Diff #249591)

Added -strict-whitespace option. Thanks!

[SouraVX]

@ikudrin and @jhenderson I've tried to address most of your comments/concerns. Are you guys Okay with these changes?

[jhenderson]

In D73086#1916681, @SouraVX wrote:

Something you may wish to consider is writing the tests as unit tests rather than using dwarfdump to test them, since the code is in a library. It may require a little extra legwork to set up initially (writing dwarf generator code, like there already is for debug info and debug line), but I think there is a decent payoff in return. It'll allow you to avoid the noise that some of these tests have.

Nice suggestion thanks!, but I think that should be taken as a separate exercise. what's your take on this?

As it could simplify the tests for this, it might be better to do that upfront. You could then keep a basic test to show that llvm-dwarfdump can dump both kinds of sections, but the detail could be tested in the unit tests. I don't feel strongly about it however.

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

30

Still missing a blank line between Version and the comment for flags.

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

850

The flip side argument is that you'd have to modify 3 functions any time you wanted to change the functionality somehow. It's not really about the number of lines of code. I'm not sure sharing the code is that hard. See semi-pseudo code below.

template <typename DWARFDebugT, typename SectionT>
static Expected<const DWARFDebugT *> getDWARFDebugMacro(
           std::unique_ptr<DWARFDebugT> &MacroPtr,
           SectionT *MacroSec,
           bool LittleEndian,
           bool IsMacro,
           DataExtractor StringExtractor) {
  if (MacroPtr)
    return MacroPtr.get();
  DWARFDataExtractor Data(MacroSec, LittleEndian, 0);
  MacroPtr.reset(new DWARFDebugT());
  if (Error Err = MacroPtr->parse(StringExtractor, Data, IsMacro))
    return std::move(Err);
  return MacroPtr.get();
}

Expected<const DWARFDebugMacro *> DWARFContext::getDebugMacinfoDWO() {
  return getDWARFDebugMacro(MacinfoDWO,
                            DObj->getMacinfoDWOSection(),
                            isLittleEndian(),
                            /*IsMacro=*/false,
                            getStringExtractor());
}

...

llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp

52–54

I wasn't interested in the quotes. I wanted you to change the comment to match what I posted, so that it is good English!

109–117

Nit: "FIXME: Add..." (with space after the ':')

llvm/test/DebugInfo/X86/debug-macro-macinfo.s

1–2 ↗

(On Diff #250036)

In new tests, I try to use '##' to distinguish comments from lit and FileCheck directives.

can dump debug_macro -> can dump both debug_macro
section -> sections
in a single object -> in the same object

4 ↗

(On Diff #250036)

|\ -> | \ for readability

5 ↗

(On Diff #250036)

I'd add two extra spaces before llvm-dwarfdump to clearly show that it is a continuation of the previous line.

4 ↗

(On Diff #249372)

This hasn't been done.

8 ↗

(On Diff #249372)

I was just confused by the DW_MACRO_define_strp causing an extra two spaces of indentation, that's all. It looks slightly broken when I read it with little prior knowledge of debug_macro sections.

30–39 ↗

(On Diff #249372)

Okay. I'd guess that you could delete most of the strings, right?

49 ↗

(On Diff #249372)

Is it using a tab instead of a space in the text. To me, I see:

.byte  1
.ascii  "DWARF_VERSION"

with the first '"' in the second line not lining up with the 1. I guess you've not written this by hand though, so if this is what the assembler produces, it's fine.

55–63 ↗

(On Diff #249372)

I was also actually referring to the extra strings. It looks like you've got several duplicates that could be deleted?

llvm/test/DebugInfo/X86/debug-macro-v5.s

7 ↗

(On Diff #250036)

# CHECK:.debug_macro contents:

same below for the other CHECK: line.

llvm/test/DebugInfo/X86/unsupported-dwarf64-debug-macro-v5.s

1 ↗

(On Diff #250036)

Same comments in this test as elsewhere re '##', and line continuations.

2 ↗

(On Diff #250036)

unsupported case where the DWARF64 flag is present in the debug_macro section header.

6 ↗

(On Diff #250036)

No real need for strict-whitespace and match-full-lines in this test case. The formatting isn't really important.

llvm/test/DebugInfo/X86/unsupported-opcode_operands_table-debug-macro-v5.s

1 ↗

(On Diff #250036)

Same comments as in other test cases.

[ikudrin]

We are almost there.

I added comments only for one test file. Please, consider them thoroughly and apply similar adjustments to the other tests.

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

95

I suppose it would be useful to add a comment that the value of 0 in Header.Version means that the parsed data is from a .debug_macinfo[.dwo] section and other fields in the Header are uninitialized in that case.

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

846

If you call this method twice, and the first call returns an error, the second call will still return some partially parsed DWARFDebugMacro object. This is inconsistent. You should call Macro.reset() before returning an error.

The same for two other similar methods.

llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp

98

The variable is used only in DW_MACRO_define_strp/DW_MACRO_undef_strp cases. It does not need to preserve the value outside the handler. Thus, why not embrace the body of those cases in braces and move the variable there making it even more local?

llvm/test/DebugInfo/X86/debug-macro-macinfo.s

4 ↗

(On Diff #250036)

I guess you do not need -dwarf-version=5. It affects the format of the generated .debug_line section, but the parser does not read it and the test does not check anything related to it.

24 ↗

(On Diff #250036)

You do not need to reference the real line table because it is not used in the parser/dumper. Just use 0 here. And fix the comment.

33 ↗

(On Diff #250036)

There should be an empty line before .section to improve readability.

42 ↗

(On Diff #250036)

In fact, this section may be removed because it is not used.

46 ↗

(On Diff #250036)

Please add comments for this section describing the values, the same way as that is done for .debug_macro.

48 ↗

(On Diff #250036)

In fact, this definition also does not affect the test and can be removed.

52–55 ↗

(On Diff #250036)

You may just use .asciz "DWARF_VERSION 4" instead. It is the same but more readable.

58–64 ↗

(On Diff #250036)

This second .debug_str and all three strings inside it are not needed. Please, remove them.

30–39 ↗

(On Diff #249372)

Only .Linfo_string3 is used. There are no references to other strings in the file, so they should be removed.

BTW, the comments for the strings are misleading, because all of them except .Linfo_string0 have different offsets than stated.

[SouraVX]

llvm/test/DebugInfo/X86/debug-macro-macinfo.s

8 ↗

(On Diff #249372)

The spacing/indentation is part of dumping here, reflecting that this macro is defined under DW_MACRO_start_file file.

llvm/test/DebugInfo/X86/debug-macro-v5.s

7 ↗

(On Diff #250036)

Is spacing the you're concerned about here ?

[jhenderson]

llvm/test/DebugInfo/X86/debug-macro-v5.s

7 ↗

(On Diff #250036)

Right. It allows easier reading of the line, because it lines up the actual content being checked with the rest of the content from the subsequent CHECK-NEXT lines.

[SouraVX]

llvm/test/DebugInfo/X86/debug-macro-macinfo.s

52–55 ↗

(On Diff #250036)

Removing this since macinfo represent info in section itself not in debug_str so this is also un-necessary! Sorry for the confusion caused here.
+ Removing other- duplicate/un-necessary string after scrutinizing.

[SouraVX]

@jhenderson Thanks for sharing pseudo code!
I forgot to mention this thing in our last discussion of refactoring the code shared by macro/macinfo. macro and macinfo are represented differently(due to their contents) to be specific DWARFSection and StringRef explicitly as a result we can't share extract DWARFDataExtractor the using same way. --
DWARFDataExtractor Data(Macinfo, LittleEndian, 0); -- this only works for macinfo
for macro you also need DWARFObject
DWARFDataExtractor MacroData(*DObj, DObj->getMacroSection(), isLittleEndian(), 0); -- only works for macro section.
+ SectionT &MacroSection argument will be const llvm::DWARFSection& -- for macro case
and StringRef --for macinfo case.
code bloating a lot -- psuedo code not compilable, -- sort of need one more extra argument to fetch information from macinfo section (StringRef)

template<typename DWARFDebugT, typename SectionT>
static Expected<const DWARFDebugT*> getDWARFDebugMacro(
                const DWARFObject &DObj,
                std::unique_ptr<DWARFDebugT> &MacroPtr,
                SectionT &MacroSection,
                bool LittleEndian,
                bool IsMacro,
                DataExtractor StringExtractor) {
        if (MacroPtr)
           return MacroPtr.get();
        if (IsMacro) {
        DWARFDataExtractor MacroData(DObj, MacroSection, LittleEndian, 0);
            if (Error Err = MacroPtr->parse(StringExtractor, MacroData, IsMacro)) {
                    Macro.reset();
              return std::move(Err);
            }
        else {
        DWARFDataExtractor MacinfoData(MacroSection, LittleEndian, 0);
        MacroPtr.reset(new DWARFDebugT());
            if (Error Err = MacroPtr->parse(StringExtractor, MacinfoData, IsMacro)) {
                    Macro.reset();
              return std::move(Err);
            }
        return MacroPtr.get();

        }
        }
}

Do you still vote for this ? Please share!

[jhenderson]

This might be a legacy thing, but it looks like DWARFSection is just a wrapper around a StringRef. Could they all use StringRef (or DWARFSection), instead of having some use one and some use the other? That might require an additional change or two, but it seems pointless having the two types to me.

[SouraVX]

In D73086#1921504, @jhenderson wrote:

This might be a legacy thing, but it looks like DWARFSection is just a wrapper around a StringRef. Could they all use StringRef (or DWARFSection), instead of having some use one and some use the other? That might require an additional change or two, but it seems pointless having the two types to me.

The rationale behind putting some section as StringRef and some as DWARFSection is based on relocations macinfo doesn't have relocation macro does have relocations hence (I represented it that way).
DWARFDataExtractor also seems to have different handling(2 constructors) for both cases(relocation and normal cases).
Besides your suggestion to use one common thing(taking StringRef for this case) for both(macro/macinfo) will introduce inconsistency(might also introduce some bugs). Since their are already some sections str line_str and more represented as StringRef while others as DWARFSection.

[SouraVX]

llvm/test/DebugInfo/X86/debug-macro-v5.s

7 ↗

(On Diff #250036)

To avoid same mistakes again, and avoiding confusion -- Following modification you're suggesting -

# CHECK:.debug_macro contents: -- CHECK pattern(1 leading space) in all test cases
should modified be modified with "2" leading spaces in all test cases i.e
# CHECK:debug_macro
and like wise in subsequent CHECK-NEXT ?

[SouraVX]

Addressed @ikudrin and @jhenderson comments!
Removed [WIP] from Title of the revision!

[SouraVX]

llvm/test/DebugInfo/X86/debug-macro-macinfo.s

4 ↗

(On Diff #249372)

Done!

[jhenderson]

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

96

parsing a macinfo...
it self -> itself

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

846

This sort of change is exactly why I think we should avoid duplicating the logic in three places. It would have been easy to add it only in one place.

llvm/test/DebugInfo/X86/debug-macro-macinfo.s

2 ↗

(On Diff #250371)

Still need to do "the same object" instead of "the single object".

2 ↗

(On Diff #249372)

I don't see a verbose case still?

llvm/test/DebugInfo/X86/debug-macro-v5.s

7 ↗

(On Diff #250036)

Thanks, this bit looks fine now.

llvm/test/DebugInfo/X86/unsupported-opcode_operands_table-debug-macro-v5.s

2 ↗

(On Diff #250371)

where the opcode_operands_table

[SouraVX]

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

96

Is it okay to keep it the way it self or something like "macinfo or macinfo.dwo section".
Since this also covers macinfo.dwo case also. ?

llvm/test/DebugInfo/X86/debug-macro-macinfo.s

2 ↗

(On Diff #249372)

I think we don't need one "verbose" case, since there is no difference b/w both cases.

[SouraVX]

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

846

I'm investigating the opportunity you discussed/mentioned in last comment(Thanks for that!) If found with constructive cases, will push a patch or two for review.

[SouraVX]

Addressed nits comments by @jhenderson
Thanks for this!

[ikudrin]

llvm/test/DebugInfo/X86/debug-macro-macinfo.s

38 ↗

(On Diff #250515)

Remove this .file line. I guess you may expect it to contribute the "File Numer" to this section, but that is not true. It has no influence on the content of this section.

In fact, the byte you described as "DW_MACINFO_define" is the actual file number. The next byte is that directive and the following byte is the line number for the entry. You should know that DW_MACINFO_define does not have an operand for file number.

I guess it would be better to use some different values for the file number and for the line number here to avoid that misinterpretation.

42 ↗

(On Diff #250515)

You may just use .asciz "DWARF_VERSION 4". It is the same. The fact that the macro string consists of the name, space, and value is not important for this test. And you have not divided the definition for "DWARF_VERSION 5" despite it follows the same rules.

llvm/test/DebugInfo/X86/debug-macro-v5.s

31 ↗

(On Diff #250515)

Not needed for the test. Remove.

llvm/test/DebugInfo/X86/unsupported-dwarf64-debug-macro-v5.s

13 ↗

(On Diff #250515)

For the consistency, this should be .quad, not .long.

[ikudrin]

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

846

This sort of change is exactly why I think we should avoid duplicating the logic in three places. It would have been easy to add it only in one place.

Totally agree.

[SouraVX]

Addressed @ikudrin nits comments. Thanks for this!

[SouraVX]

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

846

I already had discussion with @jhenderson WRT to this in previous revision, highlighted some of the implications https://reviews.llvm.org/D73086#1921493
+ I'm separately investigating that.

[SouraVX]

llvm/test/DebugInfo/X86/debug-macro-macinfo.s

42 ↗

(On Diff #250515)

And you have not divided the definition for "DWARF_VERSION 5" despite it follows the same rules.

Are you concerned regarding representation ??
DW_MACRO_define_strp forms requires the entire macro and definition(value) "DWARF_VERSION 5" to be kept as a string in debug_str section.
That's what I've done here. Am I missing something here ?

[jhenderson]

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

96

Sorry, macinfo[.dwo] was fine (I wanted to correct the like of 'a'). Feel free to reinstate that bit.

llvm/test/DebugInfo/X86/debug-macro-macinfo.s

2 ↗

(On Diff #249372)

Does verbose mode dump at a different time for .debug_macro etc, like the .debug_line code does? If it does, we should probably still have it (but obviously it can share the same checks).

[SouraVX]

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

96

Sure will do!

[SouraVX]

llvm/test/DebugInfo/X86/debug-macro-macinfo.s

2 ↗

(On Diff #249372)

No, Verbose mode isn't part of macro/macinfo since the beginning. I think it's due to nature of it's contents.

[ikudrin]

I am mostly OK with the changes at the moment. Please reach the consent with @jhenderson about code duplication and other topics he mentioned.

llvm/test/DebugInfo/X86/debug-macro-macinfo.s

47 ↗

(On Diff #250567)

Remove the comment "string offset=149". It is both useless and misleading. The real offset is 0.

42 ↗

(On Diff #250515)

Both strings, "DWARF_VERSION 4" and "DWARF_VERSION 5", are OK now.

[SouraVX]

In D73086#1926229, @ikudrin wrote:

I am mostly OK with the changes at the moment. Please reach the consent with @jhenderson about code duplication and other topics he mentioned.

Thanks for the consent @ikudrin!
@jhenderson, Are you okay with these changes?

[jhenderson]

In D73086#1926329, @SouraVX wrote:

In D73086#1926229, @ikudrin wrote:

I am mostly OK with the changes at the moment. Please reach the consent with @jhenderson about code duplication and other topics he mentioned.

Thanks for the consent @ikudrin!
@jhenderson, Are you okay with these changes?

You said you'd take a look at removing the duplication. Have you done that yet? That's what I'm waiting on.

[SouraVX]

You said you'd take a look at removing the duplication. Have you done that yet? That's what I'm waiting on.

Oh, Sorry! I think I mentioned that should be taken separately. https://reviews.llvm.org/D73086#1921509

[jhenderson]

In D73086#1926382, @SouraVX wrote:

You said you'd take a look at removing the duplication. Have you done that yet? That's what I'm waiting on.

Oh, Sorry! I think I mentioned that should be taken separately. https://reviews.llvm.org/D73086#1921509

If you want to do it separately, please do it before this patch. This patch makes the situation worse, which is obviously bad.

[SouraVX]

Refactored to remove code duplication.

Hi @jhenderson, @ikudrin, @dblaikie , I've refactored the dumping code to use a single getDWARFDebugMacro function for all dumping related to macro/macinfo. Thus removing all three redundant dumping functions getDebugMacro/Macinfo/MacinfoDWO . Resultant code looks cleaner/uniform.
@jhenderson I've briefly tried to work on https://reviews.llvm.org/D73086#1921509, hitting up multiple issues, Since this is not just macro/macinfo, a whole lot of sections are represented as DWARFSection and StringRef. This invites a huge rework.

[SouraVX]

Refactored static helper function(dumping macro/macinfo/macinfo.dwo) as a member function, making code more concise.

[dblaikie]

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

288–290

Returning a pointer to the parameter seems unnecessarily confusing - just return Error instead of Expected - callers can use the MacroPtr they passed in instead of retrieving the same thing from the return value.

294

Prefer make_unique:

MacroPtr = std::make_unique<DWARFDebugMacro>();

295–301

This probably shouldn't be dynamically allocated either - if DWARFDataExtractor is move constructible, which I guess it should be, then:

DWARFDataExtractor Data = IsMacro ? DWARFDataExtractor(... ) : DWARFDataExtractor(...);

If DWARFDataExtractor can't be move constructible then Optional<DWARFDataExtractor> could be used:

Optional<DWARFDataExtractor> Data;
if (IsMacro)
  Data.emplace(...);
else
  Data.emplace(...);

303–304

This doesn't look like it needs to be dynamically allocated - could it just be:

DWARFDataExtractor DataDWO(Context.getDWARFObj().getMacinfoDWOSection(), IsLittleEndian, 0);

307

Rather than nulling out this pointer - use a local unique_ptr and assign to MacroPtr at the end if no errors have occurred. (safer than trying to remember/make sure all the failure paths undo the work)

[SouraVX]

Addressed refactoring comments by @dblaikie. Thanks for this!
-Returned type changed to Error.
-Since DWARFDataExtractor is move constructible, removed dynamic construction of objects.
-Instead of resetting ptr, used local unique_ptr this will get moved to MacroPtr in case of successful parsing.

[dblaikie]

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

461

This (& similar elsewhere) should be written as "Macinfo->dump(OS)", the ".get()" is redundant.

@jhenderson - did you want to roll the error handling/dumping into the common code too? (looks like the "if/handle/else/dump" could go in the common function (& rename the function to parseAndDump or something) too?

[SouraVX]

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

461

Should we go ahead with this ?

[jhenderson]

Sorry, I've been quite busy with having about 15-20 different active reviews at once, some of which are not trivial in complexity. It may take me a couple of working days to respond to things.

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

290–291

It might make the code nicer if IsDWO and IsMacro are folded into one small enum e.g. MacroSecType. You can then switch on that value, and it also avoids multiple boolean arguments, which are easy to get wrong.

307

It looks to me like @dblaikie wanted the MacroPtr = std::make_unique<DWARFDebugMacro>(); line to be replaced with auto Ptr = std::make_unique<DWARFDebugMacro>(); and only assign to MacroPtr at the end.

If there is an error, then MacroPtr will remain unchanged (i.e. unitialised). This is very similar to having a strong exception safety guarantee in concept, i.e. the state hasn't changed if there's an error.

461

Since this is all within the same library, we can put the recoverable error handler calls inside the function, and simplify the interface further.

@dblaikie's first comment about get though should also be addressed.

[SouraVX]

Addressed comments/suggestion by @jhenderson and @dblaikie. Thanks a lot for this!
-Removed both boolean types and introduced new helper enum MacroSecType. Now content parsing/dumping is based on this enum.
-Simplified interface further by moving Error handling inside function, renamed this function as parseAndDumpMacroMacinfo.

[jhenderson]

llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h

276 ↗

(On Diff #251744)

Dump -> dump a

and macinfo -> or macinfo

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

320–322

Comment indentation is slightly off. Might be worth reporting a warning in the case that this is hit?

458

I'm not particularly keen that dumping and parsing happen in the same function. It feels like those are two separate tasks that should be kept independent. Perhaps a small lambda near here instead would make sense to factor out the sequence "parse -> check not null -> dump"?

[SouraVX]

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

458

Are you suggesting something sort of -
parseMacroOrMacinfo handles parsing and error handling, this can be a member function instead of a lamba for uniformities sake.

if (shouldDump(Explicit, ".debug_macro", DIDT_ID_DebugMacro,
                  DObj->getMacroSection().Data)) {
     parseMacroOrMacinfo(Macro, OS, DumpOpts, /*IsLittleEndian=*/true,
                              dwarf::Macro);
     if (Macro)
     Macro->dump(OS);
   }

I'm okay with this too :)

[jhenderson]

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

458

Most of the other sections have a pattern of "get/parse" followed by "dump", often in one line, without the error check. I think we should follow that if possible (but with the error check, or possibly by initialising the pointer to some "default" empty section in the event of a failure). In other words, yes, esssentially what you suggested. In particular, this will allow the parsing code to be used and tested independently of the printing.

[SouraVX]

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

320–322

Before revising this patch, I want to have your opinion on this -
I'm considering removing this case MacroDWO instead of emitting a warning(I'm not against it). But since that part of code case(MacroDWO) will never be hit, since we don't do it explicitly in first place here --

if (shouldDump(Explicit, ".debug_macro.dwo", DIDT_ID_DebugMacro,
                 DObj->getMacroDWOSection()))
parseMacroOrMacinfo(MacroDWO, OS, DumpOpts, /*IsLittleEndian=*/true,
                              dwarf::MacroDWO);

And it's not needed atleast for this patch.
Should I go ahead remove this case and just leave a FIXME?

[jhenderson]

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

320–322

If the behaviour from prior to your patch is unchanged, I think that's fine to do from my point of view. I wouldn't want to see things regress though.

[SouraVX]

Addressed @jhenderson comments, Thanks a lot for this!

• Refactored the code based on comments.
• Empty case MacroDWO: is still there with FIXME, just for appeasing a build warning warning: enumeration value ‘MacroDWO’ not handled in switch [-Wswitch]
• No regression found after check-all.

FIXME: might still look a bit misplaced(couple of spaces), but clang-format is doing that.

[jhenderson]

LGTM, with one minor suggestion. It might make sense to get one of the other reviewers formal approval before landing this.

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

320–322

I think the problem is that you've put the FIXME above the case statement, whereas it should be below it. If you move it to before the break, it'll look right, and it's probably the right place for it:

case dwarf::MacroDWO:
  // FIXME: Add support for debug_macro.dwo section.
  break;

[jhenderson]

In D73086#1940876, @jhenderson wrote:

It might make sense to get one of the other reviewers formal approval before landing this.

Given @ikudrin requested changes earlier, he'll need to approve it for this patch to move to "Approved" state officially, not that it matters that much.

[SouraVX]

In D73086#1940879, @jhenderson wrote:

In D73086#1940876, @jhenderson wrote:

It might make sense to get one of the other reviewers formal approval before landing this.

Given @ikudrin requested changes earlier, he'll need to approve it for this patch to move to "Approved" state officially, not that it matters that much.

Thanks a lot @jhenderson, for investing time in this!
I'll land this after other reviewers @ikudrin and @dblaikie acceptance.

[SouraVX]

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

320–322

Sure Thanks!
I'll take care of that!

[ikudrin]

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

455

I am not sure that Macro as an in-out parameter of a void method is the perfect design. It would be better if parseMacroOrMacinfo() would just return std::unique_ptr<DWARFDebugMacro>. The code here, in that case, will look like:

if (!Macro)
  Macro = parseMacroOrMacinfo(...);
if (Macro)
  Macro->dump(OS);

Much more clear, no?

455

Before the patch, isLittleEndian() was used, but now you always pass the constant true. Could you please elaborate, why the behavior has been changed?

[SouraVX]

Addressed comments by @ikudrin Thanks for this!
-made return type std_unique_ptr<DWARFDebugMacro>.
-littleEndian replaced and some cleanup.

[SouraVX]

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

455

Removed this

if (!Macro)  --this seemed redundant to me since during first parsing of section, this will always be NULL.
  Macro = parseMacroOrMacinfo(...);

[SouraVX]

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

455

Sorry, some how in effort to keeping the code concise this crept in. I've removed this since parseMacroMacinfo is a member function of DWARFContext.

[ikudrin]

llvm/include/llvm/BinaryFormat/Dwarf.h

75 ↗

(On Diff #252858)

I guess that it is better to make this enum private inside DWARFContext.

You may also want to remove MACRODWO because it is used only for an empty ("TODO") case in parseMacroOrMacinfo(). Just remove that empty case and move the "TODO" comment to this enum.

llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h

276 ↗

(On Diff #252858)

Maybe I am missing something, but I cannot find the discussion where the removal of these public methods was approved.

getDebugMacinfo() and getDebugMacinfoDWO() should not be removed from the public interface. getDebugMacro() should be added. parseMacroOrMacinfo() should be private and be called from these methods.

[SouraVX]

llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h

276 ↗

(On Diff #252858)

It was part of effort of removing the redundant code that @jhenderson and you were discussing about https://reviews.llvm.org/D73086#1927338

[SouraVX]

llvm/include/llvm/BinaryFormat/Dwarf.h

75 ↗

(On Diff #252858)

Thanks, I'll take care of that in next revision.

[SouraVX]

llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h

276 ↗

(On Diff #252858)

@ikudrin While naming parseMacroMacinfo or getMacroMacinfo can still be debatable, but semantically this parseMacroMacinfo function is doing essentially the same thing which these 2 + 1(getDebugMacro) were doing i.e parsing the contents and returning to the caller for dumping the contents.

[ikudrin]

llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h

276 ↗

(On Diff #252858)

Not exactly. Look from the perspective of a user of the DWARFContext class. Before the patch, they could just call Context->getDebugMacinfo(), but now they need to pass way more arguments, some of which are not that obvious (namely, DumpOpts) and others are references to a private member of DWARFContext (namely, MacroPtr). This is a significant degradation of the public interface of DWARFContext.

[SouraVX]

llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h

276 ↗

(On Diff #252858)

Thanks for pointing this out! I've drafted a rough patch that does not degrade the existing public interface of DWARFContext and doesn't bloat the code too much.
Here's snippet from that, there will be 3 functions(getDebugMacro/Macinfo/MacinfoDWO) calling parseMacroOrMacinfo for doing the math associated with parsing and error handling.

std::unique_ptr<DWARFDebugMacro> DWARFContext::getDebugMacro() {
   return parseMacroOrMacinfo(Macro, getRecoverableErrorHandler(),
              MACRO);
}

In subsequent dumping code we'll just check for nullptr and dump accordingly.

Macro = getDebugMacro();
   if (Macro)
     Macro->dump(OS);

Could you please share your opinion on this!

[ikudrin]

llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h

276 ↗

(On Diff #252858)

Almost. The getters should cache the result of parseMacroOrMacinfo() and return raw pointers because unique_ptr transfers ownership.

[SouraVX]

Addressed comments by @ikudrin Thanks for this!
-Replaced the return type to raw pointer. Now no un-necessary transfer of ownership involved here.
-Also changed parseMacroOrMacifo to return const DWARFDebugMacro*, in case of errors this will return nullptr. And subsequently checked while dumping.

[jhenderson]

llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h

111–113 ↗

(On Diff #253399)

Why did you change the naming style of this? LLVM coding standards say that enums should use the same naming scheme as types: https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly.

Also, why the = 1?

114–115 ↗

(On Diff #253399)

Use C++ style comments, and put this all on one line. Also add missing trailing full stop.

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

293

ParseAndDump. This is a (callable) object, not a function.

[SouraVX]

llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h

111–113 ↗

(On Diff #253399)

Ah Sorry, I renamed them this way, to avoid confusion of naming while calling
parseMacroOrMacinfo(Macro, getRecoverableErrorHandler(), MACRO);

I'll change this as per Coding Standard.

114–115 ↗

(On Diff #253399)

Ah, seems like clang-formatting is getting in way(maybe due to /**/ style comment), I'll correct it such that clang-format does't put a new line.

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

293

Sorry, didn't get what you meant here ?

[SouraVX]

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

293

Is naming not conforming to standard are you referring here parseAndDump ?

[jhenderson]

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

293

Right. Lambdas are objects, not functions, and so should follow the variable naming scheme, not the function naming scheme.

[SouraVX]

llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h

111–113 ↗

(On Diff #253399)

Just want to drop a forward note here since, naming enum also as Macro Macinfoand so forth was colliding with previous definition std::unique_ptr<DWARFDebugMacro> Macro;
error: Macro conflicts with a previous declaration Macro
So I've extended enum names as
MacroSection MacinfoSection MacinfoDwoSection.

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

293

Thanks! done locally, we'll wait for @ikudrin before revising this.

[ikudrin]

llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h

288 ↗

(On Diff #253399)

• Make parseMacroOrMacinfo() private.
• parseMacroOrMacinfo() should return a newly created std::unique_ptr<DWARFDebugMacro>.
• Remove the MacroPtr argument. Caching the result is the responsibility of getters.
• Remove the RecoverableErrorHandler argument. All getters pass the same value, and this method, being a non-static member of the same class, also has access to it.

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

455–456

This might be simplified to

if (auto Macinfo = getDebugMacinfo())
  Macinfo->dump(OS);

838–842

The getters should look like

const DWARFDebugMacro *DWARFContext::getDebugMacinfo() {
  if (!Macinfo)
    Macinfo = parseMacroOrMacinfo(MACINFO);
  return Macinfo.get();
}

[SouraVX]

Addressed comments by @jhenderson and @ikudrin Thanks for this!

[ikudrin]

LGTM. Please wait for the comments from @jhenderson and @dblaikie.

[SouraVX]

In D73086#1949559, @ikudrin wrote:

LGTM. Please wait for the comments from @jhenderson and @dblaikie.

Thanks a lot for investing time in this, I'll wait for @jhenderson and @dblaikie. In meantime can you also share some final thoughts on D72828 Mostly it's already reviewed.

[jhenderson]

Latest version looks good to me too.

[SouraVX]

@dblaikie are you Okay with these changes ?

[dblaikie]

I think it's OK - best I can tell at this point, given all the review iterations/comments/etc. *fingers crossed*

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/Support/DataExtractor.h"
+#include "llvm/DebugInfo/DWARF/DWARFDataExtractor.h"
 #include <cstdint>
 
 namespace llvm {
 
 class raw_ostream;
+class DWARFContext;
 
 class DWARFDebugMacro {
+  /// DWARFv5 section 6.3.1 Macro Information Header
+  enum HeaderFlagMask {
+#define HANDLE_MACRO_FLAGS(ID, NAME) MACRO_FLAG_##NAME = ID,
+#include "llvm/BinaryFormat/Dwarf.def"
+  };
+  struct Header {
+    /// Macro information number.
+    uint16_t Version;
+    /// flags
+    // The bits of the flags field are interpreted as a set of flags, some of
+    // which may indicate that additional fields follow. The following flags,
+    // beginning with the least significant bit, are defined: offset_size_flag:
+    //   If the offset_size_flag is zero, the header is for a 32-bit DWARF
+    //   format macro section and all offsets are 4 bytes long; if it is one,
+    //   the header is for a 64-bit DWARF format macro section and all offsets
+    //   are 8 bytes long.
+    // debug_line_offset_flag:
+    //   If the debug_line_offset_flag is one, the debug_line_offset field (see
+    //   below) is present. If zero, that field is omitted.
+    // opcode_operands_table_flag:
+    //   If the opcode_operands_table_flag is one, the opcode_operands_table
+    //   field (see below) is present. If zero, that field is omitted.
+
+    // FIXME: add support for 64-bit DWARF;
+    uint8_t Flags;
+    /// debug_line_offset
+    //   An offset in the .debug_line section of the beginning of the line
+    //   number information in the containing compilation, encoded as a 4-byte
+    //   offset for a 32-bit DWARF format macro section and an 8-byte offset for
+    //   a 64-bit DWARF format macro section
+    uint32_t DebugLineOffset;
+  };
+  uint64_t HeaderOffset;
+  Header HeaderData;
+
+  uint64_t getHeaderOffset() const { return HeaderOffset; }
+  uint16_t getVersion() const { return HeaderData.Version; }
+  uint8_t getFlags() const { return HeaderData.Flags; }
+  uint32_t getDebugLineOffset() const { return HeaderData.DebugLineOffset; }
+
+  void setHeaderOffset(uint64_t *Offset) { HeaderOffset = *Offset; }
+  void setVersion(uint16_t Version) { HeaderData.Version = Version; }
+  void setFlags(uint8_t Flags) {
+    assert(Flags == 2 &&
+           "Only DWARF32 is supported and debug_line_offset must be present.");
+    HeaderData.Flags |=
+        MACRO_FLAG_OFFSET_SIZE_32 | MACRO_FLAG_DEBUG_LINE_OFFSET;
+  }
+  void setDebugLineOffset(uint32_t DebugLineOffset) {
+    HeaderData.DebugLineOffset = DebugLineOffset;
+  }
+
   /// A single macro entry within a macro list.
   struct Entry {
     /// The type of the macro entry.
     uint32_t Type;
     union {
       /// The source line where the macro is defined.
       uint64_t Line;
       /// Vendor extension constant value.
       uint64_t ExtConstant;
     };
 
     union {
       /// The string (name, value) of the macro entry.
       const char *MacroStr;
       // An unsigned integer indicating the identity of the source file.
       uint64_t File;
       /// Vendor extension string.
       const char *ExtStr;
     };
   };
 
   using MacroList = SmallVector<Entry, 4>;
 
   /// 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 section.
+  /// Print the macro header from the debug_macro section.
+  void dumpMacroHeader(raw_ostream &OS) const;
+  /// Print the macro list found within the debug_macinfo/debug_macro section.
   void dump(raw_ostream &OS) const;
 
-  /// Parse the debug_macinfo section accessible via the 'data' parameter.
-  void parse(DataExtractor data);
+  /// Parse the debug_macro header.
+  void parseMacroHeader(DWARFDataExtractor data, uint64_t *Offset);
+
+  /// Parse the debug_macinfo/debug_macro section accessible via the 'data'
+  /// parameter.
+  void parse(DWARFContext &Context, DWARFDataExtractor data, uint64_t *Offset);
 
   /// Return whether the section has any entries.
   bool empty() const { return MacroLists.empty(); }
 };
 
 } // end namespace llvm
 
 #endif // LLVM_DEBUGINFO_DWARF_DWARFDEBUGMACRO_H

llvm/include/llvm/DebugInfo/DWARF/DWARFObject.h

public:
   virtual const DWARFSection &getFrameSection() const { return Dummy; }
   virtual const DWARFSection &getEHFrameSection() const { return Dummy; }
   virtual const DWARFSection &getLineSection() const { return Dummy; }
   virtual StringRef getLineStrSection() const { return ""; }
   virtual StringRef getStrSection() const { return ""; }
   virtual const DWARFSection &getRangesSection() const { return Dummy; }
   virtual const DWARFSection &getRnglistsSection() const { return Dummy; }
   virtual StringRef getMacinfoSection() const { return ""; }
+  virtual const DWARFSection &getMacroSection() const { return Dummy; }
   virtual StringRef getMacinfoDWOSection() const { return ""; }
   virtual const DWARFSection &getPubnamesSection() const { return Dummy; }
   virtual const DWARFSection &getPubtypesSection() const { return Dummy; }
   virtual const DWARFSection &getGnuPubnamesSection() const { return Dummy; }
   virtual const DWARFSection &getGnuPubtypesSection() const { return Dummy; }
   virtual const DWARFSection &getStrOffsetsSection() const { return Dummy; }
   virtual void
   forEachInfoDWOSections(function_ref<void(const DWARFSection &)> F) const {}

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

if (Error Err = Rnglists.extract(rnglistData, &Offset)) {
         break;
       Offset = TableOffset + Length;
     } else {
       Rnglists.dump(OS, LookupPooledAddress, DumpOpts);
     }
   }
 }
 
 static void dumpLoclistsSection(raw_ostream &OS, DIDumpOptions DumpOpts,
                                 DWARFDataExtractor Data,
                                 const MCRegisterInfo *MRI,
                                 const DWARFObject &Obj,
                                 Optional<uint64_t> DumpOffset) {
   uint64_t Offset = 0;
 
   while (Data.isValidOffset(Offset)) {
     DWARFListTableHeader Header(".debug_loclists", "locations");
     if (Error E = Header.extract(Data, &Offset)) {
       WithColor::error() << toString(std::move(E)) << '\n';
       return;
     }
 
     Header.dump(OS, DumpOpts);
 
     uint64_t EndOffset = Header.length() + Header.getHeaderOffset();
     Data.setAddressSize(Header.getAddrSize());
     DWARFDebugLoclists Loc(Data, Header.getVersion());
     if (DumpOffset) {
       if (DumpOffset >= Offset && DumpOffset < EndOffset) {
         Offset = *DumpOffset;
         Loc.dumpLocationList(&Offset, OS, /*BaseAddr=*/None, MRI, Obj, nullptr,
                              DumpOpts, /*Indent=*/0);
         OS << "\n";
         return;
       }
     } else {
       Loc.dumpRange(Offset, EndOffset - Offset, OS, MRI, Obj, DumpOpts);
     }
     Offset = EndOffset;
   }
 }
 
+static void dumpMacroSection(DWARFContext &Context, raw_ostream &OS,
+                             DWARFDataExtractor MacroData) {
+  uint64_t Offset = 0;
+  DWARFDebugMacro Macro;
+  while (MacroData.isValidOffset(Offset)) {
+    Macro.parseMacroHeader(MacroData, &Offset);
+    Macro.dumpMacroHeader(OS);
+    Macro.parse(Context, MacroData, &Offset);
+    Macro.dump(OS);
+  }
+}
+
 void DWARFContext::dump(
     raw_ostream &OS, DIDumpOptions DumpOpts,
     std::array<Optional<uint64_t>, DIDT_ID_Count> DumpOffsets) {
 
   uint64_t DumpType = DumpOpts.DumpType;
 
   StringRef Extension = sys::path::extension(DObj->getFileName());
   bool IsDWO = (Extension == ".dwo") || (Extension == ".dwp");
if (const auto *Off = shouldDump(Explicit, ".debug_frame", DIDT_ID_DebugFrame,
     getDebugFrame()->dump(OS, getRegisterInfo(), *Off);
 
   if (const auto *Off = shouldDump(Explicit, ".eh_frame", DIDT_ID_DebugFrame,
                                    DObj->getEHFrameSection().Data))
     getEHFrame()->dump(OS, getRegisterInfo(), *Off);
 
   if (shouldDump(Explicit, ".debug_macinfo", DIDT_ID_DebugMacro,
                  DObj->getMacinfoSection())) {
     getDebugMacro()->dump(OS);
   }
 
+  if (shouldDump(Explicit, ".debug_macro", DIDT_ID_DebugMacro,
+                 DObj->getMacroSection().Data)) {
+    DWARFDataExtractor Data(*DObj, DObj->getMacroSection(), isLittleEndian(),
+                            0);
+    dumpMacroSection(*this, OS, Data);
+  }
+
   if (shouldDump(Explicit, ".debug_macinfo.dwo", DIDT_ID_DebugMacro,
                  DObj->getMacinfoDWOSection())) {
     getDebugMacroDWO()->dump(OS);
   }
 
   if (shouldDump(Explicit, ".debug_aranges", DIDT_ID_DebugAranges,
                  DObj->getArangesSection())) {
     uint64_t offset = 0;
const DWARFDebugFrame *DWARFContext::getEHFrame() {
 
   DWARFDataExtractor debugFrameData(*DObj, DObj->getEHFrameSection(),
                                     isLittleEndian(), DObj->getAddressSize());
   DebugFrame.reset(new DWARFDebugFrame(getArch(), true /* IsEH */));
   DebugFrame->parse(debugFrameData);
   return DebugFrame.get();
 }
 
 const DWARFDebugMacro *DWARFContext::getDebugMacroDWO() {
   if (MacroDWO)
     return MacroDWO.get();
-
-  DataExtractor MacinfoDWOData(DObj->getMacinfoDWOSection(), isLittleEndian(),
-                               0);
+  uint64_t Offset = 0;
+  DWARFDataExtractor MacinfoDWOData(DObj->getMacinfoDWOSection(),
+                                    isLittleEndian(), 0);
   MacroDWO.reset(new DWARFDebugMacro());
-  MacroDWO->parse(MacinfoDWOData);
+  MacroDWO->parse(*this, MacinfoDWOData, &Offset);
   return MacroDWO.get();
 }
 
 const DWARFDebugMacro *DWARFContext::getDebugMacro() {
   if (Macro)
     return Macro.get();
-
-  DataExtractor MacinfoData(DObj->getMacinfoSection(), isLittleEndian(), 0);
+  uint64_t Offset = 0;
+  DWARFDataExtractor MacinfoData(DObj->getMacinfoSection(), isLittleEndian(),
+                                 0);
   Macro.reset(new DWARFDebugMacro());
-  Macro->parse(MacinfoData);
+  Macro->parse(*this, MacinfoData, &Offset);
   return Macro.get();
 }
 
 template <typename T>
 static T &getAccelTable(std::unique_ptr<T> &Cache, const DWARFObject &Obj,
                         const DWARFSection &Section, StringRef StringSection,
                         bool IsLittleEndian) {
   if (Cache)
     return *Cache;
   DWARFDataExtractor AccelSection(Obj, Section, IsLittleEndian, 0);
   DataExtractor StrData(StringSection, IsLittleEndian, 0);
   Cache.reset(new T(AccelSection, StrData));
class DWARFObjInMemory final : public DWARFObject {
   DWARFSectionMap AppleTypesSection;
   DWARFSectionMap AppleNamespacesSection;
   DWARFSectionMap AppleObjCSection;
   DWARFSectionMap NamesSection;
   DWARFSectionMap PubnamesSection;
   DWARFSectionMap PubtypesSection;
   DWARFSectionMap GnuPubnamesSection;
   DWARFSectionMap GnuPubtypesSection;
+  DWARFSectionMap MacroSection;
 
   DWARFSectionMap *mapNameToDWARFSection(StringRef Name) {
     return StringSwitch<DWARFSectionMap *>(Name)
         .Case("debug_loc", &LocSection)
         .Case("debug_loclists", &LoclistsSection)
         .Case("debug_loclists.dwo", &LoclistsDWOSection)
         .Case("debug_line", &LineSection)
         .Case("debug_frame", &FrameSection)
return StringSwitch<DWARFSectionMap *>(Name)
         .Case("debug_pubnames", &PubnamesSection)
         .Case("debug_pubtypes", &PubtypesSection)
         .Case("debug_gnu_pubnames", &GnuPubnamesSection)
         .Case("debug_gnu_pubtypes", &GnuPubtypesSection)
         .Case("apple_types", &AppleTypesSection)
         .Case("apple_namespaces", &AppleNamespacesSection)
         .Case("apple_namespac", &AppleNamespacesSection)
         .Case("apple_objc", &AppleObjCSection)
+        .Case("debug_macro", &MacroSection)
         .Default(nullptr);
   }
 
   StringRef AbbrevSection;
   StringRef ArangesSection;
   StringRef StrSection;
   StringRef MacinfoSection;
   StringRef MacinfoDWOSection;
public:
   }
   const DWARFSection &getLineSection() const override { return LineSection; }
   StringRef getStrSection() const override { return StrSection; }
   const DWARFSection &getRangesSection() const override { return RangesSection; }
   const DWARFSection &getRnglistsSection() const override {
     return RnglistsSection;
   }
   StringRef getMacinfoSection() const override { return MacinfoSection; }
+  const DWARFSection &getMacroSection() const override { return MacroSection; }
   StringRef getMacinfoDWOSection() const override { return MacinfoDWOSection; }
   const DWARFSection &getPubnamesSection() const override { return PubnamesSection; }
   const DWARFSection &getPubtypesSection() const override { return PubtypesSection; }
   const DWARFSection &getGnuPubnamesSection() const override {
     return GnuPubnamesSection;
   }
   const DWARFSection &getGnuPubtypesSection() const override {
     return GnuPubtypesSection;

llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp

 //===- DWARFDebugMacro.cpp ------------------------------------------------===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/DebugInfo/DWARF/DWARFDebugMacro.h"
 #include "llvm/BinaryFormat/Dwarf.h"
+#include "llvm/DebugInfo/DWARF/DWARFContext.h"
+#include "llvm/DebugInfo/DWARF/DWARFDataExtractor.h"
 #include "llvm/Support/WithColor.h"
 #include "llvm/Support/raw_ostream.h"
 #include <cstdint>
 
 using namespace llvm;
 using namespace dwarf;
 
+void DWARFDebugMacro::dumpMacroHeader(raw_ostream &OS) const {
+  OS << format("0x%08" PRIx64 ": ", getHeaderOffset());
+  OS << format("macro header: version = 0x%04" PRIx16 ", flags = 0x%02" PRIx8
+               ", debug_line_offset = 0x%04" PRIx32 "\n",
+               getVersion(), getFlags(), getDebugLineOffset());
+}
+
 void DWARFDebugMacro::dump(raw_ostream &OS) const {
   unsigned IndLevel = 0;
   for (const auto &Macros : MacroLists) {
     for (const Entry &E : Macros) {
-      // There should not be DW_MACINFO_end_file when IndLevel is Zero. However,
-      // this check handles the case of corrupted ".debug_macinfo" section.
+      // There should not be DW_MACINFO_end_file/DW_MACRO_end_file when IndLevel
+      // is Zero. However, this check handles the case of corrupted
+      // ".debug_macinfo/.debug_macro" section.
       if (IndLevel > 0)
         IndLevel -= (E.Type == DW_MACINFO_end_file);
       // Print indentation.
       for (unsigned I = 0; I < IndLevel; I++)
         OS << "  ";
       IndLevel += (E.Type == DW_MACINFO_start_file);
 
+      // Based on which version we are handling choose appropriate macro forms.
+      if (getVersion() >= 5)
+        WithColor(OS, HighlightColor::Macro).get() << MacroString(E.Type);
+      else
         WithColor(OS, HighlightColor::Macro).get() << MacinfoString(E.Type);
       switch (E.Type) {
       default:
-        // Got a corrupted ".debug_macinfo" section (invalid macinfo type).
+        // Got a corrupted ".debug_macinfo/.debug_macro" section (invalid
+        // macinfo type).
         break;
       case DW_MACINFO_define:
       case DW_MACINFO_undef:
+      case DW_MACRO_define_strp:
+      case DW_MACRO_undef_strp:
         OS << " - lineno: " << E.Line;
         OS << " macro: " << E.MacroStr;
         break;
       case DW_MACINFO_start_file:
         OS << " - lineno: " << E.Line;
         OS << " filenum: " << E.File;
         break;
       case DW_MACINFO_end_file:
         break;
       case DW_MACINFO_vendor_ext:
         OS << " - constant: " << E.ExtConstant;
         OS << " string: " << E.ExtStr;
         break;
       }
       OS << "\n";
     }
     OS << "\n";
   }
 }
 
-void DWARFDebugMacro::parse(DataExtractor data) {
-  uint64_t Offset = 0;
+void DWARFDebugMacro::parse(DWARFContext &Context, DWARFDataExtractor data,
+                            uint64_t *Offset) {
+  uint64_t strOffset = 0;
   MacroList *M = nullptr;
-  while (data.isValidOffset(Offset)) {
+  while (data.isValidOffset(*Offset)) {
     if (!M) {
       MacroLists.emplace_back();
       M = &MacroLists.back();
     }
     // A macro list entry consists of:
     M->emplace_back();
     Entry &E = M->back();
     // 1. Macinfo type
-    E.Type = data.getULEB128(&Offset);
+    E.Type = data.getULEB128(Offset);
 
     if (E.Type == 0) {
+      if (getVersion() >= 5)
+        return;
+      else
         // Reached end of a ".debug_macinfo" section contribution.
         continue;
     }
 
     switch (E.Type) {
     default:
-      // Got a corrupted ".debug_macinfo" section (invalid macinfo type).
-      // Push the corrupted entry to the list and halt parsing.
+      // Got a corrupted ".debug_macinfo/debug_macro" section (invalid macinfo
+      // type). Push the corrupted entry to the list and halt parsing.
       E.Type = DW_MACINFO_invalid;
       return;
     case DW_MACINFO_define:
     case DW_MACINFO_undef:
       // 2. Source line
-      E.Line = data.getULEB128(&Offset);
+      E.Line = data.getULEB128(Offset);
+      // 3. Macro string
+      E.MacroStr = data.getCStr(Offset);
+      break;
+    case DW_MACRO_define_strp:
+    case DW_MACRO_undef_strp:
+      // 2. Source line
+      E.Line = data.getULEB128(Offset);
       // 3. Macro string
-      E.MacroStr = data.getCStr(&Offset);
+      strOffset = data.getRelocatedValue(4 /*Offset Size*/, Offset);
+      E.MacroStr = Context.getStringExtractor().getCStr(&strOffset);
       break;
     case DW_MACINFO_start_file:
       // 2. Source line
-      E.Line = data.getULEB128(&Offset);
+      E.Line = data.getULEB128(Offset);
       // 3. Source file id
-      E.File = data.getULEB128(&Offset);
+      E.File = data.getULEB128(Offset);
       break;
     case DW_MACINFO_end_file:
       break;
     case DW_MACINFO_vendor_ext:
       // 2. Vendor extension constant
-      E.ExtConstant = data.getULEB128(&Offset);
+      E.ExtConstant = data.getULEB128(Offset);
       // 3. Vendor extension string
-      E.ExtStr = data.getCStr(&Offset);
+      E.ExtStr = data.getCStr(Offset);
       break;
     }
   }
 }
+
+void DWARFDebugMacro::parseMacroHeader(DWARFDataExtractor data,
+                                       uint64_t *Offset) {
+  setHeaderOffset(Offset);
+  setVersion(data.getU16(Offset));
+  setFlags(data.getU8(Offset));
+  setDebugLineOffset(data.getU32(Offset));
+}

llvm/test/DebugInfo/X86/debug-macro-v5.ll

+; RUN: %llc_dwarf --dwarf-version=5 -O0 -filetype=obj < %s | llvm-dwarfdump -v - | FileCheck %s
+
+; CHECK-LABEL:  .debug_info contents:
+; CHECK: DW_AT_macros [DW_FORM_sec_offset] (0x00000000)
+
+; CHECK-LABEL:  .debug_macro contents:
+; CHECK-NEXT:  0x00000000: macro header: version = 0x0005, flags = 0x02, debug_line_offset = 0x0000
+; CHECK-NEXT:  DW_MACRO_start_file - lineno: 0 filenum: 0
+; CHECK-NEXT:    DW_MACRO_start_file - lineno: 1 filenum: 1
+; CHECK-NEXT:      DW_MACRO_define_strp - lineno: 1 macro: FOO 5
+; CHECK-NEXT:    DW_MACRO_end_file
+; CHECK-NEXT:    DW_MACRO_start_file - lineno: 2 filenum: 2
+; CHECK-NEXT:      DW_MACRO_define_strp - lineno: 1 macro: BAR 6
+; CHECK-NEXT:    DW_MACRO_end_file
+; CHECK-NEXT:    DW_MACRO_define_strp - lineno: 4 macro: BAZ 7
+; CHECK-NEXT:    DW_MACRO_define_strp - lineno: 5 macro: YEA 8
+; CHECK-NEXT:    DW_MACRO_undef_strp - lineno: 14 macro: YEA
+; CHECK-NEXT:  DW_MACRO_end_file
+; CHECK-NEXT:  DW_MACRO_define_strp - lineno: 0 macro: __llvm__ 1
+; CHECK-NEXT:  DW_MACRO_define_strp - lineno: 0 macro: __clang__ 1
+; CHECK-NEXT:  DW_MACRO_define_strp - lineno: 0 macro: __clang_major__ 10
+; CHECK-NEXT:  DW_MACRO_define_strp - lineno: 0 macro: __clang_minor__ 0
+
+; ModuleID = 'test.c'
+source_filename = "test.c"
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+; Function Attrs: noinline nounwind optnone uwtable
+define dso_local i32 @main() #0 !dbg !25 {
+  %1 = alloca i32, align 4
+  %2 = alloca i32, align 4
+  call void @llvm.dbg.declare(metadata i32* %1, metadata !29, metadata !DIExpression()), !dbg !30
+  store i32 3, i32* %1, align 4, !dbg !30
+  call void @llvm.dbg.declare(metadata i32* %2, metadata !31, metadata !DIExpression()), !dbg !32
+  store i32 5, i32* %2, align 4, !dbg !32
+  ret i32 0, !dbg !33
+}
+; Function Attrs: nounwind readnone speculatable willreturn
+declare void @llvm.dbg.declare(metadata, metadata, metadata) #1
+
+attributes #0 = { noinline nounwind optnone uwtable }
+attributes #1 = { nounwind readnone speculatable willreturn }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!21, !22, !23}
+!llvm.ident = !{!24}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 10.0.0", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, macros: !3, splitDebugInlining: false, nameTableKind: None)
+!1 = !DIFile(filename: "test.c", directory: "/home/", checksumkind: CSK_MD5, checksum: "ef6a7032e0c7ceeef621583f2c00dc80")
+!2 = !{}
+!3 = !{!4, !17, !18, !19, !20}
+!4 = !DIMacroFile(file: !1, nodes: !5)
+!5 = !{!6, !10, !14, !15, !16}
+!6 = !DIMacroFile(line: 1, file: !7, nodes: !8)
+!7 = !DIFile(filename: "./foo.h", directory: "/home/", checksumkind: CSK_MD5, checksum: "0f0cd0e22b44f49d3944992c8dc28661")
+!8 = !{!9}
+!9 = !DIMacro(type: DW_MACINFO_define, line: 1, name: "FOO", value: "5")
+!10 = !DIMacroFile(line: 2, file: !11, nodes: !12)
+!11 = !DIFile(filename: "./bar.h", directory: "/home/", checksumkind: CSK_MD5, checksum: "bf4b34c333eaaa1d7085c25313b8d100")
+!12 = !{!13}
+!13 = !DIMacro(type: DW_MACINFO_define, line: 1, name: "BAR", value: "6")
+!14 = !DIMacro(type: DW_MACINFO_define, line: 4, name: "BAZ", value: "7")
+!15 = !DIMacro(type: DW_MACINFO_define, line: 5, name: "YEA", value: "8")
+!16 = !DIMacro(type: DW_MACINFO_undef, line: 14, name: "YEA")
+!17 = !DIMacro(type: DW_MACINFO_define, name: "__llvm__", value: "1")
+!18 = !DIMacro(type: DW_MACINFO_define, name: "__clang__", value: "1")
+!19 = !DIMacro(type: DW_MACINFO_define, name: "__clang_major__", value: "10")
+!20 = !DIMacro(type: DW_MACINFO_define, name: "__clang_minor__", value: "0")
+!21 = !{i32 7, !"Dwarf Version", i32 5}
+!22 = !{i32 2, !"Debug Info Version", i32 3}
+!23 = !{i32 1, !"wchar_size", i32 4}
+!24 = !{!"clang version 10.0.0"}
+!25 = distinct !DISubprogram(name: "main", scope: !1, file: !1, line: 9, type: !26, scopeLine: 9, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !2)
+!26 = !DISubroutineType(types: !27)
+!27 = !{!28}
+!28 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!29 = !DILocalVariable(name: "a", scope: !25, file: !1, line: 10, type: !28)
+!30 = !DILocation(line: 10, column: 5, scope: !25)
+!31 = !DILocalVariable(name: "b", scope: !25, file: !1, line: 11, type: !28)
+!32 = !DILocation(line: 11, column: 5, scope: !25)
+!33 = !DILocation(line: 12, column: 1, scope: !25)