Hi @xujuntwt95329,
Honestly I wasn't thinking to support multithreading with this initial patch so I am not surprised that it doesn't work. You are right, we should add thread information to the messages.
Actually I am a little surprised that the patch still works after all this time (more than one year), and that there have not been small changes that caused merge errors.
I am afraid that manually patching LLDB might not be a sustainable solution. :(

Thanks @xujuntwt95329! I am very happy that this was useful for WebAssembly Micro Runtime!

In D78978#2488134, @aprantl wrote:

What's the testing story for WASM going to be?

In D78978#2481358, @vwzm228 wrote:

Is there any progress about such patch and D78801？

I have implemented the debugging feature in our Wasm VM based on https://reviews.llvm.org/D78801, and it already work to attach, set breakpoint, step, show variable value, backtrace...

I am not sure if I need to change LLDB part to this one, or keep using D78801.

But if both patches wont be merged, I have to maintain a private LLDB version....

In D78978#2483354, @dblaikie wrote:

Usually the thing is to ping the review thread at most weekly & maybe search around for specific reviewers to ask if you're met with a lot of silence.

In D78978#2481358, @vwzm228 wrote:

Is there any progress about such patch and D78801？

I have implemented the debugging feature in our Wasm VM based on https://reviews.llvm.org/D78801, and it already work to attach, set breakpoint, step, show variable value, backtrace...

I am not sure if I need to change LLDB part to this one, or keep using D78801.

But if both patches wont be merged, I have to maintain a private LLDB version....

What about replacing ProcessReadMemory(addr_t addr, ...) with ProcessReadMemory(Address addr, ...), or even banning the use of lldb::addr_t on everywhere except in the bowels of Process subclasses and as an interchange for getting addresses as text from users. You might want to store lldb::addr_t's for Symbol values for space concerns, but presumably the Symbol File would know the relevant address space, so it would fix that up and always hand out Addresses.

This would be a pretty big but mostly formal change. It would seem more natural, Address is our abstraction for addresses in a process. Instead we have this odd mix of API's that take lldb::addr_t and some that take Address.

In D80863#2080996, @tlively wrote:

@paolosev Hmm, I suspect that there is a bad interaction between the code with the range check removed and the algorithm that lays out and structures the basic blocks. I will take a closer look!

I was testing the performance of a program with a big switch statement in a loop, a very common pattern in C/C++ and I came across the problem with needless range checks with br_table that this patch is fixing (Sweet! :-))
However, testing the latest version of Emscripten, with this fix, I am finding that Clang now emits "worse" bytecode for my small test program (attached{F12106893}, compiled with emcc -O3 micro-interp.c -o micro-interp.js).
The code is like:

enum Op {
    A = 0, B, C, D, E, F, G, H, I, J, K, L
};
int f(Op* ops, int len) {
    int result = 0;
    for (int i = 0; i < len; i++) {
        Op op = ops[i];
        switch (op) {
            case A: { ... break; }
            case B: { ... break; }
            case C: { ... break; }
		...
            default: { ... break; }
        }
    }
    return result;
}

Hello @clayborg , @labath,
Any thoughts on this latest patch? :-)

I get assertion failures in debug builds:

• For the DWARF expression evaluation, I am here following the suggestion of defining a Factory plugin (DWARFEvaluatorFactory) which is cached by Module. Is there a way to define a simple plugin function, avoiding all the overhead of a plugin class? Most of the state is in DWARFExpression, which is passed to the evaluator, so the amount of state to pass around is much smaller.

This patch is a work in progress where I am refactoring the code, as suggested, to remove almost all dependencies on Wasm from the LLDB core that were present in the previous patch. In particular:

The thing I am resisting is to put all of this stuff in the the ProcessGDBRemote class. Instead of trying to generalize it so that it can handle everything (and generalize to the wrong thing), I very much like the idea of introducing a WasmProcess plugin class that handles all wasm stuff. If that class happens to use parts of gdb-remote, then so be it, but it means that it's not a problem for that class to use a dialect of the gdb-remote protocol, which includes as many wasm-specific packets as it needs. Then this class can create it's own implementation of the "Unwind" interface, which will use some WasmProcess-specific apis to undwind, but that will also be ok, since both classes will be wasm-specific.

