[Lldb-commits] [PATCH] D137000: [lldb] Make callback-based formatter matching available from the CLI.

jingham requested changes to this revision. jingham added a comment. This revision now requires changes to proceed. This looks good. The one detail that remains is error handling for the command. You can't provide the --regex option and this matcher at the same time, and it is also an error to

[Lldb-commits] [PATCH] D137662: Make aliases from a raw command that isn't a top-level command work

jingham created this revision. jingham added reviewers: JDevlieghere, kastiglione, clayborg. Herald added a subscriber: jeroen.dobbelaere. Herald added a project: All. jingham requested review of this revision. Herald added a project: LLDB. Herald added a subscriber: lldb-commits. When we went to

[Lldb-commits] [PATCH] D137000: [lldb] Make callback-based formatter matching available from the CLI.

jingham accepted this revision. jingham added a comment. This revision is now accepted and ready to land. It is a bit odd to follow the what the help summary says is a valid option set and then get an error. But this is a corner case, and since you say explicitly that you can't provide both th

[Lldb-commits] [PATCH] D137000: [lldb] Make callback-based formatter matching available from the CLI.

jingham added a comment. In D137000#3902217 , @labath wrote: > In D137000#3897878 , @jgorbe wrote: > >> I'm looking at the option of using a non-printable character for the short >> flag, and at the same time mak

[Lldb-commits] [PATCH] D137684: Make sure we are stopped before we try to install functions into the target

jingham created this revision. jingham added reviewers: JDevlieghere, jasonmolenda, labath, clayborg. Herald added a subscriber: kristof.beyls. Herald added a project: All. jingham requested review of this revision. Herald added a project: LLDB. Herald added a subscriber: lldb-commits. It's actual

[Lldb-commits] [PATCH] D137684: Make sure we are stopped before we try to install functions into the target

This revision was automatically updated to reflect the committed changes. Closed by commit rGb565e7f0c4cb: Don't try to create Expressions when the process is running. (authored by jingham). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137684/new/

[Lldb-commits] [PATCH] D137662: Make aliases from a raw command that isn't a top-level command work

This revision was automatically updated to reflect the committed changes. Closed by commit rGd518ed42ae85: Handle aliasing a non-top-level command. (authored by jingham). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137662/new/ https://reviews.llvm

[Lldb-commits] [PATCH] D137860: [lldb] Allow flexible importing of in_call_stack

jingham accepted this revision. jingham added a comment. This revision is now accepted and ready to land. I made this change in situ in the version we ship in the LLDB.framework, and this does the right thing! Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://r

[Lldb-commits] [PATCH] D137682: Change IRMemoryMap's last-resort magic address to an inaddressable address so it doesn't conflict

jingham accepted this revision. jingham added a comment. This revision is now accepted and ready to land. Except that you explicitly want to use the expression parser here so you shouldn't use `p` in the test, this seems fine to me. It would be better to have a less fragile strategy for getting

[Lldb-commits] [PATCH] D139158: [LLDB][LoongArch] Make software single stepping work

jingham added a comment. Note that the two most common uses of single step in lldb are 1) stepping over the instruction under a breakpoint (which may be a branch) and 2) stepping to the next instruction from a branch instruction. So stepping won't work correctly till you get single step past a

[Lldb-commits] [PATCH] D103217: Make ignore counts work as "after stop" modifiers so they play nicely with conditions

jingham created this revision. jingham added a reviewer: JDevlieghere. jingham requested review of this revision. Herald added a project: LLDB. Herald added a subscriber: lldb-commits. Previously ignore counts were checked when we stopped to do the sync callback in Breakpoint::ShouldStop. That m

[Lldb-commits] [PATCH] D103210: [lldb] Introduce Language::MethodNameInfo

jingham added a comment. The idea seems good, but I'm not sure how you are going to extend it? "Selector" seems a pretty ObjC specific term to me. Will this be what you use to get the "method name" part of the C++ names? That would look a bit odd to me, but OTOH having a GetSelector in all t

