Page MenuHomePhabricator

jingham (Jim Ingham)
User

Projects

User does not belong to any projects.

User Details

User Since
Sep 23 2014, 5:24 PM (330 w, 2 d)

Recent Activity

Today

jingham updated subscribers of D94063: lldb: Add support for DW_AT_ranges on DW_TAG_subprograms.

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?

Fri, Jan 22, 9:42 AM · Restricted Project
jingham added a comment to D93874: [process] fix exec support on Linux.

Excellent, thanks for persisting on this!

Fri, Jan 22, 9:39 AM · Restricted Project

Yesterday

jingham added a comment to D93874: [process] fix exec support on Linux.

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...

Thu, Jan 21, 6:04 PM · Restricted Project
jingham added a comment to D95164: Make SBDebugger::CreateTargetWithFileAndArch accept lldb.LLDB_DEFAULT_ARCH.

I think the only difference is the triple is supposed to be a complete triple and the arch can be just "arm64" and like you added, we let the currently selected platform help fill it out? I don't remember why this was added. Nothing in the source history?

Thu, Jan 21, 2:56 PM · Restricted Project
jingham requested review of D95164: Make SBDebugger::CreateTargetWithFileAndArch accept lldb.LLDB_DEFAULT_ARCH.
Thu, Jan 21, 12:41 PM · Restricted Project

Wed, Jan 20

jingham committed rG98feb08e449f: Use CXX_SOURCES and point to the right source file. (authored by jingham).
Use CXX_SOURCES and point to the right source file.
Wed, Jan 20, 6:40 PM
jingham committed rGbff389120fa2: Fix a bug with setting breakpoints on C++11 inline initialization statements. (authored by jingham).
Fix a bug with setting breakpoints on C++11 inline initialization statements.
Wed, Jan 20, 6:00 PM
jingham closed D94846: Allow breakpoints to be set on C++11 inline initializers.
Wed, Jan 20, 6:00 PM · Restricted Project
jingham added a comment to D95100: [lldb/Commands] Fix short option collision for `process launch`.

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.

Wed, Jan 20, 5:15 PM · Restricted Project

Fri, Jan 15

jingham added a comment to D94846: Allow breakpoints to be set on C++11 inline initializers.

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.

Fri, Jan 15, 6:22 PM · Restricted Project
jingham requested review of D94846: Allow breakpoints to be set on C++11 inline initializers.
Fri, Jan 15, 4:47 PM · Restricted Project

Thu, Jan 14

jingham updated subscribers of D94063: lldb: Add support for DW_AT_ranges on DW_TAG_subprograms.

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".

Thu, Jan 14, 2:56 PM · Restricted Project

Tue, Jan 12

jingham updated subscribers of D93874: [process] fix exec support on Linux.

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.

Tue, Jan 12, 9:47 AM · Restricted Project

Mon, Jan 11

jingham added inline comments to D93874: [process] fix exec support on Linux.
Mon, Jan 11, 3:52 PM · Restricted Project
jingham added inline comments to D93874: [process] fix exec support on Linux.
Mon, Jan 11, 3:27 PM · Restricted Project

Fri, Jan 8

jingham added a comment to D93895: Implement vAttachWait in lldb-server.

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.

As long as we are find passing these arguments to _any_ process subclass I am find doing these as options.

Fri, Jan 8, 10:07 PM · Restricted Project
jingham added a comment to D93895: Implement vAttachWait in lldb-server.

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.

Fri, Jan 8, 5:09 PM · Restricted Project
jingham added a comment to D94077: Support unscoped enumeration members in the expression evaluator..

We can have unscoped enums in namespace and class scope and the enumerators won't leak out from those scopes. Thus we can have shadowing going on e.g.:

...

How does this change deal with those cases?

Thanks for pointing this out! My first patch didn't really take this into account, so it didn't work properly. I've made some changes to make it work, although I'm not very familiar with these parts of LLDB yet, so I'm not sure whether this approach is how it should be implemented.

Also I've noticed that LLDB's expression evaluator is not perfect with these lookups, e.g. it can find global variables from other scopes if there's no better candidate (not sure if this is a bug of a feature):

