[Lldb-commits] [PATCH] D46083: Move the persistent variable counter into Target

jingham added a comment. Eh, must have. The page really shouldn't let you overall submit if there are uncommitted comments, but whatever. Let's see if I did it right this time.. Comment at: source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp:668 ConstString ClangUs

[Lldb-commits] [PATCH] D46088: Refactor GetNextPersistentVariableName into a non-virtual method

jingham accepted this revision. jingham added a comment. This revision is now accepted and ready to land. That's fine. https://reviews.llvm.org/D46088 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/li

[Lldb-commits] [PATCH] D46395: Remove Process references from the Host module

jingham accepted this revision. jingham added a comment. This revision is now accepted and ready to land. This is a cleaner approach. Might be worth adding a comment in the MonitoringProcessLauncher.h saying that the monitoring callback in the ProcessLaunchInfo is required? You might from the

[Lldb-commits] [PATCH] D46551: Inject only relevant local variables in the expression evaluation context

jingham accepted this revision. jingham added a comment. I'm still a little sad we can't get this to happen correctly in clang's lookup, but this is a clever way to get the benefit of this workaround without paying all the cost, and is a fine temporary solution. The implementation looks okay to

[Lldb-commits] [PATCH] D31172: Move stop info override callback code from ArchSpec into Process

jingham requested changes to this revision. jingham added a comment. This revision now requires changes to proceed. It looks like you are bundling two changes into one patch, one to move the StopInfoOverrideCallback from ArchSpec to Process, and one to change how the help for the list of archite

[Lldb-commits] [PATCH] D31172: Move stop info override callback code from ArchSpec into Process

jingham added a comment. None of this isn't modeling the situation particularly clearly, since we have "architecture specific modifications to general behaviors" that aren't coming in through plugins so that it would be easy to modify the behavior for new architectures. Instead, we're requiring

[Lldb-commits] [PATCH] D31283: Expose hit count via SBBreakpointLocation.

jingham requested changes to this revision. jingham added a comment. This revision now requires changes to proceed. If you are exposing this, could you write a test to make sure it stays correct? I added a sample test that you can easily copy and modify in: packages/Python/lldbsuite/test/sample

[Lldb-commits] [PATCH] D31283: Expose hit count via SBBreakpointLocation.

jingham added a comment. BTW, other than the lack of a test, this is fine. https://reviews.llvm.org/D31283 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D31172: Move stop info override callback code from ArchSpec into Process

jingham added a comment. Yes, that seems like the cleanest solution. https://reviews.llvm.org/D31172 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D31357: Support Unit Testing debugserver

jingham added a comment. Todd added a gtest target to the lldb.xcodeproj. Presumably you could use that as a template and do the same thing in the tools/debugserver.xcproj. Then make an aggregate target in the lldb workspace that runs these two targets. https://reviews.llvm.org/D31357 ___

[Lldb-commits] [PATCH] D31368: Add support for sythetic operator dereference

jingham requested changes to this revision. jingham added a comment. This revision now requires changes to proceed. The idea here seems fine, but why does StackFrame have to know the magic $$dereference$$? Why can't it call the Dereference on the synthetic value? https://reviews.llvm.org/D3136

[Lldb-commits] [PATCH] D31366: Do not dereference std::unique_ptr by default

jingham requested changes to this revision. jingham added a comment. This revision now requires changes to proceed. This test compiles correctly on Darwin if you pass the --std=c++11 flag. lldbtest has a getstdFlag that returns the correct std value in this case. Can you get the test running o

[Lldb-commits] [PATCH] D31357: Support Unit Testing debugserver

jingham added a comment. It's a little weird to have the unit tests for debugserver in the top-level lldb directory. debugserver really is a stand-alone tool that shares no code with lldb proper. How hard would it be to put the tests under debugserver? https://reviews.llvm.org/D31357

[Lldb-commits] [PATCH] D31357: Support Unit Testing debugserver

jingham added a comment. debugserver only runs on darwin and we have no intentions of using it elsewhere. lldb-server is the way to do debugserver for new platforms. I don't think you need the generality provided by host to test debugserver, OTOH, if using those classes saves you lots of time

[Lldb-commits] [PATCH] D31366: Do not dereference std::unique_ptr by default

jingham added a comment. There's no reason you couldn't build the gnu libstdc++ on Darwin. Anyway, if that's the problem, I'm pretty sure the testsuite has a way to conditionalize on which stdlib(s) are available. That would be clearer than conditionalizing on platform. https://reviews.llvm

