[Lldb-commits] [PATCH] D71575: [LLDB] Add ObjectFileWasm plugin for WebAssembly debugging

2020-01-05 Thread Paolo Severini via Phabricator via lldb-commits 
paolosev added inline comments.



Comment at: lldb/source/Plugins/ObjectFile/wasm/ObjectFileWasm.cpp:46-63
+llvm::Optional GetVaruint7(DataExtractor §ion_header_data,
+lldb::offset_t *offset_ptr) {
+  lldb::offset_t initial_offset = *offset_ptr;
+  uint64_t value = section_header_data.GetULEB128(offset_ptr);
+  if (*offset_ptr == initial_offset || value > 127)
+return llvm::None;
+  return static_cast(value);

labath wrote:
> Maybe merge these and make the maximum width a function argument?
I'd keep the functions separate, it's better if they return different sized 
integers.



Comment at: lldb/source/Plugins/ObjectFile/wasm/ObjectFileWasm.cpp:342-343
+section_type,  // Section type.
+sect_info.offset & 0x, // VM address.
+sect_info.size, // VM size in bytes of this section.
+sect_info.offset &

labath wrote:
> Are the debug info sections actually loaded into memory for wasm? Should 
> these be zero (that's what they are for elf)?
Yes, I was thinking that the debug sections should be loaded into memory; not 
sure how this works for ELF, how does the debugger find the debug info in that 
case?



Comment at: lldb/source/Plugins/ObjectFile/wasm/ObjectFileWasm.cpp:382
+
+  DecodeSections(load_address);
+

labath wrote:
> This is strange.. I wouldn't expect that the section decoding logic should 
> depend on the actual address that the object is loaded in memory. Can you 
> explain the reasoning here?
There is a reason for this: DecodeNextSection() calls:
```ReadImageData(offset, size)```
and when the debug info sections are embedded in the wasm module (not loaded 
from a separated symbol file), ReadImageData() calls Process::ReadMemory() 
that, for a GDB remote connection, goes to ProcessGDBRemote::DoReadMemory(). 
Here I pass the offset because it also represents the specific id of the module 
loaded in the target debuggee process, as explained in the comment above.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71575/new/

https://reviews.llvm.org/D71575



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D71575: [LLDB] Add ObjectFileWasm plugin for WebAssembly debugging

2020-01-05 Thread Paolo Severini via Phabricator via lldb-commits 
paolosev updated this revision to Diff 236237.
paolosev marked 8 inline comments as done.
paolosev added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71575/new/

https://reviews.llvm.org/D71575

Files:
  lldb/include/lldb/Utility/ArchSpec.h
  lldb/source/API/SystemInitializerFull.cpp
  lldb/source/Plugins/ObjectFile/CMakeLists.txt
  lldb/source/Plugins/ObjectFile/wasm/CMakeLists.txt
  lldb/source/Plugins/ObjectFile/wasm/ObjectFileWasm.cpp
  lldb/source/Plugins/ObjectFile/wasm/ObjectFileWasm.h
  lldb/source/Utility/ArchSpec.cpp
  lldb/test/Shell/ObjectFile/wasm/basic.yaml
  lldb/test/Shell/ObjectFile/wasm/debug-sections.yaml
  lldb/tools/lldb-test/SystemInitializerTest.cpp

Index: lldb/tools/lldb-test/SystemInitializerTest.cpp
===
--- lldb/tools/lldb-test/SystemInitializerTest.cpp
+++ lldb/tools/lldb-test/SystemInitializerTest.cpp
@@ -56,6 +56,7 @@
 #include "Plugins/ObjectFile/ELF/ObjectFileELF.h"
 #include "Plugins/ObjectFile/Mach-O/ObjectFileMachO.h"
 #include "Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h"
+#include "Plugins/ObjectFile/wasm/ObjectFileWasm.h"
 #include "Plugins/Platform/Android/PlatformAndroid.h"
 #include "Plugins/Platform/FreeBSD/PlatformFreeBSD.h"
 #include "Plugins/Platform/Linux/PlatformLinux.h"
@@ -152,6 +153,7 @@
   ObjectFileELF::Initialize();
   ObjectFileMachO::Initialize();
   ObjectFilePECOFF::Initialize();
+  wasm::ObjectFileWasm::Initialize();
 
   ScriptInterpreterNone::Initialize();
 
@@ -345,6 +347,7 @@
   ObjectFileELF::Terminate();
   ObjectFileMachO::Terminate();
   ObjectFilePECOFF::Terminate();
+  wasm::ObjectFileWasm::Terminate();
 
   // Now shutdown the common parts, in reverse order.
   SystemInitializerCommon::Terminate();
Index: lldb/test/Shell/ObjectFile/wasm/debug-sections.yaml
===
--- /dev/null
+++ lldb/test/Shell/ObjectFile/wasm/debug-sections.yaml
@@ -0,0 +1,109 @@
+# RUN: yaml2obj %s > %t
+# RUN: lldb-test object-file %t | FileCheck %s
+
+# CHECK: Plugin name: wasm
+# CHECK: Architecture: wasm32-unknown-unknown-wasm
+# CHECK: UUID: 
+# CHECK: Executable: true
+# CHECK: Stripped: true
+# CHECK: Type: executable
+# CHECK: Strata: user
+# CHECK: Base VM address: 0x40
+
+# CHECK: Name: code
+# CHECK: Type: code
+# CHECK: VM address: 0x0
+# CHECK: VM size: 56
+
+# CHECK: Name: .debug_info
+# CHECK: Type: dwarf-info
+# CHECK: VM address: 0x87
+# CHECK: VM size: 80
+
+# CHECK: Name: .debug_abbrev
+# CHECK: Type: dwarf-abbrev
+# CHECK: VM address: 0xe7
+# CHECK: VM size: 67
+
+# CHECK: Name: .debug_line
+# CHECK: Type: dwarf-line
+# CHECK: VM address: 0x138
+# CHECK: VM size: 85
+
+# CHECK: Name: .debug_str
+# CHECK: Type: dwarf-str
+# CHECK: VM address: 0x19b
+# CHECK: VM size: 167
+
+--- !WASM
+FileHeader:
+  Version: 0x0001
+Sections:
+  - Type:TYPE
+Signatures:
+  - Index:   0
+ParamTypes:
+  - I32
+ReturnTypes:
+  - I32
+  - Type:FUNCTION
+FunctionTypes:   [ 0 ]
+  - Type:TABLE
+Tables:
+  - ElemType:FUNCREF
+Limits:
+  Flags:   [ HAS_MAX ]
+  Initial: 0x0001
+  Maximum: 0x0001
+  - Type:MEMORY
+Memories:
+  - Initial: 0x0002
+  - Type:GLOBAL
+Globals:
+  - Index:   0
+Type:I32
+Mutable: true
+InitExpr:
+  Opcode:  I32_CONST
+  Value:   66560
+  - Type:EXPORT
+Exports:
+  - Name:memory
+Kind:MEMORY
+Index:   0
+  - Name:square
+Kind:FUNCTION
+Index:   0
+  - Type:CODE
+Functions:
+  - Index:   0
+Locals:
+  - Type:I32
+Count:   6
+Body:238080808000210141102102200120026B21032003200036020C200328020C2104200328020C2105200420056C210620060F0B
+  - Type:CUSTOM
+Name:.debug_info
+Payload: 4C00040004010C005D0075000200360002020036009600010148000302230CA100010148049D00050400
+  - Type:CUSTOM
+Name:.debug_abbrev
+Payload: 011101250E1305030E10171B0E11011206022E0111011206030E3A0B3B0B271949133F190305000218030E3A0B3B0B4913042400030E3E0B0B0B00
+  - Type:CUSTOM
+Name:.debug_line
+Payload: 510004002F00010101FB0E0D0001010101000101673A5

[Lldb-commits] [PATCH] D67906: [lldb] Provide tab completion for target select/delete

2020-01-05 Thread Konrad Wilhelm Kleine via Phabricator via lldb-commits 
kwk added a comment.

Why don't we always print the target index? Wouldn't that be much simpler?




Comment at: lldb/source/Commands/CommandObjectTarget.cpp:76
 
-  strm.Printf("%starget #%u: %s", prefix_cstr ? prefix_cstr : "", target_idx,
-  exe_path);
+  if (prefix_cstr)
+strm << prefix_cstr;

Happy New Years! For the rest of file and in particular for the target 
properties (e.g. `arch`, `platform`), we use a one line `strm.Printf`. But I 
can understand that you don't want to introduce a `?:` for every argument of 
the `Printf`. However why don't we always print the target index? Wouldn't that 
be much simpler?


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67906/new/

https://reviews.llvm.org/D67906



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits