[PATCH] D67078: [llvm-readelf] - Allow dumping the dynamic symbols when there is no program headers.

[George Rimar via Phabricator via llvm-commits]

grimar marked an inline comment as done.
grimar added inline comments.


================
Comment at: test/tools/llvm-readobj/dyn-symbols.test:185
+      - Tag:   DT_SYMTAB
+        Value: 0
+      - Tag:   DT_NULL
----------------
MaskRay wrote:
> The test is different from https://reviews.llvm.org/D62179#1652778
> 
> In https://reviews.llvm.org/D62179#1652778, there is no program headers (corrupted phdr), so toMappedAddr returns a nullptr. Here, DT_SYMTAB is 0. When the information from DT_SYMTAB and .dynsym conflict, we probably should let DT_DYNTAB win...
> The test is different from https://reviews.llvm.org/D62179#1652778

I think not. Here is the same YAML but with the program headers:

```
--- !ELF
FileHeader:
  Class:   ELFCLASS64
  Data:    ELFDATA2LSB
  Type:    ET_DYN
  Machine: EM_X86_64
Sections:
  - Name:    .dynamic
    Type:    SHT_DYNAMIC
    Entries:
      - Tag:   DT_SYMTAB
        Value: 0
      - Tag:   DT_NULL
        Value: 0
DynamicSymbols:
  - Name: foo
ProgramHeaders:
  - Type: PT_LOAD
    Flags: [ PF_R ]
    VAddr: 0x0000
    PAddr: 0x0000
    Align: 8
    Sections:
      - Section: .dynsym
```

The 0 value for DT_SYMTAB is valid. I just removed the `ProgramHeaders`.

> When the information from DT_SYMTAB and .dynsym conflict, we probably should let DT_DYNTAB win...

I do not understand why. Particulary I do not understand why we should take .dynsym size from the section headers table
(it is the only source it seems), but its address from another place.
The much simpler logic is to try to take everything from section header at first and fallback to DT_SYMTAB. This is what this patch does.

If we have different results, then I guess it would be correct to report a warning, but I am not sure why DT_SYMTAB should win.
Doesn't it mean we have a broken object? In this case why should we make any assumptions about which place is correct and which is not?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67078/new/

https://reviews.llvm.org/D67078