[Lldb-commits] [PATCH] D103271: [lldb/Target] Select most relevant frame only in case of signal

jingham requested changes to this revision. jingham added a comment. It is incorrect to say that SelectMostRelevantFrame is only relevant for asserts. The FrameRecognizer is a generic mechanism, and you have no a priori way to know what kinds of thread stops it wants to operate on. So this pat

[Lldb-commits] [PATCH] D103349: [lldb] Don't print script output twice in HandleCommand

jingham added a comment. A command that knew it was streaming a lot of output is supposed to be able to choose to have the CommandInterpreter directly stream the results while the command is executing. That's good for something that is likely to print a lot of output, since then you don't have

[Lldb-commits] [PATCH] D103439: [lldb] Print the backtrace for all threads if the test breakpoint can't be hit

jingham added a comment. In D103439#2791324 , @teemperor wrote: > In D103439#2791298 , @JDevlieghere > wrote: > >> LGTM. Are there other places where we check this, either in `lldbutil` or >> maybe more generall

[Lldb-commits] [PATCH] D103349: [lldb] Don't print script output twice in HandleCommand

jingham added a comment. In D103349#2791614 , @JDevlieghere wrote: > Use in-class initializers I'm all for in-class initializers, but I think it is confusing to use them piecemeal, where some ivars in a class have initializers and some are initialized

[Lldb-commits] [PATCH] D103483: [lldb] Convert the default constructor’s member initializers into default member initializers

jingham added a comment. Thanks for taking this on. The in-class initialization is so much less boiler-plate and easier to read! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103483/new/ https://reviews.llvm.org/D103483 ___ lldb-commits mai

[Lldb-commits] [PATCH] D103271: [lldb/Target] Select most relevant frame only in case of signal

jingham added a comment. One of the "frame recognizers" - probably the one that's causing this particular problem - is the "assert" recognizer. The idea is that when you call "assert" in user code, you pretty much always end up with a 4 or 5 frames deep stack that wanders it's way from assert

[Lldb-commits] [PATCH] D103701: [lldb] Set return status to failed when adding a command error

jingham added a comment. This seems like a good change to me. And even if you wanted to do something cheesy like put an error into the result on spec, then later decide it was successful after all, you can always reset the result's status to the appropriate success status. This just makes the

[Lldb-commits] [PATCH] D103349: [lldb] Don't print script output twice in HandleCommand

jingham accepted this revision. jingham added a comment. This revision is now accepted and ready to land. HandleCommands really does need to do this suppression, and this is a fine way to do it. I can't think of any other place where you would need to do this suppression after an admittedly non

[Lldb-commits] [PATCH] D103271: [lldb/Target] Select most relevant frame only in case of signal

jingham added a comment. "Unwinding" the zero-th frame is really just getting the PC & FP. Since you are very likely to need this information on any given stop, lldb has accelerated versions of the gdb-remote stop reply packets that provide this information up front so this sort of request is

[Lldb-commits] [PATCH] D103271: [lldb/Target] Select most relevant frame only in case of signal

jingham added a comment. I don't think it's right to do it in GetStackFrameAtIndex. For instance, suppose lldb stops somewhere, and the user does a backtrace, then they select a different frame on the stack, and then invoke some other command that happens to ask for the frame at index 0. If t

[Lldb-commits] [PATCH] D104067: [lldb] Decouple ObjCLanguage from Symtab

jingham added a comment. This looks pretty good to me. It's a little awkward in InitNameIndexes that we look up the various NameToSymbolIndex maps by eFunctionNameType, use the function name type again to sort the names & index pairs into the bucket we looked up before. I wonder if that could

[Lldb-commits] [PATCH] D104162: NFC Fix the handling of BreakpointOptions - return references to make it clear when you will get a valid object

jingham created this revision. jingham added reviewers: aprantl, shafik. jingham requested review of this revision. Herald added a project: LLDB. Herald added a subscriber: lldb-commits. Many of the API's that returned BreakpointOptions always returned valid ones. However, internally the Break

