[PATCH] Initial commit for the llvm-dsymutil tool.

[Alexey Samsonov]

================
Comment at: tools/dsymutil/DebugMap.cpp:27
@@ +26,3 @@
+  auto InsertResult = Symbols.insert(std::make_pair(Name,
+                                                    SymbolMapping{ObjectAddress,
+                                                                  LinkedAddress}));
----------------
IIRC this might not work on MSVC, need to double-check.

================
Comment at: tools/dsymutil/DebugMap.cpp:37
@@ +36,3 @@
+  typedef StringMapEntry<SymbolMapping> MapEntryTy;
+  std::vector<const MapEntryTy *> Entries;
+  Entries.reserve(Symbols.getNumItems());
----------------
Looks like SymbolMapping is lightweight enough. This code can be faster if you just use
  std::vector<std::pair<StringRef, SymbolMapping>> Entries;
fill it with single scan over `Symbols`, and then `std::sort` it and print it.

================
Comment at: tools/dsymutil/DwarfLinker.h:35
@@ +34,3 @@
+  /// \returns false if the link encountered a fatal error.
+  bool link(const DebugMap&);
+};
----------------
See the note for MachODebugMapParser class  below - why not expose only a standalone function
  bool linkDwarf(StringRef OutputFilename, const DebugMap &DM);
in a header?

================
Comment at: tools/dsymutil/MachODebugMapParser.cpp:18
@@ +17,3 @@
+
+static ErrorOr<OwningBinary<MachOObjectFile>> createMachOBinary(StringRef file) {
+  ErrorOr<OwningBinary<Binary>> BinaryOrErr = createBinary(file);
----------------
friss wrote:
> samsonov wrote:
> > Can you make use of ObjectFile::createMachOObjectFile() here?
> I think the quantity of code would be the same because to use it I have to handle the MemoryBuffer creation myself. I'm not sure which one is preferable.
I think that using the more specific factory function would be more appropriate than calling createBinary(), and manually inspecting its output, making sure we return the same error code as Object library, etc.

================
Comment at: tools/dsymutil/MachODebugMapParser.cpp:52
@@ +51,3 @@
+  if (!PathPrefix.empty())
+    Path = PathPrefix + sys::path::get_separator().data() + Path;
+
----------------
Please use sys::path::append() here, so that you don't have to bother if `oso-prepend-path` should or should not end with a slash.

================
Comment at: tools/dsymutil/MachODebugMapParser.cpp:71
@@ +70,3 @@
+  auto MainBinaryOrError = createMachOBinary(BinaryPath);
+  if (MainBinaryOrError.getError())
+    return MainBinaryOrError.getError();
----------------
  if (auto Error = ...)
    return Error;

================
Comment at: tools/dsymutil/MachODebugMapParser.cpp:160
@@ +159,3 @@
+  if (MainBinarySymbolAddresses.empty())
+    loadMainBinarySymbols();
+
----------------
Why do you initialize it lazily, instead of just calling `loadMainBinarySymbols` right after you have initialized MainOwningBinary? 

================
Comment at: tools/dsymutil/MachODebugMapParser.cpp:181
@@ +180,3 @@
+    uint64_t Addr;
+    // The only symbols of interest are the global variables. These
+    // are the only ones that need to be queried because the address
----------------
Can/should we do smth. similar for `loadCurrentObjectFileSymbols` function?

================
Comment at: tools/dsymutil/MachODebugMapParser.h:27
@@ +26,3 @@
+
+class MachODebugMapParser {
+public:
----------------
Do you actually plan to move this object to LLVM library in future? For now, I don't see why it's convenient or desirable to expose this object in a header. You can't really do anything with it - just construct it, and then call method `parse()` once.

What is more, you probably don't want to have the instance of MainOwningBinary object lying around in memory after `parse()` returned. Can you simply expose a function
  ErrorOr<std::unique_ptr<DebugMap>> parseDebugMap(StringRef BinaryPath, StringRef PathPrefix = "");
and call the whole MachODebugMapParser an "mach-specific implementation detail", hidden in the .cpp file?

================
Comment at: tools/dsymutil/MachODebugMapParser.h:34
@@ +33,3 @@
+  /// open it.
+  void setPreprendPath(StringRef Prefix) { PathPrefix = Prefix; }
+
----------------
Is there a reason against making it a constructor argument (which can default to empty string)?

================
Comment at: tools/dsymutil/dsymutil.cpp:19
@@ +18,3 @@
+
+#include "llvm/Support/ManagedStatic.h"
+#include "llvm/Support/PrettyStackTrace.h"
----------------
Do you use it here?

================
Comment at: tools/dsymutil/dsymutil.cpp:31
@@ +30,3 @@
+
+static llvm::cl::opt<std::string> OsoPrependPath("oso-prepend-path",
+                                                 llvm::cl::desc("<path>"));
----------------
Flag description could be helpful here.

================
Comment at: tools/dsymutil/dsymutil.cpp:65
@@ +64,3 @@
+
+  llvm::DwarfLinker Linker(InputFile + ".dwarf");
+  return !Linker.link(*DebugMap.get());
----------------
If InputFile is stdin ("-"), this will be "-.dwarf"

http://reviews.llvm.org/D6242