<html><head><meta http-equiv="Content-Type" content="text/html charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Dec 9, 2014, at 3:12 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" class="">dblaikie@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><br class="Apple-interchange-newline"><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><div class="gmail_quote" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">On Tue, Dec 9, 2014 at 3:05 PM, Frederic Riss<span class="Apple-converted-space"> </span><span dir="ltr" class=""><<a href="mailto:friss@apple.com" target="_blank" class="">friss@apple.com</a>></span><span class="Apple-converted-space"> </span>wrote:<br class=""><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;"><span class="">>>! In D6242#15, @samsonov wrote:<br class="">>>>! In D6242#14, @friss wrote:<br class="">>> ping!<br class="">><br class="">> Sorry about that, will take a look at the revised patch shortly.<br class=""><br class=""></span>No worries, I wasn't really expecting anyone to do further review :-)<br class=""><span class=""><br class="">>><br class="">>> I committed this briefly this morning considering that I could count your silence as approval, but Chandler told me not to do so. Sorry about that, I really didn't mean to force anything through.<br class="">>><br class="">>> My patch's short but intense in-tree live has produced one bot failure:<span class="Apple-converted-space"> </span><a href="http://bb.pgr.jp/builders/cmake-llvm-x86_64-linux/builds/19112" target="_blank" class="">http://bb.pgr.jp/builders/cmake-llvm-x86_64-linux/builds/19112</a><br class="">>> It seems the llvm-dsymutil tool hasn't been built at all there, thus I must be missing something in the build system hooks. I tried cmake and configure builds in Debug/Release before committing, so I'm not sure what I screwed up.<br class="">><br class="">> You should probably add "llvm-dsymutil" to test/lit.cfg, next to all the other llvm-XXX tools listed there.<br class=""><br class=""></span>I don't think this will solve it. If I'm looking at the right place, then this seems to just ensure that we call the tool with its build directory added. Looking at the build log I have the feeling that llvm-dsymutil wasn't built at all. I spotted that I used the 'dsymutil' name in LLVMBuild.txt, maybe that's it. I'm just surprised that it isn't break in any of my builds.<br class=""><br class="">Thanks a bunch!<br class=""><span class=""><br class="">================<br class="">Comment at: tools/dsymutil/DebugMap.cpp:27<br class="">@@ +26,3 @@<br class="">+ auto InsertResult = Symbols.insert(std::make_pair(Name,<br class="">+ SymbolMapping{ObjectAddress,<br class="">+ LinkedAddress}));<br class="">----------------<br class=""></span><span class="">samsonov wrote:<br class="">> IIRC this might not work on MSVC, need to double-check.<br class=""></span>You remember that you made me introduce this use of insert? :-) It'd be much appreciated if you have a way to verify that.<br class=""></blockquote><div class=""><br class="">The issue Alexey was alluding to is not the insert itself, but the use of braced init for SymbolMapping{...} - MSVC doesn't support braced init, so you'd want to use () instead.<br class=""></div></div></div></blockquote><div><br class=""></div><div>I see. Thanks for the explanation!</div><div><br class=""></div><div>Fred</div><br class=""><blockquote type="cite" class=""><div class=""><div class="gmail_quote" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><div class=""> </div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;"><span class=""><br class="">================<br class="">Comment at: tools/dsymutil/DebugMap.cpp:37<br class="">@@ +36,3 @@<br class="">+ typedef StringMapEntry<SymbolMapping> MapEntryTy;<br class="">+ std::vector<const MapEntryTy *> Entries;<br class="">+ Entries.reserve(Symbols.getNumItems());<br class="">----------------<br class=""></span><span class="">samsonov wrote:<br class="">> Looks like SymbolMapping is lightweight enough. This code can be faster if you just use<br class="">> std::vector<std::pair<StringRef, SymbolMapping>> Entries;<br class="">> fill it with single scan over `Symbols`, and then `std::sort` it and print it.<br class=""></span>Why do you think it would be faster? locality?<br class=""><span class=""><br class="">================<br class="">Comment at: tools/dsymutil/DwarfLinker.h:35<br class="">@@ +34,3 @@<br class="">+ /// \returns false if the link encountered a fatal error.<br class="">+ bool link(const DebugMap&);<br class="">+};<br class="">----------------<br class=""></span><span class="">samsonov wrote:<br class="">> See the note for MachODebugMapParser class below - why not expose only a standalone function<br class="">> bool linkDwarf(StringRef OutputFilename, const DebugMap &DM);<br class="">> in a header?<br class=""></span>That's actually a very good idea. In the code I developed since this was written, I struggled to keep the implementation details of DwarfLinker hidden, and that would make it trivial.<br class=""><br class="">The choice is a bit more complicated than for MachODebugParser (which is really private) because we might want to expose the interface for lld's consumption at some point. And the free function approach might not be flexible enough depending on how much tunables I add in the linking phase.<br class=""><br class="">I'll do it anyway, we can always revisit that later.<br class=""><span class=""><br class="">================<br class="">Comment at: tools/dsymutil/MachODebugMapParser.cpp:52<br class="">@@ +51,3 @@<br class="">+ if (!PathPrefix.empty())<br class="">+ Path = PathPrefix + sys::path::get_separator().data() + Path;<br class="">+<br class="">----------------<br class=""></span><span class="">samsonov wrote:<br class="">> 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.<br class=""></span>will do<br class=""><span class=""><br class="">================<br class="">Comment at: tools/dsymutil/MachODebugMapParser.cpp:71<br class="">@@ +70,3 @@<br class="">+ auto MainBinaryOrError = createMachOBinary(BinaryPath);<br class="">+ if (MainBinaryOrError.getError())<br class="">+ return MainBinaryOrError.getError();<br class="">----------------<br class=""></span><span class="">samsonov wrote:<br class="">> if (auto Error = ...)<br class="">> return Error;<br class=""></span>yep<br class=""><span class=""><br class="">================<br class="">Comment at: tools/dsymutil/MachODebugMapParser.cpp:160<br class="">@@ +159,3 @@<br class="">+ if (MainBinarySymbolAddresses.empty())<br class="">+ loadMainBinarySymbols();<br class="">+<br class="">----------------<br class=""></span><span class="">samsonov wrote:<br class="">> Why do you initialize it lazily, instead of just calling `loadMainBinarySymbols` right after you have initialized MainOwningBinary?<br class=""></span>Because it is not necessarily used. It is strictly needed only for common globals (this patch doesn't treat the commons differently from other globals, so the symbol table is read if you have any kind of global... which I guess you always have). Yeah, this `optimization` looks at least premature, if not totally useless. I'll drop it.<br class=""><span class=""><br class="">================<br class="">Comment at: tools/dsymutil/MachODebugMapParser.cpp:181<br class="">@@ +180,3 @@<br class="">+ uint64_t Addr;<br class="">+ // The only symbols of interest are the global variables. These<br class="">+ // are the only ones that need to be queried because the address<br class="">----------------<br class=""></span><span class="">samsonov wrote:<br class="">> Can/should we do smth. similar for `loadCurrentObjectFileSymbols` function?<br class=""></span>No because symbols from the object file are always useful. The debug map contains names associated with linked addresses (with the exception of common globals that need to be looked up in the binary), these linked addresses need to be correlated with the object file addresses that we always lookup in the object file's symbol table (and any defined symbol is of interest).<br class=""><span class=""><br class="">================<br class="">Comment at: tools/dsymutil/MachODebugMapParser.h:27<br class="">@@ +26,3 @@<br class="">+<br class="">+class MachODebugMapParser {<br class="">+public:<br class="">----------------<br class=""></span><span class="">samsonov wrote:<br class="">> 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.<br class="">><br class="">> 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<br class="">> ErrorOr<std::unique_ptr<DebugMap>> parseDebugMap(StringRef BinaryPath, StringRef PathPrefix = "");<br class="">> and call the whole MachODebugMapParser an "mach-specific implementation detail", hidden in the .cpp file?<br class=""></span>As I said above, good idea. MachODebugParser should never get public anyway it is truly an implementation detail of dsymutil. When lld will want to use dsymutil's code, it will build its DebugMap objects itself anyway.<br class=""><span class=""><br class="">================<br class="">Comment at: tools/dsymutil/MachODebugMapParser.h:34<br class="">@@ +33,3 @@<br class="">+ /// open it.<br class="">+ void setPreprendPath(StringRef Prefix) { PathPrefix = Prefix; }<br class="">+<br class="">----------------<br class=""></span><span class="">samsonov wrote:<br class="">> Is there a reason against making it a constructor argument (which can default to empty string)?<br class=""></span>No real reason except that I prefer to use constructors for major properties and setters for optional/debug stuff. I can change that, it's not like the constructor will get confusing with one additional argument.<br class=""><span class=""><br class="">================<br class="">Comment at: tools/dsymutil/dsymutil.cpp:19<br class="">@@ +18,3 @@<br class="">+<br class="">+#include "llvm/Support/ManagedStatic.h"<br class="">+#include "llvm/Support/PrettyStackTrace.h"<br class="">----------------<br class=""></span><span class="">samsonov wrote:<br class="">> Do you use it here?<br class=""></span>llvm_shutdown_obj is defined there.<br class=""><span class=""><br class="">================<br class="">Comment at: tools/dsymutil/dsymutil.cpp:31<br class="">@@ +30,3 @@<br class="">+<br class="">+static llvm::cl::opt<std::string> OsoPrependPath("oso-prepend-path",<br class="">+ llvm::cl::desc("<path>"));<br class="">----------------<br class=""></span><span class="">samsonov wrote:<br class="">> Flag description could be helpful here.<br class=""></span>I have a description in my tree, must have missed merging that patch in the initial submission.<br class=""><span class=""><br class="">================<br class="">Comment at: tools/dsymutil/dsymutil.cpp:65<br class="">@@ +64,3 @@<br class="">+<br class="">+ llvm::DwarfLinker Linker(InputFile + ".dwarf");<br class="">+ return !Linker.link(*DebugMap.get());<br class="">----------------<br class=""></span><span class="">samsonov wrote:<br class="">> If InputFile is stdin ("-"), this will be "-.dwarf"<br class=""></span>hehe, nice catch. I'm on the edge as to what to do with this: hardcoding the output file (a.out.dwarf?) or simply refusing to do that (and maybe allowing it later by adding a -o option).<br class=""><br class=""><a href="http://reviews.llvm.org/D6242" target="_blank" class="">http://reviews.llvm.org/D6242</a></blockquote></div></div></blockquote></div><br class=""></body></html>