[Lldb-commits] [PATCH] D63240: [Core] Generalize ValueObject::IsRuntimeSupportValue

jingham added a comment. In D63240#1561488 , @xiaobai wrote: > @jingham: Okay, so I tried to do what you suggested and that solution doesn't > work because of ObjC++. After looking into it, it looks like Variable and > Function just ask the CompileUnit f

[Lldb-commits] [PATCH] D62887: Update the thread list before setting stop reasons with an OS plugin

jingham updated this revision to Diff 206980. jingham added a comment. Addresses Greg's question about what happens when we load a new OS plugin. Indeed we should limit the work we do only to the case where we didn't have an OS plugin, then tried to load one and succeeded. Only then do we need

[Lldb-commits] [PATCH] D63914: Make the expression parser work for missing weak symbols

jingham created this revision. jingham added reviewers: JDevlieghere, clayborg. Herald added subscribers: lldb-commits, teemperor, abidh. Herald added a project: LLDB. lldb wasn't handling weak symbols, so if you were debugging a binary that used a weak symbol, and the symbol was missing, any ref

[Lldb-commits] [PATCH] D62887: Update the thread list before setting stop reasons with an OS plugin

jingham marked 2 inline comments as done. jingham added inline comments. Comment at: source/Target/Process.cpp:3037-3041 + // Somebody might have gotten threads before now, but we need to force the + // update after we've loaded the OperatingSystem plugin or it won't get a + /

[Lldb-commits] [PATCH] D63240: [Core] Generalize ValueObject::IsRuntimeSupportValue

jingham added a comment. This looks good to me. I wonder if the SymbolContextScope -> Language calculation that you do in IsRuntimeSupportValue should really be done in SymbolContextScope? If that's going to be the policy for going from SymbolContextScope, maybe centralize it there? CHANGES

[Lldb-commits] [PATCH] D62887: Update the thread list before setting stop reasons with an OS plugin

This revision was automatically updated to reflect the committed changes. jingham marked an inline comment as done. Closed by commit rL364666: Make sure the thread list is updated before you set the stop reason (authored by jingham, committed by ). Herald added a project: LLVM. Herald added a subs

[Lldb-commits] [PATCH] D63914: Make the expression parser work for missing weak symbols

jingham updated this revision to Diff 207146. jingham added a comment. Addressed Greg's comments Repository: rLLDB LLDB CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63914/new/ https://reviews.llvm.org/D63914 Files: include/lldb/Expression/IRExecutionUnit.h include/lldb/Symbol/S

[Lldb-commits] [PATCH] D63914: Make the expression parser work for missing weak symbols

jingham marked 8 inline comments as done. jingham added inline comments. Comment at: include/lldb/Symbol/Symbol.h:258 + m_is_weak : 1, m_type : 7; Mangled m_mangled; // uniqued symbol name/mangled name pair clayborg wrote: > change to: > ``

[Lldb-commits] [PATCH] D63914: Make the expression parser work for missing weak symbols

This revision was automatically updated to reflect the committed changes. jingham marked an inline comment as done. Closed by commit rL364686: Get the expression parser to handle missing weak symbols. (authored by jingham, committed by ). Herald added a project: LLVM. Herald added a subscriber: ll

[Lldb-commits] [PATCH] D63240: [Core] Generalize ValueObject::IsRuntimeSupportValue

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

[Lldb-commits] [PATCH] D63853: [Symbol] Add DeclVendor::FindTypes

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/D63853/new/ https://reviews.llvm.org/D63853 ___ lldb-commits mailing list lldb-commi

[Lldb-commits] [PATCH] D64163: Change LaunchThread interface to return an expected.

jingham added inline comments. Comment at: lldb/source/API/SBHostOS.cpp:110-117 + llvm::Expected thread = + ThreadLauncher::LaunchThread(name, thread_function, thread_arg); + if (!thread) { +llvm::consumeError(thread.takeError()); +return {}; + } + ---

[Lldb-commits] [PATCH] D64042: [Symbol] Improve Variable::GetLanguage