[Lldb-commits] [PATCH] D31368: Add support for sythetic operator dereference

jingham requested changes to this revision. jingham added a comment. This revision now requires changes to proceed. This seems great. The only remaining thing is to document this in the Synthetic Children section of the Variable Formatting page (www/varformats.html). https://reviews.llvm.org/

[Lldb-commits] [PATCH] D31371: Stop calling ValueObject::SetName from synthetic child providers

jingham requested changes to this revision. jingham added a comment. This revision now requires changes to proceed. If SetName is the wrong thing to use for synthetic child providers, then we need to make Clone available to the SB API's as well. In any case where they want to return an extant V

[Lldb-commits] [PATCH] D31450: Battery of NetBSD support improvements

jingham resigned from this revision. jingham added a comment. Kyril, I haven't been involved in the lldb-server parts of lldb. Greg sketched out those interfaces and mostly folks working on Windows & Linux have fleshed them out. I haven't been following the design discussions for lldb-server,

[Lldb-commits] [PATCH] D31368: Add support for sythetic operator dereference

jingham requested changes to this revision. jingham added a comment. This revision now requires changes to proceed. One grammar comment in the docs, and then this is good. Comment at: www/varformats.html:1071 [3] This method is optional (starting with SVN revision 219330). Th

[Lldb-commits] [PATCH] D31368: Add support for sythetic operator dereference

jingham accepted this revision. jingham added a comment. This revision is now accepted and ready to land. Excellent! https://reviews.llvm.org/D31368 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/list

[Lldb-commits] [PATCH] D31371: Stop calling ValueObject::SetName from synthetic child providers

jingham accepted this revision. jingham added a comment. This revision is now accepted and ready to land. Good. https://reviews.llvm.org/D31371 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/

[Lldb-commits] [PATCH] D31366: Do not dereference std::unique_ptr by default

jingham accepted this revision. jingham added a comment. This revision is now accepted and ready to land. MacOS hasn't shipped with gcc for a while now. If you were serious about using gcc & its STL implementation you would install your own copies of the tools & libraries. So what's on the sys

[Lldb-commits] [PATCH] D31357: Support Unit Testing debugserver

jingham added a comment. On the small points side: for lldb code we use lower-case, _ separated names for local variables, the point being that it allow you to tell at a glance what's a local and what's an ivar. Looks like you use a mixture of the two styles? exit_status, result, etc. alongsi

[Lldb-commits] [PATCH] D31451: New C++ function name parsing logic

jingham added a comment. I'm sorry, I don't have time actually review the code here for correctness... But can you make sure that this also rejects a two or three field selector, not just "selector:" but "selector:otherField:"? That seems sufficiently different that you might get the one : bu

[Lldb-commits] [PATCH] D32100: [Expression parser] Return both types and variables

jingham accepted this revision. jingham added a comment. This revision is now accepted and ready to land. This looks okay on the lldb side. Thanks for adding the test cases. I do worry that we're going to end up looking too far afield for types when the local variable. But we can make it corre

[Lldb-commits] [PATCH] D32375: [DWARF] Fix lookup in the abstract origins of inlined blocks/functions

