[PATCH] D48810: [llvm-objdump] Add --file-headers (-f) option

[Paul Semel via Phabricator via llvm-commits]

paulsemel added a comment.

About the other informations: I think they are more "format specific" and thus I wanted to add an other patch for it. Do you want me to do it in this patch ?



================
Comment at: lib/Object/COFFObjectFile.cpp:916
+    return PE32Header->AddressOfEntryPoint;
+  return errorCodeToError(object_error::parse_failed);
+}
----------------
jhenderson wrote:
> It looks like GNU objdump prints 0 rather than failing in this case.
Yes, I figured it out this night.. I really don't know why. Do you think we might follow the behavior ?


================
Comment at: test/tools/llvm-objdump/file-headers.test:1
+# RUN: llvm-objdump -f %p/Inputs/common-symbol-elf | FileCheck %s
+
----------------
jhenderson wrote:
> I must admit that I'm surprised we're not using yaml2obj to test here. I am not familiar with llvm-objdump, and I see that using pre-canned binaries is normal for its tests, but how do you feel about using yaml2obj instead?
> 
> Assuming you'd prefer to use pre-canned binaries, is it normal in the tests to use files that aren't specifically for that test?
> 
> There should also probably be separate elf and coff test cases.
Yes, I anyway definitely need to add a COFF test. I can use yaml2obj here, this is not a problem for me, but as a lot of tests in this tool are doing this way, I just didn't want to break the move :)


================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:2135
+  if (!o->isELF() && !o->isCOFF()) {
+    outs() << "not implemented. \n";
+    return;
----------------
jhenderson wrote:
> Nit: Is there a need for the trailing whitespace in the output?
Absolutely not, fixing it !


================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:2287
     Disassemble = true;
-  if (!Disassemble
-      && !Relocations
-      && !DynamicRelocations
-      && !SectionHeaders
-      && !SectionContents
-      && !SymbolTable
-      && !UnwindInfo
-      && !PrivateHeaders
-      && !FirstPrivateHeader
-      && !ExportsTrie
-      && !Rebase
-      && !Bind
-      && !LazyBind
-      && !WeakBind
-      && !RawClangAST
-      && !(UniversalHeaders && MachOOpt)
-      && !(ArchiveHeaders && MachOOpt)
-      && !(IndirectSymbols && MachOOpt)
-      && !(DataInCode && MachOOpt)
-      && !(LinkOptHints && MachOOpt)
-      && !(InfoPlist && MachOOpt)
-      && !(DylibsUsed && MachOOpt)
-      && !(DylibId && MachOOpt)
-      && !(ObjcMetaData && MachOOpt)
-      && !(FilterSections.size() != 0 && MachOOpt)
-      && !PrintFaultMaps
-      && DwarfDumpType == DIDT_Null) {
+  if (!Disassemble && !Relocations && !DynamicRelocations && !SectionHeaders &&
+      !SectionContents && !SymbolTable && !UnwindInfo && !PrivateHeaders &&
----------------
jhenderson wrote:
> Assuming this is what clang-format does, I suppose that this is right, but it definitely does make it a lot harder to follow :-(
Woops, shouldn't have pushed this... will go back on this modification no worries :)


Repository:
  rL LLVM

https://reviews.llvm.org/D48810