- User Since
- Jun 4 2013, 6:02 AM (319 w, 6 d)
Fri, Jul 19
This looks like a good opportunity to unleash pexpect.
Thu, Jul 18
I am sorry, but the code still seems a lot more verbose to me than it should be needed to implement the given functionality. I'd like to understand why/if it's that necessary..
- address feedback from @dblaikie
- waiting to hear more opinions before updating the patch to use size_t
I haven't been keeping up with all the python cmake patches, but I'm not sure this will help the situation, as it will add another dimension to the configuration matrix. If we're still going to support cmake<3.12, then we're going to need to make the other branch of this work anyway... Maybe once cmake 3.12 gets a bit older (widely available), we could just switch to the other method and say that cmake>=3.12 is required if you're going to build lldb...
Wed, Jul 17
Add doxygen comments and fix typo.
Tue, Jul 16
I think this should be ready for a proper review now. To see the new API in action, please take a look at D64798.
- update the patch to reflect our offline conversation with @dblaikie. We decided to go for the API which makes most sense (i.e. skip all reads as soon as one of them returns an error), at least until there is evidence that this makes a difference in practice (one can always prove that there is a slowdown here with a suitable micro-benchmark).
- add tests for the new APIs
- move the refactoring of other parsing classes into a separate patch (keeping this patch solely about DataExtractor).
The change is fine, but for my own education, could you elaborate on what this "delayed calculation" is, and how does it make the test fail?
Whether a lot of these tests (e.g. all watchpoint related tests) pass or fail may depend on the exact android device you are running the tests against. However, you are the only ones running android tests ATM, so that does not matter much right now. If that ever changes, we'll have to do something smarter here...
Sorry for the delay. The patch looks fine to me. As far as testing goes, there's a "gdb-client" test suite in test/testcases/functionalities/gdb_remote_client/, which should allow you to mock gdb-server responses and verify that the right packets are being sent.
Sat, Jul 13
Fri, Jul 12
Shouldn't there be some tests that come along with this patch?
Thu, Jul 11
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 depend on the CPPLanguageRuntime plugin. There is no reverse dependency, so this may be fine, if that's how we intend things to be. However, it's not clear to me whether that's the right way to organize things. Intuitively, I'd expect the LanguageRuntime to depend on Language, and not the other way around...
Wed, Jul 10
Looks mostly good. Just a couple of style comments.
Tue, Jul 9
I'm not very familiar with the details here, but this approach seems reasonable to me...
Mon, Jul 8
I like this, though I am not really familiar with details of implementing a tablegen backend. This should make it easier to migrate to the llvmOption library, once we arrive to that point.
lldbassert(x) already expands to assert(x) in a debug (LLDB_CONFIGURATION_DEBUG) build. It sounds like you want this to assert in a non-debug build which has assertions enabled (Release+Assertions) ? If that's the case, then I think we should just change the #ifdef LLDB_CONFIGURATION_DEBUG in LLDBAssert.h into #ifdef _DEBUG..
Random idea: Should we force the user to pass something like -DLLDB_TEMPORARILY_ENABLE_LLDB_MI to cmake to make sure it gets noticed that it is going away (I don't know how many people read release notes).
Thu, Jul 4
Looks fine to me.
Yeah, that been my general objective while I was still working on android...
Wed, Jul 3
Tue, Jul 2
Ok, I got some numbers now. I went for a micro-benchmark-like setup to show some kind of a worst case scenario. The test setup is as follows:
Mon, Jul 1
I don't have time to do a full review of this today, but this seems pretty good at a first glance...
I think it will be pretty hard for a single person to gather all of these stats for all targets. So perhaps the best solution is to have the setting for the g packet, defaulting to true. After a while, if nobody notices a regression, we can just remove it...
I've also reverted the preceeding patch in this series as reverting this one has caused one of the other tests to start failing (I've tried to make a more surgical fix first, but @jankratochvil pointed out that this basically reintroduced the problem that we were trying to solve by reverting this).
LGTM. I don't know what Kamil is up to these days. It might be better to have him review patches like this, but if he's unable to do that, I'm happy to stamp them too...
Sorry about the delay, I was OOO last week. BTW, the usual rate of pinging on a patch is one week https://llvm.org/docs/DeveloperPolicy.html#code-reviews.
I don't know whether the code is correct for NetBSD, but it looks ok from a general readability perspective.
Looks good to me, but I'd suggest waiting a while to give @clayborg a chance to respond...
Does this mean that there is a bug in lldb-server, where some memory read requests fail if they span an unreadable page. Can this also be triggered by a $m packet? Would you be interested in distilling that into a test?
Besides picking at code style details, I don't believe I have anything useful to add here. :)
looks good to me too...
Tue, Jun 25
BTW, the new version was now failing on linux for a change.
Mon, Jun 24
This code should go to tools/debugserver/source/CMakeLists.txt so that it is next to the code which performs the actual code signing. Doing that will make it easier to keep it in sync with changes to code signing, as well as make it obvious that it is not in sync with them right now (there's a pretty complex interaction of various cmake options (LLDB_CODESIGN_IDENTITY, LLVM_CODESIGNING_IDENTITY, LLDB_USE_SYSTEM_DEBUGSERVER, etc.) which affects code signing, and this code is ignoring all of those)...