Also, to eliminate qWasmCallStack we could maybe add the call stack (a list of PCs) to the stop packet. The format of a stop packet is:

T AA n1:r1;n2:r2;…

In D78801#2010536, @ted wrote:

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.

So instead of qWasmMem:frame_index;addr;len, we have something like qMemSpaceRead:addr;space;len and qMemSpaceWrite:addr;space;len;data .

"space" is stub dependent - it's just a number, and can mean different things for different targets. It could be different modules, like you talk about here, or different physical memory areas like in a Motorola 56K DSP, or different ways to get at the same memory (physical/virtual/cacheable, or directly controlling MESI bits instead of relying on TLB entries) like I implemented in FSLDBG.

If we do this, other targets that want to work with memory spaces can use them instead of having to implement their own extensions.

About the expression problem - the IR Interpreter doesn't handle some complex expressions. It needs work, but most targets use JIT. On Hexagon, we only recently enabled JIT in the compiler, and haven't done it in the debugger yet, so we use the IR Interpreter for everything. Unfortunately I haven't had time to dive into it to make it better.

In D78801#2009856, @labath wrote:

What if the wasm engine actually made some effort to present a more "conventional" view of the wasm "process"? I.e., in addition to the "implied" PC, we could have an "implied" SP (and maybe an FP too). Then it could lay out the call stack and the function arguments in "memory", and point the "SP" to it so that lldb is able to reconstruct the frames&variables using the "normal" algorithm. For the sake of exposition, lets assume the following "encoding" of the 64 bit memory space:

unsigned:16 type; // 0x5555 = stack
unsigned:16 tid; // are there threads in wasm?
unsigned:16 frame; // grows down, 0x0000 = bottommost frame (main), 0x0001 = second from bottom, etc.
unsigned:16 address;

Then the engine could say that the "SP" of thread 0x1234 is 0x5555123400050000, "FP" is `0x5555123400048010 and the have the memory contents be

0x5555123400048020: // third "local" (of frame 4)
0x5555123400048018: // second "local" (of frame 4)
0x5555123400048010: // first "local" (of frame 4)
0x5555123400048008: 0x5555123400038010 // previous FP (frame 3)
0x5555123400048000: ???? // previous PC (frame 3)
0x5555123400040010: // third "argument" (of frame 4)
0x5555123400040008: // second "argument" (of frame 4)
0x5555123400040000: // first "argument" (of frame 4)
0x5555123400038020: // third "local" (of frame 3)
0x5555123400038018: // second "local" (of frame 3)
0x5555123400038010: // first "local" (of frame 3)
0x5555123400038008: 0x5555123400028010 // previous FP (frame 2)
0x5555123400038000: ???? // previous PC (frame 2)
etc.

Then all it would be needed is to translate DW_OP_WASM_location into an appropriate FP+offset combo. Somehow...

I realize that this is basically throwing the problem "over the fence", and asking the other side to deal with things, but I am starting to get sceptical that we will be able to come up with a satisfactory solution within lldb.

In D78801#2009128, @clayborg wrote:

Could we just always use memory reading and have the address contain more info? Right now you have the top 32 bits for the module ID. Could it be something like:

struct WasmAddress {
  uint64_t module_id:16;
  uint64_t space:4; // 0 == code, 1 == data, 2 == global, 3==local, 4 == stack
  uint64_t frame_id:??;
  uint64_t addr: ??;
}

This would be a bitfield that would all fit into a 64 bit value and could then be easily sent to the GDB server with the standard m and M packets.

In D78801#2009103, @clayborg wrote:

Thanks for the explanations if everything WASM related. I now understand much better what you have.

Read Wasm memory.
IN : $qWasmMem:frame_index;addr;len
OUT: $xx..xx

frame index seems weird in a memory read packet. Seems like the module ID should be passed instead of the frame index.

Reading memory could be handled with memory identifiers, or segments. Currently the packets for m and M only have an address, but someone out there must support reading from CODE and DATA address segments in the DSP world. I have an email out to Ted Woodward to see what they do for Qualcomm's hexagon DSPs. I'll let you know what I find. Maybe each WASM module can identify N segments it needs and each module would have its own unique segments. Are module ID's just 1 based indexes?

In D78801#2007083, @clayborg wrote:

So is there memory to be read from the WASM runtime? Couldn't DW_OP_WASM_location 0x0 +8 be turned into an address that can be used to read the variable? It is also unclear what DW_OP_stack_value is used for here. The DWARF expression has no idea how many bytes to read for this value unless each virtual stack location knows how big it is? What happens if you have an array of a million items? That will not fit on the DWARF expression stack and each member would need to be read from memory?

It seems like the DW_OP_WASM_location + args should result in the address of the variable being pushed into the stack and the DW_OP_stack_value should be removed. This would mean at the end of the expression the address of the variable is on the stack and LLDB will just read it using the normal memory read? Am I missing something? Are there multiple memory regions? Are variables not considered to be in memory?

Thanks for your comments!
I have refactored this code in a separate patch, https://reviews.llvm.org/D78978, removing WasmProcessGDBRemote, moving part of the logic into ProcessGDBRemote but still keeping class UnwindWasm.
Let me know what you think...

I am adding all the pieces to this patch to make the whole picture clearer; I thought to add a piece at the time to simplify reviews, but probably it ended up making things more obscure. I can always split this patch later and I need to refactor everything anyway.

What is the best way to test classes WasmProcessGDBRemote and UnwindWasm?

In D77353#1978576, @aardappel wrote:

@paolosev you may have an opinion on this, given that you're already working with this data as well?

In D74781#1944988, @dblaikie wrote:

In D74781#1944970, @sbc100 wrote:

Regarding the .debug_loc section. I did a quick experiment it looks like, even before this patch we get the expected output:

test.o:	file format WASM

.debug_loc contents:
0x00000000: 
            [0x00000005, 0x00000009): DW_OP_consts +21, DW_OP_stack_value

0x0000001d: 
            [0x0000000a, 0x0000000e): DW_OP_consts +7, DW_OP_stack_value

It looks like reason this works is because there is only one relocation per function here. i.e. there are only two relocations in the above object code. I appears the individual ranges do not themselves contain any relocations.

Hmm, what does llvm-dwarfdump -debug-loc -v print for this? I did implement DWARF loclist base address specifier support which means sometimes it'll use a base address specifier (which would be only one address) & everything would be offsets relative to that - but that's not always used.

Hmm, maybe I'm mistaken... maybe we do /always/ use base address specifiers in debug_loc now... Yep, that's it, always using them.

Sorry for the distraction & thanks for talking it through with me!

Yes, I managed to get a .debug_loc section also for Wasm with similar code, and debug_loc does not seem to present the same problems with GC'd functions.

Even without your patch? If without your patch you were getting range list entries of zero/zero, I'd expect you to get debug_loc entries with zero/zero too. Any ideas why it doesn't? They should be a similar pair of relocations (though it looks like your fix should work for debug_loc too, which is good).

Looking at the generated DWARF, this patch does not change the offsets for debug_loc. Like in the example above, there are loc entries for variables in the deleted function f2:

0x0000004a:   DW_TAG_subprogram
                DW_AT_low_pc    (0x0000000000000000)
                DW_AT_high_pc   (0x000000000000004d)
                DW_AT_frame_base        (DW_OP_WASM_location 0x0 +1, DW_OP_stack_value)
                DW_AT_GNU_all_call_sites        (true)
                DW_AT_linkage_name      ("_Z2f2Ri")
                DW_AT_name      ("f2")
                DW_AT_decl_file ("C:\dev\llvm-project\release_x64\T\main.cc")
                DW_AT_decl_line (3)
                DW_AT_type      (0x000000cd "int")
                DW_AT_external  (true)
[...]
0x00000071:     DW_TAG_variable
                  DW_AT_location        (0x00000000:
                     [0x00000014, 0x0000001b): DW_OP_consts +42, DW_OP_stack_value
                     [0x00000026, 0x0000002d): DW_OP_consts +21, DW_OP_stack_value
                     [0x0000003f, 0x0000004d): DW_OP_WASM_location 0x0 +2, DW_OP_stack_value)
                  DW_AT_name    ("i")
                  DW_AT_decl_file       ("C:\dev\llvm-project\release_x64\T\main.cc")
                  DW_AT_decl_line       (4)
                  DW_AT_type    (0x000000cd "int")

The entries seems to assume that the deleted function starts at offset 0, and ends at 0x4d, and this is true also without the patch.

In D74781#1939787, @dblaikie wrote:

In D74781#1938336, @paolosev wrote:

@sbc100 Adding a test.

@dblaikie Strangely, I can't find a way to generate a .debug_loc section for Wasm.

Maybe it's not applicable to wasm, I don't know much about it - but if it optimizes variables into registers (or out of them), then it shuold be producible.

Here's a simple test case I use for debug_loc in C++ on a silicon target:

void f1();
int main() {
  int i = 3;
  f1();
  i = 7;
  f1();
}

Compiled with optimizations enabled, the storage for 'i' will be eliminated and a DWARF location list will be used to describe 'i' as having the value 3 over the first call instruction, and the value 7 over the second call instruction.

@sbc100 Adding a test.

Thank you Sam, fixing this in calcNewValue also works and it's cleaner.

The Harbormaster errors don't seem to be real failures:

/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/lldb/source/Core/Module.cpp:9:10: error: 'lldb/Core/Module.h' file not found [clang-diagnostic-error]
#include "lldb/Core/Module.h"
         ^
/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/lldb/source/Target/Memory.cpp:9:10: error: 'lldb/Target/Memory.h' file not found [clang-diagnostic-error]
#include "lldb/Target/Memory.h"
         ^

In D75200#1894162, @clayborg wrote:

Not sure this is the right fix. The calling code should listen to how many bytes were actually read from memory and avoid touching any bytes. So we should fix Module::GetMemoryObjectFile to resize its buffer back to only the size that was read. S

In D72751#1892925, @labath wrote:

Who is the owner of this MemoryCache code I should ask for a review?

It sounds like the fix will be fairly straightforward. I think anyone on this thread should be able to review that.

Hi @paolosev, the lldb sanitized bot is flagging a container-overflow error here. I know that this /can/ have FPs when sanitized and unsanitized code is mixed, but we should be in purely sanitized code here, and this looks like a valid report. PTAL.

In D72751#1892514, @vsk wrote:

Hi @paolosev, the lldb sanitized bot is flagging a container-overflow error here. I know that this /can/ have FPs when sanitized and unsanitized code is mixed, but we should be in purely sanitized code here, and this looks like a valid report. PTAL.

http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake-sanitized/992/testReport/junit/lldb-api/functionalities_gdb_remote_client/TestWasm_py/

In D74780#1881297, @JDevlieghere wrote:

Sorting the plugins so the static loader is initialized before the WASM loader should exercise this code path.

I think it depends on whether the DynamicLoaderStatic should be a fallback. If it doesn't make sense then yes, I think we should reject that triple there.

It should not be a fallback. Ok! I'll create a new patch with this change.

@JDevlieghere added inline comments:

lldb/source/API/SystemInitializerFull.cpp 244
What's the rationale here? Plugins shouldn't rely on the order in which they are initialized. This breaks when the initializers are auto generated. Can we remove this dependency?

In D72751#1869268, @labath wrote:

yeah, go for it.

Fix patch after rebasing:

• Use macros to initialize plugins in SystemInitializer.
• Move tests from /packages/Python/lldbsuite/test/functionalities/gdb_remote_client/ to /test/API/functionalities/gdb_remote_client/

In D72751#1864138, @labath wrote:

Ok, let's give this one more try. I have a couple of inline comments for the further simplification of the test case.

In D72751#1862140, @labath wrote:

As promised, here are the comments on the new tests. I think that most of the py3 incompatibilities will go away once we get rid of the yaml preprocessing step, but it would be good to verify this with python 3 nonetheless...

Modified tests to be compatible with Python3.

In D72751#1860950, @max-kudr wrote:

There is Windows Build Bot failure http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/13427. Can you please fix or revert it?

Cannot open include file: 'Plugins/DynamicLoader/wasm-DYLD/DynamicLoaderWasmDYLD.h': No such file or directory

Thank you Derek, Jonas; I am sorry for all the trouble...

Fixing ObjectFile/wasm tests.

Rebasing.

Fixed the tests as suggested, and also added a couple more tests, to cover both the case where the Wasm module is loaded from memory and the case where it is loaded from a file.

• make the base DynamicLoader class instantiatable, and use it whenever we fail to find a specialized plugin
• same as above, but only do that for ProcessGDBRemote instances
• make ProcessGDBRemote call LoadModules() itself if no dynamic loader instance is available WDYT?

I am fine with 1 as long as we document the DynamicLoader class to say that it will call Process::LoadModules() and will be used if no specialized loader is needed for your platform. I would like to a see a solution that will work for any process plug-in and not just ProcessGDBRemote. If we change solution 3 above to say "Make lldb_private::Process call LoadModules() itself if no dynamic loader instance is available" then solution 3 is also fine.

Comments inline...

In D72751#1841498, @labath wrote:

Thanks. I am glad that we were able to sort that out.

Now that there's nothing wasm-specific in DynamicLoaderWasmDYLD::LoadModuleAtAddress, I have another question. :)

The code in that function is a subset of the base DynamicLoader::LoadModuleAtAddress method you are overriding. Is there a reason for that?
It doesn't seem like the additional stuff in the base method should hurt here. What the additional code does is that it tries to search for the object file on the local filesystem, and if it finds it, it will use that instead of copying the file over from the remote. In fact, that sounds like it could be useful here too, as copying that much data over gdb-remote isn't particularly fast..

What do you think about that?

Modified to set m_file_offset to be the correct offset of the Code section. This also simplifies the code in ObjectFileWasm to avoid a special case for the code section in ObjectFileWasm::SetLoadAddress.
The DWARF code seems to only use GetFileAddress(), which still needs to return zero for the Code section.

Thanks for the explanation! I wasn't quite clear on "executable module" here, but after your comments I realized that Target::SetExecutableModule() should not probably be called also for Wasm modules.
The point is that ObjectFileWasm::CalculateType() should return eTypeSharedLibrary, not eTypeExecutable.
With this change the first issue is easily solved: we just need to call Target::SetSectionLoadAddress() once, in ObjectFileWasm::SetLoadAddress() because Target::SetExecutableModule() -> Target::ClearModules() -> SectionLoadList::Clear() is not called, and DynamicLoaderWasmDYLD::DidAttach() can be simplified to just call ProcessGDBRemote::LoadModules().
Does this solution work for you? If so, we should look at the second point, the need to initialize m_file_addr = m_file_offset = 0 for the "code" Section in order to make the DWARF symbols work...

I have verified the logic of the dynamic loader quite carefully, but there are a couple of things to clarify.

In D71575#1823252, @jingham wrote:

BTW, I had to fix this patch (cd9e5c32302cd3b34b796683eedb072c6a1cfdc1) to build on macOS. uint64_t and size_t are differently spelled (though I think otherwise equivalent.) One is "long long unsigned int", the other "long unsigned int". I have no idea why that's true, but std::min refuses to compare a size_t and a unit64_t. Anyway, I fixed this by casting one of the two sides of the comparison. But this was causing problems because we have an api (ReadImageData) that takes a uint64_t for the offset and a size_t for the size. That seems a little weird to me, why are these different types?

Modified test to have two "inlined" yaml files and use "yaml2obj --docnum".

In D71575#1821312, @labath wrote:

Thanks. I think this is looking very good now. Excited to have this ready.

Do you have commit access?

What is the best way to test this class?

Rebasing from D71575.

In D72650#1819403, @labath wrote:

The patch looks pretty good. A reasonable way to test this would be again via lldb-test object-file . The command dumps the "unified section list" of the module, so if the debug info sections show up there, you know the symbol vendor has done it's job. You can look at test/Shell/ObjectFile/ELF/build-id-case.yaml for inspiration.

Added "lldb-test object-file" test.

I apologize for the noob question, but how do I schedule a build for this diff with Harbormaster?

Addressed more review comments:

• removed code to manage "external_debug_info" sections; logic for this will be implemented in the symbol vendor code.
• modified test code, from unittests to be Shell "lit" tests.

In D71575#1793791, @labath wrote:

A bunch more comments from me. :)

A higher level question I have is whether there's something suitable within llvm for parsing wasm object files that could be reused. I know this can be tricky with files loaded from memory etc., so it's fine if it isn't possible to do that, but I am wondering if you have considered this option.

Doh! I didn't know there was this work ongoing.
I think we could maybe continue from there, once https://reviews.llvm.org/D70930 is merged; what I would like is to have something like --split-dwo that works with WASM files and only extracts DWARF sections.

Addressed first review comments.

Thanks for all the comments! I am updating the code following your suggestions.
Next step will be to split this into three distinct patches, as suggested by Pavel.