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

[jhenderson]

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?

test/tools/llvm-objdump/archive-headers.test

2

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.

tools/llvm-objdump/llvm-objdump.cpp

2144–2145

Why not just use report_error to print the appropriate message?

I assume that testing this would be non-trivial?

2149–2151

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?

2168

Why %3? FWIW, GNU objdump appears to to do no padding on this column.

2169

Please put a blank line before this line.

2174

Please put a blank line before this block too.

2177

GNU objdump uses 6 as the minimum width of this column. Why 5?

2185

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).

2194

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?

2195

This can, on high warning levels on some compilers, lead to warnings, I believe. I'd suggest just calling it RawNameOrErr for clarity.

2201–2202

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";

2206

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.

[paulsemel]

Added Jame's suggestions.
Totally agree with all your points James (except the error one, as it might break the whole behavior)

[jhenderson]

In D48904#1152154, @paulsemel wrote:

Added Jame's suggestions.
Totally agree with all your points James (except the error one, as it might break the whole behavior)

Could you explain more about why you think that the error might break behaviour?

Please also add a test as I described earlier:

We need a test for combining -a with another switch, e.g. -d.

tools/llvm-objdump/MachODump.cpp

77–78

As this is no longer a MachO-specific option, surely it makes more sense for this to be in llvm-objdump.cpp, with the rest of the switches?

tools/llvm-objdump/llvm-objdump.cpp

2185

This should change as well, if you're not padding the first one.

2220

If you get a moment, could you either NFC change the variable names to all be capitalised. Either that, or 'C' should be 'c'

[paulsemel]

Added Jame's suggestions.

[paulsemel]

Alright :)
The fact is that this error is often occurring on the first entry when we create the archive file with GNU ar (I don't know why, I have to admit it). So, if we throw an error when we encounter this, we are simply not printing any entry of those files at all... (but the other entries are ok !)
That's actually what GNU objdump is doing. It is just reporting the file as not parsable in the archive, and going to the next file.
I think this is a good behavior. What do you think ?

tools/llvm-objdump/llvm-objdump.cpp

2144–2145

No, the fact is that, sometimes, only the first entry is ill-formed, and we still want to print the remaining part of the archive. I first did an error_report, but this leads to a non-functional option.
I think we should let it like this.

2220

let's remain this lowercase for this patch, I will do another one to correct all the variables in the file.

[jhenderson]

In D48904#1152494, @paulsemel wrote:

Alright :)
The fact is that this error is often occurring on the first entry when we create the archive file with GNU ar (I don't know why, I have to admit it). So, if we throw an error when we encounter this, we are simply not printing any entry of those files at all... (but the other entries are ok !)
That's actually what GNU objdump is doing. It is just reporting the file as not parsable in the archive, and going to the next file.
I think this is a good behavior. What do you think ?

Okay, that makes sense to me. Does GNU objdump print it on stdout, along with the rest of the information, or stderr? We should do whatever it does. We should also test it - I think a good way to do so would be to add a non-object (e.g. a text file) to the archive used for the main test, and show that the "ill-formed" message is printed, but then the rest of the check continues. What do you think?

test/tools/llvm-objdump/archive-headers-disas.test

3

I'd add before each CHECK group a check showing the file name/format line (e.g. "test.a(test.o): file format ELF64-x86-64).

6–11

I'd probably omit the actual disassembly part of the CHECK to make it less fragile to minor changes in how it is printed. It probably makes sense to just keep the first three CHECKs for each member (i.e. the --archive-headers line, the "Disassembly of..." line and the symbol name.

test/tools/llvm-objdump/archive-headers.test

4

Similar to the other test, I think it's worth checking the file name line that precedes each dump.

tools/llvm-objdump/llvm-objdump.cpp

212

This help text needs updating to remove reference to Mach-O.

[paulsemel]

Ok, so it appears that I can't trigger the error anymore, because I move the call to printArchiveChild to the DumpObject function.
In the DumpArchive function, when the object is ill-formed, we are just ignoring and going to the next file.
I think we can let it like this, as it is barely not possible to test the behavior now..

[jhenderson]

In D48904#1152865, @paulsemel wrote:

Ok, so it appears that I can't trigger the error anymore, because I move the call to printArchiveChild to the DumpObject function.
In the DumpArchive function, when the object is ill-formed, we are just ignoring and going to the next file.
I think we can let it like this, as it is barely not possible to test the behavior now..

Okay, that's fine. I'm not sure if I agree with the DumpArchive function ignoring errors completely, but that's not relevant to this change, so don't worry about it.

I've got one more question, and one small request, then I think this will be good to go on.

test/tools/llvm-objdump/archive-headers-disas.test

6–11

Sorry, maybe I confused you - I meant each member should have a disassembly check, so the later members should have it just like the earlier ones, but only the first part, like you've done here. Otherwise, it looks like the first few cases are different and special.

tools/llvm-objdump/llvm-objdump.cpp

2230–2231

Maybe a silly question, but why is this case special? Why not always print the double new line?

[paulsemel]

test/tools/llvm-objdump/archive-headers-disas.test

6–11

If there is not disassembly part, that means that there is nothing to disassemble :)
I think just putting this line for the disassembly part is ok, as this is not what we are checking here (but this still ensures that both options are working correctly together).

