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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/ObjectYAML/DWARFEmitter.h:37
 Error emitPubSection(raw_ostream &OS, const PubSection &Sect,
-                     bool IsLittleEndian);
+                     bool IsLittleEndian, bool IsGNUStyle = false);
 Error emitDebugInfo(raw_ostream &OS, const Data &DI);
----------------
Higuoxing wrote:
> MaskRay wrote:
> > Mentioning GNUPub might be better
> > 
> > 
> Sorry, what do you mean by saying "mentioning" here? Does it mean that you want me to rename the variable `IsGNUStyle` to `IsGNUPubSec`?
I can't say for certain, but I'd guess `IsGNUPubSec` is what @MaskRay is suggesting.


================
Comment at: llvm/unittests/ObjectYAML/DWARFYAMLTest.cpp:77
+  auto SectionsOrErr = DWARFYAML::emitDebugSections(Yaml);
+  EXPECT_THAT_EXPECTED(SectionsOrErr, Succeeded());
+}
----------------
jhenderson wrote:
> Higuoxing wrote:
> > jhenderson wrote:
> > > Higuoxing wrote:
> > > > jhenderson wrote:
> > > > > Higuoxing wrote:
> > > > > > jhenderson wrote:
> > > > > > > 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.
> > > > > > > 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?
> > > > > 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?
> > > > 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?
> > > 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.
> > 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;
> > }
> > ```
> That would probably make sense, since `emitDebugSections` isn't actually used at all.
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.


================
Comment at: llvm/unittests/ObjectYAML/DWARFYAMLTest.cpp:104
+
+  ASSERT_EQ(DWARFOrErr->PubNames.hasValue(), true);
+  DWARFYAML::PubSection PubNames = DWARFOrErr->PubNames.getValue();
----------------
ASSERT_TRUE instead of ASSERT_EQ, means you don't need the `true` argument.


================
Comment at: llvm/unittests/ObjectYAML/DWARFYAMLTest.cpp:107
+
+  ASSERT_EQ(PubNames.Entries.size(), 2ul);
+  ASSERT_EQ((uint32_t)PubNames.Entries[0].DieOffset, 0x1234u);
----------------
I might be wrong, but I don't think you need the `ul` suffix here (similar below for `u` suffices).


================
Comment at: llvm/unittests/ObjectYAML/DWARFYAMLTest.cpp:108
+  ASSERT_EQ(PubNames.Entries.size(), 2ul);
+  ASSERT_EQ((uint32_t)PubNames.Entries[0].DieOffset, 0x1234u);
+  ASSERT_EQ(PubNames.Entries[0].Name, "abc");
----------------
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.


================
Comment at: llvm/unittests/ObjectYAML/DWARFYAMLTest.cpp:113
+
+  ASSERT_EQ(DWARFOrErr->PubTypes.hasValue(), true);
+  DWARFYAML::PubSection PubTypes = DWARFOrErr->PubTypes.getValue();
----------------
ASSERT_TRUE


================
Comment at: llvm/unittests/ObjectYAML/DWARFYAMLTest.cpp:173
+
+  ASSERT_EQ(DWARFOrErr->GNUPubNames.hasValue(), true);
+  DWARFYAML::PubSection GNUPubNames = DWARFOrErr->GNUPubNames.getValue();
----------------
Same comments in this test as above.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82435/new/

https://reviews.llvm.org/D82435