I'd go with the "conservative" approach first. The idea of having lldb loaded inside a lit process does not excite me. One of the problems we have with dotest is that when lldb crashes during the test, it takes a part of the test driver with it which causes some tests to be skipped and the complicates the reporting of the result of the crashed test. It's not as bad right now, as there is still the main process left to report some kind of an error (back in the days when tests were run sequentially in a single process, the entire test suite would just stop), but I still think it would be nice to avoid these issues in the new framework.

I think that something like this would go a long way towards solving the problems with lit tests we're having in lldb.

In D54617#1302376, @JDevlieghere wrote:

In D54617#1302366, @labath wrote:

I am confused by differing scopes of various objects that are interacting here. It seems you are implementing capture/replay as something that can be flicked on/off at any point during a debug session (Debugger lifetime). However, you are modifying global state (the filesystem singleton, at least), which is shared by all debug sessions. If multiple debug sessions try to enable capture/replay (or even access the filesystem concurrently, as none of this is synchronized in any way), things will break horribly. This seems like a bad starting point in implementing a feature, as I don't see any way to fix incrementally in the future.

Maybe I should've added a comment (I didn't want to litter the code with design goals) but the goal of the commands is not really to enable/disable the reproducer feature. I'm currently abusing the commands to do that, but the long term goal is that you can enable/disable part of the reproducer. For example, you could say only capture GDB packets or capture only files. It makes testing a lot easier if you can isolate a single source of information. Since there's currently only GDB packets I didn't split it up (yet).

Also I piggy-backed off this to enable/disable the feature as a whole because I needed to go through the debugger. See my reply below on why and how that'll be fixed by the next patch :-)

If by "it would cause errors to no longer appear in the same order as specified by the user, but in an arbitrary order specified by the driver implementation", you mean that now for a command line like:

$ new/lldb -S /tmp/sadg --file /tmp/qwr

I get

error: file specified in --file (-f) option doesn't exist: '/tmp/qwr'

instead of

error: file specified in --source (-s) option doesn't exist: '/tmp/sadg'

then that's something I really don't care about.

I am confused by differing scopes of various objects that are interacting here. It seems you are implementing capture/replay as something that can be flicked on/off at any point during a debug session (Debugger lifetime). However, you are modifying global state (the filesystem singleton, at least), which is shared by all debug sessions. If multiple debug sessions try to enable capture/replay (or even access the filesystem concurrently, as none of this is synchronized in any way), things will break horribly. This seems like a bad starting point in implementing a feature, as I don't see any way to fix incrementally in the future.

Looks fine to me, I'd just ask you to consider shortening the method names a bit.

In D54221#1290638, @JDevlieghere wrote:

In D54221#1290572, @labath wrote:

I recall something about linux on arm having a magic unmodifiable (even by ptrace) page of memory, so this could be useful there too. However, it's not clear to me how a user is going to figure out that he needs to enable this setting. Would it make sense to automatically try setting a hardware breakpoint if software breakpoint fails?

My main concern would be that hardware breakpoints are a limited resource and not something we want to make transparent to the user, because it's only a matter of time before it fails.

I recall something about linux on arm having a magic unmodifiable (even by ptrace) page of memory, so this could be useful there too. However, it's not clear to me how a user is going to figure out that he needs to enable this setting. Would it make sense to automatically try setting a hardware breakpoint if software breakpoint fails?

Could we reverse the dependencies between the two? I.e., first add the necessary functionality to lldb-test and then write the test using that? Or just merge that into a single patch?

Thanks for the review. I have added some comments, and moved appending of the unmodified portion of the name into a separate function to aid readability.

Thanks for the review.

If it comes down to choosing between colored output going to stderr and plain output going where it should, i'd go with the second option.

In D54072#1286748, @zturner wrote:

Unfortunately then color output is impossible. Where else would the output
be expected to go?

thanks.