tools/llvm-objdump/llvm-objdump.cpp

2230–2231

When combined with other options, GNU objdump is not putting a new line, and effectively I find it nicer without it. What do you think ?

[jhenderson]

test/tools/llvm-objdump/archive-headers-disas.test

6–11

Sorry, just to be clear, do the other members have no text section then?

tools/llvm-objdump/llvm-objdump.cpp

2230–2231

Okay, that does sound sensible, but would you mind showing what you mean a bit more, with some output from both versions? Why is ArchiveHeaders special, and not, say, Disassemble?

[paulsemel]

Put back the two newlines

test/tools/llvm-objdump/archive-headers-disas.test

6–11

Yes that there is not code sections (you can test with objdump, the output is the same :) )

tools/llvm-objdump/llvm-objdump.cpp

2230–2231

Ok, I double checked, and it seems that GNU objdump is putting the two newlines too.. let's stick with what was here before then :)

[jhenderson]

Okay, LGTM. Go ahead and land when you're ready.

[paulsemel]

Arf.. Again, I forgot to mention the revision in the commit.. I'll try to think about it next time :)

[paulsemel]

Arf.. @jhenderson forgot that the time was timezone dependent.. can I push a patch ?

[jhenderson]

In D48904#1153191, @paulsemel wrote:

Arf.. @jhenderson forgot that the time was timezone dependent.. can I push a patch ?

Go ahead.

I assume you're going to use some form of regex? It would be good to not just do {{.*}}, but rather to do one that matches the format, if the format is consistent between timezone settings.

[paulsemel]

Ok, for an unknown reason, the disassembly part is not working on some bots..
I removed it, but will work on it tomorrow.
Again, I'm really sorry about it..

test/tools/llvm-objdump/archive-headers-disas.test

