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

Alexey Samsonov vonosmas at gmail.com
Wed Nov 19 21:47:49 PST 2014


================
Comment at: tools/Makefile:36
@@ -35,3 +35,3 @@
                  llvm-profdata llvm-symbolizer obj2yaml yaml2obj llvm-c-test \
-                 llvm-vtabledump verify-uselistorder
+                 llvm-vtabledump verify-uselistorder dsymutil
 
----------------
friss wrote:
> samsonov wrote:
> > I'd prefer binary name (llvm-dsymutil) to match the directory name. I'm not aware of existing policy, though.
> This is in fact on purpose. The binary is currently called llvm-dsymutil so that it doesn't interfere with the system dsymutil. When the work will be complete enough to actually replace the system dsymutil, I want to rename the binary to just dsymutil so that it takes precedence when clang invokes dsymutil. I preferred to name the directory after the expected end-result rather than renaming the directory later.
Yep, makes sense.

================
Comment at: tools/dsymutil/DebugMap.cpp:24
@@ +23,3 @@
+  SymbolMapping &Sym = Symbols[Name];
+  assert(Sym.ObjectAddress == 0 && Sym.BinaryAddress == 0 &&
+         "Symbol added twice in the same file.");
----------------
friss wrote:
> samsonov wrote:
> > Can you just use StringMap::insert() and use its return value to check if the key is already there?
> I like the simplicity of operator[] more. I'm not sure that using insert() would simplify anything especially as its return value would be used only in Assert builds. 
You can make addSymbol() return true iff the symbol wasn't there already, and assert (or print meaningful warning/error) in the caller.

================
Comment at: tools/dsymutil/DebugMap.cpp:31
@@ +30,3 @@
+void DebugMapObject::dump() const {
+  errs() << "Object file " << getObjectFilename() << '\n';
+  auto End = Symbols.end();
----------------
friss wrote:
> samsonov wrote:
> > why not outs()? (here and below). Or, better, you can probably take
> >   llvm::raw_ostream &OS
> > as an input parameter.
> AFAIK, dump() without argument is more canonical in LLVM, and it isn't usually even compiled in in non-debug builds.
> However, you are right that in this case as I use it to emit user-visible diagnostics, this convention doesn't apply.
LLVM classes often has dump() and print(llvm::raw_ostream), with former simply calling the latter.

================
Comment at: tools/dsymutil/MachODebugMapParser.h:53
@@ +52,3 @@
+
+  void newDebugMapObject(StringRef Filename);
+  void resetParserState();
----------------
friss wrote:
> samsonov wrote:
> > newDebugMapObject() doesn't actually return anything, which is slightly confusing, especially when you call
> >   return newDebugMapObject()
> > below.
> Yeah the name dates back to a version where it would actually return the allocated object. I'll change that to switchToNewDebugMapObject(Name). Do you have an issue with the return voidReturningFunc(); idiom? I kinda like it when it allows to do early exits without having to brace the if body.
I'm neutral to this idiom, as long as function name doesn't imply that it's supposed to return the object :)

http://reviews.llvm.org/D6242






More information about the llvm-commits mailing list