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

[David Blaikie]

On Tue, Dec 9, 2014 at 3:05 PM, Frederic Riss <friss at apple.com> wrote:

> >>! 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.
>

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.


>
> ================
> 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
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141209/9dd73ecc/attachment.html>