+# RUN: llvm-objdump -a -d %p/Inputs/liblong_filenames.a | FileCheck %s
+
+# CHECK: rw-r--r-- 204299/200   1416 Tue Oct 30 15:33:29 2012 1.o
+# CHECK: Disassembly of section .text:
+# CHECK: main:
+# CHECK:        0:  55  pushq %rbp
+# CHECK:        1:  48 89 e5  movq  %rsp, %rbp
+# CHECK:        4:  b8 00 00 00 00  movl  $0, %eax
+# CHECK:        9:  e8 00 00 00 00  callq 0 <main+0xe>
+# CHECK:        e:  5d  popq  %rbp
+# CHECK:        f:  c3  retq
+
+# CHECK: rw-r--r-- 204299/200   1224 Tue Oct 30 15:33:29 2012 2.o
+# CHECK: Disassembly of section .text:
+# CHECK: fn1:
+# CHECK:        0:  55  pushq %rbp
+# CHECK:        1:  48 89 e5  movq  %rsp, %rbp
+# CHECK:        4:  b8 00 00 00 00  movl  $0, %eax
+# CHECK:        9:  5d  popq  %rbp
+# CHECK:        a:  c3  retq
+
+# CHECK: rw-r--r-- 204299/200   1312 Tue Oct 30 15:33:29 2012 3.o
+# CHECK: Disassembly of section .text:
+# CHECK: fn3:
+# CHECK:        0:  55  pushq %rbp
+# CHECK:        1:  48 89 e5  movq  %rsp, %rbp
+# CHECK:        4:  b8 00 00 00 00  movl  $0, %eax
+# CHECK:        9:  5d  popq  %rbp
+# CHECK:        a:  c3  retq
+# CHECK: fn1:
+# CHECK:        b:  55  pushq %rbp
+# CHECK:        c:  48 89 e5  movq  %rsp, %rbp
+# CHECK:        f:  5d  popq  %rbp
+# CHECK:       10:  c3  retq
+
+# CHECK: rw-r--r-- 204299/200    957 Tue Oct 30 15:33:29 2012 4.o
+# CHECK: rw-r--r-- 204299/200    951 Tue Oct 30 15:33:29 2012 5.o
+# CHECK: rw-r--r-- 204299/200    951 Tue Oct 30 15:33:29 2012 6.o
+# CHECK: rw-r--r-- 204299/200    977 Tue Oct 30 15:33:29 2012 abcdefghijklmnopqrstuvwxyz1.o
+
+# CHECK: rw-r--r-- 204299/200   1272 Tue Oct 30 15:33:29 2012 abcdefghijklmnopqrstuvwxyz2.o
+# CHECK: Disassembly of section .text:
+# CHECK: bda:
+# CHECK:        0:  55  pushq %rbp
+# CHECK:        1:  48 89 e5  movq  %rsp, %rbp
+# CHECK:        4:  b8 00 00 00 00  movl  $0, %eax
+# CHECK:        9:  5d  popq  %rbp
+# CHECK:        a:  c3  retq
+
+# CHECK: rw-r--r-- 204299/200    977 Tue Oct 30 15:29:33 2012 abcdefghijklmnopq.o

test/tools/llvm-objdump/archive-headers.test

+# RUN: llvm-objdump -a %p/Inputs/liblong_filenames.a | FileCheck %s
+# RUN: llvm-objdump -archive-headers %p/Inputs/liblong_filenames.a | FileCheck %s
+
+# CHECK: rw-r--r-- 204299/200  1416 Tue Oct 30 15:33:29 2012 1.o
+# CHECK: rw-r--r-- 204299/200  1224 Tue Oct 30 15:33:29 2012 2.o
+# CHECK: rw-r--r-- 204299/200  1312 Tue Oct 30 15:33:29 2012 3.o
+# CHECK: rw-r--r-- 204299/200   957 Tue Oct 30 15:33:29 2012 4.o
+# CHECK: rw-r--r-- 204299/200   951 Tue Oct 30 15:33:29 2012 5.o
+# CHECK: rw-r--r-- 204299/200   951 Tue Oct 30 15:33:29 2012 6.o
+# CHECK: rw-r--r-- 204299/200   977 Tue Oct 30 15:33:29 2012 abcdefghijklmnopqrstuvwxyz1.o
+# CHECK: rw-r--r-- 204299/200  1272 Tue Oct 30 15:33:29 2012 abcdefghijklmnopqrstuvwxyz2.o
+# CHECK: rw-r--r-- 204299/200   977 Tue Oct 30 15:29:33 2012 abcdefghijklmnopq.o

tools/llvm-objdump/MachODump.cpp

static cl::opt<bool> FullLeadingAddr("full-leading-addr",
                                      cl::desc("Print full leading address"));
 
 static cl::opt<bool> NoLeadingHeaders("no-leading-headers",
                                       cl::desc("Print no leading headers"));
 
 cl::opt<bool> llvm::UniversalHeaders("universal-headers",
                                      cl::desc("Print Mach-O universal headers "
                                               "(requires -macho)"));
 
 cl::opt<bool>