jingham added a comment. Why did you make this a function of Variable, rather than SymbolContextScope? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64042/new/ https://reviews.llvm.org/D64042 ___ lldb-commits mailing list lldb-commits@lists

[Lldb-commits] [PATCH] D64159: [Core] Generalize ValueObject::MaybeCalculateCompleteType

jingham added inline comments. Comment at: source/Target/ObjCLanguageRuntime.cpp:403 +CompilerType +ObjCLanguageRuntime::CalculateCompleteType(CompilerType base_type) { + CompilerType type_to_return; xiaobai wrote: > clayborg wrote: > > So a main question for Ob

[Lldb-commits] [PATCH] D64365: [lldb] Let table gen create command option initializers.

jingham added a comment. Do you know how you are going to do enum option values? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64365/new/ https://reviews.llvm.org/D64365 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://list

[Lldb-commits] [PATCH] D64599: [LanguageRuntime] Move CPPLanguageRuntime into a plugin

jingham requested changes to this revision. jingham added a comment. This revision now requires changes to proceed. My model for this was that there was a CPPLanguageRuntime.cpp that contains everything you can implement about the CPP runtime that is independent of any particular implementation

[Lldb-commits] [PATCH] D64599: [LanguageRuntime] Move CPPLanguageRuntime into a plugin

jingham added a comment. In D64599#1581604 , @labath wrote: > What is the indented relationship between CPPLanguage and CPPLanguageRuntime > plugins (and generally between any Language and its LanguageRuntime)? Right > now you're having the CPPLanguage d

[Lldb-commits] [PATCH] D64599: [LanguageRuntime] Move CPPLanguageRuntime into a plugin

jingham added a comment. In D64599#1581620 , @labath wrote: > In D64599#1581598 , @jingham wrote: > > > My model for this was that there was a CPPLanguageRuntime.cpp that contains > > everything you can implement a

[Lldb-commits] [PATCH] D64599: [LanguageRuntime] Move CPPLanguageRuntime into a plugin

jingham added inline comments. Comment at: source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp:55 #include "Plugins/Language/CPlusPlus/CPlusPlusLanguage.h" +#include "Plugins/LanguageRuntime/CPlusPlus/CPPLanguageRuntime.h" xiaobai wrote: > labath

[Lldb-commits] [PATCH] D64591: [Expression] Move IRDynamicChecks to ClangExpressionParser

jingham added a comment. This is a little unclear to me. LLVMUserExpression is the class that represents "anything that uses LLVM at its back end." Both the Swift & Clang user expressions are instances of this. Running through IR and inserting little callouts is an LLVMUserExpression type th

[Lldb-commits] [PATCH] D64599: [LanguageRuntime] Move CPPLanguageRuntime into a plugin

jingham accepted this revision. jingham added a comment. This revision is now accepted and ready to land. Okay... Provided we can come up with not too torturous ways to get the ObjC and Swift support out of generic-code, it seems okay to do this as a first step. I just don't want to end up in

[Lldb-commits] [PATCH] D64591: [Expression] Move IRDynamicChecks to ClangExpressionParser

jingham added a comment. Then the IRDynamicCheck part would go with LLVMUserExpression, and the C-specific checks in Clang... CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64591/new/ https://reviews.llvm.org/D64591 ___ lldb-commits mailing

[Lldb-commits] [PATCH] D64591: [Expression] Move IRDynamicChecks to ClangExpressionParser

jingham accepted this revision. jingham added a comment. This revision is now accepted and ready to land. That would be cleaner. OTOH, the original reason for these checkers was to help people understand crashes in their expressions more clearly. Supposedly, modern languages "don't have pointe

[Lldb-commits] [PATCH] D64670: [lldb][doc] Document how our LLDB table gen initialized options

jingham accepted this revision. jingham added a comment. This revision is now accepted and ready to land. Thanks for writing this up! Repository: rLLDB LLDB CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64670/new/ https://reviews.llvm.org/D64670 ___

[Lldb-commits] [PATCH] D63813: Adjust variable formatting table

jingham accepted this revision. jingham added a comment. This revision is now accepted and ready to land. That looks good. Note that the "address" format does more than show symbol/file/line offset, it will also print the string for addresses that point into the string pool. Might be worth men

[Lldb-commits] [PATCH] D64844: [Target] Remove Target::GetScratchClangASTContext

jingham requested changes to this revision. jingham added a comment. This revision now requires changes to proceed. This seems partly wrong to me. The point is that the Target holds keeps scratch AST contexts for all the languages it supports. They are the central repository for the accumulati

[Lldb-commits] [PATCH] D64844: [Target] Remove Target::GetScratchClangASTContext

jingham added a comment. I think I understand your goal - a worthy one, BTW... But I think this is an unnecessary step along that path. After all, all the clients of the Target's Scratch TypeSystem for C-family languages should be able to do what they need to do with a TypeSystem, rather than

[Lldb-commits] [PATCH] D64844: [Target] Remove Target::GetScratchClangASTContext

jingham added a comment. Seems like in most places we're just pulling out basic types or their sizes, which we should certainly be able to do with a TypeSystem. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64844/new/ https://reviews.llvm.org/D64844 _

[Lldb-commits] [PATCH] D64844: [Target] Remove Target::GetScratchClangASTContext

jingham added a comment. The nice thing about the way the ObjCLanguageRuntime::Get was that is was only useable where we decided it was legit to use it, in the actual ObjCLanguageRuntime plugin or its direct users. If you want to keep the convenience cast function in a header in Plugins/Expres

[Lldb-commits] [PATCH] D64897: Move start-address finding to Target, implement fallback if main executable does not have a start address

jingham added a comment. I wonder if we ought to allow target properties (target as an example) that are only for testing, so they don't print when you do settings list, etc. But the we could have some settings like a "target.testing.dont-read-LC_MAIN" and that would make it easy to automate y

[Lldb-commits] [PATCH] D64853: Fix CommandInterpreter for _regex-break with options

jingham added a comment. This looks good in general. I also have a few nits, and you need to fix (and probably test) the behavior for files with spaces in their names. Comment at: lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/regex_break/T

[Lldb-commits] [PATCH] D64964: [API] Remove use of ClangASTContext from SBTarget

jingham accepted this revision. jingham added a comment. This revision is now accepted and ready to land. This looks fine to me. Makes it really clear that we need SBTarget::FindFirstTypeForLanguage, etc. But FindFirstType was always a crapshoot anyway... CHANGES SINCE LAST ACTION https://

[Lldb-commits] [PATCH] D64042: [Symbol] Improve Variable::GetLanguage

jingham accepted this revision. jingham added a comment. This revision is now accepted and ready to land. Sounds reasonable. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64042/new/ https://reviews.llvm.org/D64042 ___ lldb-commits mailing l

[Lldb-commits] [PATCH] D88483: Add possibility to get module from SBType

jingham added a comment. You can make up types in the expression parser. So for instance, I can do: In D88483#2338408 , @fallkrum wrote: > Can you please give me a couple of words on what m_static_type and > m_dynamic_type is in TypeImpl class, in what

[Lldb-commits] [PATCH] D88483: Add possibility to get module from SBType

jingham added a comment. In D88483#2339845 , @fallkrum wrote: > In D88483#2339256 , @jingham wrote: > >> In D88483#2338408 , @fallkrum wrote: >> >>> Can you please give me a

[Lldb-commits] [PATCH] D88483: Add possibility to get module from SBType

jingham added a comment. In D88483#2341538 , @fallkrum wrote: > 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) { >

[Lldb-commits] [PATCH] D90011: [lldb] Redesign Target::GetUtilityFunctionForLanguage API

jingham added a comment. LGTM. Thanks for making this easier to use. Inside lldb, if you are ever implementing something that requires a call into the inferior and you're going to do it more than once over the course of a process's life, you really should use one of these guys rather than disp

[Lldb-commits] [PATCH] D78972: Treat hasWeakODRLinkage symbols as external in REPL computations

jingham added a comment. In D78972#2041680 , @labath wrote: > 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

[Lldb-commits] [PATCH] D90318: Return actual type from SBType::GetArrayElementType

jingham added a comment. This was done on purpose, in commit 902974277d507a149e33487d32e4ba58c41451b6 . The comment there is: Data formatters: Look through array element typedefs Summary: Motivation: When formatting

[Lldb-commits] [PATCH] D90332: Mark the execution of commands in stop hooks as non-interactive

jingham created this revision. jingham added a reviewer: JDevlieghere. Herald added a project: LLDB. Herald added a subscriber: lldb-commits. jingham requested review of this revision. stop-hooks were not intended to be interactive, that would be really confusing when there are many in flight. S

[Lldb-commits] [PATCH] D89157: [lldb] Report old modules from ModuleList::ReplaceEquivalent

jingham accepted this revision. jingham added a comment. This revision is now accepted and ready to land. 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 you

[Lldb-commits] [PATCH] D89157: [lldb] Report old modules from ModuleList::ReplaceEquivalent

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

[Lldb-commits] [PATCH] D89157: [lldb] Report old modules from ModuleList::ReplaceEquivalent

jingham added a comment. In D89157#2362697 , @JosephTremoulet wrote: >> 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

[Lldb-commits] [PATCH] D78972: Treat hasWeakODRLinkage symbols as external in REPL computations

jingham updated this revision to Diff 301752. jingham added a comment. Switched to !hasLocalLinkage. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78972/new/ https://reviews.llvm.org/D78972 Files: lldb/source/Expression/IRExecutionUnit.cpp Ind

[Lldb-commits] [PATCH] D78972: Treat hasWeakODRLinkage symbols as external in REPL computations

jingham added a comment. In D78972#2353643 , @labath wrote: > 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

[Lldb-commits] [PATCH] D88483: Add possibility to get module from SBType

jingham added a comment. In D88483#2363075 , @fallkrum wrote: > @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

[Lldb-commits] [PATCH] D90332: Mark the execution of commands in stop hooks as non-interactive

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 rGa37672e2db73: Mark the execution of stop-hooks as non-interactive.

[Lldb-commits] [PATCH] D78972: Treat hasWeakODRLinkage symbols as external in REPL computations

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 rGc8c07b76b2cf: Use !hasLocalLinkage instead of listing the symbol t

[Lldb-commits] [PATCH] D88483: Add possibility to get module from SBType

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

[Lldb-commits] [PATCH] D88483: Add possibility to get module from SBType

jingham added a comment. Sorry to be picky, but you can compress this by putting the IsValid checks into your find_* routines. And the: self.assertEqual(cu_type.GetName(), type2_name) calls aren't necessary, since you already found the type by matching the name. So I think you can simplify

[Lldb-commits] [PATCH] D88483: Add possibility to get module from SBType

jingham added a comment. 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. Maybe your executable is too arti

[Lldb-commits] [PATCH] D89156: [lldb] GetSharedModule: Collect old modules in SmallVector

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

[Lldb-commits] [PATCH] D90872: [TargetList] Simplify dummy target creation

jingham added inline comments. Comment at: lldb/source/Target/TargetList.cpp:293 // FIXME: Maybe the dummy target should be per-Debugger - if (!m_dummy_target_sp || !m_dummy_target_sp->IsValid()) { + if (!m_dummy_target_sp) { ArchSpec arch(Target::GetDefaultArchitecture

[Lldb-commits] [PATCH] D90872: [TargetList] Simplify dummy target creation

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

[Lldb-commits] [PATCH] D91103: [tooling] Add support for fixits that indicate code will need reformatting

jingham added a comment. 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 that fact that this change implements that behavior is entirely non-obvious at the call site. Could

[Lldb-commits] [PATCH] D91220: [ThreadPlan] Add a test for `thread step-in -r`, NFC

jingham added a comment. It's easier to read these tests if you use "lldbutil.run_to_source_breakpoint" to remove all that boilerplate. Other than that LGTM. Thanks for adding the test! Comment at: lldb/test/API/lang/c/stepping/TestThreadStepInAvoidRegexp.py:27-45 +e

[Lldb-commits] [PATCH] D91238: Recognize __builtin_debugtrap on arm64, advance pc past it so users can continue easily

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

[Lldb-commits] [PATCH] D77153: [lldb/DataFormatters] Display null C++ pointers as nullptr

jingham accepted this revision. jingham added a comment. 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 c

[Lldb-commits] [PATCH] D92164: Make SBDebugger internal variable getter and setter not use CommandInterpreter's context

jingham requested changes to this revision. jingham added a comment. This revision now requires changes to proceed. 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

[Lldb-commits] [PATCH] D92601: [lldb] Refactor the Symbolicator initializer

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/D92601/new/ https://reviews.llvm.org/D92601 ___ lldb-commits mailing list lldb-commits

[Lldb-commits] [PATCH] D92643: [lldb] Lookup static const members in FindGlobalVariables

jingham added a comment. In D92643#2433441 , @werat wrote: > In D92643#2433428 , @labath wrote: > >> Are the static members included in the (SB)Type object that gets created >> when parsing the enclosing type? If y

[Lldb-commits] [PATCH] D93052: "target create" shouldn't save target if the command failed

jingham added a comment. 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... It seems reasonable that the Debugger might want to create targets w/o selecting them. For instance, suppose we

[Lldb-commits] [PATCH] D92164: Make CommandInterpreter's execution context the same as debugger's one.

jingham added a comment. 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: (lldb) command source file_that_contains_a_step >>> Step hits a breakpoint which has commands One of those

[Lldb-commits] [PATCH] D92164: Make CommandInterpreter's execution context the same as debugger's one.

jingham accepted this revision. jingham added a comment. This revision is now accepted and ready to land. LGTM - thanks for doing this! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92164/new/ https://reviews.llvm.org/D92164 ___ lldb-commits

[Lldb-commits] [PATCH] D93052: "target create" shouldn't save target if the command failed

jingham accepted this revision. jingham added a comment. This revision is now accepted and ready to land. LGTM, thanks for the cleanup. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93052/new/ https://reviews.llvm.org/D93052 ___ lldb-commits

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

jingham accepted this revision. jingham added a comment. This revision is now accepted and ready to land. 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. I quibble a bit w

[Lldb-commits] [PATCH] D94077: Support unscoped enumeration members in the expression evaluator.

jingham added a comment. In D94077#2486225 , @werat wrote: > In D94077#2479984 , @shafik wrote: > >> We can have unscoped enums in namespace and class scope and the enumerators >> won't leak out from those scopes.

[Lldb-commits] [PATCH] D93895: Implement vAttachWait in lldb-server

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

[Lldb-commits] [PATCH] D93895: Implement vAttachWait in lldb-server

jingham added a comment. In D93895#2488372 , @clayborg wrote: > In D93895#2488249 , @jingham wrote: > >> looks good to me too. When you get around to the wait times & intervals I'd >> argue for not doing that as a

[Lldb-commits] [PATCH] D93874: [process] fix exec support on Linux

jingham added inline comments. Comment at: lldb/test/API/functionalities/exec/TestExec.py:91-93 +# Single step to create a thread plan. We have to make sure that after exec +# we clear all existing thread plans. +thread.StepInstruction(False)

[Lldb-commits] [PATCH] D93874: [process] fix exec support on Linux

jingham added inline comments. Comment at: lldb/test/API/functionalities/exec/TestExec.py:93 +# we clear all existing thread plans. +thread.StepInstruction(False) + clayborg wrote: > On Darwin threads have unique identifiers and the thread ID befo

[Lldb-commits] [PATCH] D94846: Allow breakpoints to be set on C++11 inline initializers

jingham created this revision. jingham requested review of this revision. Herald added a project: LLDB. Herald added a subscriber: lldb-commits. The inline initializers contribute code to the constructor(s). You will step onto them in the source view as you step through the constructor, for inst

[Lldb-commits] [PATCH] D95100: [lldb/Commands] Fix short option collision for `process launch`

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

[Lldb-commits] [PATCH] D94846: Allow breakpoints to be set on C++11 inline initializers

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 rGbff389120fa2: Fix a bug with setting breakpoints on C++11 inline i

[Lldb-commits] [PATCH] D95164: Make SBDebugger::CreateTargetWithFileAndArch accept lldb.LLDB_DEFAULT_ARCH

jingham created this revision. jingham added reviewers: clayborg, jasonmolenda, labath. jingham requested review of this revision. Herald added a project: LLDB. Herald added a subscriber: lldb-commits. The header docs in SBDebugger.i suggest: target = debugger.CreateTargetWithFileAndArch (exe,

[Lldb-commits] [PATCH] D95164: Make SBDebugger::CreateTargetWithFileAndArch accept lldb.LLDB_DEFAULT_ARCH

jingham added a comment. In D95164#2513644 , @clayborg wrote: > 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

[Lldb-commits] [PATCH] D93874: [process] fix exec support on Linux

jingham accepted this revision. jingham added a comment. This revision is now accepted and ready to land. Yes, this is how I should have done it originally. Thanks! I had one suggestion for making the test more compact which you can do or not as you please. Other than the LGTM.

[Lldb-commits] [PATCH] D95164: Make SBDebugger::CreateTargetWithFileAndArch accept lldb.LLDB_DEFAULT_ARCH

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 rGf05dc40c31d1: Fix SBDebugger::CreateTargetWithFileAndArch to accep

[Lldb-commits] [PATCH] D95411: [lldb] Remove unused ThreadPlanStack::GetStackOfKind (NFC)

jingham accepted this revision. jingham added a comment. This revision is now accepted and ready to land. Herald added a subscriber: JDevlieghere. The inverse change to this would be to have all the places where we ask questions about the PlanStacks in a ThreadPlanStack through this API. I put i

[Lldb-commits] [PATCH] D95761: [lldb] Use current execution context in SBDebugger

jingham added a comment. I think it's fine to fall back to the "currently selected target" if the command interpreter doesn't have a current execution context, that should not be your first choice. For instance, if you are running this command in a Python breakpoint callback, then the current

[Lldb-commits] [PATCH] D95710: [lldb/Commands] Add command options for ScriptedProcess to ProcessLaunch

jingham added a comment. I'm not quite sure why we need the extra m_scripted_process boolean. Seems like you could key off whether we had a non-empty class_name. Is there any case where you would want to boolean set w/o having a class name yet? Comment at: lldb/include/lldb

[Lldb-commits] [PATCH] D96276: [lldb] Inline invariant params to AppleThreadPlanStepThrough (NFC)

jingham added a comment. I can't think of a really good reason why you would need to override the general "step in avoids nodebug" behavior. I'm pretty sure I was thinking of a trampoline that got you half way to somewhere interesting, at which point you would want to negotiate again for the t

[Lldb-commits] [PATCH] D96277: [lldb] Minor cleanups to ThreadPlan.h (NFC)

jingham added a comment. This is okay. It seems like CachePlanExplainsStop wasn't used because it's done by hand instead. A better patch might be to stop doing that and use the function instead? But the argument's not terribly compelling either way... Repository: rG LLVM Github Monorepo

[Lldb-commits] [PATCH] D96276: [lldb] Inline invariant params to AppleThreadPlanStepThrough (NFC)

jingham added a comment. In D96276#2549406 , @kastiglione wrote: > In D96276#2549007 , @jingham wrote: > >> It still seems to me like a trampoline which knew that to implement itself, >> all it had to do was a coup

[Lldb-commits] [PATCH] D96368: Document the "extra_args" parameter to scripted breakpoint callbacks

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. I forgot to document this when adding the feature. Added docs here in both the python-reference.rst and in the

[Lldb-commits] [PATCH] D96370: Pass enviroment variables to python scripts.

jingham requested changes to this revision. jingham added a comment. This revision now requires changes to proceed. Modifying the target environment in order to add something to the host lldb's PYTHONPATH seems very counterintuitive to me. It is misusing the purpose of the target environment se

[Lldb-commits] [PATCH] D96368: Document the "extra_args" parameter to scripted breakpoint callbacks

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG365b186c242b: Add documentation for the extra_args parameter to breakpoint commands. (authored by jingham). Repository: rG LLVM Github Monorepo C

[Lldb-commits] [PATCH] D96368: Document the "extra_args" parameter to scripted breakpoint callbacks

jingham added inline comments. Comment at: lldb/source/Commands/CommandObjectBreakpointCommand.cpp:127 + +def breakpoint_callback(frame, bp_loc, extra_args, dict): + # Your code goes here kastiglione wrote: > How about `internal_dict`, the name used elsewhere, a

[Lldb-commits] [PATCH] D78242: [lldb/Docs] Add some more info about the test suite layout

jingham added a comment. This is great, thanks for doing this. You point people at lldbutils.py as a source for utility functions (like print_stacktrace, etc...) But it is maybe also worth pointing them at the lldbtest.py for extensions runCmd and some of the other testing primitives we've ad

[Lldb-commits] [PATCH] D78421: Fix out of sync source code/executable when debugging

jingham added a comment. I can see the cases where this would help, but the way you are doing it could lead to some odd behavior that might be confusing. Suppose I have two shared libraries, libBar.dylib and libNotBar.dylib in my project. I debug the process, find a bug in FileFromNotBar.c. S

[Lldb-commits] [PATCH] D78972: Treat hasWeakODRLinkage symbols as external in REPL computations

jingham created this revision. jingham added a reviewer: JDevlieghere. jingham added a project: LLDB. Herald added a subscriber: lldb-commits. The way both the REPL and the --top-level mode of the expression parser work is that they compile and JIT the code the user provided, and then look throug

[Lldb-commits] [PATCH] D78972: Treat hasWeakODRLinkage symbols as external in REPL computations

jingham added a comment. In D78972#2007493 , @labath wrote: > > I don't have a way to write a C-based test for this as I don't know how to > > craft a user expression in C that will make such a thing. I asked around a > > bit and nobody had an easy way t

[Lldb-commits] [PATCH] D78972: Treat hasWeakODRLinkage symbols as external in REPL computations

jingham added a comment. In D78972#2010033 , @labath wrote: > In D78972#2008129 , @jingham wrote: > > > In D78972#2007493 , @labath wrote: > > > > > > I don't have a way to w

[Lldb-commits] [PATCH] D78972: Treat hasWeakODRLinkage symbols as external in REPL computations

jingham added a comment. In D78972#2012796 , @labath wrote: > In D78972#2011571 , @jingham wrote: > > > That one does work all the way, including calling the function. That > > should be surprising. It shouldn't

[Lldb-commits] [PATCH] D79308: [lldb-server] Reset stop reason of all threads when resuming

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

[Lldb-commits] [PATCH] D79554: [lldb/Dataformatter] Add support to CF{Dictionary, Set}Ref types

jingham added a comment. The way the ValueObject code works w.r.t. Synthetic child providers is that you first look up and make a ValueObject for the actual value the user requested, (for instance a ValueObjectVariable or a ValueObjectChild or a ValueObjectConstResult for expressions, etc.), th

[Lldb-commits] [PATCH] D79554: [lldb/Dataformatter] Add support to CF{Dictionary, Set}Ref types

jingham added inline comments. Comment at: lldb/source/Core/ValueObject.cpp:2846 + if (llvm::isa(compiler_type.GetTypeSystem())) { +if (HasSyntheticValue()) { + TargetSP target_sp = GetTargetSP(); mib wrote: > friss wrote: > > I am understan

[Lldb-commits] [PATCH] D79659: [lldb/Commands] Add ability to run shell command on the host.

jingham added a comment. I do something similar with CommandObjectThreadStepWithTypeAndScope so that I can share most of that command code, if you want to see an example of doing that. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79659/new/ http

[Lldb-commits] [PATCH] D79659: [lldb/Commands] Add ability to run shell command on the host.

jingham added a comment. We certainly shouldn't duplicate the code for CommandObjectPlatformShell. Why don't you just add a "m_use_host_platform" ivar to CommandObjectPlatformShell, and make two instances of CommandObjectPlatformShell, one with m_use_host_platform set to false and added as a s

[Lldb-commits] [PATCH] D79666: Complete breakpoint enable/disable/delete/modify with a list of breakpoint IDs

jingham added a comment. This is great, but you can also specify breakpoints by name. Should be possible to also complete on the list of breakpoint names. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79666/new/ https://reviews.llvm.org/D79666

<    12   13   14   15   16   17   18   19   >