[Lldb-commits] [PATCH] D43837: Move option parsing out of the Args class

jingham added a comment. I don't have any objections to the contents of this patch per se. But I wonder if having to do all this work to separate the uses of Args from the options parser so we don't drag it in in some low level uses doesn't rather mean the Args class was not appropriate for us

[Lldb-commits] [PATCH] D43857: Speed up TestWatchpointMultipleThreads

jingham added a comment. Getting watchpoints to propagate to threads that are created after the watchpoint has been set is one of the trickier parts of watchpoint propagation. On macOS, (where we don't get new thread creation notification) we rely on the kernel to propagate the control regist

[Lldb-commits] [PATCH] D43884: [lldb] Extract more fields from NSException values

kubamracek created this revision. kubamracek added reviewers: jingham, jasonmolenda. This patch teaches LLDB about more fields on NSException Obj-C objects, specifically we can now retrieve the "name" and "reason" of an NSException. The goal is to eventually be able to have SB API that can provi

[Lldb-commits] [PATCH] D43837: Move option parsing out of the Args class

labath added a comment. In https://reviews.llvm.org/D43837#1022303, @jingham wrote: > I don't have any objections to the contents of this patch per se. But I > wonder if having to do all this work to separate the uses of Args from the > options parser so we don't drag it in in some low level u

[Lldb-commits] [PATCH] D43886: [lldb] Add GetCurrentException and GetCurrentExceptionBacktrace APIs to SBThread

kubamracek created this revision. kubamracek added reviewers: jasonmolenda, jingham. Herald added a subscriber: mgorny. This adds new APIs and commands to deal with exceptions (mostly Obj-C exceptions): - SBThread and Thread get GetCurrentException API, which returns an SBValue/ValueObjectSP wi

[Lldb-commits] [PATCH] D43857: Speed up TestWatchpointMultipleThreads

labath added a comment. That's a good point. I think that at least TestConcurrentEvents do it that way, but they are testing a different thing. I think it makes sense to test both things here actually. I'll create one test which sets the watchpoint before thread creation and one after. BTW, do

[Lldb-commits] [PATCH] D43837: Move option parsing out of the Args class

jingham added a comment. I still worry a bit because there's another unstated responsibility for Args which is that even though it is going to get used at a very high level in lldb it has to NOT depend on anything you don't want lldb-server to depend on. That seems like a more slippery respons

[Lldb-commits] [PATCH] D43837: Move option parsing out of the Args class

zturner added inline comments. Comment at: include/lldb/Interpreter/Options.h:123-126 + llvm::Expected Parse(const Args &args, + ExecutionContext *execution_context, + lldb::PlatformSP platform_sp, +

[Lldb-commits] [PATCH] D43857: Speed up TestWatchpointMultipleThreads

jingham added a comment. Thanks! While it seems possible somebody could write code that only sets the control registers on one thread and then gets tired and goes out for a beer, that seems like a really unlikely error. I don't think it's worth complicating the test to catch that possibility.

[Lldb-commits] [PATCH] D43837: Move option parsing out of the Args class

labath added a comment. In https://reviews.llvm.org/D43837#1022430, @jingham wrote: > I still worry a bit because there's another unstated responsibility for Args > which is that even though it is going to get used at a very high level in > lldb it has to NOT depend on anything you don't want l

[Lldb-commits] [PATCH] D43837: Move option parsing out of the Args class

zturner added a comment. It would be nice if we could eventually get rid of the need to pass in a platform and execution context here, but that's work for another day. Comment at: include/lldb/Interpreter/Options.h:123-126 + llvm::Expected Parse(const Args &args, +

[Lldb-commits] [PATCH] D43837: Move option parsing out of the Args class

labath added inline comments. Comment at: include/lldb/Interpreter/Options.h:123-126 + llvm::Expected Parse(const Args &args, + ExecutionContext *execution_context, + lldb::PlatformSP platform_sp, +

[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions

labath added inline comments. Comment at: lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:814 value = value - header->p_vaddr; found_offset = true; owenpshaw wrote: > labath wrote: > > Ok so the issue is that here we use the vir

[Lldb-commits] [PATCH] D43837: Move option parsing out of the Args class

jingham added a comment. Okay, that sounds good then. Will you enforce the rule about the Utilities directory socially or by some mechanism? https://reviews.llvm.org/D43837 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llv

[Lldb-commits] [PATCH] D43837: Move option parsing out of the Args class

zturner added a comment. In https://reviews.llvm.org/D43837#1022554, @jingham wrote: > Okay, that sounds good then. Will you enforce the rule about the Utilities > directory socially or by some mechanism? If you mean the rule that Utility can't depend on anything else, I think it's enforced

[Lldb-commits] [PATCH] D43837: Move option parsing out of the Args class

labath added a comment. In https://reviews.llvm.org/D43837#1022554, @jingham wrote: > Okay, that sounds good then. Will you enforce the rule about the Utilities > directory socially or by some mechanism? You will get a link error because the Utility unittest executable will have missing symb

[Lldb-commits] [PATCH] D43837: Move option parsing out of the Args class

labath updated this revision to Diff 136355. labath added a comment. Add a slightly longer comment to the Parse function operation. https://reviews.llvm.org/D43837 Files: include/lldb/Interpreter/Args.h include/lldb/Interpreter/Options.h packages/Python/lldbsuite/test/functionalities/comp

[Lldb-commits] [PATCH] D43837: Move option parsing out of the Args class

jingham added a comment. The separate gtest directories don't get build separately the way Todd set the Xcode project up. The tests get built into one combo .a file that links to liblldbcore.a, and then into the test binary. So the Xcode build wouldn't see that failure. Since you are buildin

[Lldb-commits] [PATCH] D43857: Speed up TestWatchpointMultipleThreads

labath updated this revision to Diff 136361. labath added a comment. This makes the inferior more simple and deterministic by using a single thread. Now we have one test which sets the watchpoint before the thread is created and one that sets it after. I've also simplified the test a bit with new

[Lldb-commits] [lldb] r326367 - Revert "[lldb] Use vFlash commands when writing to target's flash memory regions"

Author: labath Date: Wed Feb 28 12:42:29 2018 New Revision: 326367 URL: http://llvm.org/viewvc/llvm-project?rev=326367&view=rev Log: Revert "[lldb] Use vFlash commands when writing to target's flash memory regions" This reverts commit r326261 as it introduces inconsistencies in the handling of l

[Lldb-commits] [PATCH] D43857: Speed up TestWatchpointMultipleThreads

xiaobai added a comment. Thanks for taking the time to do this! Can you please double check some of the formatting in the python code? It looks like it's not indented properly. https://reviews.llvm.org/D43857 ___ lldb-commits mailing list lldb-com

[Lldb-commits] [PATCH] D43857: Speed up TestWatchpointMultipleThreads

labath updated this revision to Diff 136368. labath added a comment. Replace all tabs in the python file and clang-format the cpp file. https://reviews.llvm.org/D43857 Files: packages/Python/lldbsuite/test/functionalities/watchpoint/multiple_threads/TestWatchpointMultipleThreads.py packag

[Lldb-commits] [lldb] r326369 - Adapt some tests to work with PPC64le architecture

Author: labath Date: Wed Feb 28 12:57:26 2018 New Revision: 326369 URL: http://llvm.org/viewvc/llvm-project?rev=326369&view=rev Log: Adapt some tests to work with PPC64le architecture Summary: Merge branch 'master' into adaptPPC64tests Reviewers: clayborg, alexandreyy, labath Reviewed By: clayb

[Lldb-commits] [PATCH] D42917: Adapt some tests to work with PPC64le architecture

This revision was automatically updated to reflect the committed changes. Closed by commit rL326369: Adapt some tests to work with PPC64le architecture (authored by labath, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D42917?vs=13285

[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions

owenpshaw added a comment. > And I'm not even completely clear about your case. I understand you write the > module to the physical address, but what happens when you actually go around > to debugging it? Is it still there or does it get copied/mapped/whatever to > the virtual address? In my

[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions

labath added a comment. In https://reviews.llvm.org/D42145#1022717, @owenpshaw wrote: > > And I'm not even completely clear about your case. I understand you write > > the module to the physical address, but what happens when you actually go > > around to debugging it? Is it still there or does

[Lldb-commits] [lldb] r326374 - Add ability to collect memory limit.

Author: hanming Date: Wed Feb 28 14:18:45 2018 New Revision: 326374 URL: http://llvm.org/viewvc/llvm-project?rev=326374&view=rev Log: Add ability to collect memory limit. Reviewer: Jason Molenda Modified: lldb/trunk/tools/debugserver/debugserver.xcodeproj/project.pbxproj lldb/trunk/too

[Lldb-commits] [PATCH] D43912: [Symbol] Add InvalidType, a force-checked recoverable error

vsk created this revision. vsk added reviewers: labath, zturner, davide, aprantl, lhames. LLDB CompilerTypes may be invalid, i.e they should be checked for validity before use. Compared to a more typical model in which only valid types exist [1], this has a few disadvantages: - The debugger is g

[Lldb-commits] [PATCH] D43912: [Symbol] Add InvalidType, a force-checked recoverable error

aprantl added a comment. > Going forward, we should transition to a model in which CompilerTypes are > either valid or do not exist. I don't understand very well how the LLDB type system works so excuse my naive questions: Does this account for lazyness? I.e., could there be value in having an

[Lldb-commits] [PATCH] D43912: [Symbol] Add InvalidType, a force-checked recoverable error

JDevlieghere added inline comments. Comment at: include/lldb/Symbol/CompilerType.h:446 +/// A force-checked error used to describe type construction failures. +class InvalidType : public llvm::ErrorInfo { +public: I think this should be called `InvalidTypeError`?

[Lldb-commits] [PATCH] D43857: Speed up TestWatchpointMultipleThreads

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

[Lldb-commits] [lldb] r326378 - Fix up the gtest targets for changes in the UnwindAssembly tests.

Author: jingham Date: Wed Feb 28 14:41:11 2018 New Revision: 326378 URL: http://llvm.org/viewvc/llvm-project?rev=326378&view=rev Log: Fix up the gtest targets for changes in the UnwindAssembly tests. Modified: lldb/trunk/lldb.xcodeproj/project.pbxproj Modified: lldb/trunk/lldb.xcodeproj/proj

[Lldb-commits] [PATCH] D43912: [Symbol] Add InvalidType, a force-checked recoverable error

labath added a comment. I agree that we should start using this in a lot more places. I think you should be able to shorten the logging code a bit by using the LLDB_LOG_ERROR macro, which I've created for this purpose. Comment at: source/Plugins/ExpressionParser/Clang/ClangE

[Lldb-commits] [PATCH] D43912: [Symbol] Add InvalidType, a force-checked recoverable error

zturner added inline comments. Comment at: source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp:382-387 + if (llvm::Error E = user_type_or_err.takeError()) { +std::string Reason = llvm::toString(std::move(E)); if (log) - log->Printf("Persistent variabl

[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions

owenpshaw added a comment. > Since there is just one caller of this function maybe we don't even need to > that fancy. Could the LoadInMemory function do the shifting itself? > I'm thinking of something like where it takes the load bias as the argument > and then adds that to the physical addre

[Lldb-commits] [PATCH] D43912: [Symbol] Add InvalidType, a force-checked recoverable error

jingham added a comment. In https://reviews.llvm.org/D43912#1022916, @aprantl wrote: > > Going forward, we should transition to a model in which CompilerTypes are > > either valid or do not exist. > > I don't understand very well how the LLDB type system works so excuse my > naive questions: Do

[Lldb-commits] [PATCH] D43912: [Symbol] Add InvalidType, a force-checked recoverable error

vsk added inline comments. Comment at: source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp:335-340 +if (llvm::Error E = user_type_or_err.takeError()) { + std::string Reason = llvm::toString(std::move(E)); + if (log) +log->Printf("%s", Reason.c_

[Lldb-commits] [PATCH] D43912: [Symbol] Add InvalidType, a force-checked recoverable error

vsk updated this revision to Diff 136433. vsk marked an inline comment as done. vsk edited the summary of this revision. vsk added a comment. - Address some review feedback. https://reviews.llvm.org/D43912 Files: include/lldb/Symbol/CompilerType.h include/lldb/Utility/Log.h source/Plugins

[Lldb-commits] [PATCH] D43912: [Symbol] Add InvalidType, a force-checked recoverable error

vsk added a comment. In https://reviews.llvm.org/D43912#1023078, @jingham wrote: > In https://reviews.llvm.org/D43912#1022916, @aprantl wrote: > > > > Going forward, we should transition to a model in which CompilerTypes are > > > either valid or do not exist. > > > > I don't understand very wel

[Lldb-commits] [PATCH] D43912: [Symbol] Add InvalidType, a force-checked recoverable error

jingham added a comment. The problem with your RETURN_IF_UNEXPECTED macro is that it make it impossible to put a breakpoint only on the "type was invalid case." To catch that happening while debugging you'd have to write a conditional breakpoint that also checks the error. That seems like som

[Lldb-commits] [lldb] r326399 - Add another entitlement that we need for debugserver.

Author: jmolenda Date: Wed Feb 28 17:04:07 2018 New Revision: 326399 URL: http://llvm.org/viewvc/llvm-project?rev=326399&view=rev Log: Add another entitlement that we need for debugserver. Modified: lldb/trunk/tools/debugserver/source/debugserver-entitlements.plist Modified: lldb/trunk/too

[Lldb-commits] [PATCH] D43912: [Symbol] Add InvalidType, a force-checked recoverable error

labath added inline comments. Comment at: include/lldb/Utility/Log.h:249-254 + ::lldb_private::Log *log_private = (log); \ + if (log_private) \ +log_private->FormatError(::s

[Lldb-commits] [PATCH] D43912: [Symbol] Add InvalidType, a force-checked recoverable error

jingham added a comment. The phase where we are converting everything over seems exactly when we want to be able to easily stop on the error cases when debugging, which using a macro that contains the return makes impossible. https://reviews.llvm.org/D43912 _

[Lldb-commits] [PATCH] D43912: [Symbol] Add InvalidType, a force-checked recoverable error

vsk updated this revision to Diff 136450. vsk added a comment. > The problem with your RETURN_IF_UNEXPECTED macro is that it make it > impossible to put a breakpoint only on the "type was invalid case." To catch > that happening while debugging you'd have to write a conditional breakpoint > tha

[Lldb-commits] [PATCH] D43912: [Symbol] Add InvalidType, a force-checked recoverable error

vsk added inline comments. Comment at: include/lldb/Utility/Log.h:249-254 + ::lldb_private::Log *log_private = (log); \ + if (log_private) \ +log_private->FormatError(::std:

Re: [Lldb-commits] [lldb] r323803 - Compile the LLDB tests out-of-tree.

Hey Adrian, Did you mean to write cls.getBuildArtifact() here? I'm seeing a Python exception locally when I run check-lldb-single: Traceback (most recent call last): File "/Users/vsk/src/llvm.org-lldbsan/llvm/tools/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 584, in tearDownClass

Re: [Lldb-commits] [lldb] r323803 - Compile the LLDB tests out-of-tree.

That looks reasonable, since this is a class method, yes. Thanks for spotting it! Would you mind committing the change? -- adrian > On Feb 28, 2018, at 6:46 PM, Vedant Kumar wrote: > > Hey Adrian, > > Did you mean to write cls.getBuildArtifact() here? > > I'm seeing a Python exception locall

[Lldb-commits] [lldb] r326412 - We were getting the wrong dynamic type if there were two classes with the same basename.

Author: jingham Date: Wed Feb 28 18:44:34 2018 New Revision: 326412 URL: http://llvm.org/viewvc/llvm-project?rev=326412&view=rev Log: We were getting the wrong dynamic type if there were two classes with the same basename. There's a bug in FindTypes, it ignores the exact flag if you pass a name

Re: [Lldb-commits] [lldb] r323803 - Compile the LLDB tests out-of-tree.

Will test and commit momentarily, thanks! vedant > On Feb 28, 2018, at 6:47 PM, Adrian Prantl wrote: > > That looks reasonable, since this is a class method, yes. Thanks for spotting > it! Would you mind committing the change? > > -- adrian > >> On Feb 28, 2018, at 6:46 PM, Vedant Kumar >

[Lldb-commits] [lldb] r326414 - [test] Restore cleanup behavior in TestQuoting.py

Author: vedantk Date: Wed Feb 28 19:03:38 2018 New Revision: 326414 URL: http://llvm.org/viewvc/llvm-project?rev=326414&view=rev Log: [test] Restore cleanup behavior in TestQuoting.py Before the change to compile tests out-of-tree, the cleanup classmethod in TestQuoting.py would remove a temp fil

Re: [Lldb-commits] [lldb] r323803 - Compile the LLDB tests out-of-tree.

Done, r326414 > On Feb 28, 2018, at 6:48 PM, Vedant Kumar wrote: > > Will test and commit momentarily, thanks! > > vedant > >> On Feb 28, 2018, at 6:47 PM, Adrian Prantl > > wrote: >> >> That looks reasonable, since this is a class method, yes. Thanks for >> spottin

Re: [Lldb-commits] [lldb] r323803 - Compile the LLDB tests out-of-tree.

Thanks! > On Feb 28, 2018, at 7:06 PM, Vedant Kumar wrote: > > Done, r326414 > >> On Feb 28, 2018, at 6:48 PM, Vedant Kumar > > wrote: >> >> Will test and commit momentarily, thanks! >> >> vedant >> >>> On Feb 28, 2018, at 6:47 PM, Adrian Prantl >>

Re: [Lldb-commits] [PATCH] D43912: [Symbol] Add InvalidType, a force-checked recoverable error

What about having the macro do the log printing in a function so you can put the breakpoint in the function? On Wed, Feb 28, 2018 at 5:56 PM Vedant Kumar via Phabricator < revi...@reviews.llvm.org> wrote: > vsk added inline comments. > > > > Comment at: include/lldb/Utility/Log.h:

[Lldb-commits] [PATCH] D43884: [lldb] Extract more fields from NSException values

davide added a comment. I like the way this patch is structured, some comments inline. Comment at: packages/Python/lldbsuite/test/lang/objc/exceptions/TestObjCExceptions.py:1-4 +# encoding: utf-8 +""" +Test lldb Obj-C exception support. +""" This looks like it

[Lldb-commits] [PATCH] D43884: [lldb] Extract more fields from NSException values

kubamracek added inline comments. Comment at: packages/Python/lldbsuite/test/lang/objc/exceptions/TestObjCExceptions.py:1-4 +# encoding: utf-8 +""" +Test lldb Obj-C exception support. +""" davide wrote: > This looks like it doesn't need any interactivity whatsoe

[Lldb-commits] [PATCH] D43884: [lldb] Extract more fields from NSException values

davide added a comment. LG modulo the test. Update that, I'll take another look and approve it. Thanks for your contribution! Comment at: packages/Python/lldbsuite/test/lang/objc/exceptions/TestObjCExceptions.py:1-4 +# encoding: utf-8 +""" +Test lldb Obj-C exception support.