-    llvm::ArchiveHeaders("archive-headers",
-                         cl::desc("Print archive headers for Mach-O archives "
-                                  "(requires -macho)"));
-
-cl::opt<bool>
     ArchiveMemberOffsets("archive-member-offsets",
                          cl::desc("Print the offset to each archive member for "
                                   "Mach-O archives (requires -macho and "
                                   "-archive-headers)"));
 
 cl::opt<bool>
     llvm::IndirectSymbols("indirect-symbols",
                           cl::desc("Print indirect symbol table for Mach-O "

tools/llvm-objdump/llvm-objdump.cpp

 cl::opt<bool> llvm::FileHeaders(
     "file-headers",
     cl::desc("Display the contents of the overall file header"));
 
 static cl::alias FileHeadersShort("f", cl::desc("Alias for --file-headers"),
                                   cl::aliasopt(FileHeaders));
 
 cl::opt<bool>
+    llvm::ArchiveHeaders("archive-headers",
+                         cl::desc("Print archive headers for Mach-O archives "));
+
+cl::alias
+ArchiveHeadersShort("a", cl::desc("Alias for --archive-headers"),
+                    cl::aliasopt(ArchiveHeaders));
+
+cl::opt<bool>
     llvm::PrintImmHex("print-imm-hex",
                       cl::desc("Use hex format for immediate values"));
 
 cl::opt<bool> PrintFaultMaps("fault-map-section",
                              cl::desc("Display contents of faultmap section"));
 
 cl::opt<DIDumpType> llvm::DwarfDumpType(
     "dwarf", cl::init(DIDT_Null), cl::desc("Dump of dwarf debug sections:"),
if (o->isMachO()) {
     return;
   }
   report_error(o->getFileName(), "Invalid/Unsupported object file format");
 }
 
 static void printFileHeaders(const ObjectFile *o) {
   if (!o->isELF() && !o->isCOFF())
     report_error(o->getFileName(), "Invalid/Unsupported object file format");
 
   Triple::ArchType AT = o->getArch();
   outs() << "architecture: " << Triple::getArchTypeName(AT) << "\n";
   Expected<uint64_t> StartAddrOrErr = o->getStartAddress();
   if (!StartAddrOrErr)
     report_error(o->getFileName(), StartAddrOrErr.takeError());
   outs() << "start address: "
          << format("0x%0*x", o->getBytesInAddress(), StartAddrOrErr.get())
          << "\n";
 }
 
-static void DumpObject(ObjectFile *o, const Archive *a = nullptr) {
+static void printArchiveChild(StringRef Filename, const Archive::Child &C) {
+  Expected<sys::fs::perms> ModeOrErr = C.getAccessMode();
+  if (!ModeOrErr) {
+    consumeError(ModeOrErr.takeError());
+    outs() << "ill-formed archive entry.\n";
+    return;
+  }
+  sys::fs::perms Mode = ModeOrErr.get();
+  outs() << ((Mode & sys::fs::owner_read) ? "r" : "-");
+  outs() << ((Mode & sys::fs::owner_write) ? "w" : "-");
+  outs() << ((Mode & sys::fs::owner_exe) ? "x" : "-");
+  outs() << ((Mode & sys::fs::group_read) ? "r" : "-");
+  outs() << ((Mode & sys::fs::group_write) ? "w" : "-");
+  outs() << ((Mode & sys::fs::group_exe) ? "x" : "-");
+  outs() << ((Mode & sys::fs::others_read) ? "r" : "-");
+  outs() << ((Mode & sys::fs::others_write) ? "w" : "-");
+  outs() << ((Mode & sys::fs::others_exe) ? "x" : "-");
+
+  outs() << " ";
+
+  Expected<unsigned> UIDOrErr = C.getUID();
+  if (!UIDOrErr)
+    report_error(Filename, UIDOrErr.takeError());
+  unsigned UID = UIDOrErr.get();
+  outs() << format("%d/", UID);
+
+  Expected<unsigned> GIDOrErr = C.getGID();
+  if (!GIDOrErr)
+    report_error(Filename, GIDOrErr.takeError());
+  unsigned GID = GIDOrErr.get();
+  outs() << format("%-d ", GID);
+
+  Expected<uint64_t> Size = C.getRawSize();
+  if (!Size)
+    report_error(Filename, Size.takeError());
+  outs() << format("%6" PRId64, Size.get()) << " ";
+
+  StringRef RawLastModified = C.getRawLastModified();
+  unsigned Seconds;
+  if (RawLastModified.getAsInteger(10, Seconds))
+    outs() << "(date: \"" << RawLastModified
+           << "\" contains non-decimal chars) ";
+  else {
+    // Since ctime(3) returns a 26 character string of the form:
+    // "Sun Sep 16 01:03:52 1973\n\0"
+    // just print 24 characters.
+    time_t t = Seconds;
+    outs() << format("%.24s ", ctime(&t));
+  }
+
+  StringRef Name = "";
+  Expected<StringRef> NameOrErr = C.getName();
+  if (!NameOrErr) {
+    consumeError(NameOrErr.takeError());
+    Expected<StringRef> RawNameOrErr = C.getRawName();
+    if (!RawNameOrErr)
+      report_error(Filename, NameOrErr.takeError());
+    Name = RawNameOrErr.get();
+  } else {
+    Name = NameOrErr.get();
+  }
+  outs() << Name << "\n";
+}
+
+static void DumpObject(ObjectFile *o, const Archive *a = nullptr,
+                       const Archive::Child *c = nullptr) {
   StringRef ArchiveName = a != nullptr ? a->getFileName() : "";
   // Avoid other output when using a raw option.
   if (!RawClangAST) {
     outs() << '\n';
     if (a)
       outs() << a->getFileName() << "(" << o->getFileName() << ")";
     else
       outs() << o->getFileName();
-    outs() << ":\tfile format " << o->getFileFormatName() << "\n\n";
+    outs() << ":\tfile format " << o->getFileFormatName() << "\n";
+    if (!ArchiveHeaders || MachOOpt)
+      outs() << "\n";
   }
 
+  if (ArchiveHeaders && !MachOOpt)
+    printArchiveChild(a->getFileName(), *c);
   if (Disassemble)
     DisassembleObject(o, Relocations);
   if (Relocations && !Disassemble)
     PrintRelocations(o);
   if (DynamicRelocations)
     PrintDynamicRelocations(o);
   if (SectionHeaders)
     PrintSectionHeaders(o);
if (DwarfDumpType != DIDT_Null) {
     std::unique_ptr<DIContext> DICtx = DWARFContext::create(*o);
     // Dump the complete DWARF structure.
     DIDumpOptions DumpOpts;
     DumpOpts.DumpType = DwarfDumpType;
     DICtx->dump(outs(), DumpOpts);
   }
 }
 
-static void DumpObject(const COFFImportFile *I, const Archive *A) {
+static void DumpObject(const COFFImportFile *I, const Archive *A,
+                       const Archive::Child *C = nullptr) {
   StringRef ArchiveName = A ? A->getFileName() : "";
 
   // Avoid other output when using a raw option.
   if (!RawClangAST)
     outs() << '\n'
            << ArchiveName << "(" << I->getFileName() << ")"
            << ":\tfile format COFF-import-file"
            << "\n\n";
 
+  if (ArchiveHeaders && !MachOOpt)
+    printArchiveChild(A->getFileName(), *C);
   if (SymbolTable)
     printCOFFSymbolTable(I);
 }
 
 /// Dump each object file in \a a;
 static void DumpArchive(const Archive *a) {
   Error Err = Error::success();
   for (auto &C : a->children(Err)) {
     Expected<std::unique_ptr<Binary>> ChildOrErr = C.getAsBinary();
     if (!ChildOrErr) {
       if (auto E = isNotObjectErrorInvalidFileType(ChildOrErr.takeError()))
         report_error(a->getFileName(), C, std::move(E));
       continue;
     }
     if (ObjectFile *o = dyn_cast<ObjectFile>(&*ChildOrErr.get()))
-      DumpObject(o, a);
+      DumpObject(o, a, &C);
     else if (COFFImportFile *I = dyn_cast<COFFImportFile>(&*ChildOrErr.get()))
-      DumpObject(I, a);
+      DumpObject(I, a, &C);
     else
       report_error(a->getFileName(), object_error::invalid_file_type);
   }
   if (Err)
     report_error(a->getFileName(), std::move(Err));
 }
 
 /// Open file and figure out how to dump it.
if (!Disassemble
       && !FirstPrivateHeader
       && !ExportsTrie
       && !Rebase
       && !Bind
       && !LazyBind
       && !WeakBind
       && !RawClangAST
       && !(UniversalHeaders && MachOOpt)
-      && !(ArchiveHeaders && MachOOpt)
+      && !ArchiveHeaders
       && !(IndirectSymbols && MachOOpt)
       && !(DataInCode && MachOOpt)
       && !(LinkOptHints && MachOOpt)
       && !(InfoPlist && MachOOpt)
       && !(DylibsUsed && MachOOpt)
       && !(DylibId && MachOOpt)
       && !(ObjcMetaData && MachOOpt)
       && !(FilterSections.size() != 0 && MachOOpt)