[PATCH] [llvm-symbolizer] Introduce the -dsym-hint option.

[Alexey Samsonov]

I'm not sure if adding user-facing Verbosity option is a good idea, in this patch it looks merely like a debugging aid.

If we decide to introduce it, I'd prefer a separate patch, with a careful audit of all existing code and reasonable decisions of what and when to print (for instance, it makes sense to print our steps in attempt to resolve .gnu_debuglink).

================
Comment at: docs/CommandGuide/llvm-symbolizer.rst:18
@@ -17,1 +17,3 @@
 
+OPTIONS
+-------
----------------
Existing options are already listed below (after EXAMPLE)

================
Comment at: docs/CommandGuide/llvm-symbolizer.rst:31
@@ +30,3 @@
+
+ Path to .dSYM bundle to search for debug info for the object files.
+
----------------
Please specify that this flag may be provided multiple times and that it's relevant only for Mac OS.

================
Comment at: tools/llvm-symbolizer/LLVMSymbolize.cpp:216
@@ +215,3 @@
+// 
+// If the .dSYM bundle name is different from the executable name
+static
----------------
Looks like unfinished sentence.

================
Comment at: tools/llvm-symbolizer/LLVMSymbolize.cpp:303
@@ -295,5 +302,3 @@
 
-LLVMSymbolizer::BinaryPair
-LLVMSymbolizer::getOrCreateBinary(const std::string &Path) {
-  BinaryMapTy::iterator I = BinaryForPath.find(Path);
-  if (I != BinaryForPath.end())
+ErrorOr<MachO::uuid_command> getUuidFromObjectFile(const ObjectFile *Obj) {
+  const MachOObjectFile *file = dyn_cast<const MachOObjectFile>(Obj);
----------------
Looks like this routine is generally usable and should reside somewhere in libLLVMObject. At the very least, add a FIXME about it.

================
Comment at: tools/llvm-symbolizer/LLVMSymbolize.cpp:304
@@ +303,3 @@
+ErrorOr<MachO::uuid_command> getUuidFromObjectFile(const ObjectFile *Obj) {
+  const MachOObjectFile *file = dyn_cast<const MachOObjectFile>(Obj);
+  MachOObjectFile::LoadCommandInfo Command = file->getFirstLoadCommandInfo();
----------------
dyn_cast may return null. Either handle this, or take MachOObjectFile as an input argument.

================
Comment at: tools/llvm-symbolizer/LLVMSymbolize.cpp:319
@@ +318,3 @@
+
+bool darwinDsymMatchesBinary(const ObjectFile *DbgObj, const ObjectFile *Obj) {
+  ErrorOr<MachO::uuid_command> dbg_uuid_or_error = getUuidFromObjectFile(DbgObj);
----------------
static

================
Comment at: tools/llvm-symbolizer/LLVMSymbolize.cpp:328
@@ +327,3 @@
+
+void LLVMSymbolizer::populateDsymPaths(const std::string &ExePath,
+                                       std::vector<std::string> &OutPaths) {
----------------
I still think this should be a static helper, which should take Opts.DsymHints as an argument. Also, consider checking --dsym_hint values in the main binary (probably, even reporting an error is incorrect value is passed there), so that "Opts.DsymHints" are always reasonable.

================
Comment at: tools/llvm-symbolizer/LLVMSymbolize.cpp:330
@@ +329,3 @@
+                                       std::vector<std::string> &OutPaths) {
+  StringRef filename = sys::path::filename(ExePath);
+  const std::string &resource_path =
----------------
 Variable names should start with capital letter.

================
Comment at: tools/llvm-symbolizer/LLVMSymbolize.cpp:334
@@ +333,3 @@
+  OutPaths.push_back(resource_path);
+  for (auto &path : Opts.DsymHints) {
+    if (sys::path::extension(path) == ".dSYM")
----------------
const auto &path

================
Comment at: tools/llvm-symbolizer/LLVMSymbolize.cpp:345
@@ +344,3 @@
+  // resource directory.
+  std::vector<std::string> dsym_paths;
+  populateDsymPaths(ExePath, dsym_paths);
----------------
DsymPaths

================
Comment at: tools/llvm-symbolizer/LLVMSymbolize.cpp:361
@@ +360,3 @@
+        return DbgObj; 
+      } else {
+        if (Opts.Verbose)
----------------
just use "else if" here.

================
Comment at: tools/llvm-symbolizer/LLVMSymbolize.cpp:367
@@ +366,3 @@
+      if (Opts.Verbose)
+        errs() << "Error: no such file\n";
+    }
----------------
You can use EC.message() - this can be a different kind of error.

================
Comment at: tools/llvm-symbolizer/LLVMSymbolize.cpp:379
@@ -300,3 +378,3 @@
     return I->second;
   Binary *Bin = nullptr;
   Binary *DbgBin = nullptr;
----------------
Please get rid of Bin/DbgBin here, use local variables with small scope where appropriate instead.

================
Comment at: tools/llvm-symbolizer/LLVMSymbolize.cpp:387
@@ -306,2 +386,3 @@
     // Check if it's a universal binary.
     Bin = ParsedBinary.getBinary().get();
+    Obj = getObjectFileFromBinary(Bin, ArchName);
----------------
So, you have this sequence scattered throughout the code (at least three times):
  OwningBinary<Binary> B = std::move(BinaryOrErr.get());
  Bin = B.getBinary().get();
  Obj = getObjectFileFromBinary(Bin, ArchName);

Can it be simplified or moved to a function?




================
Comment at: tools/llvm-symbolizer/LLVMSymbolize.cpp:394
@@ -322,3 +393,3 @@
     // Try to locate the debug binary using .gnu_debuglink section.
     if (!DbgBin) {
       std::string DebuglinkName;
----------------
if (!DbgObj) ?

================
Comment at: tools/llvm-symbolizer/LLVMSymbolize.h:67
@@ -64,2 +66,3 @@
 private:
   typedef std::pair<Binary*, Binary*> BinaryPair;
+  typedef std::pair<ObjectFile*, ObjectFile*> ObjectPair;
----------------
BinaryPair is unused now.

================
Comment at: tools/llvm-symbolizer/LLVMSymbolize.h:99
@@ -91,1 +98,3 @@
   ObjectFileForArchMapTy ObjectFileForArch;
+  typedef std::map<std::pair<std::string, std::string>, ObjectPair>
+      ObjectPairForPathArchTy;
----------------
typedef is not really needed now in C++11 world. ObjectPairForPathArchTy::iterator can be replaced with auto.

http://reviews.llvm.org/D5705