This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by Higuoxing on Jun 23 2020, 11:15 PM.

Details

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.

Diff Detail

Event Timeline

Higuoxing created this revision.Jun 23 2020, 11:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 23 2020, 11:15 PM

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 added a comment.EditedJun 24 2020, 1:58 AM

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*?

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?

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.

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.

Looks reasonable. Please add a unit test.

## Descripor can never be mapped into Entry.Descriptor

typo: Descriptor

Higuoxing updated this revision to Diff 273200.Jun 24 2020, 5:55 PM
  • Add unit tests.
  • Use DWARFContext to save context information.
Higuoxing marked an inline comment as done.Jun 24 2020, 6:02 PM
Higuoxing added inline comments.
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 edited the summary of this revision. (Show Details)Jun 24 2020, 6:03 PM
Higuoxing updated this revision to Diff 273215.Jun 24 2020, 7:10 PM

Format codes & Make premerge-check-bot happy.

jhenderson added inline comments.Jun 25 2020, 1:35 AM
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 marked an inline comment as done.Jun 25 2020, 1:52 AM
Higuoxing added inline comments.
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 added inline comments.Jun 25 2020, 2:11 AM
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 marked 4 inline comments as done.Jun 25 2020, 2:20 AM
Higuoxing added inline comments.
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 added inline comments.Jun 25 2020, 2:32 AM
llvm/unittests/ObjectYAML/DWARFYAMLTest.cpp
29–35 ↗(On Diff #273215)

Exactly. Sounds good.

Higuoxing marked 3 inline comments as not done.Jun 25 2020, 2:42 AM

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

Higuoxing updated this revision to Diff 274325.Jun 29 2020, 9:00 PM
Higuoxing marked 4 inline comments as done.

Address comments.

jhenderson added inline comments.Jun 30 2020, 1:30 AM
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 marked an inline comment as done.Jun 30 2020, 5:21 AM
Higuoxing added inline comments.
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 accepted this revision.Jun 30 2020, 5:53 AM

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?

This revision is now accepted and ready to land.Jun 30 2020, 5:53 AM
Higuoxing marked an inline comment as done.Jun 30 2020, 6:03 AM
Higuoxing added inline comments.
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 added inline comments.Jun 30 2020, 6:49 AM
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 marked an inline comment as done.Jun 30 2020, 11:51 PM
Higuoxing added inline comments.
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 added inline comments.Jul 1 2020, 2:13 AM
llvm/unittests/ObjectYAML/DWARFYAMLTest.cpp
77 ↗(On Diff #274325)

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

Higuoxing updated this revision to Diff 274988.Jul 1 2020, 7:31 PM

Add one helper function for parsing YAML file.

MaskRay added inline comments.Jul 1 2020, 8:53 PM
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 updated this revision to Diff 275006.Jul 1 2020, 10:23 PM
Higuoxing marked 2 inline comments as done.

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 added inline comments.Jul 2 2020, 3:15 AM
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 updated this revision to Diff 275122.Jul 2 2020, 7:43 AM
Higuoxing marked 6 inline comments as done.

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?

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 updated this revision to Diff 275336.Jul 3 2020, 2:58 AM
Higuoxing marked 2 inline comments as done.

Address review comments.

llvm/unittests/ObjectYAML/DWARFYAMLTest.cpp
107 ↗(On Diff #275006)

Thanks! I learned something new today.

This revision was automatically updated to reflect the committed changes.