[DWARFYAML][debug_gnu_*] Add the missing context `IsGNUStyle`. NFC.

[Higuoxing]

Summary

This patch helps add the missing context IsGNUStyle. Before this patch, yaml2obj cannot parse the YAML description of 'debug_gnu_pubnames' and 'debug_gnu_pubtypes' correctly due to the missing context.

In other words, if we have

DWARF:
  debug_gnu_pubtypes:
    Length:
      TotalLength: 0x1234
    Version:    2
    UnitOffset: 0x1234
    UnitSize:   0x4321
    Entries:
      - DieOffset:  0x12345678
        Name:       abc
        Descriptor: 0x00      ## Descriptor can never be mapped into Entry.Descriptor

yaml2obj will complain that "error: unknown key 'Descriptor'".

This patch helps resolve this problem.

[jhenderson]

You need some form of testing for this. You might want to use Mach-O for testing it in this case.

llvm/lib/ObjectYAML/DWARFYAML.cpp

66

Let's be explicit about the type here. It's not obvious to me from looking at this what the type returned by getContext is.

[grimar]

Such approach generally feels unsafe. Should we instead introduce some struct, lets say DWARFContext which will be:

struct DWARFContext {
  DWARFYAML::PubSection* Section;
  bool IsGNUStyle;
}

so that we'll always know that getContext returns DWARFContext*?

[Higuoxing]

In D82435#2110862, @jhenderson wrote:

You need some form of testing for this. You might want to use Mach-O for testing it in this case.

Unfortunately, yaml2macho doesn't support emitting the .debug_gnu_pub* sections :-(
What about using unit test here?

In D82435#2110879, @grimar wrote:

Such approach generally feels unsafe. Should we instead introduce some struct, lets say DWARFContext which will be:

struct DWARFContext {
  DWARFYAML::PubSection* Section;
  bool IsGNUStyle;
}

so that we'll always know that getContext returns DWARFContext*?

Good idea! I'll have a try.

[jhenderson]

In D82435#2110942, @Higuoxing wrote:

In D82435#2110862, @jhenderson wrote:

You need some form of testing for this. You might want to use Mach-O for testing it in this case.

Unfortunately, yaml2macho doesn't support emitting the .debug_gnu_pub* sections :-(
What about using unit test here?

If it's straightforward, unit-testing would always be my preference. If not, just rebase your other patch on this one, to show that the fix actually fixes it.

[MaskRay]

Looks reasonable. Please add a unit test.

## Descripor can never be mapped into Entry.Descriptor

typo: Descriptor

[Higuoxing]

• Add unit tests.
• Use DWARFContext to save context information.

[Higuoxing]

llvm/lib/ObjectYAML/DWARFYAML.cpp

51

@jhenderson : Let's be explicit about the type here. It's not obvious to me from looking at this what the type returned by getContext is.

IO.getContext() returns void *. It might be a MachOYAML::Object& or a ELFYAML::Object& here. I guess we cannot cast it to an explicit type here.

[Higuoxing]

Format codes & Make premerge-check-bot happy.

[jhenderson]

llvm/lib/ObjectYAML/DWARFYAML.cpp

51

You could just replace auto with void, I believe. It helps make it clearer.

llvm/unittests/ObjectYAML/DWARFYAMLTest.cpp

29–35

Rather than capturing stderr, you could use: EXPECT_THAT_EXPECTED(SectionsOrErr, FailedWithMessage("error message"));

38

I'd put the passing cases for each section above the failing cases.

79–85

Same as above.

[Higuoxing]

llvm/unittests/ObjectYAML/DWARFYAMLTest.cpp

29–35

I think the simplest way to check the error message is capturing the stderr here because the error message is printed to the errs(). If we use FailedWithMessage() we will get Invalid argument.

[jhenderson]

llvm/unittests/ObjectYAML/DWARFYAMLTest.cpp

29–35

Ah, I see the problem. It looks like yaml::Input has a default error handling that consumes and prints to errs() the real error, before returning an error code. in error(), which in turn gets passed up the stack from emitDebugSections.

Probably a separate change, but I think we can and should improve this, as it's incosistent with other error handling in emitDebugSections. It looks like it should be straightforward to pass in a custom error handler to the yaml::Input constructor used within emitDebugSections to handle the error (i.e. by storing it and reporting it up the tree). That'll allow us to cleanly unit test it here. A longer term improvement would have yaml::Input store a real Error rather a std::error_code, but that looks like it might be quite a bit of work, and probably best to leave.

What do you think?

[Higuoxing]

llvm/unittests/ObjectYAML/DWARFYAMLTest.cpp

29–35

I'd like to have a custom error handler and rebase this patch on that. Besides, it's good for unit testing, right?

[jhenderson]

llvm/unittests/ObjectYAML/DWARFYAMLTest.cpp

29–35

Exactly. Sounds good.

[Higuoxing]

These comments are not addressed yet. They are marked by an accident...

[Higuoxing]

Address comments.

[jhenderson]

llvm/unittests/ObjectYAML/DWARFYAMLTest.cpp

77

I think in the two passing cases, it would be good to check the value of the Descriptor field in the Entries. Ideally, I'd also test the other fields in tha YAML, but that might be outside the scope of this change, and better deferred to a later change.

[Higuoxing]

llvm/unittests/ObjectYAML/DWARFYAMLTest.cpp

77

it would be good to check the value of the Descriptor field in the Entries

Thanks, I want to do it as well. Can we use DWARF parser in lib/DebugInfo here?

[jhenderson]

LGTM, if the suggestion inline isn't possible. We can test it in the lit tests.

llvm/unittests/ObjectYAML/DWARFYAMLTest.cpp

77

I'd be slightly reluctant to do so, as there's a risk of circular testing again. Are the actual YAML data structures not available at this point then?

[Higuoxing]

llvm/unittests/ObjectYAML/DWARFYAMLTest.cpp

77

Yes, the actual YAML data structure isn't available yet. DWARFYAML::emitDebugSections() returns StringMap<std::unique_ptr<MemoryBuffer>>. What do you think of having another function that returns the parsed YAML structure for unit testing?

[jhenderson]

llvm/unittests/ObjectYAML/DWARFYAMLTest.cpp

77

Would this function need to be in the main library, or can it be in the unit test? If in the library, I'd want it to be small and simple. It's generally not ideal to have too many hooks in the main library that are there purely for testing, I feel.

[Higuoxing]

llvm/unittests/ObjectYAML/DWARFYAMLTest.cpp

77

What about having a helper function in this test? I think we can replace the DWARFYAML::emitDebugSections() with parseDWARFYAML() in this unit testing file since this file is for testing the YAML syntax rather than debug sections generating.

static Expected<DWARFYAML::Data>
parseDWARFYAML(StringRef Yaml, bool IsLittleEndian,
                               bool Is64bit) {
  DWARFYAML::Data Data;
  Data.IsLittleEndian = IsLittleEndian;
  Data.Is64bit = Is64bit;

  SMDiagnostic GeneratedDiag;
  yaml::Input YIn(
      Yaml, /*Ctxt=*/nullptr,
      [](const SMDiagnostic &Diag, void *DiagContext) {
        *static_cast<SMDiagnostic *>(DiagContext) = Diag;
      },
      &GeneratedDiag);

  YIn >> Data;
  if (YIn.error())
    return createStringError(YIn.error(), GeneratedDiag.getMessage());

  return Data;
}

[jhenderson]

llvm/unittests/ObjectYAML/DWARFYAMLTest.cpp

77

That would probably make sense, since emitDebugSections isn't actually used at all.

[Higuoxing]

Add one helper function for parsing YAML file.

[MaskRay]

llvm/include/llvm/ObjectYAML/DWARFEmitter.h

37

Mentioning GNUPub might be better

llvm/include/llvm/ObjectYAML/DWARFYAML.h

