[wasm] Always treat DWARF expression addresses as load addresses

[pfaffe]

Summary

When resolving absolute addresses for DW_OP_addr or DW_OP_addrx, these are always load addresses rather than file addresses in wasm.

[pfaffe]

I'm having some trouble coming up with a good test for this in the absence of a real wasm target. any suggestions appreciated!

[DavidSpickett]

What does it mean the"lack of a real wasm target", do you mean a compile target? I don't have the context so I'm guessing clang can't compile *to* wasm currently and that means you can't easily make a reproducer that way.

I wonder if you could build something from C targeting <something not wasm> but which has the DWARF construct you need, then force it to be loaded as if it were a wasm object. Seems like that would trip all sorts of assertions though.

The other way is to hand craft an object file, perhaps using obj2yaml? Though I don't know if that will let you define the DWARF in a nice readable way, or whether you'd have to put it in as binary data. (this sort of thing is a bit out of my wheel house)

[DavidSpickett]

And if you mean there's no wasm runtime to load the object file into to actually run it - ignore what I just said :)

I'm not sure there is a way around that. Short of building the file and hacking up enough of it so that lldb saw eCore_wasm32.

I thought maybe you could use a corefile instead but A: I don't know that wasm supports those and B: that we support what it produces either.

[pfaffe]

Managed to add a unit test for the DW_OP_addr path!

[DavidSpickett]

You're missing context for the diff https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface (though I could swear it was there before).

Can you explain the reasoning for the rule, is there a wasm DWARF doc that explains it? Maybe it is just common sense given what wasm is, but I've not used it myself.

lldb/unittests/Expression/DWARFExpressionTest.cpp

435

There are two target && target->GetArchitecture().GetCore() == ArchSpec::eCore_wasm32 checks, does this cover both?

(will probably be obvious once the diff has context)

[pfaffe]

Sorry for the missing context, I forgot that's manual!

The most complete description of wasm DWARF is here: https://yurydelendik.github.io/webassembly-dwarf/ (via https://github.com/WebAssembly/tool-conventions/blob/main/Debugging.md). But it doesn't really discuss the details of this rule.
In essence, the reason is that wasm file sections aren't mapped into wasm memory. A wasm program can therefore not access any data from file sections, and hence no address will ever point to the file. Consequently for varibles that have statically known addresses encoded as DW_OP_addr, these addresses should be interpreted as load addresses, not file addresses.

[DavidSpickett]

In essence, the reason is that wasm file sections aren't mapped into wasm memory. A wasm program can therefore not access any data from file sections, and hence no address will ever point to the file.

This is enough for me, please add some version of that as a comment in both places where you check for wasm.

[pfaffe]

Full context with arcanist

[pfaffe]

Add comments.

[pfaffe]

Bring back lost changes...

[DavidSpickett]

lldb/unittests/Expression/DWARFExpressionTest.cpp

435

I think the answer to this is no, you need to add a test for DW_OP_GNU_addr_index? Not sure.

[pfaffe]

Add unittest for DW_OP_addrx as well.

[DavidSpickett]

LGTM, thanks!

lldb/source/Expression/DWARFExpression.cpp

if (error_ptr)
       error_ptr->SetErrorString(
           "no location, value may have been optimized out");
     return false;
   }
   std::vector<Value> stack;
 
   Process *process = nullptr;
   StackFrame *frame = nullptr;
+  Target *target = nullptr;
 
   if (exe_ctx) {
     process = exe_ctx->GetProcessPtr();
     frame = exe_ctx->GetFramePtr();
+    target = exe_ctx->GetTargetPtr();
   }
   if (reg_ctx == nullptr && frame)
     reg_ctx = frame->GetRegisterContext().get();
 
   if (initial_value_ptr)
     stack.push_back(*initial_value_ptr);
 
   lldb::offset_t offset = 0;
if (log && log->GetVerbose()) {
                 DW_OP_value_to_name(op));
     }
 
     switch (op) {
     // The DW_OP_addr operation has a single operand that encodes a machine
     // address and whose size is the size of an address on the target machine.
     case DW_OP_addr:
       stack.push_back(Scalar(opcodes.GetAddress(&offset)));
+      if (target &&
+          target->GetArchitecture().GetCore() == ArchSpec::eCore_wasm32) {
+        // wasm file sections aren't mapped into memory, therefore addresses can
+        // never point into a file section and are always LoadAddresses.
+        stack.back().SetValueType(Value::ValueType::LoadAddress);
+      } else {
         stack.back().SetValueType(Value::ValueType::FileAddress);
         // Convert the file address to a load address, so subsequent
         // DWARF operators can operate on it.
         if (frame)
           stack.back().ConvertToLoadAddress(module_sp.get(),
                                             frame->CalculateTarget().get());
+      }
       break;
 
     // The DW_OP_addr_sect_offset4 is used for any location expressions in
     // shared libraries that have a location like:
     //  DW_OP_addr(0x1000)
     // If this address resides in a shared library, then this virtual address
     // won't make sense when it is evaluated in the context of a running
     // process where shared libraries have been slid. To account for this, this
case DW_OP_GNU_addr_index: {
         if (error_ptr)
           error_ptr->SetErrorString("DW_OP_GNU_addr_index found without a "
                                     "compile unit being specified");
         return false;
       }
       uint64_t index = opcodes.GetULEB128(&offset);
       lldb::addr_t value = dwarf_cu->ReadAddressFromDebugAddrSection(index);
       stack.push_back(Scalar(value));
+      if (target &&
+          target->GetArchitecture().GetCore() == ArchSpec::eCore_wasm32) {
+        // wasm file sections aren't mapped into memory, therefore addresses can
+        // never point into a file section and are always LoadAddresses.
+        stack.back().SetValueType(Value::ValueType::LoadAddress);
+      } else {
         stack.back().SetValueType(Value::ValueType::FileAddress);
+      }
     } break;
 
     // OPCODE: DW_OP_GNU_const_index
     // OPERANDS: 1
     //      ULEB128: index to the .debug_addr section
     // DESCRIPTION: Pushes an constant with the size of a machine address to
     // the stack from the .debug_addr section with the base address specified
     // by the DW_AT_addr_base attribute and the 0 based index is the ULEB128

lldb/unittests/Expression/DWARFExpressionTest.cpp

 //===-- DWARFExpressionTest.cpp -------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "lldb/Expression/DWARFExpression.h"
 #include "Plugins/Platform/Linux/PlatformLinux.h"
+#include "Plugins/SymbolFile/DWARF/DWARFDebugInfo.h"
 #include "Plugins/TypeSystem/Clang/TypeSystemClang.h"
 #include "TestingSupport/Symbol/YAMLModuleTester.h"
 #include "lldb/Core/Debugger.h"
 #include "lldb/Core/Value.h"
 #include "lldb/Core/dwarf.h"
 #include "lldb/Host/HostInfo.h"
 #include "lldb/Symbol/ObjectFile.h"
 #include "lldb/Utility/StreamString.h"
TEST_F(DWARFExpressionMockProcessTest, DW_OP_deref) {
   EXPECT_THAT_EXPECTED(Evaluate({DW_OP_lit4}, {}, {}, &exe_ctx),
                        llvm::HasValue(Scalar(4)));
   // Implicit location: *0x4.
   // Evaluate returns LLDB_INVALID_ADDRESS for all load addresses.
   EXPECT_THAT_EXPECTED(
       Evaluate({DW_OP_lit4, DW_OP_deref, DW_OP_stack_value}, {}, {}, &exe_ctx),
       llvm::HasValue(GetScalar(32, 0x07060504, false)));
 }
+
+TEST_F(DWARFExpressionMockProcessTest, WASM_DW_OP_addr) {
+  // Set up a wasm target
+  ArchSpec arch("wasm32-unknown-unknown-wasm");
+  lldb::PlatformSP host_platform_sp =
+      platform_linux::PlatformLinux::CreateInstance(true, &arch);
+  ASSERT_TRUE(host_platform_sp);
+  Platform::SetHostPlatform(host_platform_sp);
+  lldb::DebuggerSP debugger_sp = Debugger::CreateInstance();
+  ASSERT_TRUE(debugger_sp);
+  lldb::TargetSP target_sp;
+  lldb::PlatformSP platform_sp;
+  debugger_sp->GetTargetList().CreateTarget(*debugger_sp, "", arch,
+                                            lldb_private::eLoadDependentsNo,
+                                            platform_sp, target_sp);
+
+  ExecutionContext exe_ctx(target_sp, false);
+  // DW_OP_addr takes a single operand of address size width:
+  uint8_t expr[] = {DW_OP_addr, 0x40, 0x0, 0x0, 0x0};
+  DataExtractor extractor(expr, sizeof(expr), lldb::eByteOrderLittle,
+                          /*addr_size*/ 4);
+  Value result;
+  Status status;
+  ASSERT_TRUE(DWARFExpression::Evaluate(
+      &exe_ctx, /*reg_ctx*/ nullptr, /*module_sp*/ {}, extractor,
+      /*unit*/ nullptr, lldb::eRegisterKindLLDB,
+      /*initial_value_ptr*/ nullptr,
+      /*object_address_ptr*/ nullptr, result, &status))
+      << status.ToError();
+
+  ASSERT_EQ(result.GetValueType(), Value::ValueType::LoadAddress);
+}
+
+TEST_F(DWARFExpressionMockProcessTest, WASM_DW_OP_addr_index) {
+  const char *yamldata = R"(
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_EXEC
+  Machine: EM_386
+DWARF:
+  debug_abbrev:
+    - Table:
+        - Code:            0x00000001
+          Tag:             DW_TAG_compile_unit
+          Children:        DW_CHILDREN_no
+          Attributes:
+            - Attribute:       DW_AT_addr_base
+              Form:            DW_FORM_sec_offset
+
+  debug_info:
+    - Version:         5
+      AddrSize:        4
+      UnitType:        DW_UT_compile
+      Entries:
+        - AbbrCode:        0x00000001
+          Values:
+            - Value:           0x8 # Offset of the first Address past the header
+        - AbbrCode:        0x0
+
+  debug_addr:
+    - Version: 5
+      AddressSize: 4
+      Entries:
+        - Address: 0x1234
+        - Address: 0x5678
+)";
+
+  // Can't use DWARFExpressionTester from above because subsystems overlap with
+  // the fixture.
+  SubsystemRAII<ObjectFileELF, SymbolFileDWARF> subsystems;
+  llvm::Expected<TestFile> file = TestFile::fromYaml(yamldata);
+  EXPECT_THAT_EXPECTED(file, llvm::Succeeded());
+  auto module_sp = std::make_shared<Module>(file->moduleSpec());
+  auto *dwarf_cu = llvm::cast<SymbolFileDWARF>(module_sp->GetSymbolFile())
+                       ->DebugInfo()
+                       .GetUnitAtIndex(0);
+  ASSERT_TRUE(dwarf_cu);
+  dwarf_cu->ExtractDIEsIfNeeded();
+
+  // Set up a wasm target
+  ArchSpec arch("wasm32-unknown-unknown-wasm");
+  lldb::PlatformSP host_platform_sp =
+      platform_linux::PlatformLinux::CreateInstance(true, &arch);
+  ASSERT_TRUE(host_platform_sp);
+  Platform::SetHostPlatform(host_platform_sp);
+  lldb::DebuggerSP debugger_sp = Debugger::CreateInstance();
+  ASSERT_TRUE(debugger_sp);
+  lldb::TargetSP target_sp;
+  lldb::PlatformSP platform_sp;
+  debugger_sp->GetTargetList().CreateTarget(*debugger_sp, "", arch,
+                                            lldb_private::eLoadDependentsNo,
+                                            platform_sp, target_sp);
+
+  ExecutionContext exe_ctx(target_sp, false);
+  // DW_OP_addrx takes a single leb128 operand, the index in the addr table:
+  uint8_t expr[] = {DW_OP_addrx, 0x01};
+  DataExtractor extractor(expr, sizeof(expr), lldb::eByteOrderLittle,
+                          /*addr_size*/ 4);
+  Value result;
+  Status status;
+  ASSERT_TRUE(DWARFExpression::Evaluate(
+      &exe_ctx, /*reg_ctx*/ nullptr, /*module_sp*/ {}, extractor, dwarf_cu,
+      lldb::eRegisterKindLLDB,
+      /*initial_value_ptr*/ nullptr,
+      /*object_address_ptr*/ nullptr, result, &status))
+      << status.ToError();
+
+  ASSERT_EQ(result.GetValueType(), Value::ValueType::LoadAddress);
+  ASSERT_EQ(result.GetScalar().UInt(), 0x5678u);
+}