- User Since
- Jun 4 2013, 6:02 AM (285 w, 1 d)
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.
Mon, Nov 19
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
error: file specified in --file (-f) option doesn't exist: '/tmp/qwr'
error: file specified in --source (-s) option doesn't exist: '/tmp/sadg'
then that's something I really don't care about.
Sun, Nov 18
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.
Thu, Nov 15
Wed, Nov 14
Mon, Nov 12
Sat, Nov 10
Looks fine to me, I'd just ask you to consider shortening the method names a bit.
Wed, Nov 7
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?
Tue, Nov 6
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.
Mon, Nov 5
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.
Sun, Nov 4
Sat, Nov 3
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.
Fri, Nov 2
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.
Thu, Nov 1
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.
Wed, Oct 31
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.
Tue, Oct 30
Ok, if that's how you guys feel, then I won't stand in your way. I am fine with this patch then.
Mon, Oct 29
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.
Sun, Oct 28
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.
Sat, Oct 27
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(...).
Thu, Oct 25
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.
Tue, Oct 23
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.
Oct 19 2018
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.
Oct 17 2018
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.
Oct 16 2018
Thanks for the review.
Oct 15 2018
- rename Db into (Abstract)ManglingParser
- revert Allocator changes
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...
Oct 10 2018
Oct 8 2018
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.
Oct 7 2018
Oct 6 2018
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.
Oct 5 2018
Oct 3 2018
Oct 2 2018
Oct 1 2018
Sep 30 2018
Sep 26 2018
Sep 25 2018
Ooh, nice catch. This must have been fun to debug.