[llvm-objdump/mac] Add new function starts print mode

[keith]

Summary

This updates the --function-starts argument to now accept 3 different
modes, addrs for just printing the addresses of the function starts
(previous behavior), names for just printing the names of the function
starts, and both to print them both side by side.

In general if you're debugging function starts issues it's useful to see
the symbol name alongside the address. This also mirrors Apple's
dyldinfo -function_starts command which prints both.

[keith]

Update docs

[keith]

Rebase

[jhenderson]

I'm not a Mach-O person, but I've got some nitpick comments on the general area.

llvm/docs/CommandGuide/llvm-objdump.rst

359 ↗

(On Diff #465483)

In other tools (I'm looking at llvm-symbolizer specifically at least), the format would be --function-starts [=<addrs|names|both>], to indicate a) the value is optional, and b) what the possible values are.

361 ↗

(On Diff #465483)

I'm not a Mach-O developer, and so am not particular familair with the object format. Is it obvious to someone who is familiar what the meaning of the addrs/names options actually is?

llvm/tools/llvm-objdump/llvm-objdump.h

16

I don't think you need the include. You can forward declare opt::Arg, I believe?

[keith]

Improve documentation, remove unnecessary import

[jhenderson]

Okay, no more comments from me. The rest will need reviewing by someone with more knowledge of the area.

[smeenai]

Generally looks good to me. Adding Nico in case he has any feedback as well.

llvm/tools/llvm-objdump/MachODump.cpp

1101

So if we can't find the address associated with a symbol, we don't print anything for it? Does that match dyldinfo's behavior? Should we have a test for it?

llvm/tools/llvm-objdump/MachODump.h

35

Nit: I think an enum class would be nicer here.

[keith]

Add test for empty behavior

[keith]

New test requires https://reviews.llvm.org/D135988

llvm/tools/llvm-objdump/MachODump.cpp

1101

So turns out that dyldinfo prints a ? beside the address in this case. I've mirrored that here and added a test, but in the case you only ask for names I stilled opted to print nothing since it doesn't seem like a ton of ?'s w/o any other info would be very useful output, and if you have _some_ symbols it might distract you from those. Interested to hear thoughts on this since names only not a mode dyldinfo has.

[smeenai]

LGTM

llvm/tools/llvm-objdump/MachODump.cpp

1101

Yeah, that sounds reasonable to me.

llvm/tools/llvm-objdump/MachODump.h

35

Sorry for being nitty again, but you can remove the FS prefix now that it's an enum class (which was the motivation for my suggestion).

[keith]

Remove unnecessary enum type prefix

llvm/tools/llvm-objdump/MachODump.h

35

good point, thanks!

llvm/test/tools/llvm-objdump/MachO/function-starts.test

 ## This test verifies that llvm-objdump correctly prints function starts data.
 
-RUN: llvm-objdump --macho --function-starts %p/Inputs/hello.exe.macho-i386 | FileCheck %s --check-prefix=32-BIT
+RUN: llvm-objdump --macho --function-starts %p/Inputs/hello.exe.macho-i386 | FileCheck %s --check-prefix=32-BIT --implicit-check-not=_main
+RUN: llvm-objdump --macho --function-starts=addrs %p/Inputs/hello.exe.macho-i386 | FileCheck %s --check-prefix=32-BIT --implicit-check-not=_main
 32-BIT: 00001f40
 
-RUN: llvm-objdump --macho --function-starts %p/Inputs/hello.exe.macho-x86_64 | FileCheck %s --check-prefix=64-BIT
+RUN: llvm-objdump --macho --function-starts=names %p/Inputs/hello.exe.macho-i386 | FileCheck %s --check-prefix=32-BIT-NAMES
+32-BIT-NAMES: {{^}}_main
+
+RUN: llvm-objdump --macho --function-starts=both %p/Inputs/hello.exe.macho-i386 | FileCheck %s --check-prefix=32-BIT-BOTH
+32-BIT-BOTH: 00001f40 _main
+
+RUN: llvm-objdump --macho --function-starts %p/Inputs/hello.exe.macho-x86_64 | FileCheck %s --check-prefix=64-BIT --implicit-check-not=_main
+RUN: llvm-objdump --macho --function-starts=addrs %p/Inputs/hello.exe.macho-x86_64 | FileCheck %s --check-prefix=64-BIT --implicit-check-not=_main
 64-BIT: 0000000100000f30
+
+RUN: llvm-objdump --macho --function-starts=names %p/Inputs/hello.exe.macho-x86_64 | FileCheck %s --check-prefix=64-BIT-NAMES
+64-BIT-NAMES: {{^}}_main
+
+RUN: llvm-objdump --macho --function-starts=both %p/Inputs/hello.exe.macho-x86_64 | FileCheck %s --check-prefix=64-BIT-BOTH
+64-BIT-BOTH: 0000000100000f30 _main

llvm/tools/llvm-objdump/MachODump.h

 namespace opt {
 class InputArgList;
 } // namespace opt
 
 namespace objdump {
 
 void parseMachOOptions(const llvm::opt::InputArgList &InputArgs);
 
+enum FunctionStartsMode { FSAddrs, FSNames, FSBoth, FSNone };
+
 // MachO specific options
 extern bool Bind;
 extern bool DataInCode;
 extern std::string DisSymName;
 extern bool DylibId;
 extern bool DylibsUsed;
 extern bool ExportsTrie;
 extern bool FirstPrivateHeader;
 extern bool FullLeadingAddr;
-extern bool FunctionStarts;
+extern FunctionStartsMode FunctionStartsType;
 extern bool IndirectSymbols;
 extern bool InfoPlist;
 extern bool LazyBind;
 extern bool LeadingHeaders;
 extern bool LinkOptHints;
 extern bool ObjcMetaData;
 extern bool Rebase;
 extern bool Rpaths;

llvm/tools/llvm-objdump/MachODump.cpp

 static bool UseDbg;
 static std::string DSYMFile;
 bool objdump::FullLeadingAddr;
 bool objdump::LeadingHeaders;
 bool objdump::UniversalHeaders;
 static bool ArchiveMemberOffsets;
 bool objdump::IndirectSymbols;
 bool objdump::DataInCode;
-bool objdump::FunctionStarts;
+FunctionStartsMode objdump::FunctionStartsType = FSNone;
 bool objdump::LinkOptHints;
 bool objdump::InfoPlist;
 bool objdump::DylibsUsed;
 bool objdump::DylibId;
 bool objdump::Verbose;
 bool objdump::ObjcMetaData;
 std::string objdump::DisSymName;
 bool objdump::SymbolicOperands;
void objdump::parseMachOOptions(const llvm::opt::InputArgList &InputArgs) {
   UseDbg = InputArgs.hasArg(OBJDUMP_g);
   DSYMFile = InputArgs.getLastArgValue(OBJDUMP_dsym_EQ).str();
   FullLeadingAddr = InputArgs.hasArg(OBJDUMP_full_leading_addr);
   LeadingHeaders = !InputArgs.hasArg(OBJDUMP_no_leading_headers);
   UniversalHeaders = InputArgs.hasArg(OBJDUMP_universal_headers);
   ArchiveMemberOffsets = InputArgs.hasArg(OBJDUMP_archive_member_offsets);
   IndirectSymbols = InputArgs.hasArg(OBJDUMP_indirect_symbols);
   DataInCode = InputArgs.hasArg(OBJDUMP_data_in_code);
-  FunctionStarts = InputArgs.hasArg(OBJDUMP_function_starts);
+  if (const opt::Arg *A = InputArgs.getLastArg(OBJDUMP_function_starts_EQ)) {
+    FunctionStartsType = StringSwitch<FunctionStartsMode>(A->getValue())
+                             .Case("addrs", FSAddrs)
+                             .Case("names", FSNames)
+                             .Case("both", FSBoth)
+                             .Default(FSNone);
+    if (FunctionStartsType == FSNone)
+      invalidArgValue(A);
+  }
   LinkOptHints = InputArgs.hasArg(OBJDUMP_link_opt_hints);
   InfoPlist = InputArgs.hasArg(OBJDUMP_info_plist);
   DylibsUsed = InputArgs.hasArg(OBJDUMP_dylibs_used);
   DylibId = InputArgs.hasArg(OBJDUMP_dylib_id);
   Verbose = !InputArgs.hasArg(OBJDUMP_non_verbose);
   ObjcMetaData = InputArgs.hasArg(OBJDUMP_objc_meta_data);
   DisSymName = InputArgs.getLastArgValue(OBJDUMP_dis_symname).str();
   SymbolicOperands = !InputArgs.hasArg(OBJDUMP_no_symbolic_operands);
for (const MachOObjectFile::LoadCommandInfo &LC : O->load_commands()) {
     if (LC.C.cmd == MachO::LC_FUNCTION_STARTS) {
       MachO::linkedit_data_command FunctionStartsLC =
           O->getLinkeditDataLoadCommand(LC);
       O->ReadULEB128s(FunctionStartsLC.dataoff, FunctionStarts);
       break;
     }
   }
 
+  DenseMap<uint64_t, StringRef> SymbolNames;
+  if (FunctionStartsType == FSNames || FunctionStartsType == FSBoth) {
+    for (SymbolRef Sym : O->symbols()) {
+      if (Expected<uint64_t> Addr = Sym.getAddress()) {
+        if (Expected<StringRef> Name = Sym.getName()) {
+          SymbolNames[*Addr] = *Name;
+        }
+      }
+    }
+  }
+
   for (uint64_t S : FunctionStarts) {
     uint64_t Addr = BaseSegmentAddress + S;
+    if (FunctionStartsType == FSNames) {
+      auto It = SymbolNames.find(Addr);
+      if (It != SymbolNames.end())
+        outs() << It->second << "\n";
+    } else {
       if (O->is64Bit())
-      outs() << format("%016" PRIx64, Addr) << "\n";
+        outs() << format("%016" PRIx64, Addr);
       else
-      outs() << format("%08" PRIx32, static_cast<uint32_t>(Addr)) << "\n";
+        outs() << format("%08" PRIx32, static_cast<uint32_t>(Addr));
+
+      if (FunctionStartsType == FSBoth) {
+        outs() << " " << SymbolNames.lookup(Addr);
+      }
+      outs() << "\n";
+    }
   }
 }
 
 static void PrintDataInCodeTable(MachOObjectFile *O, bool verbose) {
   MachO::linkedit_data_command DIC = O->getDataInCodeLoadCommand();
   uint32_t nentries = DIC.datasize / sizeof(struct MachO::data_in_code_entry);
   outs() << "Data in code table (" << nentries << " entries)\n";
   outs() << "offset     length kind\n";

 static void ProcessMachO(StringRef Name, MachOObjectFile *MachOOF,
                          StringRef ArchiveMemberName = StringRef(),
                          StringRef ArchitectureName = StringRef()) {
   // If we are doing some processing here on the Mach-O file print the header
   // info.  And don't print it otherwise like in the case of printing the
   // UniversalHeaders or ArchiveHeaders.
   if (Disassemble || Relocations || PrivateHeaders || ExportsTrie || Rebase ||
       Bind || SymbolTable || LazyBind || WeakBind || IndirectSymbols ||
-      DataInCode || FunctionStarts || LinkOptHints || DylibsUsed || DylibId ||
-      Rpaths || ObjcMetaData || (!FilterSections.empty())) {
+      DataInCode || FunctionStartsType != FSNone || LinkOptHints ||
+      DylibsUsed || DylibId || Rpaths || ObjcMetaData ||
+      (!FilterSections.empty())) {
     if (LeadingHeaders) {
       outs() << Name;
       if (!ArchiveMemberName.empty())
         outs() << '(' << ArchiveMemberName << ')';
       if (!ArchitectureName.empty())
         outs() << " (architecture " << ArchitectureName << ")";
       outs() << ":\n";
     }
if (MachOOF->getHeader().filetype == MachO::MH_KEXT_BUNDLE &&
       DisassembleMachO(FileName, MachOOF, "__TEXT_EXEC", "__text");
     else
       DisassembleMachO(FileName, MachOOF, "__TEXT", "__text");
   }
   if (IndirectSymbols)
     PrintIndirectSymbols(MachOOF, Verbose);
   if (DataInCode)
     PrintDataInCodeTable(MachOOF, Verbose);
-  if (FunctionStarts)
+  if (FunctionStartsType != FSNone)
     PrintFunctionStarts(MachOOF);
   if (LinkOptHints)
     PrintLinkOptHints(MachOOF);
   if (Relocations)
     PrintRelocations(MachOOF, Verbose);
   if (SectionHeaders)
     printSectionHeaders(MachOOF);
   if (SectionContents)

llvm/tools/llvm-objdump/ObjdumpOpts.td

 def indirect_symbols : Flag<["--"], "indirect-symbols">,
   HelpText<"Print indirect symbol table for Mach-O objects (requires --macho)">,
   Group<grp_mach_o>;
 
 def data_in_code : Flag<["--"], "data-in-code">,
   HelpText<"Print the data in code table for Mach-O objects (requires --macho)">,
   Group<grp_mach_o>;
 
-def function_starts : Flag<["--"], "function-starts">,
-  HelpText<"Print the function starts table for "
-           "Mach-O objects (requires --macho)">,
+def function_starts_EQ : Joined<["--"], "function-starts=">,
+  HelpText<"Print the function starts table for Mach-O objects. "
+           "Options: addrs (default), names, both (requires --macho)">,
+  Values<"addrs,names,both">,
   Group<grp_mach_o>;
 
+def : Flag<["--"], "function-starts">, Alias<function_starts_EQ>,
+  AliasArgs<["addrs"]>, Group<grp_mach_o>;
+
 def link_opt_hints : Flag<["--"], "link-opt-hints">,
   HelpText<"Print the linker optimization hints for "
            "Mach-O objects (requires --macho)">,
   Group<grp_mach_o>;
 
 def info_plist : Flag<["--"], "info-plist">,
   HelpText<"Print the info plist section as strings for "
            "Mach-O objects (requires --macho)">,

llvm/tools/llvm-objdump/llvm-objdump.h

 //===--- llvm-objdump.h -----------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_TOOLS_LLVM_OBJDUMP_LLVM_OBJDUMP_H
 #define LLVM_TOOLS_LLVM_OBJDUMP_LLVM_OBJDUMP_H
 
 #include "llvm/ADT/StringSet.h"
 #include "llvm/DebugInfo/DIContext.h"
 #include "llvm/MC/MCDisassembler/MCDisassembler.h"
 #include "llvm/Object/Archive.h"
+#include "llvm/Option/Arg.h"
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/DataTypes.h"
 
 namespace llvm {
 class StringRef;
 class Twine;
 
 namespace object {

 
 template <typename T, typename... Ts>
 T unwrapOrError(Expected<T> EO, Ts &&... Args) {
   if (EO)
     return std::move(*EO);
   reportError(EO.takeError(), std::forward<Ts>(Args)...);
 }
 
+void invalidArgValue(const llvm::opt::Arg *A);
+
 std::string getFileNameForError(const object::Archive::Child &C,
                                 unsigned Index);
 SymbolInfoTy createSymbolInfo(const object::ObjectFile *Obj,
                               const object::SymbolRef &Symbol);
 
 } // namespace objdump
 } // end namespace llvm
 
 #endif

llvm/tools/llvm-objdump/llvm-objdump.cpp

if (const opt::Arg *A = InputArgs.getLastArg(ID)) {
     if (!llvm::to_integer(V, Value, 0)) {
       reportCmdLineError(A->getSpelling() +
                          ": expected a non-negative integer, but got '" + V +
                          "'");
     }
   }
 }
 
-static void invalidArgValue(const opt::Arg *A) {
+void objdump::invalidArgValue(const opt::Arg *A) {
   reportCmdLineError("'" + StringRef(A->getValue()) +
                      "' is not a valid value for '" + A->getSpelling() + "'");
 }
 
 static std::vector<std::string>
 commaSeparatedValues(const llvm::opt::InputArgList &InputArgs, int ID) {
   std::vector<std::string> Values;
   for (StringRef Value : InputArgs.getAllArgValues(ID)) {
if (DisassembleAll || PrintSource || PrintLines ||
     Disassemble = true;
 
   if (!ArchiveHeaders && !Disassemble && DwarfDumpType == DIDT_Null &&
       !DynamicRelocations && !FileHeaders && !PrivateHeaders && !RawClangAST &&
       !Relocations && !SectionHeaders && !SectionContents && !SymbolTable &&
       !DynamicSymbolTable && !UnwindInfo && !FaultMapSection &&
       !(MachOOpt &&
         (Bind || DataInCode || DylibId || DylibsUsed || ExportsTrie ||
-         FirstPrivateHeader || FunctionStarts || IndirectSymbols || InfoPlist ||
-         LazyBind || LinkOptHints || ObjcMetaData || Rebase || Rpaths ||
-         UniversalHeaders || WeakBind || !FilterSections.empty()))) {
+         FirstPrivateHeader || FunctionStartsType != FSNone ||
+         IndirectSymbols || InfoPlist || LazyBind || LinkOptHints ||
+         ObjcMetaData || Rebase || Rpaths || UniversalHeaders || WeakBind ||
+         !FilterSections.empty()))) {
     T->printHelp(ToolName);
     return 2;
   }
 
   DisasmSymbolSet.insert(DisassembleSymbols.begin(), DisassembleSymbols.end());
 
   llvm::for_each(InputFilenames, dumpInput);
 
   warnOnNoMatchForSections();
 
   return EXIT_SUCCESS;
 }