Thanks. We can always re-add the check if things take longer than expected.

It looks like diff now contains the changes from D61833 as well. It would be better to upload just the changes relative to the previous patch in the series, otherwise things get confusing about what is being changed where. It's kind of obvious here, but you can use the "parent revision" field in phabricator to indicate that one diff depends on another.

BTW, there's no fundamental reason why a 64-bit lldb-server has to use instruction emulation for debugging arm32 binaries. Since all arm64 chips support hardware single stepping, we could just teach the arm32 native register context to issue PTRACE_SINGLESTEP commands when build in 64-bit mode, and increase debugging reliability while decreasing lldb-server size. (The same is probably also true for a 32-bit lldb-server running on a 64-bit kernel, but that's something that we would have to detect at runtime.)

GetRemoteConnectionURI sounds fine.

BTW, given that we recommend using ninja pretty much everywhere, you might want to include set(CMAKE_GENERATOR Ninja)

Replying here as this is the relevant patch.

• rebase on top of the DWARFUnitHeader patch

Rebase so that this does not require any debug_types support.

In D62073#1507001, @aprantl wrote:

Nice! FYI, we have a bot that runs the LLDB dotests against DWARF5 (the last stage http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake-matrix/) to track the progress.

The patch looks fine, modulo some inline nits. I am somewhat doubtful about the name of the new function though. When I first saw GetConnectURI, i thought this would be the URI used to connect to this socket, but in reality it is the URI of the other end (right?). In that case, I think GetRemoteURI would be better. Or maybe, borrowing from the BSD socket vocabulary, GetPeerURI ? WDYT?

Ok, let's give this a shot.

I don't see a problem with changing lldb's behavior to add special treatment for address zero. It's not the most ideal solution (that would be to avoid generating the debug info in the first place), but given that we have to accept inputs from other linkers too (which also implement this behavior), it's probably the best thing we can do now.

The TypeUnit and CompileUnit classes are pretty much identical now, but that'll change once we start parsing type signatures and cross-referencing them.

Yeah, neither of the options look really appealing... Still, I can't escape the feeling that we are doing something wrong, as the installation process should not be that complicated. Or is the framework support in cmake just not good enough?

I think this looks better than the original. Given that the only callers of SourceInitFile pass a constant value for the InitFileLocation argument, what I'd consider also doing is getting rid of the function and the enum, and making SourceInitFileHome/SourceInitFileCwd the main entry points.

In D61952#1504942, @sgraenitz wrote:

Actually, why not make the copy operations PRE_BUILD actions of the test suite instead of POST_BUILD actions of their executables?

Obsoleted by D61908.

Adrian is on vacation now, but given that he was just waiting until you resolve my comments (which you have), I think we don't have to wait for him.

Thanks. This is very straight-forward, so I don't think another set of eyes is needed (though I expect Greg is keeping at least one eye on this).

Seems like a nice cleanup. Given that code code already assumed that the DWARFUnit can't be null, I'd just delete the null checks..

• don't bail out if you see the debug_types section
• rename a variable in the test to work around the old "Multiple internal symbols found for 'a'" problem

In D62008#1504845, @jankratochvil wrote:

On Fedora 29 x86_64 I get:

...

In D61956#1504761, @sgraenitz wrote:

Instead lldb could have something like GreenDragon-lldb-base.cmake that provides and explains reasonable default settings for a range of bots/environments. Actual build bot configs in zorg could then be handled the same way as downstream repos.

In D53753#1504645, @aleksandr.urakov wrote:

In D53753#1504589, @labath wrote:

When we evaluate an expression, we jit a bunch of opcodes into the inferior memory and then have it execute them. For that to work, we need to allocate some memory in order to store the opcodes. We cannot use the general expression engine to jit that expression, as we would be back to square one, so we manually set the PC to the entry point of the mmap function, and set the argument values as if it was being called. Then we just let the inferior loose and have it allocate the memory for us and return it. For this to work, we need abi knowledge both to correctly set the arguments of mmap, and to retrieve its result.

