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

Alexey Samsonov vonosmas at gmail.com
Tue Oct 14 11:25:20 PDT 2014


LGTM, once the following minor comments are addressed.

Please run the patch through clang-format-diff before submitting. Thanks for working on this!

================
Comment at: tools/llvm-symbolizer/LLVMSymbolize.cpp:320
@@ +319,3 @@
+  DsymPaths.push_back(getDarwinDWARFResourceForPath(ExePath, Filename));
+  for (const auto &path : Opts.DsymHints) {
+    DsymPaths.push_back(getDarwinDWARFResourceForPath(path, Filename));
----------------
s/path/Path

================
Comment at: tools/llvm-symbolizer/LLVMSymbolize.cpp:356
@@ +355,3 @@
+    if (!Obj)
+      return std::make_pair(nullptr, nullptr);
+    addOwningBinary(std::move(B));
----------------
We still need to cache (nullptr, nullptr) pair in ObjectPairForPathArch. For example, if the path is invalid and provided 50 times, we don't want to print diagnostics 50 times in error() function. So, at least in the way the code is structured now, we'll have to increase the indentation.

================
Comment at: tools/llvm-symbolizer/LLVMSymbolize.cpp:358
@@ -310,1 +357,3 @@
+    addOwningBinary(std::move(B));
     if (Bin->isMachO() || Bin->isMachOUniversalBinary()) {
+      if (auto MachObj = dyn_cast<const MachOObjectFile>(Obj))
----------------
Do you need this check now? Looks like it's enough to check that Obj is a MachOObjectFile (and you won't need "Bin" variable in this case).

http://reviews.llvm.org/D5705






More information about the llvm-commits mailing list