[Lldb-commits] [PATCH] D104162: NFC Fix the handling of BreakpointOptions - return references to make it clear when you will get a valid object

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGcfb96d845a68: Convert functions that were returning BreakpointOpti

[Lldb-commits] [PATCH] D104437: Add test for functions with extended characters.

jingham added a comment. It's quite common to use the lldbtest.TestBase.runCmd or lldbtest.TestBase.expect to test command line commands in the API tests. The latter has expect-like results checking which makes writing these checks easy. Even if you are testing a command-line command, drivin

[Lldb-commits] [PATCH] D104488: Create synthetic symbol names on demand to improve memory consumption and startup times.

jingham added a comment. Are the Symbol ID's for unnamed symbols the same each time you read in a symbol file? While the unnamed_symbol symbol names are not significant, it would be good if you were crashing in __lldb_unnamed_symbol111 on one lldb run, you would also crash in the same named sy

[Lldb-commits] [PATCH] D104488: Create synthetic symbol names on demand to improve memory consumption and startup times.

jingham added a comment. In D104488#2831818 , @clayborg wrote: > In D104488#2830992 , @jingham wrote: > >> Are the Symbol ID's for unnamed symbols the same each time you read in a >> symbol file? While the unnam

[Lldb-commits] [PATCH] D104488: Create synthetic symbol names on demand to improve memory consumption and startup times.

jingham added a comment. Some comments on comments... Comment at: lldb/include/lldb/Symbol/Symtab.h:224 + /// + /// We started not always generating symbol names for synthetic symbols and + /// we want to be able to lookup synthetic symbols by name when needed. This

[Lldb-commits] [PATCH] D104488: Create synthetic symbol names on demand to improve memory consumption and startup times.

jingham added a comment. Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104488/new/ https://reviews.llvm.org/D104488 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.or

[Lldb-commits] [PATCH] D104488: Create synthetic symbol names on demand to improve memory consumption and startup times.

jingham added a comment. I'm fine with the change. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104488/new/ https://reviews.llvm.org/D104488 ___ lldb-commits mailing list lldb-commits@lists.llvm.org htt

[Lldb-commits] [PATCH] D104893: Add data formatter support for a missing NSDictionary variant

jingham updated this revision to Diff 354418. jingham added a comment. Herald added a project: LLDB. Herald added a subscriber: lldb-commits. Formatting Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104893/new/ https://reviews.llvm.org/D104893 Fil

[Lldb-commits] [PATCH] D93479: [lldb] Simplify the is_finalized logic in process and make it thread safe.

jingham added a comment. Huh... I fear you are going to have to debug this further on your end. I can't see anything else suspect in this patch. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93479/new/ https://reviews.llvm.org/D93479 __

[Lldb-commits] [PATCH] D104404: Change PathMappingList::RemapPath to return an optional result (NFC)

jingham accepted this revision. jingham added a comment. This revision is now accepted and ready to land. LGTM. If the return type of the new function is not too awful to type, maybe don't use auto? It says it's returning a path, which is usually a string, not a FileSpec... But that's more a

[Lldb-commits] [PATCH] D104951: [lldb] Use the non-locking variant of objc_copyRealizedClassList

jingham accepted this revision. jingham added a comment. This revision is now accepted and ready to land. That seems fine to me. We already only allow one thread to run when we make this call, and copyRealizedClassList is read only, so at worst this will crash if the list is getting edited, but

[Lldb-commits] [PATCH] D105038: [lldb/Interpreter] Fix session-save-on-quit when using ^D

jingham added a comment. Maybe a test? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105038/new/ https://reviews.llvm.org/D105038 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.l

[Lldb-commits] [PATCH] D105038: [lldb/Interpreter] Fix session-save-on-quit when using ^D