Kamil, I think you're the best qualified person to look at the lldb-server changes right now, so I'd like to hear your opinion on the other changes too. Otherwise, I'll just commit this some time next week.

Thanks for your patience. This looks good to me now.

A very nice use of the structured demangler.

lldb-server is used on architectures that don't have python (readily) available, and it uses the same codebase as the rest of lldb. (Granted, it only needs a small subset of that codebase, and this subset doesn't/shouldn't care about python, but we aren't able to split out that part cleanly (yet)). So I don't think requiring python is a good idea.

In D54020#1285539, @JDevlieghere wrote:

In D54020#1285201, @labath wrote:

I am not sure what do you mean by "translating paths" in the VFS. If you mean something like "return a FILE * for /whatever/my_reproducer/vfs/bin/ls when I ask for /bin/ls", then I think that's a good idea, as it's most likely to work with all of our existing code (e.g. mmapping). Although, in this case, i am not sure how much benefit will the llvm VFS bring to the game, as most of the operations could then be implemented by plainly prepending some prefix to a given path.

Let me try to explain this better. This is mostly a question about what kind of API the VFS (which lives in LLVM) provides to deal with files in lldb (i.e. through FILE* and file descriptors).

The first possibility is providing a method in the VFS that takes a "virtual" path and returns the "underlying" path. Something like Optional<path> vfs::getUnderlyingPath(path). This doesn't just have to be a prefix thing but you are right that it's mostly going to be just that. The problem is that this method wouldn't work for virtual file systems that don't have files on disk. Hence the lack of generality.

I am not sure what do you mean by "translating paths" in the VFS. If you mean something like "return a FILE * for /whatever/my_reproducer/vfs/bin/ls when I ask for /bin/ls", then I think that's a good idea, as it's most likely to work with all of our existing code (e.g. mmapping). Although, in this case, i am not sure how much benefit will the llvm VFS bring to the game, as most of the operations could then be implemented by plainly prepending some prefix to a given path.

It's not fully clear to me from the previous comments if you are proceeding with this or not, but in case you are, I have made comments inline. I see that you've added some lit tests, but I also think you it would be good add some unit tests for the name parser functionality per-se (similar to the existing name parsing tests), as it is much easier to see what is going on from those.

Thanks for bearing with me. I am very happy with how this turned out in the end. I have more suggestion on how to clean up the initialization, which you can incorporate if you like it, but I don't want to hold up the patch over that.

Why did you change the function name? I think it would be nice to keep "executable" or something to that effect in the name (e.g. the llvm function has "program"), given that the underlying function checks for the executable bit, and so it cannot be used for general searches.

In D53532#1280734, @JDevlieghere wrote:

In D53532#1280018, @labath wrote:

In D53532#1279022, @JDevlieghere wrote:

Address Pavel's feedback:

• Change to Initialize method which can only be called once.

