[PATCH] Initial commit for the llvm-dsymutil tool.
Frederic Riss
friss at apple.com
Tue Dec 9 15:05:50 PST 2014
>>! In D6242#15, @samsonov wrote:
>>>! In D6242#14, @friss wrote:
>> ping!
>
> Sorry about that, will take a look at the revised patch shortly.
No worries, I wasn't really expecting anyone to do further review :-)
>>
>> 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.
>>
>> My patch's short but intense in-tree live has produced one bot failure: http://bb.pgr.jp/builders/cmake-llvm-x86_64-linux/builds/19112
>> 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.
>
> You should probably add "llvm-dsymutil" to test/lit.cfg, next to all the other llvm-XXX tools listed there.
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.
Thanks a bunch!
================
Comment at: tools/dsymutil/DebugMap.cpp:27
@@ +26,3 @@
+ auto InsertResult = Symbols.insert(std::make_pair(Name,
+ SymbolMapping{ObjectAddress,
+ LinkedAddress}));
----------------
samsonov wrote:
> IIRC this might not work on MSVC, need to double-check.
You remember that you made me introduce this use of insert? :-) It'd be much appreciated if you have a way to verify that.
================
Comment at: tools/dsymutil/DebugMap.cpp:37
@@ +36,3 @@
+ typedef StringMapEntry<SymbolMapping> MapEntryTy;
+ std::vector<const MapEntryTy *> Entries;
+ Entries.reserve(Symbols.getNumItems());
----------------
samsonov wrote:
> 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.
Why do you think it would be faster? locality?
================
Comment at: tools/dsymutil/DwarfLinker.h:35
@@ +34,3 @@
+ /// \returns false if the link encountered a fatal error.
+ bool link(const DebugMap&);
+};
----------------
samsonov wrote:
> See the note for MachODebugMapParser class below - why not expose only a standalone function
> bool linkDwarf(StringRef OutputFilename, const DebugMap &DM);
> in a header?
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.
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.
I'll do it anyway, we can always revisit that later.
================
Comment at: tools/dsymutil/MachODebugMapParser.cpp:52
@@ +51,3 @@
+ if (!PathPrefix.empty())
+ Path = PathPrefix + sys::path::get_separator().data() + Path;
+
----------------
samsonov wrote:
> 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.
will do
================
Comment at: tools/dsymutil/MachODebugMapParser.cpp:71
@@ +70,3 @@
+ auto MainBinaryOrError = createMachOBinary(BinaryPath);
+ if (MainBinaryOrError.getError())
+ return MainBinaryOrError.getError();
----------------
samsonov wrote:
> if (auto Error = ...)
> return Error;
yep
================
Comment at: tools/dsymutil/MachODebugMapParser.cpp:160
@@ +159,3 @@
+ if (MainBinarySymbolAddresses.empty())
+ loadMainBinarySymbols();
+
----------------
samsonov wrote:
> Why do you initialize it lazily, instead of just calling `loadMainBinarySymbols` right after you have initialized MainOwningBinary?
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.
================
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
----------------
samsonov wrote:
> Can/should we do smth. similar for `loadCurrentObjectFileSymbols` function?
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).
================
Comment at: tools/dsymutil/MachODebugMapParser.h:27
@@ +26,3 @@
+
+class MachODebugMapParser {
+public:
----------------
samsonov wrote:
> 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?
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.
================
Comment at: tools/dsymutil/MachODebugMapParser.h:34
@@ +33,3 @@
+ /// open it.
+ void setPreprendPath(StringRef Prefix) { PathPrefix = Prefix; }
+
----------------
samsonov wrote:
> Is there a reason against making it a constructor argument (which can default to empty string)?
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.
================
Comment at: tools/dsymutil/dsymutil.cpp:19
@@ +18,3 @@
+
+#include "llvm/Support/ManagedStatic.h"
+#include "llvm/Support/PrettyStackTrace.h"
----------------
samsonov wrote:
> Do you use it here?
llvm_shutdown_obj is defined there.
================
Comment at: tools/dsymutil/dsymutil.cpp:31
@@ +30,3 @@
+
+static llvm::cl::opt<std::string> OsoPrependPath("oso-prepend-path",
+ llvm::cl::desc("<path>"));
----------------
samsonov wrote:
> Flag description could be helpful here.
I have a description in my tree, must have missed merging that patch in the initial submission.
================
Comment at: tools/dsymutil/dsymutil.cpp:65
@@ +64,3 @@
+
+ llvm::DwarfLinker Linker(InputFile + ".dwarf");
+ return !Linker.link(*DebugMap.get());
----------------
samsonov wrote:
> If InputFile is stdin ("-"), this will be "-.dwarf"
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).
http://reviews.llvm.org/D6242
More information about the llvm-commits
mailing list