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

[Alexander Potapenko]

================
Comment at: test/tools/llvm-symbolizer/Inputs/dsym-test.c:1
@@ +1,2 @@
+int main() {
+  return 0;
----------------
samsonov wrote:
> Please provide a comment with sequence of compiler / dsymutil invocations yielding the all the binaries you check in to Inputs/
Done

================
Comment at: test/tools/llvm-symbolizer/Inputs/dsym-test.c:2
@@ +1,3 @@
+int main() {
+  return 0;
+}
----------------
samsonov wrote:
> Do you need to add lit.local.cfg to this directory to exclude .c files from testsuite?
I think no, lit doesn't attempt to run this file.

================
Comment at: tools/llvm-symbolizer/LLVMSymbolize.cpp:101
@@ -99,3 +100,3 @@
   SymbolDesc SD = { Address, Address };
-  SymbolMapTy::const_iterator it = M.upper_bound(SD);
-  if (it == M.begin())
+  auto symbol_iterator = SymbolMap.upper_bound(SD);
+  if (symbol_iterator == SymbolMap.begin())
----------------
samsonov wrote:
> SymbolIterator
Fixed in r219682.

================
Comment at: tools/llvm-symbolizer/LLVMSymbolize.cpp:327
@@ +326,3 @@
+  ErrorOr<MachO::uuid_command> bin_uuid_or_error = Obj->findAndGetUuidCommand();
+  if (error(dbg_uuid_or_error.getError()) ||
+      error(bin_uuid_or_error.getError()))
----------------
samsonov wrote:
> Is it OK for a binary to not have UUID command? I think that if the object is well-formed, but doesn't have UUID command, you shouldn't use error() wrapper, as it prints error messages to stderr.
No, it's not ok. However this code has changed in the latest version of the patch.

================
Comment at: tools/llvm-symbolizer/LLVMSymbolize.cpp:334
@@ +333,3 @@
+
+void LLVMSymbolizer::populateDsymPaths(const std::string &ExePath,
+                                       std::vector<std::string> &OutPaths) {
----------------
samsonov wrote:
> Probably it's ok to even sink this function down to lookUpDsymFile (I don't see how it can be generally useful).
Done

================
Comment at: tools/llvm-symbolizer/LLVMSymbolize.cpp:337
@@ +336,3 @@
+  StringRef Filename = sys::path::filename(ExePath);
+  const std::string &resource_path =
+      getDarwinDWARFResourceForPath(ExePath, Filename);
----------------
samsonov wrote:
> ResourcePath (or just get rid of local variable).
Done

================
Comment at: tools/llvm-symbolizer/LLVMSymbolize.cpp:347
@@ +346,3 @@
+    const MachOObjectFile *MachExeObj, const std::string &ArchName) {
+  ObjectFile *result = nullptr;
+  // On Darwin we may find DWARF in separate object file in
----------------
samsonov wrote:
> This variable is not used.
Removed.

================
Comment at: tools/llvm-symbolizer/LLVMSymbolize.cpp:383
@@ +382,3 @@
+    Binary *Bin = B.getBinary().get();
+    Obj = getObjectFileFromBinary(Bin, ArchName);
+    addOwningBinary(std::move(B));
----------------
samsonov wrote:
> Please check that the code works if Obj is nullptr
It should. I've inserted a return statement just for the case.

================
Comment at: tools/llvm-symbolizer/LLVMSymbolize.cpp:386
@@ -309,12 +385,3 @@
     if (Bin->isMachO() || Bin->isMachOUniversalBinary()) {
-      // On Darwin we may find DWARF in separate object file in
-      // resource directory.
-      const std::string &ResourcePath =
-          getDarwinDWARFResourceForPath(Path);
-      BinaryOrErr = createBinary(ResourcePath);
-      std::error_code EC = BinaryOrErr.getError();
-      if (EC != errc::no_such_file_or_directory && !error(EC)) {
-        OwningBinary<Binary> B = std::move(BinaryOrErr.get());
-        DbgBin = B.getBinary().get();
-        addOwningBinary(std::move(B));
-      }
+      const MachOObjectFile *MachObj = dyn_cast<const MachOObjectFile>(Obj);
+      if (MachObj)
----------------
samsonov wrote:
>   if (auto MachObj = dyn_cast<const MachOObjectFile>(Obj))
>     DbgObj = ...;
Done

================
Comment at: tools/llvm-symbolizer/LLVMSymbolize.h:95
@@ +94,3 @@
+      ObjectFileForArch;
+  std::map<std::pair<std::string, std::string>, ObjectPair>
+      ObjectPairForPathArch;
----------------
samsonov wrote:
> Thanks for the typedef cleanup. You may want to commit it separately as a cleanup change before the main patch.
Done in r219682.

================
Comment at: tools/llvm-symbolizer/LLVMSymbolize.h:130
@@ -124,5 +129,3 @@
   };
-  typedef std::map<SymbolDesc, StringRef> SymbolMapTy;
-  SymbolMapTy Functions;
-  SymbolMapTy Objects;
+  std::map<SymbolDesc, StringRef> Functions, Objects;
 };
----------------
samsonov wrote:
> Please define each member on a separate line.
Fixed in r219682.

================
Comment at: tools/llvm-symbolizer/llvm-symbolizer.cpp:132
@@ +131,3 @@
+    } else {
+      errs() << "Warning: dSYM hint must have the '.dSYM' extension.\n";
+    }
----------------
samsonov wrote:
> You may want to print the invalid flag value here.
Done.

http://reviews.llvm.org/D5705