118

Minor nit: IsGNUPubSec is probably sufficient.

GNUPub is derived from .debug_gnu_pubnames .debug_gnu_pubtypes

[Higuoxing]

Address comments.

Thanks for reviewing!

llvm/include/llvm/ObjectYAML/DWARFEmitter.h

37

Sorry, what do you mean by saying "mentioning" here? Does it mean that you want me to rename the variable IsGNUStyle to IsGNUPubSec?

[jhenderson]

llvm/include/llvm/ObjectYAML/DWARFEmitter.h

37

I can't say for certain, but I'd guess IsGNUPubSec is what @MaskRay is suggesting.

llvm/unittests/ObjectYAML/DWARFYAMLTest.cpp

77

Ah, I see what you mean now. emitDebugSections is used by various bits of testing which are for testing the DWARF parser. Yeah, we don't want that here. I'd recommend updating the other tests in due course too, to use the new function.

104

ASSERT_TRUE instead of ASSERT_EQ, means you don't need the true argument.

107

I might be wrong, but I don't think you need the ul suffix here (similar below for u suffices).

108

Use EXPECT_EQ when it doesn't matter if this fails for being able to do the rest of the test. Use ASSERT_EQ when it's not possible to do the rest of the test, e.g. due to it being a size check.

113

ASSERT_TRUE

173

Same comments in this test as above.

[Higuoxing]

Address review comments.

Thanks for reviewing!

llvm/unittests/ObjectYAML/DWARFYAMLTest.cpp

77

Sure, can I do it in a follow-up patch?

107

My compiler complains that comparison of integer expressions of different signedness: ‘const long unsigned int’ and ‘const int’. So I have to add the suffices to suppress the warning.

[jhenderson]

LGTM, with the ul to u change in two places.

llvm/unittests/ObjectYAML/DWARFYAMLTest.cpp

77

Sounds good.

107

Huh. In this case, I'd argue this is a spurious warning since the 2 is a literal and trivially can be converted to unsigned for comparisons. This might be caused by gtest rather than your compiler though.

Either way, I wouldn't use ul - I'd use u if you have to use anything. size() returns a size_t whose size is not necessarily unsigned long. size_t is 32 bits or 64 bits depending on the system (i.e. a 32 or 64 bit host OS typically). long is 32 bits or 64 bits depending on the system too, but with different rules (the standard merely dictates that it is at least as large as an int and on Windows it's only 4 bytes).

[Higuoxing]

Address review comments.

llvm/unittests/ObjectYAML/DWARFYAMLTest.cpp

107

Thanks! I learned something new today.

llvm/include/llvm/ObjectYAML/DWARFEmitter.h

 struct PubSection;
 
 Error emitDebugAbbrev(raw_ostream &OS, const Data &DI);
 Error emitDebugStr(raw_ostream &OS, const Data &DI);
 
 Error emitDebugAranges(raw_ostream &OS, const Data &DI);
 Error emitDebugRanges(raw_ostream &OS, const Data &DI);
 Error emitPubSection(raw_ostream &OS, const PubSection &Sect,
-                     bool IsLittleEndian);
+                     bool IsLittleEndian, bool IsGNUStyle = false);
 Error emitDebugInfo(raw_ostream &OS, const Data &DI);
 Error emitDebugLine(raw_ostream &OS, const Data &DI);
 Error emitDebugAddr(raw_ostream &OS, const Data &DI);
 
 Expected<StringMap<std::unique_ptr<MemoryBuffer>>>
 emitDebugSections(StringRef YAMLString, bool ApplyFixups = false,
                   bool IsLittleEndian = sys::IsLittleEndianHost);
 } // end namespace DWARFYAML
 } // end namespace llvm
 
 #endif // LLVM_OBJECTYAML_DWARFEMITTER_H

llvm/include/llvm/ObjectYAML/DWARFYAML.h

