This looks like a good opportunity to unleash pexpect.

In D63165#1591665, @amccarth wrote:

I'm trying to think through the implications of this always-use-an-lldb-server approach on cross-platform postmortem debugging. I'll have to do some studying, but I guess this patch doesn't change anything in that regard.

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

In D63713#1591371, @probinson wrote:

I have been reminded that there's also a desire to make DataExtractor work with 64-bit section sizes. Maybe Cursor should use a 64-bit offset (i.e., size_t not uint32_t), and then migrating from non-Cursor to Cursor APIs will also do the 64-bit transition? We need to bite that bullet at some point. (I kind of expect y'all to say, no way do that later, which is fine; mainly I wanted to refresh that in our collective minds.)

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...

lgtm

Add doxygen comments and fix typo.

In D64769#1587992, @kusmour wrote:

The test set the breakpoint to the return statement, even thought the compiler flag -O0 was set, the first hit to that bp showed that g_common_1 = 0 which actually should be 21. The test assume when we hit the return statement, the value should already be calculated, but that's not the case for NDK20 clang.

So we reproduce the test manually, turned out that the value was calculated later. lldb first hit the return statement then jump back to line 20. By looking at the assembly that clang provided, it indeed jump back and forth.

It's possible that the jumping behavior is caused by optimization, considering there're some other tests having similar problem, and the behavior changed a lot from NDK 19 to 20. I made this change because this is irrelevant to testing global variables.

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).

In D64782#1586815, @JDevlieghere wrote:

I've updated swig on GreenDragon, and the Windows bot is running 3.0. I'm not sure about the Debian bot, as it's incremental it doesn't print the version number. Regardless, I'll keep an eye on the bots of course.

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.

In D64647#1583178, @mgorny wrote:

In D64647#1583125, @labath wrote:

Shouldn't there be some tests that come along with this patch?

I was actually hoping that the test suite already covers what needs to be covered, and buildbot would tell me which tests were fixed ;-).

Shouldn't there be some tests that come along with this patch?

In D64599#1581598, @jingham wrote:

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 of the CPP language runtime, and then a plugin instance (in this case the ItaniumABILanguageRuntime) that contains all the bits that are specific to a particular implementation. Then putting the CPPLanguageRuntime.cpp in Target lets you know that this has to only contain the generic parts of the runtime behavior. That still seems to me a useful distinction.

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...

In D64546#1580618, @friss wrote:

This might mitigate the issue, but timeouts like this are bound to fail in some circumstances (machine load, ...). It's more work, but can we instead have the inferior produce an observable side effect (eg, print some output) and synchronize on this?

Looks mostly good. Just a couple of style comments.

I'm not very familiar with the details here, but this approach seems reasonable to me...

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).

Looks fine to me.

In D64118#1568656, @xiaobai wrote:

This patch looks fine to me overall. I'm unsure if LLDB has a guarantee about which NDK version its compatible with, but I'm alright with staying up to date with the latest release

Yeah, that been my general objective while I was still working on android...

Thank you.

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:

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).

In D63868#1563873, @jankratochvil wrote:

In D63868#1563802, @labath wrote:

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?

OK, I will try to reproduce it - you are right GDB documentation says for the m packet: "The reply may contain fewer addressable memory units than requested if the server was able to read only part of the region of memory. "

And LLDB is using ReadMemory in NativeProcessProtocol::ReadMemoryWithoutTrap from GDBRemoteCommunicationServerLLGS::Handle_memory_read.

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...

@jankratochvil wrote:

That is because it fixed failing ReadMemory (due to PAGE_SIZE size reading shared library name overlapping to a next page which is accidentally unmapped)

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...

BTW, the new version was now failing on linux for a change.

In D63713#1556126, @dblaikie wrote:

I'm kind of thinking a change to the cursor as Greg suggested might be better than wrapping the stream further - and probably using llvm::Error.

& yeah, I think using Error and saying "parsing failures are problems that must be handled" rather than allowing for accidentally ignoring failures,

Amounting to something like this:

Cursor C;
DataExtractor D = ...;
D.getU32(C);
D.getU64(C);
...
if (Error E = C.getError())
  ...;

That way early exits (if your record type has optional fields, etc) you can't accidentally forget your error checking (because if there's an Error inside the Cursor it'd have to be checked otherwise it'll assert that it's unchecked, etc)

Cursor implementation.

In D63540#1550858, @clayborg wrote:

So I am fine with symbols having zero size being in the symbol table. I would be fine not changing anything in the sorting and leaving some symbols with zero size, we just need to fix:

Symbol *Symtab::FindSymbolAtFileAddress(addr_t file_addr);

To ignore zero sized symbols when it finds them _if_ there is another symbol that has a size for that address. Wouldn't that fix the issue here?

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)...