Got it, thanks! There's a different approach on Windows: for now we just call VirtualAllocEx, which can allocate memory in another process. But it will not work for the remote debugging case.

In D53753#1504539, @aleksandr.urakov wrote:

In D53753#1504361, @labath wrote:

I'm not sure what exactly are the consequences of not using the correct ABI definition here. I think the case where the difference may start to become obvious is if you try to get argument values of a function for which you don't have debug info for.

It sounds strange to me... If we don't have symbols for a function, then we can't even know amount of its arguments, so how can we retrieve them? Also e.g. on Windows x86 both stdcall, ccall, thiscall and fastcall are commonly used, so it would be strange to use some "default" ABI...

BTW, it looks like the attach test is still flaky http://lab.llvm.org:8014/builders/lldb-x86_64-debian/builds/1541/steps/test/logs/stdio, though it looks like the failures are super-rare (this is the first time I've seen it do that). Judging by the error message alone, it's not clear to me whether this is due to the sleep being too small, or if there's something else going on.

Thanks for explaining. I think this is good to go, sans the merging of the two tests I requested, and the comment fix that @clayborg noticed.

In D53753#1504320, @aleksandr.urakov wrote:

@lanza Hello! AFAIU, these values have nothing to do with the Microsoft x64 calling convention. They are used by ABISysV_x86_64, which specifies ABI for working with code injectable into a process being debugged. It requires six arguments to be passed through registers (see GetArgumentValues).

If I understand right, SymbolFile retrieves the information about arguments location on the stack, so there's no need to support different calling conventions in LLDB for this purpose. Please, correct me if I'm wrong.

I'm not very familiar with frameworks and complexities involved in creating them, but the fact that we need to delete stuff from the build tree in order to install properly makes me think that there is something fishy going on. Is there no way to arrange things so that this can be avoided? For instance, what if we set the build output paths to be separate and disjoint locations for each target. Then use a separate target, or some POST_BUILD commands to copy/symlink the files to construct an build-tree framework, and have the install targets create the install-tree framework from the original build output paths?

I think this is a bit more readable. I've included some suggestions which I think could make it even better.

I wouldn't want to over-proliferate these, but in general I think this is a good idea. What I would find particularly useful is if we checked in the configurations used by all the bots in here, as that would make it easier to figure out what's going on if one of them breaks and the reason is not obvious...

Address review comments.

In D61713#1502617, @mgorny wrote:

In D61713#1502606, @labath wrote:

In D61713#1502580, @mgorny wrote:

Is this test supposed to work portably? It fails on NetBSD buildbot: http://lab.llvm.org:8011/builders/netbsd-amd64/builds/20101/steps/run%20unit%20tests/logs/FAIL%3A%20LLDB%3A%3Acompiler-full-path.test

I think we can just add --mode=link to the test to avoid it trying to mess with the linker (since that is not important here anyway).

Did you mean --mode=compile? I can confirm it fixes the test. Should I commit it?

In D61713#1502580, @mgorny wrote:

Is this test supposed to work portably? It fails on NetBSD buildbot: http://lab.llvm.org:8011/builders/netbsd-amd64/builds/20101/steps/run%20unit%20tests/logs/FAIL%3A%20LLDB%3A%3Acompiler-full-path.test

I like this patch in a lot of ways, because it makes the code cleaner, and prepares us for DWARF5, which can reference entries in other files in a lot of interesting ways.

• use enum class for the section type
• put DIERef as a member of HashedNameToDie::DIEInfo

If this iteration is going to be used a lot, I'd recommend taking a bit of time to implement an iterator abstraction over the language runtimes. It takes a bit longer to set up, but I hope we can all agree that for (runtime: process->GetLanguageRuntimes()) runtime->foo(); is more readable than process->ForEachLanguageRuntime([] (runtime) { runtime->foo(); }). This is particularly true if you need some sort of a control flow construct (continue, break, return) in the loop "body".