Page MenuHomePhabricator

[obj2yaml] Add support for dumping the .debug_str section.
ClosedPublic

Authored by Higuoxing on Aug 31 2020, 3:34 AM.

Details

Summary

This patch adds support for dumping the .debug_str section to obj2yaml.

Diff Detail

Event Timeline

Higuoxing created this revision.Aug 31 2020, 3:34 AM
Herald added a project: Restricted Project. · View Herald Transcript
Higuoxing requested review of this revision.Aug 31 2020, 3:34 AM
Higuoxing updated this revision to Diff 288915.Aug 31 2020, 3:39 AM

Fix test case (c)

Harbormaster completed remote builds in B70080: Diff 288912.
grimar added inline comments.Aug 31 2020, 4:24 AM
llvm/test/tools/obj2yaml/ELF/DWARF/debug-str.yaml
29

I'd add a short comment for each of these sub-cases to make it clear what they want to test.

34

I think you have SHDR TYPE FLAGS ADDRALIGN ENTSIZE prefixes in each of the sub cases.
Perhaps merge them into single prefix, e.g COMMON?

56

Probably you need a test showing that by default we don't print "Sections:" with the YAML below.
I.e. a case when you don't override fields for this particular YAML.

llvm/tools/obj2yaml/elf2yaml.cpp
213

Perhaps it would be easier to read it when there is no CommonProperties variable,
but an early return is used instead.

if (RawSec->Type != ELF::SHT_PROGBITS || !RawSec->Link.empty() ||
    RawSec->Info || (uint64_t)RawSec->AddressAlign != 1 ||
    RawSec->Address)
  return true;

if (SecName == "debug_str")
  return !(RawSec->EntSize && (uint64_t)*RawSec->EntSize == 1) ||
         RawSec->Flags.getValueOr(ELFYAML::ELF_SHF(0)) !=
             ELFYAML::ELF_SHF(ELF::SHF_MERGE | ELF::SHF_STRINGS);

return RawSec->EntSize || RawSec->Flags;
grimar added inline comments.Aug 31 2020, 4:36 AM
llvm/tools/obj2yaml/elf2yaml.cpp
418

I'd probably expect to see that dumpDebugStrings returns an error too, btw.
I can imagine cases when it should error out probably:

  1. A string table that doesn't start with the null-byte.
  2. A string table that doesn't end with the null byte.
jhenderson added inline comments.Sep 1 2020, 2:31 AM
llvm/test/tools/obj2yaml/ELF/DWARF/debug-str.yaml
27
56

Seems like adding this to the BASIC case above would be enough, right? You just need to show that when the object has default header properties, it doesn't add it to "Sections" too.

llvm/tools/obj2yaml/elf2yaml.cpp
418

.debug_str is not an ELF SHT_STRTAB and doesn't require a leading null-byte by the standard. It does require a trailing null byte (since the contents is a series of null terminated strings).

As dumpDebugStrings already exists, this change should just work with its current behaviour, and a separate patch should address error handling, if needed.

grimar added inline comments.Sep 1 2020, 2:37 AM
llvm/test/tools/obj2yaml/ELF/DWARF/debug-str.yaml
56

Yes, seems the first YAML and this one can be merged.

Higuoxing updated this revision to Diff 289624.Sep 2 2020, 7:57 PM
Higuoxing marked 7 inline comments as done.

Address comments.

It looks like FileCheck isn't happy when I put the BASIC: checking labels before the COMMON: labels, so I adjust the order a little.

Thanks for reviewing!

llvm/test/tools/obj2yaml/ELF/DWARF/debug-str.yaml
56

I merged them into the first YAML.

llvm/tools/obj2yaml/elf2yaml.cpp
418

Yes, I think I want to do it in a follow-up patch. I've fixed the behavior for llvm-dwarfdump recently and it seems that we can refactor it to reuse the parser in lib/DebugInfo.

Higuoxing updated this revision to Diff 289633.Sep 2 2020, 10:04 PM

Add test case (c): Test dumping an empty .debug_str section.

jhenderson added inline comments.Sep 3 2020, 12:43 AM
llvm/test/tools/obj2yaml/ELF/DWARF/debug-str.yaml
14

Here and in each other case, rather than having to put leading 0s in every value, you could switch to FileCheck numeric variables. I think the invocation is something like:

# RUN: FIleCheck ... -D#%x,ADDRALIGN=1
...
#  COMMON-NEXT:   AddressAlign: 0x[[#%.16x, ADDRALIGN]]
llvm/tools/obj2yaml/elf2yaml.cpp
209

I'm struggling with this code. Under what circumstances can RawSec->EntSize by None here? Similar question for the Flags below.