Process 3768045 stopped
* thread #1, name = 'a.out', stop reason = breakpoint 1.1
    frame #0: 0x0000000000401116 a.out`main at global.cpp:2:13
   1    namespace Foo { int x = 1; }
-> 2    int main() {};
(lldb) p x
(int) $0 = 1
Fri, Jan 8, 12:26 PM · Restricted Project

Dec 18 2020

jingham accepted D93479: [lldb] Simplify the is_finalized logic in process and make it thread safe..

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 18 2020, 5:58 PM · Restricted Project

Dec 11 2020

jingham accepted D93052: "target create" shouldn't save target if the command failed.

LGTM, thanks for the cleanup.

Dec 11 2020, 10:33 AM · Restricted Project
jingham accepted D92164: Make CommandInterpreter's execution context the same as debugger's one..

LGTM - thanks for doing this!

Dec 11 2020, 10:32 AM · Restricted Project

Dec 10 2020

jingham added a comment to D92164: Make CommandInterpreter's execution context the same as debugger's one..

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:

Dec 10 2020, 5:47 PM · Restricted Project
jingham added a comment to D93052: "target create" shouldn't save target if the command failed.

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 10 2020, 12:05 PM · Restricted Project

Dec 4 2020

jingham added a comment to D92643: [lldb] Lookup static const members in FindGlobalVariables.

Are the static members included in the (SB)Type object that gets created when parsing the enclosing type? If yes, we might be able to retrieve them that way. Whether that would be cleaner -- I'm not sure...

(I would expect they are included, as otherwise they would be unavailable to the expression evaluator.)

I think they're not as of now, but there's a patch from @teemperor to add them there -- D81471

Also, I was looking at Type/TypeSystem and it seems there's no API right now to get static members from a Type.

Dec 4 2020, 9:53 AM · Restricted Project

Dec 3 2020

jingham updated subscribers of D91734: [FastISel] Flush local value map on every instruction.

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.

Dec 3 2020, 2:51 PM · Restricted Project, debug-info, Restricted Project
jingham accepted D92601: [lldb] Refactor the Symbolicator initializer.

LGTM

Dec 3 2020, 2:16 PM · Restricted Project

Nov 30 2020

jingham requested changes to D92164: Make CommandInterpreter's execution context the same as debugger's one..

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.

Nov 30 2020, 4:52 PM · Restricted Project
jingham added a comment to rG53a14a47ee89: [lldb] Fix TestThreadStepOut.py after "Flush local value map on every….

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 30 2020, 2:12 PM

Nov 13 2020

jingham committed rG5a4b2e1541f3: The AssertRecognizer used the module from a frames SC without checking it was… (authored by jingham).
The AssertRecognizer used the module from a frames SC without checking it was…
Nov 13 2020, 11:42 AM

Nov 11 2020

jingham accepted D77153: [lldb/DataFormatters] Display null C++ pointers as nullptr.

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.

Nov 11 2020, 5:36 PM · Restricted Project
jingham added a comment to D91238: Recognize __builtin_debugtrap on arm64, advance pc past it so users can continue easily.

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 11 2020, 3:42 PM · Restricted Project

Nov 10 2020

jingham added a comment to D91220: [ThreadPlan] Add a test for `thread step-in -r`, NFC.

It's easier to read these tests if you use "lldbutil.run_to_source_breakpoint" to remove all that boilerplate.

Nov 10 2020, 4:19 PM · Restricted Project

Nov 9 2020

jingham added a comment to D91103: [tooling] Add support for fixits that indicate code will need reformatting.

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 9 2020, 2:50 PM · Restricted Project, Restricted Project

Nov 5 2020

jingham added a comment to D90872: [TargetList] Simplify dummy target creation.

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...

Nov 5 2020, 1:43 PM · Restricted Project
jingham added inline comments to D90872: [TargetList] Simplify dummy target creation.
Nov 5 2020, 1:40 PM · Restricted Project

Oct 30 2020

jingham accepted D89156: [lldb] GetSharedModule: Collect old modules in SmallVector.

LGTM

Oct 30 2020, 11:17 AM · Restricted Project

Oct 29 2020

jingham added a comment to D88483: Add possibility to get module from SBType.

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.

