Fix comment about "a" registers

Change "word_size = m_is_rv64 ? 8 : 4" to "word_size = reg_size"

Remove disabling JIT and enabling IR Interpreter function calls, as requested
by Jason Molenda.

Fix crash when there was no process (found by API tests)
Remove bugs in function call setup when using IR interpreter (found by API tests)
Change flag tests to use lldb_private::Flags (requested by @jasonmolenda )

@DavidSpickett update on testing: I'm running tests. I found an issue in the IR Interpreter when calling class methods. Somehow a 32 bit pointer gets the upper 32 bits in the 64 bit uint it's stored in set to 0xffffffff, which causes an assert. Downstream we've changed the assert should probably to an error so it doesn't take down the debugger. I can make this change here, but I wonder if it should be a separate change. For now I'm skipping the tests that cause this. Progress is slow, but steady.

Implement Jason Molenda's request to use an ivar instead of static variables.

In D159101#4627705, @DavidSpickett wrote:

Great. Could you include that in the commit message? Seems like we get a lot of questions about how mature risc-v support is, so we can point to this as a milestone for that.

In D159101#4627705, @DavidSpickett wrote:

I have some vague idea that maybe we could put a hack in the test suite to use the qemu-user platform instead of lldb-server. To at least give it a go, but I haven't tried it myself yet.

Updated with David Spickett's comments

In D159101#4625539, @DavidSpickett wrote:

Also could you provide a list of things you have tested with qemu. So I can get an idea of how sure we can be any of this works / be a test plan, albeit manual for anyone updating this code in future.

Adding commenters from https://reviews.llvm.org/D62732 as reviewers.

With this change, I'm able to debug RISC-V 32 and 64 bit applications using Linux user space QEMU, using @labath 's QemuUser platform. Simple expression parsing with the IR Interpreter is working. Complex expression parsing with JIT is not tested.

In D155117#4521393, @labath wrote:

In D155117#4510512, @ted wrote:

In D155117#4505538, @labath wrote:

I am wondering if we actually need the second step (the architecture setting) here. The main reason it exists is the usage in GetSupportedArchitectures (which is called before a target is created) it seems like the value derived from the target should always be more correct. WDYT? What are the values you get in steps 2 and 3 for your use case?

I don't think we need to specify the architecture, because I think we can always get it from the triple. There might be a case where someone is using a qemu that isn't named the same as the Triple ArchName, but that case could be covered by emulator-path.

Yeah, that's my thinking as well. Given this, I think we should drop the setting part and just go with the target value directly.

In D155107#4534020, @clayborg wrote:

So Ted, can you verify that nothing is missing in RISCV since this is already the way it works for x86/x86_64 and arm32/arm64? I am thinking like Jason is where I wonder if something isn't just hooked up right for RISCV somehow. Or if you can verify that nothing changes for x86/x86_64 or arm32/arm64 if this setting is enabled and there are no symbolication regressions in the comments if this does get enabled for the architectures where it is working as expected...

In D155107#4511967, @clayborg wrote:

Looks like other disassemblers already show full addresses for the branches and calls (at least arm64 does from my output above), so not sure why RISCV would require this setting, but x86_64 and arm64 wouldn't because it is already working that way??

As for the GetSupportedArchitectures case, downstream I left in the code that checks the property, but changed the return {}; to

return {ArchSpec(llvm::Triple("riscv32-unknown-linux")),
        ArchSpec(llvm::Triple("riscv64-unknown-linux"))};

In D155107#4504667, @jasonmolenda wrote:

Isn't it better to print branches within a function as an offset, given that our disassembly format by default lists the offset of each instruction. So instead of looking for a 6-digit long hex address, you're looking for a decimal offset in the output? I'm not sure if you disagree with my opinion, or if you have an example where it is clearer to have an address that I'm not seeing.

In D155117#4505538, @labath wrote:

I am wondering if we actually need the second step (the architecture setting) here. The main reason it exists is the usage in GetSupportedArchitectures (which is called before a target is created) it seems like the value derived from the target should always be more correct. WDYT? What are the values you get in steps 2 and 3 for your use case?

@jasonmolenda friendly ping

@jasonmolenda I discovered the same issue in another way - create a python command and load it with "command script import". Here is test.py:

Fix test to exit lldb

In D109738#3052624, @jingham wrote:

That is clearly wrong: an address is never enough to restore a breakpoint from one session to the next even if you can turn ASLR off. After all, the breakpoint could be in a dlopened library.