Higuoxing updated this revision to Diff 289657.Sep 3 2020, 1:37 AM
Higuoxing marked an inline comment as done.

Update.

llvm/tools/obj2yaml/elf2yaml.cpp
209

I check it because EntSize and Flags are Optional values. It seems that you are right, I cannot create a test case that make them None. Could you please tell me why they cannot be None here?

grimar added inline comments.Sep 3 2020, 1:46 AM
llvm/tools/obj2yaml/elf2yaml.cpp
209

Well. I haven't debugged this, but I *think* they can be None.

Looking on the code, we have:

Error ELFDumper<ELFT>::dumpCommonSection(const Elf_Shdr *Shdr,
                                         ELFYAML::Section &S) {
...
  if (Shdr->sh_entsize != getDefaultShEntSize<ELFT>(S.Type))
    S.EntSize = static_cast<llvm::yaml::Hex64>(Shdr->sh_entsize);

So i'd expect to have None value when sh_entsize == 0. Isn't it true?

jhenderson added inline comments.Sep 3 2020, 1:46 AM
llvm/tools/obj2yaml/elf2yaml.cpp
209

@grimar is more familiar with this code than I am, so perhaps he can explain. I unfortunately have higher priority work that needs doing today, or I'd go digging myself!

grimar added inline comments.Sep 3 2020, 7:09 AM
llvm/tools/obj2yaml/elf2yaml.cpp
209

So yes, I've debugged this and it is possible to get None for EntSize when sh_entsize == 0.
(I've used the YAML below).

--- !ELF
FileHeader:
  Class: ELFCLASS64
  Data:  ELFDATA2LSB
  Type:  ET_EXEC
Sections:
  - Name:   .debug_str
    Type:    SHT_PROGBITS
    EntSize: 0
    Size:    10
    ShFlags: 0

When there are no flags, we have 'None` for Flags.

Higuoxing updated this revision to Diff 289855.Sep 3 2020, 9:26 PM

Check the optional value.

llvm/tools/obj2yaml/elf2yaml.cpp
209

Thank you @grimar . I reproduce it using the YAML provided by you with a slight modification.

--- !ELF
FileHeader:
  Class: ELFCLASS64
  Data:  ELFDATA2LSB
  Type:  ET_EXEC
Sections:
  - Name:         .debug_str
    Type:         SHT_PROGBITS
    AddressAlign: 1
    Flags:        []

Because the default value of AddressAlign is 0. If we don't specify it, this function will return in the first if branch and it will not reach the second if branch where the optional value is dereferenced.

if (RawSec->Type != ELF::SHT_PROGBITS || !RawSec->Link.empty() ||
    RawSec->Info || RawSec->AddressAlign != 1 || RawSec->Address)
                    ^~~~~~~~~~~~~~~~~~~~~~~~~
  return true;
jhenderson added inline comments.Sep 4 2020, 12:37 AM
llvm/test/tools/obj2yaml/ELF/DWARF/debug-str.yaml
37–39

I think you need an ENTSIZE= 0 test case too, since that would flag up if we dereferenced the Entsize value without checking.

llvm/tools/obj2yaml/elf2yaml.cpp
209

Can you do this? I think it's nice to be symmetrical with the flags line below.

Higuoxing updated this revision to Diff 290132.Sep 6 2020, 3:48 AM
Higuoxing marked 2 inline comments as done.

I modified the getDefaultShEntSize() function in this revision. If the section name is '.debug_str', the default value of sh_entsize is 1 rather than 0.

llvm/tools/obj2yaml/elf2yaml.cpp
209

I mark this as done since this comment is not applied here ...

jhenderson added inline comments.Sep 7 2020, 1:03 AM
llvm/tools/obj2yaml/elf2yaml.cpp
213

I'm not sure casting this here is the clearest of things. I'd actually make it more explicit and check it doesn't equal 0 (if the (bool) is even required).

Higuoxing updated this revision to Diff 290200.Sep 7 2020, 1:15 AM
Higuoxing marked an inline comment as done.

Explicitly check the sh_flag.

jhenderson accepted this revision.Sep 7 2020, 1:22 AM

LGTM, with suggestion.

llvm/tools/obj2yaml/elf2yaml.cpp
213

It may be best to introduce a helper variable so that we don't repeat RawSec->Flags.getValueOr(ELFYAML::ELF_SHF(0)) twice.

This revision is now accepted and ready to land.Sep 7 2020, 1:22 AM
Higuoxing updated this revision to Diff 290219.Sep 7 2020, 2:29 AM
Higuoxing marked an inline comment as done.

Avoid using Flags.getValueOr() twice.