- User Since
- Sep 23 2014, 5:24 PM (287 w, 6 d)
Fri, Mar 27
Thu, Mar 26
Addressed most of the review comments. I don't see that using file check would make the plan output checking any easier. The current function seems clear and easy to use. Trying to dynamically insert the passed in matches into a filecheck test file and then run file check on that doesn't seem like it would be any easier than what is here. I don't need any of the substitutions or any of the other fancy things FileCheck provides, that just seems like overkill. But maybe I'm misunderstanding what you meant.
Addressed most of Pavel's review comments.
Wed, Mar 25
IIUC, you should be able to test the actual effect you are trying to achieve by having a large source file, running "source list" to get it into the File Manager, then try to open the source file for writing in a process in a subshell that won't have the same mmap. That should fail without the patch (or with the setting turned to cache-on) and succeed with it.
Greg originally designed the macOS equivalent of this, so I've added him.
Tue, Mar 24
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.
Mon, Mar 23
Fri, Mar 20
Except for the suggestion to use eCommandRequiresTarget for the new command, this looks good.
Is there any command-based way to see the entire environment that a process will get when it launches? By populating target.env-vars with the inherited environment there was a way to mostly do that, but I don't see that anymore. It seems a shame not to be able to see that...
Thu, Mar 19
This test just seems wrong to me. We can pretend that processes that haven't crashed don't have crash_info, and manually suppress the output in lldb if the stop state isn't a crash. But it seems useful to ask "what are the crash_info bits in flight right now" even if you haven't crashed. So that doesn't seem right to me. Or you have to acknowledge that you have no way of knowing whether there is crash info data hanging around, in which case any test that depends on there being none is an invalid test. I'd delete this whole test, since it seems to me like it will only ever succeed by accident.
Sorry, forgot to select the action...
Tue, Mar 17
Mon, Mar 16
This looks fine to me.
Fri, Mar 13
Yes, a key/value approach would be a better API. Looks like the Environment class would make that pretty easy to wrap up, as well.
Thu, Mar 12
Thanks for doing this, it will be useful!
If I'm following the logic correctly, if you run a target with inherit-env off, and then do:
Is this one okay as well?
Removed Update till the next patch, and fixed a bug in PushPlan that preparing the next patch uncovered.
Removed ThreadPlanStackMap::Update, fixed a bug in PushPlan (can't std::move the ThreadPlan, THEN log it...)
The result from EvaluateExpression is pretty much always going to be valid, since the result holds the error. The correct way to do the first check is:
BTW, don't worry about the OS_ACTIVITY_DT_MODE env var. That's something you have to set when debugging or the Foundation loggging method (NSLog) will go to the system console not to the current terminal, so we force it on all processes on macOS...
Wed, Mar 11
Seems fine to me.
Tue, Mar 10
Fix some clang-format nits. I didn't change:
Addressed Pavel's review comments.
Addressed Pavel's review comments.
Mon, Mar 9
Fri, Mar 6
Thu, Mar 5
Attempt to appease clang-format.
Another point to make clear here. You might wonder whether ThreadPlan::GetThread should return an Optional<Thread> or something like that, since there might not be a Thread associated with the ThreadPlan.
Wed, Mar 4
Seems great to me. Getting the host platform is a generally useful thing to be able to do, and I can't think of anything you would want to add to this, so that is fine to.
LGTM, thanks for fixing this.
Tue, Mar 3
I think this is just a bug in argument completion.
You mean for BreakpointResolverScripted::CreateImplementationIfNeeded? Passing in the BreakpointSP is fine.
Feb 25 2020
Feb 21 2020
Feb 13 2020
Targets and Breakpoints should only ever be managed by SP in lldb, so enforcing that seems like a good cleanup.
I wonder if it wouldn't be better to assert in GetBreakpoint. Except when you are making the resolver, you should never have a breakpoint resolver without a valid breakpoint. And there's no point in calling GetBreakpoint when you know you haven't set it yet. You assert after most of the calls to GetBreakpoint, but not all. Of the ones you don't, I think most of them should be.
The only hesitation I have about this is if we are still printing this noise in demangled names, then the name of the type you see for a variable will be different from what you see when a method of that type ends up in a backtrace. That might be confusing. OTOH, the correct solution to that problem, IMO, is to remove the noise from backtraces, not keep it in the types...
Feb 12 2020
This way the only bit of the matrix of "restricting function name breakpoints by containing source file" that we don't support is "set a breakpoint on this function, but ONLY when it is inlined into another CU, NOT when it is in its defining CU." That seems the least useful of the options, and I'm happy to trade off not adding another option to the already overstuffed "break set" list of options. And remember, if you actually needed this it would be straightforward to write a scripted breakpoint resolver to do that job.
Feb 11 2020
Feb 10 2020
Feb 7 2020
Might also be good to fix the crash. If you don't support software breakpoints, you shouldn't get asked what your breakpoint opcode is.
Feb 6 2020
We were probably getting away with this because ValueObjectRegisterSet's children don't really need their parent to construct their values? But still, this is not how ValueObject children should work...
I think the ValueObjectRegisterSet::CreateChildAtIndex was wrong originally. It doesn't make it's children by passing itself in as the parent of the child, but just makes an independent ValueObject. You can fix that in your version by passing the ValueObjectRegisterSet's manager.
The one use I had for these clean targets was when you are making a new test and getting the test source to compile it is really tempting to run make in the test directory. It's a lot slower to go run the test, see the compile fail, fix something, etc... than just run make. But if you do that and don't clean, then when you actually go to run the test, it will fail because make sees the product in the source directory and won't build it in the out-of-line directory, but the test will look for it there.
Is there ever a reason other than performance why you would want NOT to consult both the Compile Unit name and also look for DW_AT_decl_file is performance? That doesn't seem clear to me.
Feb 5 2020
Oops, sorry. Looks fine.
Jan 31 2020
The current behavior does make tests less dependent on the exact details of lldb's command output. If there were two independent pieces of data in a command output that you wanted to compare against, you would have to fix some tests if you ever changed the ordering in the result. It was the case that the gdb test suite hampered the evolution of the command output because it was just too much of a pain to go change the tests. For instance, you really couldn't muck with breakpoint reporting output or stop notifications; the pain of fixing up the testsuite was way too high. But in this case, the fix would just be reordering the substrings in the Python array. So though this sort of thing makes me a little uneasy, in this case I think it's fine.
Jan 28 2020
I don't think you want to put anything in m_constructor_errors in this case either. It's fine to log something out, but ValidatePlan uses m_constructor_errors to help report a problem. If the breakpoint failed to take for some other reason, then we would report a permissions problem which is not the reason for the failure.
Jan 24 2020
Jan 23 2020
Addressed remaining review comments.
Addresses the rest of Jonas' comments.
LGTM. This is my bad. I put in the parsing for -C in CommandObjectBreakpointSet in an (apparently futile) attempt to remind myself to reserve -C for column number...
Jan 22 2020
Thanks for the suggestions! A couple were not to my taste, but I fixed all the others.
Addressed Jonas' review comments.
Jan 21 2020
Except for one typo in the comment (thanks for adding that BTW) looks good.
Jan 16 2020
There's a copy paste error in the first header. Other than that, this is fine, thanks for adding the entries!
Jan 15 2020
BTW, I had to fix this patch (cd9e5c32302cd3b34b796683eedb072c6a1cfdc1) to build on macOS. uint64_t and size_t are differently spelled (though I think otherwise equivalent.) One is "long long unsigned int", the other "long unsigned int". I have no idea why that's true, but std::min refuses to compare a size_t and a unit64_t. Anyway, I fixed this by casting one of the two sides of the comparison. But this was causing problems because we have an api (ReadImageData) that takes a uint64_t for the offset and a size_t for the size. That seems a little weird to me, why are these different types?