Prints were there for my debugging. They don't serve a purpose, so I've removed them.

Remove unneeded prints from test

[lldb] Handle malformed qfThreadInfo reply

I've got a test written. It doesn't crash like the debugger in the wild does, but it does give a tid of 0 for each thread I ask about. So I can assert if the threads don't have the correct tid.

I agree about the test. I'll work on one.

In D109738#3000608, @jingham wrote:

BTW, do you know what change made this stop working?

I created the original bug because a change made LLDB stop working on address breakpoints if they were created before the program is run. Setting a breakpoint on an address is a typical case with embedded applications, especially if you want to debug startup code.

LGTM. Thanks Michał!

Why would we not want to support this on other targets, like Arm? Fixes for AIX shouldn't impact other things.

Fixed formatting flagged by Lint: Pre-merge checks

Hi Paulo,
@clayborg asked me to look at this, because I've worked with systems that have multiple address spaces. I was thinking, instead of a WebAssembly specific memory read, we should implement an optional generic memory read and write with memory space support.

File llvm/test/DebugInfo/X86/vla.ll was modified so the diff did not apply anymore. object-emission was removed from it, so I'm removing it from this diff.

@jingham friendly ping

Friendly ping

Removed the code that sets m_constructor_errors when GetLoadAddressPermissions returns False, as requested by @jingham .

Wow @jingham you posted your comment and I landed the patch at the same time!

In D71372#1813703, @labath wrote:

In D71372#1813142, @ted wrote:

In D71372#1811594, @labath wrote:

In D71372#1810687, @ted wrote:

I've got another failure case for this. If the remote gdbserver doesn't implement qMemoryRegionInfo or qXfer:memory-map:read, thread step-out will fail.
....

That's a good point Ted. I think we should give targets which don't support fetching permissions the benefit of the doubt, and treat all memory as potentially executable. Would removing the return statement from the if(!GetLoadAddressPermissions) branch solve your problem? If so, can you whip up a patch for that?

Removing the return statement fixes the issue. I'll put up a patch. Keeping the m_constructor_errors.Printf line doesn't cause a failure; it might be useful to keep that in case the breakpoint can't be created for other reasons. What do you think?

I don't care much either way.. Since you have this kind of a target around, you can judge whether printing this error/warning after each "finish" would be useful or just annoying. Another possibility would be to don't print the error to the command output, but still emit something into the log...

In D71372#1811594, @labath wrote:

In D71372#1810687, @ted wrote:

I've got another failure case for this. If the remote gdbserver doesn't implement qMemoryRegionInfo or qXfer:memory-map:read, thread step-out will fail.
....

That's a good point Ted. I think we should give targets which don't support fetching permissions the benefit of the doubt, and treat all memory as potentially executable. Would removing the return statement from the if(!GetLoadAddressPermissions) branch solve your problem? If so, can you whip up a patch for that?

I've got another failure case for this. If the remote gdbserver doesn't implement qMemoryRegionInfo or qXfer:memory-map:read, thread step-out will fail.

llvm/tools/lldb/tools/driver/Driver.cpp:495:12: error: comparison of integers of different signs: 'ssize_t' (aka 'long') and 'size_t' (aka 'unsigned long') [-Werror,-Wsign-compare]

if (nrwr != commands_size) {
    ~~~~ ^  ~~~~~~~~~~~~~

1 error generated.

This doesn't look correct to me - it looks like there are 1 too many #endifs. I think the one at line 179 should be removed - it should have been replaced by the #else that is at line 180.

In D57552#1381126, @jingham wrote:

That sounds like it will have to be fixed somewhere else, and not part of this patch then. Can you file a bug to get that fixed?

In D57552#1381046, @jingham wrote:

In D57552#1380782, @ted wrote:

FYI, @jingham , this is also an issue with DYLD plugins, if the file in the link map starts with ".". "image search-paths add . /path/to/my/libraries" fails in this case, because "./library.so" gets changed to "library.so".

This should fix that issue as well, since image search-paths are also PathMappingLists. I don't see any tests for this, however. Do you have something handy you could turn into a test?

@sgraenitz I've found an issue with this patch, using the Visual Studio 2015 generator.

FYI, @jingham , this is also an issue with DYLD plugins, if the file in the link map starts with ".". "image search-paths add . /path/to/my/libraries" fails in this case, because "./library.so" gets changed to "library.so".

Another issue: