[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

59

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 ↗

(On Diff #273215)

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

38 ↗

(On Diff #273215)

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

79–85 ↗

(On Diff #273215)

Same as above.

[Higuoxing]

llvm/unittests/ObjectYAML/DWARFYAMLTest.cpp

29–35 ↗

(On Diff #273215)

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 ↗

(On Diff #273215)

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 ↗

(On Diff #273215)

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 ↗

(On Diff #273215)

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 ↗

(On Diff #274325)

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 ↗

(On Diff #274325)

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 ↗

(On Diff #274325)

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 ↗

(On Diff #274325)

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 ↗

(On Diff #274325)

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 ↗

(On Diff #274325)

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 ↗

(On Diff #274325)

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 ↗

(On Diff #274988)

Mentioning GNUPub might be better

llvm/include/llvm/ObjectYAML/DWARFYAML.h

118 ↗

(On Diff #274988)

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 ↗

(On Diff #274988)

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 ↗

(On Diff #274988)

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

llvm/unittests/ObjectYAML/DWARFYAMLTest.cpp

104 ↗

(On Diff #275006)

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

107 ↗

(On Diff #275006)

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

108 ↗

(On Diff #275006)

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 ↗

(On Diff #275006)

ASSERT_TRUE

173 ↗

(On Diff #275006)

Same comments in this test as above.

77 ↗

(On Diff #274325)

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.

[Higuoxing]

Address review comments.

Thanks for reviewing!

llvm/unittests/ObjectYAML/DWARFYAMLTest.cpp

107 ↗

(On Diff #275006)

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.

77 ↗

(On Diff #274325)

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

[jhenderson]

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

llvm/unittests/ObjectYAML/DWARFYAMLTest.cpp

107 ↗

(On Diff #275006)

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).

77 ↗

(On Diff #274325)

Sounds good.

[Higuoxing]

Address review comments.

llvm/unittests/ObjectYAML/DWARFYAMLTest.cpp

107 ↗

(On Diff #275006)

Thanks! I learned something new today.

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) {
   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);
+  if (DWARF.PubNames || DWARF.PubTypes || DWARF.GNUPubNames ||
+      DWARF.GNUPubTypes || !IO.outputting()) {
+    auto *OldContext = IO.getContext();
+    bool IsGNUStyle = false;
+    IO.setContext(&IsGNUStyle);
     IO.mapOptional("debug_pubnames", DWARF.PubNames);
     IO.mapOptional("debug_pubtypes", DWARF.PubTypes);
+
+    IsGNUStyle = true;
     IO.mapOptional("debug_gnu_pubnames", DWARF.GNUPubNames);
     IO.mapOptional("debug_gnu_pubtypes", DWARF.GNUPubTypes);
+    IO.setContext(OldContext);
+  }
   IO.mapOptional("debug_info", DWARF.CompileUnits);
   IO.mapOptional("debug_line", DWARF.DebugLines);
   IO.mapOptional("debug_addr", DWARF.DebugAddr);
 }
 
 void MappingTraits<DWARFYAML::Abbrev>::mapping(IO &IO,
                                                DWARFYAML::Abbrev &Abbrev) {
   IO.mapOptional("Code", Abbrev.Code);
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::PubSection *>(IO.getContext())->IsGNUStyle)
     IO.mapRequired("Descriptor", Entry.Descriptor);
   IO.mapRequired("Name", Entry.Name);
 }
 
 void MappingTraits<DWARFYAML::PubSection>::mapping(
     IO &IO, DWARFYAML::PubSection &Section) {
-  auto OldContext = IO.getContext();
+  bool *IsGNUStyle = static_cast<bool *>(IO.getContext());
+  Section.IsGNUStyle = *IsGNUStyle;
   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);
+  IO.setContext(IsGNUStyle);
 }
 
 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);