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.
Details
Diff Detail
Event Timeline
llvm/tools/llvm-readobj/ELFDumper.cpp | ||
---|---|---|
151 | 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 | ||
53 | 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". | |
90 | Similar to above, how about: "Show we print a warning for an invalid relocation table entry size stored in a DT_RELAENT entry"? | |
127 | Same comment as above. | |
164 | 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." | |
1938 | clang-format |
- 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. | |
llvm/tools/llvm-readobj/ELFDumper.cpp | ||
140–141 | I've updated "entity"->"entry" in the error text too. |
I've forgot to address the comment about changing section size printed to hex. Going to do it.
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"? | |
1874 | I'd move the " has " to the usage site, since it's always present, and the error message wouldn't make sense without it. |
llvm/tools/llvm-readobj/ELFDumper.cpp | ||
---|---|---|
139 | Ok. Seems leaving the "Error prefix" sentence for the comment is fine? | |
1874 | ErrPrefix is often empty. E.g. messages like: Do you mean to add "has" to the usage site when ErrPrefix is not empty? |
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? |
llvm/tools/llvm-readobj/ELFDumper.cpp | ||
---|---|---|
139 | Sounds good to me. Will apply, thanks! |
llvm/test/tools/llvm-readobj/ELF/dyn-symbols.test | ||
---|---|---|
360 | The behavior of GNU readelf looks broken to me. --- !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 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:
|
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. |
llvm/test/tools/llvm-readobj/ELF/dyn-symbols.test | ||
---|---|---|
360 | Yes, but in the context of the DT_SYMENT: |
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. |
llvm/test/tools/llvm-readobj/ELF/dyn-symbols.test | ||
---|---|---|
360 | OK. I'll prepare a patch for this. |
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)".