I suppose this is slightly better, but what I meant was to have a static (like all other functions with the same name) Initialize function, which has to be called before the first call to Instance(). During normal startup, this could be called from SystemInitializer, but I am not sure how would that work for record/replay (that's why I was asking how you planned to initialize those). It would require to setup the record/replay machinery very early on, which may be tricky. But on the other hand, you probably want to initialize these as early as possible anyway.

Do you think something like that would be possible?

Alright, I see what you mean. You know more about the architecture, but it looks like we initialize the system before even reading the command line arguments in the driver. I have to read the reproducer before I can set the appropriate file system, so I'm not sure if this is possible.

Ok, if that's how you guys feel, then I won't stand in your way. I am fine with this patch then.

In D53532#1279022, @JDevlieghere wrote:

Address Pavel's feedback:

• Change to Initialize method which can only be called once.

lgtm

I like this change a lot. I was hoping something like this could be done, when I saw that we were copying patches around.

lgtm. Sorry about the trouble.

It sounds like at least part of this could be tested by adding the ability to dump dependent modules to lldb-test object-file (which I think would fit very nicely within the current information printed by that command). I seem to recall seeing code like that in some patch but it seems to be missing here now.

For the purposes of testing I decided to bypass lldb-test and go with a scripted lldbinit file. I'm sticking with this approach wherever possible to prove that this is "true" cross-platform debugger functionality. However, there are definite limitations. For example, the output format of type lookup has some limitations. It doesn't print the layout of the type in any kind of detail (e.g. field offsets), it doesn't support lookup by regex, it doesn't print the underlying type of an enumeration, it doesn't support limiting the scope of the search to specific kinds of types, so there is definitely room for lldb-test to come into the picture here to expand the coverage since we have full control over the output format.

Now that I know your use case (I forgot that you're working on the replay now, and the mention of VFS threw me off in a completely different direction), I agree that having a single global filesystem is fine. As for the increased verbosity, I wonder if we should make the fs accessor a free function instead of a member (so that you write something like hostFS().Symlink(..) instead of FileSystem::instance().Symlink(...).

I also think it we should try to keep FileSpec in the Utility module. We should be able to do that by deleting the "utility" functions in the FileSpec class, and updating everything to go through the FileSystem class.

Back when the FileSystem class was introduced, the idea was that *it* would eventually become the gateway to the real filesystem, and FileSpec would just deal with abstract path manipulation.

Yes it sounds like the situation is very much similar here as it is in dwarf land -- the compilers there also don't tend to emit unwind info for prologues and epilogues.

That's an interesting code snippet. Thanks for sharing that.

It looks like it should be easy to write a test for this by hand-crafting a minimal .s file and feeding it to lldb-test.

In D52461#1266302, @aleksandr.urakov wrote:

`operator<<A>'::`2'::B::operator>

Thanks for the review.

• rename Db into (Abstract)ManglingParser
• revert Allocator changes
• re-clang-format

In D53086#1261704, @aleksandr.urakov wrote:

As for aligned stack cross-platform problems, I mean also problems with stack unwinding. They seem to appear on non-Windows too. It's because x86AssemblyInspectionEngine doesn't support stack alignment now. I've made some changes locally to fix it, but they are still raw to publish. The main idea is to save one more frame address (along with CFA) for every row of an unwind plan (I've called this AFA - aligned frame address), and add an analysis for and esp, ... etc. to x86AssemblyInspectionEngine. What do you think about a such approach?

Do we have a test for this? It sounds like it shouldn't be hard to replicate the dependent library load failure in a test...

I can't say I have done a particularly thorough review, but code seems well structured, and it looks like I could make sense of it if I went through the effort of figuring out what various pdb record types mean. These are the things that came to mind while looking at this.

In D52404#1257347, @vadimcn wrote:

Thanks,
What changed in SWIG 3.0.11?

Sorry for the delay. I *think* this should be fine, but with these things its hard to tell whether it won't break someone's build/release setup. However, it does solve a real problem (I've hit this issue myself a couple of times on windows), so I think we should give it a try.

In D52618#1250909, @zturner wrote:

One idea would be to define some lit substitutions like %debuginfo. It’s
true you can produce a gcc style command line that will be equivalent to a
clang-cl invocation but it won’t be easy. eg you’ll needing to pass
-fms-compatibility as well as various -I for includes.

It may be easier to have substitutions instead

In D52461#1250555, @aleksandr.urakov wrote:

I've tried to parse with it a name like

N0::`unnamed namespase'::Name

and it can't parse it correctly. May be it just can't parse MSVC demangled names?

In D52461#1249259, @aleksandr.urakov wrote:

It seems that CPlusPlusLanguage::MethodName is backed by LLDB CPlusPlusNameParser, which can't parse demangled names...

In D52532#1246173, @krytarowski wrote:

I was wondering whether we want to normalize this inside the kernel and always advance the Program Counter.. but it's easier to manage it in userland.

Ooh, nice catch. This must have been fun to debug.