- User Since
- Sep 23 2014, 5:24 PM (330 w, 2 d)
If you are just loading an object file and the looking at the line table or something like that, would a UnitTest be more suitable?
Excellent, thanks for persisting on this!
The ThreadPlanStack always has one base thread plan. You can't get rid of that one, the system relies on its always being there. Despite it's name, DiscardThreadPlans doesn't actually discard all the thread plans, just all but the base thread plan... So I think that's neither here nor there...
Wed, Jan 20
We shouldn't lightly change options that are commonly used. But we haven't made a strong statement about this the way we do for API. I'm more concerned about people having to rewrite/build and distribute their code to accommodate a new lldb than having to learn a new character for a not very commonly used option.
Fri, Jan 15
If you set a breakpoint on source lines with no line table entries, lldb slides the actual match forward to the nearest line table entry to the given line number. That's necessary for instance because if you lay out your function definitions over multiple lines they won't all get line table entries, but we don't want people to have to guess which of them was... Same is true of more complicated compound statements.
Thu, Jan 14
How were you setting the breakpoint in the case where it was failing from the command line? The breakpoint you are examining in the test in your patches is a "source regular expression" breakpoint. That would be "break set -p". If you were also doing that in the command line and got a different result that would be very odd, since neither interface (CLI or SB) does much work before handing the request off to the common routine. If (as I suspect) you were doing "b main.c:40" in the command line, then you should make a breakpoint with SBTarget.BreakpointCreateByLocation and see if that shows the same problem. That should do the same job as "break set --file --line".
Tue, Jan 12
Thanks for looking into this further! The thing to figure out is who still has a reference to either the Thread * or to the ThreadPlanStack over the destruction of the thread. That shouldn't be allowed to happen.
Mon, Jan 11
Fri, Jan 8
looks good to me too. When you get around to the wait times & intervals I'd argue for not doing that as a GDBRemote specific addition, as Greg was suggesting above. There's nothing gdb-remote specific about how long you want the debug agent to wait around for some process to show up.
Dec 18 2020
LGTM. It looks like we never made use of the distinction between "started to finalize" and "done finalizing", so just marking it at the start of finalization seems fine.
Dec 11 2020
LGTM, thanks for the cleanup.
LGTM - thanks for doing this!
Dec 10 2020
This all looks fine except I'm not sure you can have a single override context. The lldb command line is only a bit recursive, but you can have sequences like:
It's a little awkward that you have the "do_select" parameter with a default argument of "true" but then you explicitly pass "true" every time you use it...
Dec 4 2020
Dec 3 2020
Note, lldb has a bunch of special handling for line 0 code. For instance, when we are pushing a breakpoint past the prologue we will keep pushing it forward over line number 0 lines. Those are compiler generated and in general people don't want to stop there. Similarly, if you are stepping through line 3 and the next line entry after 3 is line 0 we keep stepping till we get to a non-zero line.
Nov 30 2020
I don't see any reason for, and lots of reasons against having more than one source of truth for a Debugger's "Currently Selected ExecutionContext". In particular, I can't see any good uses of the Debugger and the CommandInterpreter being able to have different "currently selected targets/threads/frames". For instance, I think people would generally be surprised if calling SBDebugger.SetSelectedTarget didn't also change the default target that subsequent command interpreter commands use.
At present, step-out stops right on the return to the parent frame. That's to support stepping into and out of nested function calls on a single source line. So step-out is often going to end up stopping somewhere a little awkward w.r.t. the line tables, and since there is no standard for where compilers emit line table markers, lldb has to be flexible and handle whatever seems roughly reasonable. It seems strange that should be any more work to do after calling step_out_of_here on that line, so having clang assign the return to the function call line seemed a bit odd. The current clang/icc/gcc behavior certainly makes more sense.
Nov 13 2020
Nov 11 2020
It seems weird that even if you had a summary formatter for some pointer type that was trying to print "DANGER WILL ROBINSON" when the pointer value was 0x0, we will override that and print "nullptr" in a C++ context or "nil" in an ObjC context. Seems like even if the value is Nil we should first consult the ValueObject's summary value and only do the nullptr/nil computation if it was empty.
The attraction of having stub fix up the PC in the stop after hitting the trap for breakpoints (in this case by moving the PC before the trap on architectures that execute the trap) was that this decision could be made simply in the stub, but if lldb had to check DECR_PC_AFTER_BREAK to understand a breakpoint stop, that would need to happen in a bunch of different places (as it did in gdb) and would be easier to get wrong.
Nov 10 2020
It's easier to read these tests if you use "lldbutil.run_to_source_breakpoint" to remove all that boilerplate.
Nov 9 2020
IIUC, the expression parser part of this change suppresses any Fixits that are clang-tidy type rewrites, is that right? If so that is indeed the correct behavior. But the fact that this change implements that behavior is entirely non-obvious at the call site. Could you either add a comment here explaining to point of the test or maybe wrap the test in a method on the FixItHint class so that it's self documenting?
Nov 5 2020
Oh, my bad, apparently we were leaving the dummy target out of the TargetList. That's a little odd, but I probably did it so that's not so unusual... Ignore my comments...
Oct 30 2020
Oct 29 2020
The test failed in the new form at line 77. So we got the CU for compile_unit1.c and it was valid. But it didn't contain an SBType for compile_unit1_type. Probably the same thing is true in compile_unit2.c you just didn't get there.
Sorry to be picky, but you can compress this by putting the IsValid checks into your find_* routines. And the:
This test is still assuming (1) that main.c is CompUnit(0), etc. You don't know that's true. And that there is only one type in each CU (and type 0 is the one you want). Neither of these is guaranteed. Can you check all these things, I'd rather not have to go a bunch of rounds on this.
Switched to !hasLocalLinkage.
All the parts of D89156 that are just being consistent about always passing old_modules as a vector seem okay. But then you get to a point where you shouldn't really have multiple modules replacing a single one so you aren't really sure what to do about it. That part makes me a little uneasy.
Oct 28 2020
I can't see anything wrong with this, it seems to follow the practice of other parts of GetSharedModule. I'm still not quite certain why I've never seen an instance where the absence of your fix has caused problems, and it isn't clear from the patch why that is. If there something special about handling breakpad files that moves you out of the normal code path for replacing overridden modules? So I still have a lurking feeling I'm missing something?
This was done on purpose, in commit 902974277d507a149e33487d32e4ba58c41451b6. The comment there is:
Oct 23 2020
LGTM. Thanks for making this easier to use.
Oct 20 2020
I don't know how this JIT Profiling code is wired into the regular JIT mechanism. I assume it is opt-in, in which case it shouldn't have any effect on expression parsing. If it isn't opt in, we should have a way to opt out on the "don't do work whose outcome you don't care about" principle.
Oct 19 2020
Oct 16 2020
Oct 15 2020
That's nicer, thanks!
Oct 14 2020
Use lldb-instr instead of hand-generating the reproducer wrappers.
Thanks, that's a little better.
Oct 13 2020
IIUC, the problem with ReplaceModule will get fixed in a dependent patch, right? So the last bit of this patch will get cleaned up by that change. If that's right, then this LGTM.
The return from a SIGSEGV or SIGILL has less information than the equivalent exception, so reporting the exceptions should be the default, but having a switch for people who need the exception to propagate is okay.
The change makes sense.
Oct 8 2020
Adopt Pavel's suggestion to do "nosuppress/nostop/notify" for SIGCONT instead of "nosuppress/nostop/nonotify".
Oct 7 2020
Oct 5 2020
Oct 2 2020
Oct 1 2020
Architecture is more specific to the structural aspect of a thing, whereas Design is more general. For instance, the rules for the SB API don’t really seem like a description of an Architecture, but more some general principles for doing the thing.
I like the name Design better than Architecture for the top level. For instance the structureddataplugins is not really Architecture, it's the design of a fairly small component of lldb
Ah, Adrian is right, you should probably move Architecture to Design.
Sep 30 2020
We should probably outlaw the shared pointer from weak pointer constructor in lldb. It looks like the only safe way to use it is in a try/catch block (or by preflighting the weak pointer with lock which renders it pretty much pointless.)
Sep 29 2020
Use std::weak_ptr::lock to make the shared pointer, rather than std::shared_ptr::shared_ptr(std::weak_ptr) (sort of), as the latter throws.
Sep 28 2020
Check safely for an unset plugin, then try to get the plugin and if either of those fails, use the underlying ObjectSP's Dump method to dump the data.
Jonas just revived the long Doc that Todd wrote about the Structured Data printing plugins. Seems like that is a useful thing, so I'm not in favor of getting rid of it just yet.
Sep 25 2020
Sep 24 2020
But, sure, if nobody can come up with a use for this plugin dealie, I'll get rid of it. Otherwise I guess we should make a default plugin that calls "Dump"?
I am happier removing things when I know what they were for??? Not really sure why this plugin architecture is there. My guess is it is part of Todd's os_log facility, but I'm not really sure.
Sep 23 2020
Addressed review comments.
Addressed review comments.