Oct 29 2020, 4:52 PM · Restricted Project
jingham added a comment to D88483: Add possibility to get module from SBType.

Sorry to be picky, but you can compress this by putting the IsValid checks into your find_* routines. And the:

Oct 29 2020, 4:40 PM · Restricted Project
jingham committed rG32a85b268a01: This is a preliminary version of the test for https://reviews.llvm.org/D88483. (authored by jingham).
This is a preliminary version of the test for https://reviews.llvm.org/D88483.
Oct 29 2020, 4:40 PM
jingham added a comment to D88483: Add possibility to get module from SBType.

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.

Oct 29 2020, 3:16 PM · Restricted Project
jingham committed rGfa5a13276764: Provide a reasonable value for PATH_MAX if the lldb headers don't provide it. (authored by jingham).
Provide a reasonable value for PATH_MAX if the lldb headers don't provide it.
Oct 29 2020, 3:03 PM
jingham committed rGc8c07b76b2cf: Use !hasLocalLinkage instead of listing the symbol types (authored by jingham).
Use !hasLocalLinkage instead of listing the symbol types
Oct 29 2020, 2:44 PM
jingham closed D78972: Treat hasWeakODRLinkage symbols as external in REPL computations.
Oct 29 2020, 2:44 PM · Restricted Project
jingham committed rGa37672e2db73: Mark the execution of stop-hooks as non-interactive. (authored by jingham).
Mark the execution of stop-hooks as non-interactive.
Oct 29 2020, 2:42 PM
jingham closed D90332: Mark the execution of commands in stop hooks as non-interactive.
Oct 29 2020, 2:42 PM · Restricted Project
jingham added a comment to D88483: Add possibility to get module from SBType.

@JDevlieghere @jingham api test failed after my commit:
http://lab.llvm.org:8011/#/builders/68/builds/1040
Had no such an error on my computer, please help to figure out what went wrong.

Oct 29 2020, 2:32 PM · Restricted Project
jingham added a comment to D78972: Treat hasWeakODRLinkage symbols as external in REPL computations.

Right -- I can understand that. I'm not sure whether this is a real issue -- I'd expect that these compiler-internal symbols would normally have private (not "internal") linkage (and hence be caught by line 330) -- but I'm not insisting on exposing all of the internal linkage symbols either.

However, I think the phrase "meant to be exported" brings us back to the beginning, full circle. Objects with "weak" linkage are also "meant" to be exported (== externally visible in a normal compilation), just like their weak_odr counterparts. I think that the real bug here is that we've started special-casing individual linkage types, and that it's only a matter of time before we find ourselves needing to add another linkage type to this list. Llvm has a bunch of functions to check for the properties of linkage types, and it seems to me like we should be able to pick one of them. So, if we want to just expose the functions that would be visible during a normal compilation, the function to use is hasLocalLinkage() (negation of it, that is)

Oct 29 2020, 2:16 PM · Restricted Project
jingham updated the diff for D78972: Treat hasWeakODRLinkage symbols as external in REPL computations.

Switched to !hasLocalLinkage.

Oct 29 2020, 2:13 PM · Restricted Project
jingham added a comment to D89157: [lldb] Report old modules from ModuleList::ReplaceEquivalent.

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.

Yeah, I wouldn't claim that the handling of the multiple-old-module case there is ideal. My thinking is that it makes an incremental step in the right direction, though -- the same potential for having multiple old modules is there with or without the change; the change makes the issue more apparent to readers of the code, makes note of it in the relevant log if we hit the issue at runtime, and refactors the code so there is one place to handle the issue. If you think it's best to hold off on that until we have a better way to actually handle the case, I can see the logic in that, just let me know and I'll rebase this to fix the bug in the meantime.

Thanks!

Oct 29 2020, 12:12 PM · Restricted Project
jingham added a comment to D89157: [lldb] Report old modules from ModuleList::ReplaceEquivalent.

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 29 2020, 11:10 AM · Restricted Project

Oct 28 2020

jingham accepted D89157: [lldb] Report old modules from ModuleList::ReplaceEquivalent.

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?

Oct 28 2020, 3:26 PM · Restricted Project
jingham requested review of D90332: Mark the execution of commands in stop hooks as non-interactive.
Oct 28 2020, 12:16 PM · Restricted Project
jingham added a comment to D90318: Return actual type from SBType::GetArrayElementType.

This was done on purpose, in commit 902974277d507a149e33487d32e4ba58c41451b6. The comment there is:

Oct 28 2020, 10:57 AM · Restricted Project

Oct 23 2020

jingham added a comment to D78972: Treat hasWeakODRLinkage symbols as external in REPL computations.

I don't think we need to have different behavior for repl and --top-level. I'm mainly just confused about what is the right behavior to aim for.

So, assuming we want these to behave as if everything was in a single TU, my next question is: What is the purpose of the external flag? In this setup, it seems like everything should be "external" (i.e., made available to subsequent expressions)...

Oct 23 2020, 3:10 PM · Restricted Project
jingham added a comment to D90011: [lldb] Redesign Target::GetUtilityFunctionForLanguage API.

LGTM. Thanks for making this easier to use.

Oct 23 2020, 10:35 AM · Restricted Project

Oct 20 2020

jingham added a comment to D86435: Profiling the code generated by MCJIT engine using Intel VTune profiler.

@jingham @clayborg Do either of you recognize the error that seems to be triggered? It looks like it's consistent across the tests, or at least the ones that I looked at:

FAIL: test_non_cpp_language_dwarf (TestImportStdModule.ImportStdModule)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/mnt/disks/ssd0/agent/llvm-project/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 1825, in test_method
    return attrvalue(self)
  File "/mnt/disks/ssd0/agent/llvm-project/lldb/packages/Python/lldbsuite/test/decorators.py", line 134, in wrapper
    return func(*args, **kwargs)
  File "/mnt/disks/ssd0/agent/llvm-project/lldb/test/API/commands/expression/import-std-module/basic/TestImportStdModule.py", line 38, in test_non_cpp_language
    lldbutil.run_to_source_breakpoint(self,
  File "/mnt/disks/ssd0/agent/llvm-project/lldb/packages/Python/lldbsuite/test/lldbutil.py", line 953, in run_to_source_breakpoint
    return run_to_breakpoint_do_run(test, target, breakpoint, launch_info,
  File "/mnt/disks/ssd0/agent/llvm-project/lldb/packages/Python/lldbsuite/test/lldbutil.py", line 875, in run_to_breakpoint_do_run
    test.assertEqual(process.GetState(), lldb.eStateStopped)
AssertionError: 10 != 5
Oct 20 2020, 2:46 PM · Restricted Project
jingham added a comment to D86435: Profiling the code generated by MCJIT engine using Intel VTune profiler.

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 20 2020, 11:05 AM · Restricted Project
jingham added a comment to D88483: Add possibility to get module from SBType.

Thanks a lot Jim for explanations, now it makes sense to me.
Have one more question on class Type. Writing API test for the patch defined 2 functions like this:

void *func1(int) {
        return NULL;
}

void *func2(int) {
        return NULL;
}

Tried to find function type using target.FindFirstType('func1') and got nothing as a result. Call to module.GetTypes().GetTypeAtIndex(0).GetName() contained:
"void *(int)". Looking through clang::Decls found out that clang::FunctionDecl is child of clang::ValueDecl and it finally became clear to me that "func1", "func2" are not types at all but simply symbols (variables?) of the same type "void *(int)". Therefore module where above 2 functions were declared contained only 1 type "void *(int)". Investigating how DWARFASTParser works noticed that it do generates 2 Type instances for 2 defined functions with m_name variable set to "func1", "func2" respectively and m_compiler_type referencing the same "void *(int)" (but not in it's string representation of course).
So the question is: Does class Type represents not only a type but also a variable (symbol or identifier, don't know how to name it correctly in this situation)?

Oct 20 2020, 10:21 AM · Restricted Project

Oct 19 2020

jingham added a comment to D88483: Add possibility to get module from SBType.

Can you please give me a couple of words on what m_static_type and m_dynamic_type is in TypeImpl class, in what cases can it be initialized with both of those types?

ValueObjectDynamic value uses a TypeImpl to store its type, and it stores both the static type (the one returned by the parsed expression or was in the DWARF variable records for some local value) and the dynamic type it figured out by looking at the value in memory in this TypeImpl. See for instance ValueObjectDynamicValue::UpdateValue. ValueObjectDynamicValue could also get its static type from its parent. I'm not sure why it doesn't do that and stores it in its TypeImpl instead; presumably this was more convenient somewhere.

Thanks for explanation.
Could please tell if I understood you correctly: from the start we load into some module’s type system some type for example from DWARF, but during runtime it may be altered and as a consequence ‘original’ static type becomes dynamic in-memory type? It still seems to me I do not completely understand relation between static and it’s dynamic part, if it is correctly to say so. Would like to understand the origin of this.

Oct 19 2020, 2:47 PM · Restricted Project
jingham added a comment to D88483: Add possibility to get module from SBType.

Can you please give me a couple of words on what m_static_type and m_dynamic_type is in TypeImpl class, in what cases can it be initialized with both of those types?

Oct 19 2020, 10:19 AM · Restricted Project

Oct 16 2020

jingham accepted D89589: [lldb] Implement ObjCExceptionThrowFrameRecognizer::GetName().

LGTM

Oct 16 2020, 1:22 PM · Restricted Project

Oct 15 2020

jingham committed rG6754caa9bf21: Add an SB API to get the SBTarget from an SBBreakpoint (authored by jingham).
Add an SB API to get the SBTarget from an SBBreakpoint
Oct 15 2020, 2:29 PM
jingham closed D89358: Add an API to get an SBBreakpoint's owning SBTarget.
Oct 15 2020, 2:29 PM · Restricted Project
jingham accepted D89315: [debugserver] Add option to propagate SIGSEGV to target process.

That's nicer, thanks!

Oct 15 2020, 9:54 AM · Restricted Project

Oct 14 2020

jingham updated the diff for D89358: Add an API to get an SBBreakpoint's owning SBTarget.

Use lldb-instr instead of hand-generating the reproducer wrappers.

Oct 14 2020, 12:11 PM · Restricted Project
jingham added a comment to D89315: [debugserver] Add option to propagate SIGSEGV to target process.

Thanks, that's a little better.

Oct 14 2020, 10:48 AM · Restricted Project

Oct 13 2020

jingham requested review of D89358: Add an API to get an SBBreakpoint's owning SBTarget.
Oct 13 2020, 6:47 PM · Restricted Project
jingham added a comment to D89156: [lldb] GetSharedModule: Collect old modules in SmallVector.

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.

Oct 13 2020, 3:26 PM · Restricted Project
jingham added a comment to D89315: [debugserver] Add option to propagate SIGSEGV to target process.

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.

Oct 13 2020, 1:50 PM · Restricted Project
jingham added a comment to D89157: [lldb] Report old modules from ModuleList::ReplaceEquivalent.

The change makes sense.

Oct 13 2020, 11:27 AM · Restricted Project

Oct 8 2020

jingham committed rGa68ffb19d392: Change the default handling of SIGCONT to nosuppress/nostop/notify (authored by jingham).
Change the default handling of SIGCONT to nosuppress/nostop/notify
Oct 8 2020, 3:24 PM
jingham closed D89019: Change the default handling of SIGCONT to nostop/noprint.
Oct 8 2020, 3:24 PM · Restricted Project
jingham updated the diff for D89019: Change the default handling of SIGCONT to nostop/noprint.

Adopt Pavel's suggestion to do "nosuppress/nostop/notify" for SIGCONT instead of "nosuppress/nostop/nonotify".

Oct 8 2020, 11:57 AM · Restricted Project

Oct 7 2020

jingham requested review of D89019: Change the default handling of SIGCONT to nostop/noprint.
Oct 7 2020, 4:54 PM · Restricted Project
jingham committed rG81b11c91070f: Fix a macOS build break caused by 3dfb94986170. (authored by jingham).
Fix a macOS build break caused by 3dfb94986170.
Oct 7 2020, 3:01 PM

Oct 5 2020

jingham committed rGbe66987e2047: Fix raciness in the StopHook check for "has the target run". (authored by jingham).
Fix raciness in the StopHook check for "has the target run".
Oct 5 2020, 3:45 PM
jingham closed D88753: Fix raciness in the check for whether a stop hook has run the target.
Oct 5 2020, 3:44 PM · Restricted Project

Oct 2 2020

jingham requested review of D88753: Fix raciness in the check for whether a stop hook has run the target.
Oct 2 2020, 1:07 PM · Restricted Project
jingham added a comment to D87868: [RFC] When calling the process mmap try to call all found instead of just the first one.

That sounds like a plan. FWIW, here's the implementation I hacked up today:

Status NativeProcessLinux::AllocateMemory(size_t size, uint32_t permissions,
                                          lldb::addr_t &addr) {
  PopulateMemoryRegionCache();
  auto region_it = llvm::find_if(m_mem_region_cache, [](const auto &pair) {
    return pair.first.GetExecutable() == MemoryRegionInfo::eYes;
  });
  if (region_it == m_mem_region_cache.end())
    return Status("No executable memory region found!");
  addr_t exe_addr = region_it->first.GetRange().GetRangeBase();

  NativeThreadLinux &thread = *GetThreadByID(GetID());
  assert(thread.GetState() == eStateStopped);

  int prot = PROT_NONE;
  if (permissions & ePermissionsReadable)
    prot |= PROT_READ;
  if (permissions & ePermissionsWritable)
    prot |= PROT_WRITE;
  if (permissions & ePermissionsExecutable)
    prot |= PROT_EXEC;

  NativeRegisterContextLinux &reg_ctx = thread.GetRegisterContext();
  DataBufferSP registers_sp;
  reg_ctx.ReadAllRegisterValues(registers_sp);
  uint8_t memory[2];
  size_t bytes_read;
  ReadMemory(exe_addr, memory, 2, bytes_read);

  reg_ctx.SetPC(exe_addr);
  reg_ctx.WriteRegisterFromUnsigned(lldb_rax_x86_64, SYS_mmap);
  reg_ctx.WriteRegisterFromUnsigned(lldb_rdi_x86_64, 0);
  reg_ctx.WriteRegisterFromUnsigned(lldb_rsi_x86_64, size);
  reg_ctx.WriteRegisterFromUnsigned(lldb_rdx_x86_64, prot);
  reg_ctx.WriteRegisterFromUnsigned(lldb_r10_x86_64,
                                    MAP_ANONYMOUS | MAP_PRIVATE);
  reg_ctx.WriteRegisterFromUnsigned(lldb_r8_x86_64, -1);
  reg_ctx.WriteRegisterFromUnsigned(lldb_r9_x86_64, 0);
  WriteMemory(exe_addr, "\x0f\x05", 2, bytes_read);
  PtraceWrapper(PTRACE_SINGLESTEP, thread.GetID(), nullptr, nullptr);
  int status;
  ::pid_t wait_pid = llvm::sys::RetryAfterSignal(-1, ::waitpid, thread.GetID(),
                                                 &status, __WALL);
  assert((unsigned)wait_pid == thread.GetID());
  addr = reg_ctx.ReadRegisterAsUnsigned(lldb_rax_x86_64, -ESRCH);

  WriteMemory(exe_addr, memory, 2, bytes_read);
  reg_ctx.WriteAllRegisterValues(registers_sp);

  if (addr > -4096)
    return Status(-addr, eErrorTypePOSIX);
  return Status();
}

It needs more error handling, and generalization to non-x86 architectures, but it actually works, and is enough to make all but one test pass (TestBitfields.py seems to fail due to some data corruption -- quite puzzling).

Very cool. That seems simple enough to allow us to try this out, at least on unix variants. Does PTRACE_SINGLESTEP cause _only_ the current thread to single step and keep all other threads paused? We can't allow any other threads making any progress when the process is briefly resumed in a multi-threaded process.

This nice thing is lldb-server _is_ compiled natively for each system so we can rely on system headers for the sys call numbers if they are available. So looks like this could be made to work. Did you also do the deallocate memory? I ran a few expressions and I am not sure that we ever use the deallocate memory packets to debugserver on mac, so it might not be needed.

Oct 2 2020, 12:50 PM · Restricted Project

Oct 1 2020

jingham updated subscribers of D88516: [lldb] Add a "design" section to the documentation..

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.

Oct 1 2020, 4:47 PM · Restricted Project
jingham added a comment to D88516: [lldb] Add a "design" section to the documentation..

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

Oct 1 2020, 4:28 PM · Restricted Project
jingham added a comment to D88516: [lldb] Add a "design" section to the documentation..

Ah, Adrian is right, you should probably move Architecture to Design.

Oct 1 2020, 4:27 PM · Restricted Project
jingham accepted D88516: [lldb] Add a "design" section to the documentation..

LGTM

Oct 1 2020, 4:26 PM · Restricted Project

Sep 30 2020

jingham committed rGafaeb6af79a4: Fix crash in SBStructuredData::GetDescription() when there's no… (authored by jingham).
Fix crash in SBStructuredData::GetDescription() when there's no…
Sep 30 2020, 11:49 AM
jingham closed D88266: Check that the "StructuredData Plugin weak pointer" is good before trying to turn it into a shared pointer.
Sep 30 2020, 11:49 AM · Restricted Project
jingham updated subscribers of D88266: Check that the "StructuredData Plugin weak pointer" is good before trying to turn it into a shared pointer.

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 30 2020, 10:42 AM · Restricted Project

Sep 29 2020

jingham updated the diff for D88266: Check that the "StructuredData Plugin weak pointer" is good before trying to turn it into a shared pointer.

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 29 2020, 5:57 PM · Restricted Project
jingham added a comment to D88266: Check that the "StructuredData Plugin weak pointer" is good before trying to turn it into a shared pointer.

Jim: were you able to repro this with a simple a.out program? Is there something worse going on here like maybe incompatible libc++/libstdc++ implementations?

Sep 29 2020, 5:56 PM · Restricted Project
jingham added a reverting change for rGf775fe59640a: Revert "Add the ability to write target stop-hooks using the ScriptInterpreter.": rG1b1d9815987a: Revert "Revert "Add the ability to write target stop-hooks using the….
Sep 29 2020, 12:01 PM
jingham committed rG1b1d9815987a: Revert "Revert "Add the ability to write target stop-hooks using the… (authored by jingham).
Revert "Revert "Add the ability to write target stop-hooks using the…
Sep 29 2020, 12:01 PM
jingham added inline comments to D88266: Check that the "StructuredData Plugin weak pointer" is good before trying to turn it into a shared pointer.
Sep 29 2020, 10:49 AM · Restricted Project

Sep 28 2020

jingham updated the diff for D88266: Check that the "StructuredData Plugin weak pointer" is good before trying to turn it into a shared pointer.

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.

Sep 28 2020, 5:48 PM · Restricted Project
jingham added a comment to D88266: Check that the "StructuredData Plugin weak pointer" is good before trying to turn it into a shared pointer.

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 28 2020, 1:25 PM · Restricted Project

Sep 25 2020

jingham committed rGb65966cff65b: Add the ability to write target stop-hooks using the ScriptInterpreter. (authored by jingham).
Add the ability to write target stop-hooks using the ScriptInterpreter.
Sep 25 2020, 3:45 PM
jingham closed D88123: Add the ability to write 'target stop-hooks' in Python.
Sep 25 2020, 3:45 PM · Restricted Project

Sep 24 2020

jingham added a comment to D88266: Check that the "StructuredData Plugin weak pointer" is good before trying to turn it into a shared pointer.

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"?

Sep 24 2020, 4:59 PM · Restricted Project
jingham added a comment to D88266: Check that the "StructuredData Plugin weak pointer" is good before trying to turn it into a shared pointer.

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 24 2020, 4:58 PM · Restricted Project
jingham requested review of D88266: Check that the "StructuredData Plugin weak pointer" is good before trying to turn it into a shared pointer.
Sep 24 2020, 3:30 PM · Restricted Project

Sep 23 2020

jingham added a comment to D88123: Add the ability to write 'target stop-hooks' in Python.

Addressed review comments.

Sep 23 2020, 5:51 PM · Restricted Project
jingham updated the diff for D88123: Add the ability to write 'target stop-hooks' in Python.

Addressed review comments.

Sep 23 2020, 5:48 PM · Restricted Project