struct PubEntry {
   StringRef Name;
 };
 
 struct PubSection {
   InitialLength Length;
   uint16_t Version;
   uint32_t UnitOffset;
   uint32_t UnitSize;
-  bool IsGNUStyle = false;
   std::vector<PubEntry> Entries;
-
-  PubSection() = default;
-  PubSection(bool IsGNUStyle) : IsGNUStyle(IsGNUStyle) {}
 };
 
 struct FormValue {
   llvm::yaml::Hex64 Value;
   StringRef CStr;
   std::vector<llvm::yaml::Hex8> BlockData;
 };
 
 struct Entry {
   llvm::yaml::Hex32 AbbrCode;
   std::vector<FormValue> Values;
 };
 
+/// Class that contains helpful context information when mapping YAML into DWARF
+/// data structures.
+struct DWARFContext {
+  bool IsGNUStylePubSec = false;
+};
+
 struct Unit {
   InitialLength Length;
   uint16_t Version;
   llvm::dwarf::UnitType Type; // Added in DWARF 5
   uint32_t AbbrOffset;
   uint8_t AddrSize;
   std::vector<Entry> Entries;
 };

llvm/lib/ObjectYAML/DWARFEmitter.cpp

for (auto DebugRanges : DI.DebugRanges) {
     ++EntryIndex;
   }
 
   return Error::success();
 }
 
 Error DWARFYAML::emitPubSection(raw_ostream &OS,
                                 const DWARFYAML::PubSection &Sect,
-                                bool IsLittleEndian) {
+                                bool IsLittleEndian, bool IsGNUStyle) {
   writeInitialLength(Sect.Length, OS, IsLittleEndian);
   writeInteger((uint16_t)Sect.Version, OS, IsLittleEndian);
   writeInteger((uint32_t)Sect.UnitOffset, OS, IsLittleEndian);
   writeInteger((uint32_t)Sect.UnitSize, OS, IsLittleEndian);
   for (auto Entry : Sect.Entries) {
     writeInteger((uint32_t)Entry.DieOffset, OS, IsLittleEndian);
-    if (Sect.IsGNUStyle)
+    if (IsGNUStyle)
       writeInteger((uint8_t)Entry.Descriptor, OS, IsLittleEndian);
     OS.write(Entry.Name.data(), Entry.Name.size());
     OS.write('\0');
   }
 
   return Error::success();
 }
 

llvm/lib/ObjectYAML/DWARFYAML.cpp

