This is an archive of the discontinued LLVM Phabricator instance.

[llvm-readobj] - Improve error message reported by DynRegionInfo.
ClosedPublic

Authored by grimar on Jan 27 2020, 7:55 AM.

Details

Summary

DynRegionInfo is a helper class used to create memory ranges.
It is used for many things and can report errors.
Errors reported currently do not provide a good diagnostic.
This patch fixes it and adds a test for each possible case.

Diff Detail

Event Timeline

grimar created this revision.Jan 27 2020, 7:55 AM
Herald added a project: Restricted Project. · View Herald Transcript
MaskRay added inline comments.Jan 27 2020, 12:10 PM
llvm/tools/llvm-readobj/ELFDumper.cpp
153

This order may be more natural: invalid size (32) of section with index 1

Should section sizes be in hex in the warnings? Not sure, but I feel like typical sections with entsize will have a more readable size (and entsize) when it is in hex.

llvm/test/Object/invalid.test
140

I'm not sure how to work this into the way you've written the code, but this doesn't read well. It should really be "invalid size (48) or entity size (32) of section with index 1". Perhaps this wording might work too: "section with index 1 has invalid size (48) or entity size (32)".

llvm/test/tools/llvm-readobj/ELF/broken-dynamic-reloc.test
54

This doesn't read well (the relocation table isn't stored in the DT_RELASZ entry...). I think it should be something like "Show we print a warning for an invalid relocation table size stored in a DT_RELASZ entry".

91

Similar to above, how about: "Show we print a warning for an invalid relocation table entry size stored in a DT_RELAENT entry"?

128

Same comment as above.

165

Same as above.

llvm/test/tools/llvm-readobj/ELF/dyn-symbols.test
358

it has a priority -> the dynamic tag has priority

360

Aside: why is it not using the DT_SYMENT dynamic tag?

368

This sort of message should be "invalid size (1) of section..." The current format isn't clear that the (1) is the size.

404

Nit: no new line at EOF.

llvm/tools/llvm-readobj/ELFDumper.cpp
140–141

ELF gabi refers to items in sections with non-zero sh_entsize as "entries", so this should probably be "Entry size name".

Change the last sentence to "If this field is empty, errors will not mention the entry size."

1946–1948

clang-format

grimar updated this revision to Diff 240851.Jan 28 2020, 6:06 AM
grimar marked 14 inline comments as done.
  • Addressed review comments.
llvm/test/tools/llvm-readobj/ELF/dyn-symbols.test
360

I do not know. Perhaps we want to fix this. Also, I think this area is not well tested.
I can investigate it in a follow-up.

llvm/tools/llvm-readobj/ELFDumper.cpp
140–141

I've updated "entity"->"entry" in the error text too.

grimar planned changes to this revision.EditedJan 28 2020, 6:16 AM

I've forgot to address the comment about changing section size printed to hex. Going to do it.

jhenderson added a comment.EditedJan 29 2020, 1:51 AM

All other changes look good to me.

llvm/test/tools/llvm-readobj/ELF/dyn-symbols.test
360

Sounds good.

llvm/tools/llvm-readobj/ELFDumper.cpp
139

Let's make this a little more generic. How about something like "InfoContext"?

1883

I'd move the " has " to the usage site, since it's always present, and the error message wouldn't make sense without it.

grimar marked 4 inline comments as done.Jan 29 2020, 2:09 AM
grimar added inline comments.
llvm/tools/llvm-readobj/ELFDumper.cpp
139

Ok. Seems leaving the "Error prefix" sentence for the comment is fine?

1883

ErrPrefix is often empty. E.g. messages like:
invalid DT_RELASZ value (24) or DT_RELAENT value (255) does not suppose to have this prefix about section index.

Do you mean to add "has" to the usage site when ErrPrefix is not empty?

jhenderson added inline comments.Jan 29 2020, 3:01 AM
llvm/tools/llvm-readobj/ELFDumper.cpp
139

Yes, I think so. The comment might rot if it starts getting used for other things, but I don't think it matters too much in this case.

1883

Oh, I missed that. I'd probably do as you suggest - add "has" when the prefix is non-empty.

grimar updated this revision to Diff 241091.Jan 29 2020, 3:19 AM
grimar marked 2 inline comments as done.
  • Addressed review comments.
jhenderson accepted this revision.Jan 30 2020, 1:23 AM

LGTM, with the suggested change.

llvm/tools/llvm-readobj/ELFDumper.cpp
139

Having seen the change, I don't think I like InfoContext, sorry! How about just Context?

This revision is now accepted and ready to land.Jan 30 2020, 1:23 AM
grimar marked 2 inline comments as done.Jan 30 2020, 1:30 AM
grimar added inline comments.
llvm/tools/llvm-readobj/ELFDumper.cpp
139

Sounds good to me. Will apply, thanks!

