[PATCH] D63475: [llvm-readobj] Allow --hex-dump/--string-dump to dump multiple sections

[George Rimar via Phabricator via llvm-commits]

grimar accepted this revision.
grimar added a comment.
This revision is now accepted and ready to land.

LGTM with a 2 nits/suggestions. But please hold on to see if other reviewers has something to say.



================
Comment at: test/tools/llvm-readobj/hex-dump.test:2
+Both 9 and .strtab refer to .strtab. Test we dump the section only once.
+RUN: llvm-readobj -x 9 -x .strtab -x .strtab %p/Inputs/trivial.obj.elf-x86-64 2>&1 | \
+RUN:   FileCheck %s --check-prefix=ELF
----------------
grimar wrote:
> I'd suggest starting the lines from '#'. That will be useful for git history
> when we convert all inputs to YAML and inline them here.
> 
> Also, you list `-x .strtab` twice, but `-x 9` only once, so may be list it twice too?
"Also, you list -x .strtab twice, but -x 9 only once, so may be list it twice too?"

This part was not addressed. I think you should list `-x 9` twice too:
```
llvm-readobj -x 9 -x 9 -x .strtab -x .strtab
```


================
Comment at: tools/llvm-readobj/ObjDumper.cpp:70
+      reportWarning(formatv("could not find section '{0}'", S.first()).str());
+  for (std::pair<int, bool> S : SecIndices)
+    if (!S.second)
----------------
MaskRay wrote:
> grimar wrote:
> > grimar wrote:
> > > The same. You should use `std::map` I think.
> > Or, if your intention was to keep the insertion order in the outpuit, you probably need `MapVector`.
> The logic is:
> 
> ```
> for each section:
>   if section name is specified by -x or section index is specified by -x:
>     dump()
> ```
> 
> The insertion order doesn't matter. I'll use `std::map` for simplicity.
What I was thinking about is that in theory we could print the ""could not find section..." warnings in the same order as `-x` is used in the command line. Though and I also do not think it is really important/useful.


================
Comment at: tools/llvm-readobj/ObjDumper.cpp:47
+    else
+      SecNames.emplace(Section, false);
+
----------------
I think we usually wrap the `for` body in such cases:

```
for (StringRef Section : Sections) {
  if (!Section.getAsInteger(0, SecIndex))
    SecIndices.emplace(SecIndex, false);
   else
    SecNames.emplace(Section, false);
}
```


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63475