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

[Alexander Potapenko]

Addressed the comments.
WDYT is better: a) commit the binaries and .dSYM files for testing to the repository, or b) generate them on the fly?

================
Comment at: docs/CommandGuide/llvm-symbolizer.rst:18
@@ -17,1 +17,3 @@
 
+OPTIONS
+-------
----------------
samsonov wrote:
> Existing options are already listed below (after EXAMPLE)
Indeed. Sorry, I've missed those.

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

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

================
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);
----------------
samsonov wrote:
> Looks like this routine is generally usable and should reside somewhere in libLLVMObject. At the very least, add a FIXME about it.
Sent http://reviews.llvm.org/D5752 for review

================
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();
----------------
samsonov wrote:
> dyn_cast may return null. Either handle this, or take MachOObjectFile as an input argument.
Done

================
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);
----------------
samsonov wrote:
> static
Done

================
Comment at: tools/llvm-symbolizer/LLVMSymbolize.cpp:328
@@ +327,3 @@
+
+void LLVMSymbolizer::populateDsymPaths(const std::string &ExePath,
+                                       std::vector<std::string> &OutPaths) {
----------------
samsonov wrote:
> 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.
I've moved the check for ".dSYM" extension to the main binary, so this function has become really small now.
I think it's ok to make it a private member, or otherwise I can move it into lookUpDsymFile.

================
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 =
----------------
samsonov wrote:
>  Variable names should start with capital letter.
Done (hope I've fixed them all)

================
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")
----------------
samsonov wrote:
> const auto &path
done

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

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

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

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

================
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);
----------------
samsonov wrote:
> 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?
> 
> 
> 
I've tried to compactify these a bit.
But moving just these three lines into a separate function doesn't work because B is passed to addOwningBinary() later.
However it's not always correct to include addOwningBinary() into that function, because the former isn't called after every sequence you're talking about.
We won't get much from moving two of these three lines into a function OTOH.

================
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;
----------------
samsonov wrote:
> if (!DbgObj) ?
Fixed

================
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;
----------------
samsonov wrote:
> BinaryPair is unused now.
Removed.

================
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;
----------------
samsonov wrote:
> typedef is not really needed now in C++11 world. ObjectPairForPathArchTy::iterator can be replaced with auto.
Removed typedefs except for ObjectPair which is used in several class members' declarations.

http://reviews.llvm.org/D5705