jingham accepted this revision. jingham added a comment. This revision is now accepted and ready to land. One nit about the handling of colors. Comment at: lldb/source/Core/Debugger.cpp:611 + if (cmd_interpreter.GetSaveSessionOnQuit()) { +CommandReturnObject result(/*color

[Lldb-commits] [PATCH] D105038: [lldb/Interpreter] Fix session-save-on-quit when using ^D

jingham requested changes to this revision. jingham added a comment. This revision now requires changes to proceed. Shouldn't have also said accepted yet... Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105038/new/ https://reviews.llvm.org/D105038

[Lldb-commits] [PATCH] D105038: [lldb/Interpreter] Fix session-save-on-quit when using ^D

jingham accepted this revision. jingham added a comment. This revision is now accepted and ready to land. Excellent Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105038/new/ https://reviews.llvm.org/D105038

[Lldb-commits] [PATCH] D105136: [lldb Look for the mangled objc_copyRealizedClassList_nolock symbol name

jingham accepted this revision. jingham added a comment. This revision is now accepted and ready to land. LGTM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105136/new/ https://reviews.llvm.org/D105136 ___ lldb-commits mailing list lldb-commi

[Lldb-commits] [PATCH] D93479: [lldb] Simplify the is_finalized logic in process and make it thread safe.

jingham added a comment. In D93479#2850104 , @aharries-upmem wrote: > In D93479#2841049 , @jingham wrote: > >> Huh... I fear you are going to have to debug this further on your end. I >> can't see anything else

[Lldb-commits] [PATCH] D105215: [lldb] Remove CPlusPlusLanguage from Mangled

jingham added a comment. In D105215#2850821 , @bulbazord wrote: > I kind of feel that `Language::GetDemangledFunctionNameWithoutArguments` may > be a bit too specific for a generalized language plugins. I think it may be > worth it to make `Mangled` an

[Lldb-commits] [PATCH] D105389: [lldb] Add AllocateMemory/DeallocateMemory to the SBProcess API

jingham added a comment. Could you put a docstring describing the API's in the SBProcess.i file. It is annoying to have to do this in two places but given the way SWIG works we don't have a better alternative. This is where the help in the Python REPL comes from so it's useful to have it ther

[Lldb-commits] [PATCH] D105470: [lldb] Clear children of ValueObject on value update

jingham added a comment. This change is wrong for ValueObjectConstResult's. The original point of ValueObjectConstResult's was to store the results of expressions so that, even if the process is not at the original stop point, you could still check the value. It should have the value it had a

[Lldb-commits] [PATCH] D105389: [lldb] Add AllocateMemory/DeallocateMemory to the SBProcess API

jingham accepted this revision. jingham added a comment. LGTM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105389/new/ https://reviews.llvm.org/D105389 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/

[Lldb-commits] [PATCH] D105470: [lldb] Clear children of ValueObject on value update

jingham added a comment. In D105470#2866731 , @werat wrote: > Thanks for the prompt review! > >> This change is wrong for ValueObjectConstResult's. The original point of >> ValueObjectConstResult's was to store the results of expressions so that, >> eve

[Lldb-commits] [PATCH] D105698: [lldb/Target] Fix event handling during process launch

jingham accepted this revision. jingham added a comment. This revision is now accepted and ready to land. This LGTM. This code is fetching events by hand off the event queue and then resending them, so it makes sense that it should stop the ones it is handling from leaking out to the Process li

[Lldb-commits] [PATCH] D106004: [lldb] Always call DestroyImpl from Process::Finalize

jingham accepted this revision. jingham added a comment. This revision is now accepted and ready to land. This seems better to me. It's a little odd that the Process was short-cutting DoDestroy for these states. How can it know that one of the plugins might not need to handle that case? As a

[Lldb-commits] [PATCH] D105914: [lldb] Make TargetList iterable

jingham accepted this revision. jingham added a comment. This revision is now accepted and ready to land. LGTM, these Iterables make the code so much easier to read. Plus, though it's highly unlikely to happen, we shouldn't let another thread muck with the target list while we're Destroying its

[Lldb-commits] [PATCH] D106122: Add a mutex to guard access to the ThreadPlanStack class

jingham created this revision. jingham added reviewers: JDevlieghere, kastiglione. jingham requested review of this revision. Herald added a project: LLDB. Herald added a subscriber: lldb-commits. We've seen reports of crashes (none we've been able to reproduce locally) that look like th

[Lldb-commits] [PATCH] D106122: Add a mutex to guard access to the ThreadPlanStack class

jingham added a comment. In D106122#2884543 , @kastiglione wrote: > I believe it does need to be a reclusive mutex. Yes, I could refactor this to have a no-lock version, but I don't think it's worth complicating things. Repository: rG LLVM Github M

[Lldb-commits] [PATCH] D106514: Fix the logic so stop-hooks get run after a breakpoint that ran an expression

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGbcce8e0fccc1: Fix the logic so stop-hooks get run after a breakpoint that ran an expression (authored by jingham). Herald added a project: LLDB. Hera

[Lldb-commits] [PATCH] D106623: Fix a logic error in "break delete --disabled" when no "protected" breakpoints are specified.

jingham created this revision. jingham added a reviewer: JDevlieghere. jingham requested review of this revision. Herald added a project: LLDB. Herald added a subscriber: lldb-commits. Fix "break delete --disabled" with no arguments. The code that figured out which breakpoints to delete w

[Lldb-commits] [PATCH] D106712: Remember to check whether the current thread is stopped for a no-stop signal

jingham created this revision. jingham added reviewers: clayborg, JDevlieghere. jingham requested review of this revision. Herald added a project: LLDB. Herald added a subscriber: lldb-commits. When calculating the "currently selected thread" in Process::HandleStateChangedEvent, we check whether

[Lldb-commits] [PATCH] D106623: Fix a logic error in "break delete --disabled" when no "protected" breakpoints are specified.

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG0018c7123be3: Fix "break delete --disabled" with no arguments. (au

[Lldb-commits] [PATCH] D106712: Remember to check whether the current thread is stopped for a no-stop signal

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG910353c1048e: When calculating the "currently selected thread" in

[Lldb-commits] [PATCH] D105215: [lldb] Remove CPlusPlusLanguage from Mangled

jingham accepted this revision. jingham added a comment. This revision is now accepted and ready to land. I still think the "Demangled" part of the GetDemangledFunctionNameWithoutArguments is redundant. I'm not sure what sense it would make to get a mangled name without arguments? And you coul

[Lldb-commits] [PATCH] D107015: Add "current" token for the -t option to "break set/modify"

jingham created this revision. jingham added reviewers: kastiglione, JDevlieghere. Herald added a subscriber: dang. jingham requested review of this revision. Herald added a project: LLDB. Herald added a subscriber: lldb-commits. It's fairly common to want to set a breakpoint that is limited to th

[Lldb-commits] [PATCH] D107079: [lldb] Change the log format to JSON instead of plain text

jingham added a comment. When you are using the logging for a reliably reproducible problem (some parsing issue, for instance) then there's no harm to the general "turn on all logging and we'll sort it out on our end". But for any issues that are timing sensitive the problem is that too much l

[Lldb-commits] [PATCH] D106171: [lldb] Avoid moving ThreadPlanSP from plans vector

jingham accepted this revision. jingham added a comment. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D106171/new/ https://reviews.llvm.org/D106171 ___ lldb-commits mailing list lldb-commits@lists.l

[Lldb-commits] [PATCH] D68671: Add the ability to pass extra args to a Python breakpoint command function

jingham added inline comments. Comment at: lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py:21 @expectedFailureAll(oslist=["windows"], bugnumber="llvm.org/pr24528") -def test_breakpoint_command_sequence(self): +

[Lldb-commits] [PATCH] D107015: Add "current" token for the -t option to "break set/modify"

jingham added a comment. You can have options with optional values, but I don't favor using those. They have slightly odd behavior in the parser since you have to disambiguate them when they have a value, and them when they happen to be next to an argument. They aren't as easy to document, an

[Lldb-commits] [PATCH] D107660: [lldb] Upstream support for Foundation constant classes

jingham requested changes to this revision. jingham added a comment. This revision now requires changes to proceed. Just a few little nits. Comment at: lldb/source/Plugins/Language/ObjC/Cocoa.cpp:509 +float flt_value = 0.0f; +memcpy(&flt_value, &flt_as_int, sizeof(flt_a

[Lldb-commits] [PATCH] D107660: [lldb] Upstream support for Foundation constant classes

jingham accepted this revision. jingham added a comment. This revision is now accepted and ready to land. If the float/double pattern wasn't introduced here, then it's fine by me to fix it in a follow-on patch. Otherwise, LGTM. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107660/new/

[Lldb-commits] [PATCH] D107386: [LLDB][GUI] Add Breakpoints window

jingham added a comment. In D107386#2932037 , @OmarEmaraDev wrote: > @clayborg With you suggestions it currently looks like this. The concern I > have is that lines are now 36 characters longer, and some important > information are now truncated. The b

[Lldb-commits] [PATCH] D107015: Add "current" token for the -t option to "break set/modify"

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGf362b05d0dcd: Add a "current" token to the ThreadID option to brea

[Lldb-commits] [PATCH] D107673: Use LC_DYLD_EXPORTS_TRIE to locate the dyld trie structure if present

jingham created this revision. jingham added a reviewer: JDevlieghere. jingham requested review of this revision. Herald added a project: LLDB. Herald added a subscriber: lldb-commits. The pointer to the dyld trie data structure which lldb needs to parse to get "trampoline kinds" on Darwin used t

[Lldb-commits] [PATCH] D107673: Use LC_DYLD_EXPORTS_TRIE to locate the dyld trie structure if present

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGbfeb281fbd8e: Use LC_DYLD_EXPORTS_TRIE to locate the dyld trie structure if present (authored by jingham). Repository: rG LLVM Github Monorepo CH

[Lldb-commits] [PATCH] D107679: [lldb] Move Objective-C constants into ObjCConstants.h

jingham accepted this revision. jingham added a comment. This revision is now accepted and ready to land. Nice, thanks for cleaning this up. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107679/new/ https://reviews.llvm.org/D107679 ___ lldb-c

[Lldb-commits] [PATCH] D107521: [lldb/Plugins] Introduce Scripted Interface Factory

jingham added a comment. This is a good start, but it seems like there's still a lot of boilerplate once you get to the python side that could be trimmed down. For instance, ScriptedProcessPythonInterface::GetThreadWithID is almost entirely generic. The only things that make it specific to thi

[Lldb-commits] [PATCH] D107869: [LLDB][GUI] Add Process Launch form

jingham added a comment. In D107869#2939922 , @OmarEmaraDev wrote: > One thing to note as well is that the target environment seem to include many > environment variables like PATH and HOME, I don't think those should be > included. If I am debugging

[Lldb-commits] [PATCH] D107869: [LLDB][GUI] Add Process Launch form

jingham added a comment. In D107869#2940016 , @OmarEmaraDev wrote: > @jingham I wasn't arguing that we should remove those environment variables, > on the contrary. Greg was suggesting that we populate the environment field > with the target environmen

[Lldb-commits] [PATCH] D107869: [LLDB][GUI] Add Process Launch form

jingham added a comment. In D107869#2940428 , @clayborg wrote: > In D107869#2940016 , @OmarEmaraDev > wrote: > >> @jingham I wasn't arguing that we should remove those environment variables, >> on the contrary.

[Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

jingham added a comment. Do modern Linux's not have posix_spawn? If it exists that's a better interface, and lets the system handle a lot of the complicated machinations you have to do by hand if you roll it yourself out of fork and exec. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D

[Lldb-commits] [PATCH] D108228: Fix error handling in the libcxx std::string formatter

jingham created this revision. jingham added a reviewer: JDevlieghere. jingham requested review of this revision. Herald added a project: LLDB. Herald added a subscriber: lldb-commits. The formatter tries to get the data field of the std::string, and to check whether that fails it just checks tha

[Lldb-commits] [PATCH] D108228: Fix error handling in the libcxx std::string formatter

jingham added a comment. Raphael's analysis of what the test needs is right. We always check pointers for validity before we do operations on them, so we wouldn't have tried to get the summary. The reason I passed the string reference to a function and check it there is because from many year

[Lldb-commits] [PATCH] D108510: [lldb] Allow to register frame recognizers applied beyond the first instruction

jingham added a comment. One of the key reasons to use "frame recognizers" is to provide argument values to functions that don't have debug information. That generally only works when stopped at the first instruction, since otherwise you have to follow the value as it moves through the code, w

[Lldb-commits] [PATCH] D108335: [lldb] Move UnixSignals subclasses to lldbTarget

jingham added a comment. I appreciate the aim, but I don't think moving a whole bunch of platform specific files from Plugins/Process/Utility to Target is the right way to go. If anything that's moving platform specific files further up into generic code. Since the list of valid signals and th

[Lldb-commits] [PATCH] D108229: [lldb] Refactor Module::LookupInfo constructor

jingham accepted this revision. jingham added a comment. This revision is now accepted and ready to land. I like the change in general, anywhere that we're explicitly listing languages in generic code we're probably doing it wrong... So far as I can tell you've reconstructed the old behaviors c

[Lldb-commits] [PATCH] D108510: [lldb] Allow to register frame recognizers applied beyond the first instruction

jingham accepted this revision. jingham added a comment. This revision is now accepted and ready to land. LGTM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108510/new/ https://reviews.llvm.org/D108510 ___ lldb-commits mailing list lldb-commi

[Lldb-commits] [PATCH] D107079: [lldb] Add an option for emitting LLDB logs as JSON

jingham added a comment. I have a few little grammatical nits. The suggestion to use EnumArgs rather than a fresh argument type you can do or not as you see fit. It really seems to me that SBDebugger.EnableLog should provide a way to set the OutputFormat. For humans, the plain text is probabl

[Lldb-commits] [PATCH] D107521: [lldb/Plugins] Introduce Scripted Interface Factory

jingham added a comment. I just started reading this but I have to go now. Had one comment about errors from Dispatch. I'll look more later. Comment at: lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp:80 - return static_cast(*should_stop);

[Lldb-commits] [PATCH] D74157: [lldb/API] NFC: Reformat SBThread::GetStopDescription()

jingham added inline comments. Comment at: lldb/source/Target/Thread.cpp:601 raw_stop_description = stop_info_sp->GetDescription(); +assert(!raw_stop_description.empty() && + "StopInfo returned an empty description."); It is possible to have a

[Lldb-commits] [PATCH] D74255: [LLDB] Add support for AVR breakpoints

jingham added a comment. 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74255/new/ https://reviews.llvm.org/D74

[Lldb-commits] [PATCH] D73921: Assert that a subprogram should have a name when parsing DWARF

jingham added a comment. In D73921#1855961 , @davide wrote: > DWARFASTParserClang looks to me the wrong layer to fix this. Why can't this > be caught in the generic DWARF Parser? > I also believe that it's better if dwarfdump -verify crashes on this, rat

[Lldb-commits] [PATCH] D74136: [LLDB] WIP: Optionally follow DW_AT_decl_file when setting breakpoint

jingham added a comment. In D74136#1869622 , @kwk wrote: > @labath @jingham to summarize from what I read here and what I chatted about > with @labath , the following is a possible way to go for now, right? > > 1. We're not going to introduce my flag. >

[Lldb-commits] [PATCH] D74136: [LLDB] WIP: Optionally follow DW_AT_decl_file when setting breakpoint

jingham added a comment. 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 u

[Lldb-commits] [PATCH] D74136: [LLDB] WIP: Optionally follow DW_AT_decl_file when setting breakpoint

jingham added a comment. In D74136#1871751 , @labath wrote: > In D74136#1870029 , @jingham wrote: > > > In D74136#1869622 , @kwk wrote: > > > > > @labath @jingham to summariz

[Lldb-commits] [PATCH] D74478: [lldb] Let TypeSystemClang::GetDisplayTypeName remove anonymous and inline namespaces.

jingham added a comment. 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

[Lldb-commits] [PATCH] D74556: [lldb] Don't call CopyForBreakpoint from a Breakpoint's constructor

jingham accepted this revision. jingham added a comment. This revision is now accepted and ready to land. LGTM Repository: rLLDB LLDB CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74556/new/ https://reviews.llvm.org/D74556 ___ lldb-commi

[Lldb-commits] [PATCH] D74557: [lldb] Make BreakpointResolver hold weak_ptr instead of raw pointer to breakpoint

jingham added a comment. 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 asse

[Lldb-commits] [PATCH] D74558: [lldb] Make shared_from_this-related code safer

jingham accepted this revision. jingham added a comment. This revision is now accepted and ready to land. LGTM Targets and Breakpoints should only ever be managed by SP in lldb, so enforcing that seems like a good cleanup. Repository: rLLDB LLDB CHANGES SINCE LAST ACTION https://reviews.l

[Lldb-commits] [PATCH] D75004: Fix a race between lldb's packet timeout and killing the profile thread

jingham created this revision. jingham added a reviewer: clayborg. Herald added a project: LLDB. Herald added a subscriber: lldb-commits. The debugserver profile thread used to suspend itself between samples with a usleep. When you detach or kill, MachProcess::Clear would delay replying to the in

[Lldb-commits] [PATCH] D75004: Fix a race between lldb's packet timeout and killing the profile thread

This revision was automatically updated to reflect the committed changes. Closed by commit rG3cd13c4624b5: Fix a race between lldb's packet timeout and the profile thread's usleep. (authored by jingham). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D

[Lldb-commits] [PATCH] D74557: [lldb] Make BreakpointResolver hold weak_ptr instead of raw pointer to breakpoint

jingham accepted this revision. jingham added a comment. This revision is now accepted and ready to land. You mean for BreakpointResolverScripted::CreateImplementationIfNeeded? Passing in the BreakpointSP is fine. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74557/new/ https://review

[Lldb-commits] [PATCH] D75418: tab completion for process signal

jingham added a comment. I think this is just a bug in argument completion. The CommandInterpreter needs to sync up with the currently selected target/process/thread/frame before running commands. It COULD do that by having every API that changes the selected T/P/T/F above notify the CommandIn

[Lldb-commits] [PATCH] D74903: [lldb][testsuite] Create a SBDebugger instance for each test

jingham accepted this revision. jingham added a comment. 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. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74903/new/

[Lldb-commits] [PATCH] D75711: [NFC] Have ThreadPlans hold onto the Process & TID, rather than the Thread

jingham created this revision. jingham added reviewers: friss, clayborg. Herald added subscribers: lldb-commits, danielkiss, kristof.beyls. Herald added a project: LLDB. This is the first part of a multi-part commit. This change makes the ThreadPlans use the process & TID to specify which thread

[Lldb-commits] [PATCH] D75711: [NFC] Have ThreadPlans hold onto the Process & TID, rather than the Thread

jingham added a comment. Another point to make clear here. You might wonder whether ThreadPlan::GetThread should return an Optional or something like that, since there might not be a Thread associated with the ThreadPlan. I tried that out but it made the code really ugly. When I thought about

[Lldb-commits] [PATCH] D75711: [NFC] Have ThreadPlans hold onto the Process & TID, rather than the Thread

jingham updated this revision to Diff 248638. jingham added a comment. Attempt to appease clang-format. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75711/new/ https://reviews.llvm.org/D75711 Files: lldb/include/lldb/Target/ThreadPlan.h lldb/

<    7   8   9   10   11   12   13   14   15   16   >