jingham added a comment. Except for the comment about FindFirstChildWithAbstractOrigin, this seems fine. Comment at: source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:3723-3742 +static DWARFDIE FindAnyChildWithAbstractOrigin(const DWARFDIE &context) { + for (DWARFDIE can

[Lldb-commits] [PATCH] D32503: Remove unused code related to CPlusPlusLanguage::FindEquivalentNames

jingham added a comment. This was used at some point. I'd be happier deleting if if I understood the reason why it is no longer needed. Repository: rL LLVM https://reviews.llvm.org/D32503 ___ lldb-commits mailing list lldb-commits@lists.llvm.or

[Lldb-commits] [PATCH] D32168: [LLDB][MIPS] Fix TestStepOverBreakpoint.py failure

jingham requested changes to this revision. jingham added a comment. This revision now requires changes to proceed. The substance seems fine. I'm not sure I would guess what GetInstructionsCount with canSetBreakpoint == true would do without reading the code. You could fix this with a more exp

[Lldb-commits] [PATCH] D32168: [LLDB][MIPS] Fix TestStepOverBreakpoint.py failure

jingham requested changes to this revision. jingham added a comment. This revision now requires changes to proceed. Couple tiny tweaks to the comment, and this is good to go. Comment at: include/lldb/API/SBInstructionList.h:36-38 + // Its return the number of instructions betw

[Lldb-commits] [PATCH] D32598: Prime module caches outside of the module list lock for better parallelism

jingham added a reviewer: clayborg. jingham added a comment. Adding Greg as a Reviewer. Repository: rL LLVM https://reviews.llvm.org/D32598 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/l

[Lldb-commits] [PATCH] D32598: Prime module caches outside of the module list lock for better parallelism

jingham requested changes to this revision. jingham added a comment. This revision now requires changes to proceed. Instead of having the cache priming be determined by platform or something like that, it would be better to add a setting (on the target level seems right) that controls this. I t

[Lldb-commits] [PATCH] D32598: Prime module caches outside of the module list lock for better parallelism

jingham added a comment. Ah, sorry. I was talking about an lldb, which you access through the "setting set/get" command. Those are actually implemented by adding a property on the class in question. Look in Target.cpp for the TargetProperties class. You'll just want to add another property

[Lldb-commits] [PATCH] D32598: Prime module caches outside of the module list lock for better parallelism

jingham requested changes to this revision. jingham added a comment. This revision now requires changes to proceed. This is picky but can you call it "symbol-cache-priming". The help text explains it, but this is a very specific cache so we should scope it in case we have some other one later o

[Lldb-commits] [PATCH] D32598: Prime module caches outside of the module list lock for better parallelism

jingham added a comment. Yes, I like that better too. Repository: rL LLVM https://reviews.llvm.org/D32598 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D32598: Prime module caches outside of the module list lock for better parallelism

jingham added a comment. That test is good. That tests the lazy lookup of the dwarf type indices. The other thing this changes is symbol reading. Can you also add a similar test that relies on finding a symbol in a shared library we might not have read in? It probably fine to add it to this

[Lldb-commits] [PATCH] D32598: Prime module caches outside of the module list lock for better parallelism

jingham accepted this revision. jingham added a comment. This revision is now accepted and ready to land. Looks good. Thanks for working on this! Repository: rL LLVM https://reviews.llvm.org/D32598 ___ lldb-commits mailing list lldb-commits@list

[Lldb-commits] [PATCH] D32626: Make the symbol demangling loop order independent

jingham added a reviewer: clayborg. jingham added a comment. Adding Greg as a reviewer. You can generally see from the CODE_OWNERS.txt file who has overall responsibility for areas in lldb, and you should at least assign them as a reviewer. For Symbol parsing stuff that's definitely Greg. R

[Lldb-commits] [PATCH] D32598: Prime module caches outside of the module list lock for better parallelism

jingham closed this revision. jingham added a comment. Sure. Closed with r301609. Repository: rL LLVM https://reviews.llvm.org/D32598 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-c

[Lldb-commits] [PATCH] D32168: [LLDB][MIPS] Fix TestStepOverBreakpoint.py failure

jingham accepted this revision. jingham added a comment. This revision is now accepted and ready to land. Looks good, thanks! https://reviews.llvm.org/D32168 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mai

[Lldb-commits] [PATCH] D32732: "target variable" not showing all global variable if we print any global variable before execution starts

jingham added a reviewer: jingham. jingham requested changes to this revision. jingham added a comment. This revision now requires changes to proceed. I actually think the behavior you are seeing is the designed behavior, but it isn't clearly documented. Target variable is designed to help manag

[Lldb-commits] [PATCH] D30007: [lldb] Provide API to know which sanitizer generated an eStopReasonInstrumentation

jingham accepted this revision. jingham added a comment. This revision is now accepted and ready to land. Sorry for the delay. That's fine. https://reviews.llvm.org/D30007 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.

[Lldb-commits] [PATCH] D32732: "target variable" not showing all global variable if we print any global variable before execution starts

jingham added a comment. Yes, I was printing a global variable that WASN'T defined in the source file I later stopped in. So it worked for me. The bug is really that looking up a variable by name goes through and adds only that variable to the CU that defines it, but the CU doesn't make note

[Lldb-commits] [PATCH] D33083: [Expression parser] Look up module symbols before hunting globally

jingham added a comment. Yes, this seems obviously right. https://reviews.llvm.org/D33083 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D33083: [Expression parser] Look up module symbols before hunting globally

jingham added a comment. Actually, I take that back. Why do you have to call FindGlobalDataSymbol twice? Shouldn't FindGlobalDataSymbol do that work for you. After all you passed in the module. It should itself prefer symbols in the module... It also seems wrong that we're just picking the

[Lldb-commits] [PATCH] D33083: [Expression parser] Look up module symbols before hunting globally

jingham added a comment. If you just have symbols, you can't tell whether private symbols were visible to the current frame or not, but the likelihood is not 0 (I guess it's something like 1/number of CU's). OTOH, your expression could never have seen a private symbol from another module, and

[Lldb-commits] [PATCH] D33083: [Expression parser] Look up module symbols before hunting globally

jingham added a comment. But note that the real solution to that problem is implementing some syntax for "symbol from CU", "symbol from Function" etc, as we've discussed in the past. Then what we do by default will be less important, since you have a way to override it. https://reviews.llvm.

[Lldb-commits] [PATCH] D33083: [Expression parser] Look up module symbols before hunting globally

jingham added a comment. I think Greg's right. I can't see anything expression parser specific to how you do this search. I was a little worried that a lot of the other searches will return multiple names (maybe sorting to indicate closeness.) So this one might be confusing. But the API na

[Lldb-commits] [PATCH] D32732: "target variable" not showing all global variable if we print any global variable before execution starts

jingham added a comment. That looks like the right way to do it. What was your thinking behind returning null rather than the partial list of variables already parsed if can_create is false? That doesn't seem like what somebody who is bothering to pass in can_create false intends. Repositor

[Lldb-commits] [PATCH] D33167: Get rid of some uses of StringConvert and reduce some indentation

jingham requested changes to this revision. jingham added a comment. This revision now requires changes to proceed. This looks fine. Like Kamil, I think it would help to document your new interfaces. Going away from StringConvert, you are switching from an API that gives a value on fail to one

[Lldb-commits] [PATCH] D33241: Add Status -- llvm::Error glue

jingham accepted this revision. jingham added a comment. This revision is now accepted and ready to land. This seems fine to me. Zachary didn't give reasons why he didn't like the conversion form so I can't really assess that point. The use in the ErrorConversion test case seems pretty natural

[Lldb-commits] [PATCH] D33283: RunThreadPlan: Fix halting logic in IgnoreBreakpoints = false

jingham requested changes to this revision. jingham added a comment. This revision now requires changes to proceed. The first code site did: checking for thread plan success, then check for hit breakpoint, then for anything else (thread plan failed or other stop reason.) The code in the first o

[Lldb-commits] [PATCH] D33426: Introduce new command: thread unique-stacks

jingham requested changes to this revision. jingham added a comment. This revision now requires changes to proceed. The idea is great. I think it is a little confusing that you would do: (lldb) thread backtrace all to get all threads but (lldb) thread unique-stacks to backtrace the unique s

[Lldb-commits] [PATCH] D33426: Introduce new command: thread backtrace unique

jingham requested changes to this revision. jingham added a comment. This revision now requires changes to proceed. Pavel's right, it would be good to add a test case. You could modify the test case in packages/Python/lldbsuite/test/functionalities/thread/num_threads/ to this end. Note this Te

[Lldb-commits] [PATCH] D33283: RunThreadPlan: Fix halting logic in IgnoreBreakpoints = false

jingham added a comment. That looks right to me, and is much nicer to read. I think "plan failure" once you've actually kicked off the execution of a function calling thread plan is theoretical, there are plenty of ways the plan can fail, though at present all the ways I can think of would happ

[Lldb-commits] [PATCH] D33283: RunThreadPlan: Fix halting logic in IgnoreBreakpoints = false

jingham accepted this revision. jingham added a comment. This revision is now accepted and ready to land. Oh, yeah, check the checkbox, Jim... https://reviews.llvm.org/D33283 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llv

[Lldb-commits] [PATCH] D33283: RunThreadPlan: Fix halting logic in IgnoreBreakpoints = false

jingham accepted this revision. jingham added a comment. This revision is now accepted and ready to land. The fix seems good. The fact that a breakpoint hit while evaluating an expression doesn't check the condition is a known limitation. You've got to protect against artificial recursions in h

[Lldb-commits] [PATCH] D32732: "target variable" not showing all global variable if we print any global variable before execution starts

jingham accepted this revision. jingham added a comment. This revision is now accepted and ready to land. Okay, that seems reasonable. Repository: rL LLVM https://reviews.llvm.org/D32732 ___ lldb-commits mailing list lldb-commits@lists.llvm.org h

[Lldb-commits] [PATCH] D33812: [TypeSystem] Handle Clang AttributedTypes

jingham accepted this revision. jingham added a comment. This revision is now accepted and ready to land. That seems correct since we aren't recording the attributes in the type flags. The only question I have is that you're assuming something that has a TypeClass of Attributed will always conve

[Lldb-commits] [PATCH] D33812: [TypeSystem] Handle Clang AttributedTypes

jingham added a comment. Okay. If the attributed types were hand-built types from DWARF, I would still worry a bit about the getModifiedType() call. But these only come from modules, and those are built by orthodox methods so they should be self-consistent. Repository: rL LLVM https://re

[Lldb-commits] [PATCH] D33426: Introduce new command: thread backtrace unique

jingham accepted this revision. jingham added a comment. This revision is now accepted and ready to land. This looks fine to me. https://reviews.llvm.org/D33426 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/

[Lldb-commits] [PATCH] D34236: Delete ProcessLauncherPosix

jingham added a comment. This patch doesn't remove support for posix_spawn on OS X - it is moving to the macOS Host.mm. Kind of sad as posix_spawn was supposed to remove a lot of the platform-dependent and error-prone parts of process launching from the system. But you have to live with what

[Lldb-commits] [PATCH] D34553: Shorten sanitizer plugin names

jingham accepted this revision. jingham added a comment. This revision is now accepted and ready to land. I try to avoid making cryptic file names just to get them under some character count, since that also makes code hard to comprehend particularly for new users. But in this case everybody ca

[Lldb-commits] [PATCH] D34872: Update lldb architecture docs

jingham accepted this revision. jingham added a comment. This revision is now accepted and ready to land. Thanks! https://reviews.llvm.org/D34872 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinf

[Lldb-commits] [PATCH] D35083: [TypeSystem] Guard the global `ASTSourceMap` with a mutex

jingham added a comment. This is an awfully complex solution which in the end doesn't actually enforce that you take the lock to get the SourceMap. You have to know to wrap the access in this WithExclusiveSourceMap. Wouldn't it be simpler to make GetSourceMap take a reference to a std::unique

[Lldb-commits] [PATCH] D35083: [TypeSystem] Guard the global `ASTSourceMap` with a mutex

jingham added a comment. The ThreadSafety analysis stuff looks interesting, but just to be sure I understand: You said "alternatively we could..." but from what I could tell the thread safety is markup on already extant mechanisms. It didn't look like it provided any locking on it's on, it on

[Lldb-commits] [PATCH] D35083: [TypeSystem] Guard the global `ASTSourceMap` with a mutex

jingham added a comment. Ah, maybe you meant applying the thread safety annotation to Sean's solution to enforce the contract. That's an interesting solution, but makes a non-straightforward solution even more non-straightforward, so I agree this isn't the best example... Repository: rL LL

[Lldb-commits] [PATCH] D35083: [TypeSystem] Guard the global `ASTSourceMap` with a mutex

jingham added a comment. In https://reviews.llvm.org/D35083#802439, @spyffe wrote: > Responded to Lang's comments inline. > > **Jim**: you say this patch "doesn't actually enforce that you take the lock > to get the SourceMap." How do you get the source map otherwise? It's static > inside the

[Lldb-commits] [PATCH] D35083: [TypeSystem] Guard the global `ASTSourceMap` with a mutex

jingham added a comment. This is a fairly common idiom in lldb and seems to me quite obvious. If the API to get an object requires a lock guard of some sort, then you have to hold the lock while using the object. As a general practice requiring a wrapper like this for every use of a should be

[Lldb-commits] [PATCH] D31172: Move stop info override callback code from ArchSpec into Process

jingham added a comment. This is a good addition. However, it seems to me that since you only need an ArchSpec to make one of these Architecture plugins, which plugin you get seems fully determined by the Target, not the Process. I understand that the only need for it at present is to do a Pr

[Lldb-commits] [PATCH] D31172: Move stop info override callback code from ArchSpec into Process

jingham added a comment. Sorry I wasn't clear. I meant that since the Target knows everything it needs to know to vend the correct Architecture plugin, you should get it from the Target, not the Process. In general, I think that the highest class in the stack that can vend a plugin is the one

[Lldb-commits] [PATCH] D31172: Move stop info override callback code from ArchSpec into Process

jingham added a comment. What Greg said... https://reviews.llvm.org/D31172 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D35607: Extend 'target symbols add' to load symbols from a given file by UUID.

jingham requested changes to this revision. jingham added a comment. This revision now requires changes to proceed. This seems like an okay change. You want to make sure that passing the file name and UUID into AddModuleSymbols doesn't override a UUID mismatch against the symbol file however yo

[Lldb-commits] [PATCH] D35311: lldb-server tests: Add support for testing debugserver

jingham added a comment. This isn't about this patch, but replying to: > debugserver replies to the k packet instead of just dropping the connection - > stopping code adjusted, although we should probably consider aligning the > behavior of the two stubs in this case Especially when dealing wi

[Lldb-commits] [PATCH] D35525: Fix "help format" output

jingham added a comment. Yes, your version clearly has the test right. I was wondering why this didn't have a more dramatic ill-effect, but it turns out that this function is only used for printing argument help text, which it clearly messes up: (lldb) help format --

[Lldb-commits] [PATCH] D35525: Fix "help format" output

jingham added a comment. BTW, the second listing was after applying your patch, showing it clearly does the right thing... https://reviews.llvm.org/D35525 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailm

[Lldb-commits] [PATCH] D35740: Fix PR33875 by distinguishing between DWO and clang modules

jingham accepted this revision. jingham added a comment. This revision is now accepted and ready to land. I was under the impression that this was found by a test case failure. Anyway, given that we've got different kinds of DWARF CU's, wouldn't it be better to add an attribute describing them,

[Lldb-commits] [PATCH] D35760: Expose active and available platform lists via SBDebugger API

jingham requested changes to this revision. jingham added a comment. This revision now requires changes to proceed. This seems fine. I have a little quibble with the name, inline, and a testing suggestion. Comment at: packages/Python/lldbsuite/test/functionalities/platform/T

[Lldb-commits] [PATCH] D35760: Expose active and available platform lists via SBDebugger API

jingham added a comment. Huh, not sure how the other comment vanished. I was concerned that "GetAvailablePlatformAtIndex" didn't actually get a platform, so the name was confusing. I suggested GetAvailablePlatformInfoAtIndex as more accurate. Repository: rL LLVM https://reviews.llvm.org/D

[Lldb-commits] [PATCH] D35083: [TypeSystem] Guard the global `ASTSourceMap` with a mutex

jingham accepted this revision. jingham added a comment. This revision is now accepted and ready to land. Ack! I thought when you said you were "going to simply pass..." you hadn't done it yet, so I didn't look at the source changes. Anyway, this looks fine to me. Repository: rL LLVM https

[Lldb-commits] [PATCH] D35881: Expose process instance info via SB API

jingham accepted this revision. jingham added a comment. This revision is now accepted and ready to land. This is a useful addition. I don't have a problem with it going in as is. It is unfortunate that we don't have an LLDB_INVALID_{USER,GROUP}_ID we could use to tell us whether the ID was inv

[Lldb-commits] [PATCH] D36126: Fix incorrect use of std::unique

jingham accepted this revision. jingham added a comment. This revision is now accepted and ready to land. This bug has been there since the first llvm.org checkin. I'm curious, did it actually cause a problem or did you just see it while reading? The patch is right. Most of the other uses elid

[Lldb-commits] [PATCH] D36485: Add existing unit tests to Xcode project

jingham accepted this revision. jingham added a comment. This revision is now accepted and ready to land. Looks good. https://reviews.llvm.org/D36485 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/lis

[Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

jingham requested changes to this revision. jingham added a comment. This seems like an awkward solution to me. Having CanResume and DoResume seems like it over-determines the problem. Why wasn't it enough for DoResume to fail and the error to be propagated? I'm also uncomfortable that this is

[Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

jingham added a comment. But whatever the solution, you should never use llvm::report_fatal_error. It's fine to put in an lldb_assert which will assert when the error is hit in the test suite, but not in released lldb's. After all, the worst that will happen if somebody working on lldb were t

[Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

jingham added a comment. lldb is a library used in other programs (Xcode, VSCode etc) and it can support many simultaneous debug sessions and each debug session can support many simultaneous targets. Unless this failure is going to make all the other debug sessions fail, and the other target s

[Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

jingham added a comment. As for the overdetermined remark, it's what you suspect, that you are now asking two questions to get one answer, did the resume succeed. If the resume didn't succeed the state should be set back to stopped no matter why it didn't succeed. So you are fixing only one s

[Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

jingham added a comment. And I don't understand your answer to Adrian. In the case of core files, DoResume is clearly failing so if the error WERE handled correctly, why would there be any work needed? https://reviews.llvm.org/D37651 ___ lldb-com

[Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

jingham accepted this revision. jingham added a comment. This looks okay for now. It will end up sending a Running & then a Stopped event. That's a little awkward, but that happens in the ordinary course of debugging anyway so it shouldn't freak anybody out. It would be better to find a place

[Lldb-commits] [PATCH] D33213: Use socketpair on all Unix platforms

jingham added a comment. Pavel & Eugene haven't marked it accepted yet, but from the comments is looks like they are both okay with the change. So it looks to me like everything is done but checking the code in. If you have checkin privileges, then just check it in and as Greg says keep an ey

[Lldb-commits] [PATCH] D37923: Implement interactive command interruption

jingham added a comment. In the original discussion of this feature (it was on the mailing list), it sounded like the specific cast that motivated Leonard in adding this feature was when there's a command that's in the process of generating tons of output, and you want to interrupt the tons of

[Lldb-commits] [PATCH] D37923: Implement interactive command interruption

jingham added a comment. And in general, interruption should be as responsive as you can make it. When I tell lldb to "Stop right now" it really should do as close as possible to "stop right now". Not, :-"excuse me I have 10 more pages of output to dump". It's not uncommon to dump some info,

[Lldb-commits] [PATCH] D37923: Implement interactive command interruption

jingham accepted this revision. jingham added a comment. I don't quite understand the comment about signals adding indeterminacy. No signal delivery is required to test this part. The lldb driver has a sigint handler that calls SBDebugger::DispatchInputInterrupt. But since you aren't testing

[Lldb-commits] [PATCH] D38394: Fix dumping of characters with non-standard sizes

jingham added a comment. This looks fine to me. I'd give Greg a little time to weigh in, this is much more his code than mine. But I don't see any problem with this, and thanks for the tests! Comment at: source/Core/DumpDataExtractor.cpp:275-281 + // Reject invalid ite

[Lldb-commits] [PATCH] D38394: Fix dumping of characters with non-standard sizes

jingham added a comment. See inlined comments. Comment at: source/Core/DumpDataExtractor.cpp:275-281 + // Reject invalid item_byte_size. + if (item_byte_size > 8) { +s->Printf("error: unsupported byte size (%" PRIu64 ") for char format", + (ui

[Lldb-commits] [PATCH] D37923: Implement interactive command interruption

jingham accepted this revision. jingham added a comment. This revision is now accepted and ready to land. That looks fine. https://reviews.llvm.org/D37923 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailma

[Lldb-commits] [PATCH] D36347: Add new script to launch lldb and set breakpoints for diagnostics all diagnostics seen.

jingham added a comment. I can't see anything wrong with the SB API use here. I don't feel qualified to comment on the most effective workflow for an analysis I've never had to do, however. https://reviews.llvm.org/D36347 ___ lldb-commits mailing

[Lldb-commits] [PATCH] D35556: Add data formatter for libc++'s forward_list

jingham added a comment. Will do. I was sick all last week so I have a bit of a backlog, I'll get to this as soon as I can. https://reviews.llvm.org/D35556 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mai

[Lldb-commits] [PATCH] D35556: Add data formatter for libc++'s forward_list

jingham requested changes to this revision. jingham added a comment. This revision now requires changes to proceed. It looks like the behavior of the two derived list classes here are only partially factored out. See the individual comments but it looks like there is a lot more that could be sh

[Lldb-commits] [PATCH] D36347: New lldb python module for adding diagnostic breakpoints

jingham requested changes to this revision. jingham added a comment. This revision now requires changes to proceed. Use the form of the command that gets an SBExecutionContext, then you can avoid having to cache the target at all. Comment at: utils/clangdiag.py:98-100 +def the

[Lldb-commits] [PATCH] D36347: New lldb python module for adding diagnostic breakpoints

jingham added a comment. Yes definitely use names for your breakpoints. It makes them easier to handle. Note starting a month or two ago you can set names to not get deleted except by an explicit gesture. That hasn't shown up in releases yet, but once you can rely on its being there, you can

<    1   2   3   4   5   6   7   8   9   10   >