[PATCH] D48904: [llvm-objdump] Add --archive-headers (-a) option

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Fun fact: the GNU objdump documentation uses --archive-headers in its initial switch list, but --archive-header in its description! In fact, it supports all three of "-a", "--archive-header" and "--archive-headers", due to its inexact matching of switches (i.e. it also supports --archive-head as an alias, for example).

Also, I noticed that GNU objdump prints a brief summary of the file before each entry in the dump - something like:

  test.o:      file format elf64-x86-64
  <header dump>
  
  test 2.o:      file format elf64-x86-64

I see that an equivalent of this is printed as part of `DumpObject`. This leads me to think three things:

1. The printing of the information you are adding here should be printed inside `DumpObject` (and first within that function before anything else).
2. llvm-objdump prints the name as test.a(test.o), which means that the "In Archive ..." printing is unnecessary.
3. We need a test for combining -a with another switch, e.g. -d.

What are your thoughts on this?



================
Comment at: test/tools/llvm-objdump/archive-headers.test:1
+#RUN: llvm-objdump -a %p/Inputs/liblong_filenames.a | FileCheck %s
+
----------------
As noted in D48810, please add a duplicate line using the long form as well.

Also, please put a space between the '#' and the RUN/CHECK directives, as it makes it a little easier to read.


================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:2129-2130
+  if (!ModeOrErr) {
+    consumeError(ModeOrErr.takeError());
+    outs() << "ill-formed archive entry.\n";
+    return;
----------------
Why not just use report_error to print the appropriate message?

I assume that testing this would be non-trivial?


================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:2134-2136
+  // FIXME: this first dash, "-", is for (Mode & S_IFMT) == S_IFREG.
+  // But there is nothing in sys::fs::perms for S_IFMT or S_IFREG.
+  outs() << "-";
----------------
Using an admittedly old version of GNU objdump (2.24), this dash doesn't appear at all when I use --archive-headers. Maybe we shouldn't output it either?


================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:2153
+  unsigned UID = UIDOrErr.get();
+  outs() << format("%3d/", UID);
+  Expected<unsigned> GIDOrErr = C.getGID();
----------------
Why %3? FWIW, GNU objdump appears to to do no padding on this column.


================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:2154
+  outs() << format("%3d/", UID);
+  Expected<unsigned> GIDOrErr = C.getGID();
+  if (!GIDOrErr)
----------------
Please put a blank line before this line.


================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:2159
+  outs() << format("%-3d ", GID);
+  Expected<uint64_t> Size = C.getRawSize();
+  if (!Size)
----------------
Please put a blank line before this block too.


================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:2162
+    report_error(Filename, Size.takeError());
+  outs() << format("%5" PRId64, Size.get()) << " ";
+
----------------
GNU objdump uses 6 as the minimum width of this column. Why 5?


================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:2170
+  else {
+    // Since cime(3) returns a 26 character string of the form:
+    // "Sun Sep 16 01:03:52 1973\n\0"
----------------
cime(3) -> ctime(3) presumably?

I think what you are doing here is fine, but again, it's worth noting that GNU objdump doesn't print the day (i.e. "Sun" for your example).


================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:2179
+  if (!NameOrErr) {
+    consumeError(NameOrErr.takeError());
+    Expected<StringRef> NameOrErr = C.getRawName();
----------------
I'm not sure I like us just throwing away this error, since it might indicate a problem that is not clear from the raw file name. Maybe it's worth warning if that error is reported, but the raw name does not result in an error. What do you think?


================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:2180
+    consumeError(NameOrErr.takeError());
+    Expected<StringRef> NameOrErr = C.getRawName();
+    if (!NameOrErr)
----------------
This can, on high warning levels on some compilers, lead to warnings, I believe. I'd suggest just calling it RawNameOrErr for clarity.


================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:2186-2187
+  } else {
+    StringRef Name = NameOrErr.get();
+    outs() << Name << "\n";
+  }
----------------
You can avoid duplicating the printing code here, by doing something like:

```
StringRef Name = "";
Expected<StringRef> NameOrErr = C.getName();
if (!NameOrErr) {
  ...
  Expected<StringRef> RawNameOrErr = C.getRawName();
  ...
  Name = RawNameOrErr.get();
} else {
  Name = NameOrErr.get();
}
outs() << Name << "\n";
```

or possibly better yet, if you are able to reuse NameOrErr:

```
Expected<StringRef> NameOrErr = C.getName();
if (!NameOrErr) {
  ...
  NameOrErr = C.getRawName();
  ...
}
outs() << NameOrErr.get() << "\n";
```


================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:2191
+
+static void printArchiveHeaders(StringRef Filename, const Archive *A) {
+  outs() << "In archive " << Filename << ":\n";
----------------
I'm not sure this is a good name for this function. Possibly better would be "printArchiveSummary" or similar, since it's not actually doing any header printing. Or probably better, just place this inline, since it's only one line and is only called in one place.


Repository:
  rL LLVM

https://reviews.llvm.org/D48904