Page MenuHomePhabricator

[lldb/Reproducers] Always collect the whole dSYM in the reproducer
ClosedPublic

Authored by JDevlieghere on Mar 23 2020, 11:54 PM.

Details

Summary

The FileCollector in LLDB collects every files that's used during a debug session when capture is enabled. This ensures that the reproducer only contains the files necessary to reproduce. This approach is not a good fit for the dSYM bundle, which is a directory on disk, but should be treated as a single unit. On macOS LLDB have automatically find the matching dSYM for a binary by its UUID. Having a incomplete dSYM in a reproducer can break debugging even when reproducers are disabled.

This patch adds a custom FileCollector to LLDB that is aware of dSYM. When it encounters a dSYM bundle, it iterates over everything inside it and adds it to the reproducer. While this might seem like overkill right now, having custom FileCollector is going to be inevitable as soon as we want to be smarter in what we include in the reproducer. For example, to keep its size small, we might want to skip system libraries in the future.

Diff Detail

Event Timeline

JDevlieghere created this revision.Mar 23 2020, 11:54 PM

It's not clear to me why this is needed.

I mean, if lldb touches any of the files inside the dsym bundle, then they should be automatically recorded already, right? And if it doesn't then it does not need them...

It sounds to me like we are failing to capture some of the file accesses. Can this be fixed by catching those instead?

It's not clear to me why this is needed.

I mean, if lldb touches any of the files inside the dsym bundle, then they should be automatically recorded already, right? And if it doesn't then it does not need them...

The dSYM can contain other resources than just the Mach-O companion file, such as script for the OS plugins or opt remarks, which might not be used by the reproducers or even LLDB at all. Once you have the reproducer on your system, tools will find and use it because spotlight indexed it. Having only a partial dSYM is really undesirable as it can break LLDB and other tools in really unexpected ways.

aprantl added inline comments.Mar 24 2020, 9:40 AM
lldb/include/lldb/Utility/FileCollector.h
26 ↗(On Diff #252233)

This is something I've struggled with before. If I'm inheriting from an LLVM class do I use LLVM or LLDB naming schemes?

lldb/source/Utility/FileCollector.cpp
1 ↗(On Diff #252233)

The C++ is redundant. This is a .cpp file, so it's not ambiguous which language this is.

It's not clear to me why this is needed.

I mean, if lldb touches any of the files inside the dsym bundle, then they should be automatically recorded already, right? And if it doesn't then it does not need them...

The dSYM can contain other resources than just the Mach-O companion file, such as script for the OS plugins or opt remarks, which might not be used by the reproducers or even LLDB at all. Once you have the reproducer on your system, tools will find and use it because spotlight indexed it. Having only a partial dSYM is really undesirable as it can break LLDB and other tools in really unexpected ways.

Interesting. Could that be fixed by adding the funny .noindex suffix to the reproducer name (or some subdirectory of it)?

It's not clear to me why this is needed.

I mean, if lldb touches any of the files inside the dsym bundle, then they should be automatically recorded already, right? And if it doesn't then it does not need them...

The dSYM can contain other resources than just the Mach-O companion file, such as script for the OS plugins or opt remarks, which might not be used by the reproducers or even LLDB at all. Once you have the reproducer on your system, tools will find and use it because spotlight indexed it. Having only a partial dSYM is really undesirable as it can break LLDB and other tools in really unexpected ways.

Interesting. Could that be fixed by adding the funny .noindex suffix to the reproducer name (or some subdirectory of it)?

Yes, it's an alternative I've considered. I prefer this approach because (1) conceptually a dSYM is a single unit, the fact that the bundle is actually a directory on disk is just an implementation detail and (2) it allows you to use the dSYM even when reproducer replay fails. As we're discovering bugs in the reproducers, we can usually still use the files inside it to the debug the issue, even if replay fails.

It's not clear to me why this is needed.

I mean, if lldb touches any of the files inside the dsym bundle, then they should be automatically recorded already, right? And if it doesn't then it does not need them...

The dSYM can contain other resources than just the Mach-O companion file, such as script for the OS plugins or opt remarks, which might not be used by the reproducers or even LLDB at all. Once you have the reproducer on your system, tools will find and use it because spotlight indexed it. Having only a partial dSYM is really undesirable as it can break LLDB and other tools in really unexpected ways.

Interesting. Could that be fixed by adding the funny .noindex suffix to the reproducer name (or some subdirectory of it)?

Yes, .noindex would prevent Spotlight from finding it. lldb might still find it though if it happens to be next to the executable.

Also, the dSYM bundle for a binary contains both the debug information and scripts that may be useful in debugging that binary - data formatters and the like. So even if the original debugging session crashed before they got around to using any of the facilities in their dSYM, they are still likely to come in handy in analyzing the state of the app in the reproducer. For this reason, I think we should treat dSYM's as special and capture them in full.

Ok, you really want to collect the full dsym, then who am I to stop you. :)

But implementing that collection in the FileCollector does not seem right, because at the filesystem level, dsyms are not special (which is why you needed to reverse-engineer them from the path). Maybe if we create some sort of an API to register files/directories of "special interest", then we could have something (SymbolVendorMacOSX, most likely) call it to record the full dsym when it knows it has encountered it.

Then we could have a similar api to register files of special disinterest, which could be used to selectively omit things from the reproducer. That part may be also interesting for using reproducers internally at google, because the binaries there are too big to be shipped around all the time.

Make the whole thing generic.

Wow. This is a lot more generic than I had mind. But at the same time, it does not seem generic enough. :( Like, for instance, using only component-based matching, one couldn't ignore /usr/lib while keeping $BUILD/lib or whatever.

What I had in mind for your use case was a simple function (*) like repro::???->recordInterestingDirectory(StringRef), and calling this dynamically from SymbolVendorMacOSX when it opens a dsym (so, approximately around here, maybe after some refactoring to avoid computing the dsym directory twice). That seems to be a lot more flexible, requires less plumbing, and about as equally obtrusive for the resulting code as attempting to statically describe the interesting directories via these matchers.

It's true that may need some kind of matchers for ignoring directories (**), but these should probably work on the full path, not individual components. And maybe they too shouldn't be fully static, so that they can be controlled by the context, or the user... I don't know -- we don't have to design this part now.

So what do you think about just adding the recordInterestingDirectory function? Would that be sufficient for your needs?

(*) I don't know which class should that function me a member of (if any). However, I think it would be nice if it was something in the repro namespace (and not the FileSystem class) to make it clear that this is happening for the sake of reproducers.

(**) I can see how my previous comment could be interpreted as a request to implement ignoring directories. That was not my intention, I am sorry. I was just musing about possible future directions (e.g. some function like ignoreUniniterestingDirectory).

Implement Pavel's suggestion

labath accepted this revision.Mar 26 2020, 12:32 PM
This revision is now accepted and ready to land.Mar 26 2020, 12:32 PM
This revision was automatically updated to reflect the committed changes.