This is an archive of the discontinued LLVM Phabricator instance.

[ObjectYAML][DWARF] Remove unused context. NFC.
ClosedPublic

Authored by Higuoxing on Jun 22 2020, 11:22 PM.

Details

Summary

The context is unused. This patch helps remove it.

Diff Detail

Event Timeline

Higuoxing created this revision.Jun 22 2020, 11:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 22 2020, 11:22 PM
Higuoxing edited the summary of this revision. (Show Details)Jun 22 2020, 11:23 PM

Are you sure it's unused? I took a quick look and the function void MappingTraits<DWARFYAML::PubEntry>::mapping uses it to determine the style (GNU/non-GNU).

Are you sure it's unused? I took a quick look and the function void MappingTraits<DWARFYAML::PubEntry>::mapping uses it to determine the style (GNU/non-GNU).

Yeah, the GNU-style is determined by DWARFYAML::PubSection rather than DWARFYAML::Data. The DWARF context is never used.

void MappingTraits<DWARFYAML::PubEntry>::mapping(IO &IO,
                                                 DWARFYAML::PubEntry &Entry) {
  IO.mapRequired("DieOffset", Entry.DieOffset);
  if (reinterpret_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();   // unused                            |
  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);
}

Besides, the implementation of mapping YAML to PubSection is buggy. If we map debug_gnu_pubnames/debug_gnu_pubtypes to PubSection, the GNUStyle is always false and Descriptor can never be mapped into Entry.Descriptor.
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      ## Descripor can never be mapped into Entry.Descriptor

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

But if we map PubSection to debug_gnu_pubnames/debug_gnu_pubtypes, that is fine since the GNUStyle has been set by obj2yaml.

I'm wondering if we need to add support for emitting the .debug_gnu_pubnames and .debug_gnu_pubtypes since no test case relies on them.

jhenderson accepted this revision.Jun 23 2020, 1:37 AM

Thanks, I see. LGTM. I can't comment on the .debug_gnu_pub* stuff, but I wouldn't bother adding support for it at this point. It can be added/fixed later if you have time, but other aspects are probably of greater priority. It might be a good idea to file some kind of bug though.

This revision is now accepted and ready to land.Jun 23 2020, 1:37 AM

It might be a good idea to file some kind of bug though.

Thanks, I'll do it later. It seems that Bugzilla isn't working right now. I've reported it to the list.

This revision was automatically updated to reflect the committed changes.