SetVector<StringRef> DWARFYAML::Data::getUsedSectionNames() const {
   if (PubTypes)
     SecNames.insert("debug_pubtypes");
   return SecNames;
 }
 
 namespace yaml {
 
 void MappingTraits<DWARFYAML::Data>::mapping(IO &IO, DWARFYAML::Data &DWARF) {
+  auto *OldContext = IO.getContext();
+  DWARFYAML::DWARFContext DWARFCtx;
+  IO.setContext(&DWARFCtx);
   IO.mapOptional("debug_str", DWARF.DebugStrings);
   IO.mapOptional("debug_abbrev", DWARF.AbbrevDecls);
   if (!DWARF.ARanges.empty() || !IO.outputting())
     IO.mapOptional("debug_aranges", DWARF.ARanges);
   if (!DWARF.DebugRanges.empty() || !IO.outputting())
     IO.mapOptional("debug_ranges", DWARF.DebugRanges);
   IO.mapOptional("debug_pubnames", DWARF.PubNames);
   IO.mapOptional("debug_pubtypes", DWARF.PubTypes);
+  DWARFCtx.IsGNUStylePubSec = true;
   IO.mapOptional("debug_gnu_pubnames", DWARF.GNUPubNames);
   IO.mapOptional("debug_gnu_pubtypes", DWARF.GNUPubTypes);
   IO.mapOptional("debug_info", DWARF.CompileUnits);
   IO.mapOptional("debug_line", DWARF.DebugLines);
   IO.mapOptional("debug_addr", DWARF.DebugAddr);
+  IO.setContext(OldContext);
 }
 
 void MappingTraits<DWARFYAML::Abbrev>::mapping(IO &IO,
                                                DWARFYAML::Abbrev &Abbrev) {
   IO.mapOptional("Code", Abbrev.Code);
   IO.mapRequired("Tag", Abbrev.Tag);
   IO.mapRequired("Children", Abbrev.Children);
   IO.mapRequired("Attributes", Abbrev.Attributes);
void MappingTraits<DWARFYAML::Ranges>::mapping(IO &IO,
   IO.mapOptional("Offset", DebugRanges.Offset);
   IO.mapOptional("AddrSize", DebugRanges.AddrSize);
   IO.mapRequired("Entries", DebugRanges.Entries);
 }
 
 void MappingTraits<DWARFYAML::PubEntry>::mapping(IO &IO,
                                                  DWARFYAML::PubEntry &Entry) {
   IO.mapRequired("DieOffset", Entry.DieOffset);
-  if (reinterpret_cast<DWARFYAML::PubSection *>(IO.getContext())->IsGNUStyle)
+  if (static_cast<DWARFYAML::DWARFContext *>(IO.getContext())->IsGNUStylePubSec)
     IO.mapRequired("Descriptor", Entry.Descriptor);
   IO.mapRequired("Name", Entry.Name);
 }
 
 void MappingTraits<DWARFYAML::PubSection>::mapping(
     IO &IO, DWARFYAML::PubSection &Section) {
-  auto OldContext = IO.getContext();
-  IO.setContext(&Section);
-
   IO.mapRequired("Length", Section.Length);
   IO.mapRequired("Version", Section.Version);
   IO.mapRequired("UnitOffset", Section.UnitOffset);
   IO.mapRequired("UnitSize", Section.UnitSize);
   IO.mapRequired("Entries", Section.Entries);
-
-  IO.setContext(OldContext);
 }
 
 void MappingTraits<DWARFYAML::Unit>::mapping(IO &IO, DWARFYAML::Unit &Unit) {
   IO.mapRequired("Length", Unit.Length);
   IO.mapRequired("Version", Unit.Version);
   if (Unit.Version >= 5)
     IO.mapRequired("UnitType", Unit.Type);
   IO.mapRequired("AbbrOffset", Unit.AbbrOffset);

llvm/tools/obj2yaml/dwarf2yaml.cpp

Error dumpDebugRanges(DWARFContext &DCtx, DWARFYAML::Data &Y) {
   return ErrorSuccess();
 }
 
 static DWARFYAML::PubSection dumpPubSection(const DWARFContext &DCtx,
                                             const DWARFSection &Section,
                                             bool IsGNUStyle) {
   DWARFDataExtractor PubSectionData(DCtx.getDWARFObj(), Section,
                                     DCtx.isLittleEndian(), 0);
-  DWARFYAML::PubSection Y(IsGNUStyle);
+  DWARFYAML::PubSection Y;
   uint64_t Offset = 0;
   dumpInitialLength(PubSectionData, Offset, Y.Length);
   Y.Version = PubSectionData.getU16(&Offset);
   Y.UnitOffset = PubSectionData.getU32(&Offset);
   Y.UnitSize = PubSectionData.getU32(&Offset);
   while (Offset < Y.Length.getLength()) {
     DWARFYAML::PubEntry NewEntry;
     NewEntry.DieOffset = PubSectionData.getU32(&Offset);

llvm/unittests/ObjectYAML/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
+  DebugInfoDWARF
   Object
   ObjectYAML
   )
 
 add_llvm_unittest(ObjectYAMLTests
+  DWARFYAMLTest.cpp
   ELFYAMLTest.cpp
   MinidumpYAMLTest.cpp
   YAML2ObjTest.cpp
   YAMLTest.cpp
   )
 
 target_link_libraries(ObjectYAMLTests PRIVATE LLVMTestingSupport)

llvm/unittests/ObjectYAML/DWARFYAMLTest.cpp

+//===- DWARFYAMLTest.cpp - Tests for DWARFYAML.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/ObjectYAML/DWARFYAML.h"
+#include "llvm/ObjectYAML/DWARFEmitter.h"
+#include "llvm/Testing/Support/Error.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+
+TEST(DWARFPubSections, TestUnexpectedDescriptor) {
+  StringRef Yaml = R"(
+debug_pubnames:
+  Length:
+    TotalLength: 0x1234
+  Version:       2
+  UnitOffset:    0x4321
+  UnitSize:      0x00
+  Entries:
+    - DieOffset:  0x1234
+      Descriptor: 0x12
+      Name:       abcd
+)";
+  testing::internal::CaptureStderr();
+  auto SectionsOrErr = DWARFYAML::emitDebugSections(Yaml);
+  EXPECT_THAT_ERROR(SectionsOrErr.takeError(), Failed());
+  ASSERT_EQ(testing::internal::GetCapturedStderr(),
+            std::string("YAML:10:19: error: unknown key 'Descriptor'\n"
+                        "      Descriptor: 0x12\n"
+                        "                  ^~~~\n"));
+}
+
+TEST(DWARFPubSections, TestDWARFPubSections) {
+  StringRef Yaml = R"(
+debug_pubnames:
+  Length:
+    TotalLength: 0x1234
+  Version:       2
+  UnitOffset:    0x4321
+  UnitSize:      0x00
+  Entries:
+    - DieOffset:  0x1234
+      Name:       abc
+    - DieOffset:  0x4321
+      Name:       def
+debug_pubtypes:
+  Length:
+    TotalLength: 0x1234
+  Version:       2
+  UnitOffset:    0x4321
+  UnitSize:      0x00
+  Entries:
+    - DieOffset:  0x1234
+      Name:       abc
+    - DieOffset:  0x4321
+      Name:       def
+)";
+  auto SectionsOrErr = DWARFYAML::emitDebugSections(Yaml);
+  EXPECT_THAT_EXPECTED(SectionsOrErr, Succeeded());
+}
+
+TEST(DWARFGNUPubSections, TestMissingDescriptor) {
+  StringRef Yaml = R"(
+debug_gnu_pubnames:
+  Length:
+    TotalLength: 0x1234
+  Version:       2
+  UnitOffset:    0x4321
+  UnitSize:      0x00
+  Entries:
+    - DieOffset: 0x1234
+      Name:      abcd
+)";
+  testing::internal::CaptureStderr();
+  auto SectionsOrErr = DWARFYAML::emitDebugSections(Yaml);
+  EXPECT_THAT_ERROR(SectionsOrErr.takeError(), Failed());
+  ASSERT_EQ(testing::internal::GetCapturedStderr(),
+            std::string("YAML:9:7: error: missing required key 'Descriptor'\n"
+                        "    - DieOffset: 0x1234\n"
+                        "      ^\n"));
+}
+
+TEST(DWARFGNUPubSections, TestDWARFGNUPubSections) {
+  StringRef Yaml = R"(
+debug_gnu_pubnames:
+  Length:
+    TotalLength: 0x1234
+  Version:       2
+  UnitOffset:    0x4321
+  UnitSize:      0x00
+  Entries:
+    - DieOffset:  0x1234
+      Descriptor: 0x12
+      Name:       abc
+    - DieOffset:  0x4321
+      Descriptor: 0x34
+      Name:       def
+debug_gnu_pubtypes:
+  Length:
+    TotalLength: 0x1234
+  Version:       2
+  UnitOffset:    0x4321
+  UnitSize:      0x00
+  Entries:
+    - DieOffset:  0x1234
+      Descriptor: 0x12
+      Name:       abc
+    - DieOffset:  0x4321
+      Descriptor: 0x34
+      Name:       def
+)";
+  auto SectionsOrErr = DWARFYAML::emitDebugSections(Yaml);
+  EXPECT_THAT_EXPECTED(SectionsOrErr, Succeeded());
+}