- User Since
- Jun 4 2013, 6:02 AM (211 w, 5 d)
Fri, Jun 23
Thu, Jun 22
Go back to the decltype version
Wed, Jun 21
I like the direction this is going in, but I see places for more cleanup. The main three items are:
- making destruction cleaner
- avoiding global variables
- making ReadCyclicBuffer readable
Tue, Jun 20
If there are no further comments, can I commit this?
Mon, Jun 19
Well that's the whole thread group idea. Currently we have only one thread group i.e the process group (all existing un traced threads + newly spawned ones).
With separate "trace all threads" and "trace new threads", it will be multiple thread groups. For e.g
Lets say an application spawns 10 threads, now here we can end up with 9 thread groups in the worst case if the user calls "trace all threads" after each
new spawned thread.
Now I wanted to avoid having multiple thread groups coz the implementation will become more complex.
Thanks for spelling that out. However, it still does not sound like a convincing use case to me. Why would the user start to trace just one thread, only to later change his mind and trace the whole process instead. I'm not saying that can't happen, but it seems like something that we should discourage rather than try to support. The messaging I'd give to the user is: "you should trace the whole process unless you have a good reason not to". In case of a single threaded application, it won't make a difference, and if that app suddenly turns multithreaded, it will be traced as well. If he happens to have a single-thread trace running and later chooses to have a full process traced, he can always cancel that one and initiate a full trace instead.
I am not sure I can bring much to this, so I'll take a back seat.
Fri, Jun 16
Fix the function, as suggested by EricWF.
Unfortunately, posix_spawn does not satisfy all our needs for launching processes on non-darwin platforms (according to its design rationale, that was never the intention). The most notable one is the "launch a process for debugging". Darwin seems to have added extensions which make it possible, so it makes sense to keep using it. However, it also makes the usage of that function non-portable. The only reason FreeBSD was able to get away with it was because it rolled it's own version of "launch for debug" code (source/Plugins/Process/FreeBSD/ProcessMonitor.cpp). After switching to a more generic launching primitive, that code could disappear.
Thu, Jun 15
Is this good to go now? :)
Second round of comments. I still see a lot of places with redundant checks for what appear to be operational invariants. I'd like to get those cleared up to make the code flow better.
Wed, Jun 14
I've renamed the function.
Any thoughts on this? Or suggestions for other reviewers?
I've looked into the other test failures I was seeing on arm64 after merging this in. I have a fix for this up in D34199.
Tue, Jun 13
Use a switch instead of indexing the array with an enum value
Mon, Jun 12
Committed as r305197. I needed to tweak the test logic a bit, as not even thread.StepOut() did exactly what we needed there, because it was resuming also all other threads even though it was not necessary. Take a look at the resulting commit if you want to see the differences: (spoiler: the magic command that worked was thread continue #id)
Fri, Jun 9
Thu, Jun 8
Thanks for the review. I'm not entirely proud of how I implemented this, but I hope it's not too ugly either. The reason I opted for this approach is that I needed to implement the "if the module is null then decrement the address and recompute" logic in two places. RegisterContextLLDB (computes the backtrace) and StackFrame (computes the symbol name to display). This avoids the duplication, but adds another argument to a bunch of functions. I don't really have an idea on how to make this better, but maybe you will think of something.
Wed, Jun 7
Cool. Sorry about all the back-and-forth and thanks for the patience.
Tue, Jun 6
I'd like to commit this this week. If you have any objections to how I implemented this, let me know, so I can adjust the approach.
I am going to commit this, so we can move forward with the remote testing aspect.
Mon, Jun 5
Thank you for adding the support for dwarf 4 CIE. I think it's a good start, but I believe the version check needs to be done in a smarter way.
Fri, Jun 2
- fix freebsd typo
- use ::waitpid consistently
I am going to come back to this later, I'm going to create one or two more cleanup diffs which this will depend on.