[PATCH] D62349: Change ELF tools to allow multiple sections per file.

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/test/Object/multiple-sections.yaml:2
+# RUN: yaml2obj %s -o %t.o
+# RUN: llvm-readelf --headers %t.o | FileCheck %s
+
----------------
I would suggest to use
`llvm-readobj -a --elf-cg-profile --addrsig`

(llvm-readelf is a GNU style and seems does not implement dumpers for versioning sections and SHT_LLVM_* sections yet.
Also, `-a` or `--headers` does not trigger `--elf-cg-profile`/`--addrsig` it seems).

And then perhaps I would check that at least something was dumped:

```
# CHECK:      Version symbols
# CHECK-NEXT:   Section Name: .versym

# CHECK: SHT_GNU_verdef
# CHECK: SHT_GNU_verneed
# CHECK: CGProfile
# CHECK: Addrsig
```



================
Comment at: llvm/test/Object/multiple-sections.yaml:4
+
+# CHECK: ELF Header:
+
----------------
Please add a short description of the test in comment.
(i.e. "here we are testing that multiple sections in a object does not trigger error", or something like that)


================
Comment at: llvm/test/Object/multiple-sections.yaml:21
+    Flags:           [ ]
+    AddressAlign:    0x0000000000000001
+    Content:         ''
----------------
You do not need `AddressAlign` field here and below I think.


================
Comment at: llvm/test/Object/multiple-sections.yaml:78
+Symbols:         
+  - Name:            f
+    Type:            STT_FUNC
----------------
If I correctly understand you need this symbol to trigger creation of the first .symtab.
But does not seem you need `.text` section, can it probably be local?

```
Symbols:         
  - Name:            f
```


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:1410
       DynSymtabName = unwrapOrError(Obj->getSectionName(&Sec));
       DynamicStringTable = unwrapOrError(Obj->getStringTableForSymtab(Sec));
       break;
----------------
I think the following is more correct:

```
if (!DynSymRegion.Size) {
  DynSymRegion = createDRIFrom(&Sec);
  // This is only used (if Elf_Shdr present)for naming section in GNU style
  DynSymtabName = unwrapOrError(Obj->getSectionName(&Sec));
  DynamicStringTable = unwrapOrError(Obj->getStringTableForSymtab(Sec));
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62349