This revision was automatically updated to reflect the committed changes.
grimar marked an inline comment as done.
grimar marked an inline comment as done.Feb 12 2020, 2:39 AM
grimar added inline comments.
llvm/test/tools/llvm-readobj/ELF/dyn-symbols.test
360

The behavior of GNU readelf looks broken to me.
See: when we have DT_SYMENT == 0x123 and a .dynsym of size of 0x1:

--- !ELF
FileHeader:
  Class:   ELFCLASS64
  Data:    ELFDATA2LSB
  Type:    ET_DYN
  Machine: EM_X86_64
Sections:
  - Name:    .dynamic
    Type:    SHT_DYNAMIC
    Entries:
      - Tag:   DT_SYMTAB
        Value: 0x100
      - Tag:   DT_SYMENT
        Value: 0x123
      - Tag:   DT_NULL
        Value: 0
  - Name:    .dynsym
    Type:    SHT_DYNSYM
    Address: 0x100
    Size:    0x1
ProgramHeaders:
  - Type: PT_LOAD
    VAddr: 0x100
    Sections:
      - Section: .dynsym

what produce the object with following section headers:

Section Headers:
  [Nr] Name              Type             Address           Offset
       Size              EntSize          Flags  Link  Info  Align
  [ 0]                   NULL             0000000000000000  00000000
       0000000000000000  0000000000000000           0     0     0
readelf: Warning: [ 1]: Link field (0) should index a string section.
  [ 1] .dynamic          DYNAMIC          0000000000000000  00000078
       0000000000000030  0000000000000010           0     0     0
readelf: Warning: [ 2]: Link field (0) should index a string section.
  [ 2] .dynsym           DYNSYM           0000000000000100  000000a8
       0000000000000001  0000000000000018   A       0     1     0
  [ 3] .strtab           STRTAB           0000000000000000  000000a9
       0000000000000001  0000000000000000           0     0     1
  [ 4] .shstrtab         STRTAB           0000000000000000  000000aa
       0000000000000024  0000000000000000           0     0     1

(Note: .dynsym has sh_entsize == 0x18. It is a correct value set by yaml2obj
implicitly. The size is broken as expected and == 0x1).

I have following outputs:

Case 1:

umb@ubuntu:~/tests/121$ readelf --symbols test.o
readelf: Error: Section .dynsym has an invalid sh_entsize of 0x18

Symbol table '.dynsym' contains 0 entries:
   Num:    Value          Size Type    Bind   Vis      Ndx Name
readelf: Error: Section .dynsym has an invalid sh_entsize of 0x18

Case 2:

umb@ubuntu:~/tests/121$ readelf --symbols test.o -D
readelf: Error: Section .dynsym has an invalid sh_entsize of 0x18

Dynamic symbol information is not available for displaying symbols.

Well... there are many broken things here I see:

  1. ".dynsym has an invalid sh_entsize" it is not true. The sh_size is broken, but 0x18 is a valid value for sh_entsize.
  2. Duplicate error messages reported in the case #1. It reminds me about plans to switch to reportUniqueWarning. Perhaps it is a time to do this.
  3. The different output. I.e. in the first case we see the dynamic symbols section header, in the second we don't have it. Looks a bit inconsistent to me. In both cases we dump symbols.
  4. Error messages started from the upper-case. I see we have the same thing in "llvm-readobj\ELF\broken-group.test", but our other tests use the lower case form (as expected, I believe).
  5. Error? Why not just a warning...
jhenderson added inline comments.Feb 12 2020, 2:51 AM
llvm/test/tools/llvm-readobj/ELF/dyn-symbols.test
360

That does seem very broken in various ways. Perhaps worth reporting to GNU? Either way, I don't think we need to follow any of that behaviour.

grimar marked an inline comment as done.Feb 12 2020, 3:11 AM
grimar added inline comments.
llvm/test/tools/llvm-readobj/ELF/dyn-symbols.test
360

Yes, but in the context of the DT_SYMENT:
I think I do not understand why this value is usefull/was introduced.
E.g. LLD has the following line addInt(DT_SYMENT, sizeof(Elf_Sym));.
I'd expect other linkers to have something the same.
So seems we have 2 possible cases: 32/64 versions of a symbol. LLD works really well as we know...
Is it possible that something else can be valid as a value for the DT_SYMENT?
I just expect that such things are baked too hard and perhaps we do not want to
care about DT_SYMENT? Or may be should just check and report if it is != sizeof(Elf_Sym)?

jhenderson added inline comments.Feb 12 2020, 3:30 AM
llvm/test/tools/llvm-readobj/ELF/dyn-symbols.test
360

I guess a warning if the DT_SYMENT does not match the symbol size of the appropriate ELF variety is fine.

grimar marked an inline comment as done.Feb 12 2020, 3:38 AM
grimar added inline comments.
llvm/test/tools/llvm-readobj/ELF/dyn-symbols.test
360

OK. I'll prepare a patch for this.

llvm/test/Object/invalid.test