[lldb] Add settings for expression evaluation memory allocations.

[kuilpd]

Summary

Expression evaluation allocates memory for storing intermediate data during evaluation. For it to work properly it has to be allocated within target's available address space, for example within first 0xFFFF bytes for the 16-bit MSP430. The memory for such targets can be very tightly packed, but not all targets support GetMemoryRegionInfo API to pick an unused region, like MSP430 with MSPDebug GDB server.

These settings allow the programmer to manually pick precisely where and how much memory to allocate for expression evaluation in order not to overlap with existing data in process memory.

[jingham]

lldb/source/Expression/LLVMUserExpression.cpp

340

This doesn't seem appropriate in generic code. You should ask the Arch plugin for this.

[bulbazord]

lldb/source/Expression/LLVMUserExpression.cpp

340

Oh, I missed this in the previous patch. Yeah, we shouldn't be hardcoding llvm::Triple::msp430 in generic code. You're introducing even more uses of this in this patch, we should abstract this out into a plugin like Jim suggested.

[kuilpd]

lldb/source/Expression/LLVMUserExpression.cpp

340

Do you mean creating a new MSP430 plugin in lldb/source/Plugins/Architecture/, or accessing the existing ABI plugin? Should I create new methods for getting stack frame size in each existing plugin?

[bulbazord]

lldb/source/Expression/LLVMUserExpression.cpp

340

You could probably put this in the ABI plugin? The base ABI class could have a virtual method like GetStackFrameSize that returns 512 * 1024 and the MSP430 subclass can override it and return 512 instead. That should work, I think.

[kuilpd]

Made an ABI method that returns a stack frame size for the target.

Removed the condition for alignment. The default value doesn't matter too much even for a 16-bit target, no need to add another ABI method just for this. The programmer can use the settings to change it if necessary.

[asl]

lldb/source/Expression/IRMemoryMap.cpp

180–181

is arch unused now?

[kuilpd]

Removed unused variable arch.

[bulbazord]

Mostly looks good to me, just a small thing about an implementation detail.

lldb/source/Expression/LLVMUserExpression.cpp

38

What in this file uses something from the lldb namespace now?

340–348

A few things:

• I don't think you need to re-extract process from exe_ctx, the variable process at the beginning of this function should have a valid Process pointer in it already (see: LockAndCheckContext at the beginning of this function). We already assume target is valid and use it in the same way.
• If we can't get an ABI from the Process, do we want to assume a stack frame size of 512 * 1024? Maybe there is a use case I'm missing, but if we don't have an ABI I'm not convinced that PrepareToExecuteJITExpression should succeed. LLDB will need some kind of ABI information to correctly set up the register context to call the JITed code anyway.

[kuilpd]

lldb/source/Expression/LLVMUserExpression.cpp

38

ABI class

340–348

• I tried that, but a lot of tests fail on GetABI() after that, so had to re-extract it. Not sure why.
• 512 * 1024 is what was hardcoded there by default. It makes sense that ABI has to exist, but leaving no default value in case if it's not retreived is weird as well. Or should we return an error in that case?

[bulbazord]

lldb/source/Expression/LLVMUserExpression.cpp

340–348

How do the tests fail? If the process is wrong that sounds like a pretty bad bug :(

I would think that we could return false with some logging or an error would be appropriate if we don't have an ABI. I may be misunderstanding something but I would think that the tests should still pass when we return false there... I hope.

[kuilpd]

lldb/source/Expression/LLVMUserExpression.cpp

340–348

Tried returning false, even more tests failed.
Did some digging, turns out expression can work without any target, right after starting LLDB.
So tests like this one fail because there is no target or process.

[bulbazord]

Sorry I should have brought this up earlier but I've noticed you don't have any tests with this change. Is it possible you could add something there? Something to make sure that these settings work as expected.

Sorry for the churn btw, I really appreciate your patience here.

lldb/source/Expression/LLVMUserExpression.cpp

340–348

Ah, right, I forgot! A given expression, if simple enough, might be compiled to LLVM IR and then interpreted instead of being compiled to machine code and run in the inferior... The fact that a stack frame size is even relevant in that case is strange but there's not much we can do about it without further refactors. Does something like this work then? If not, this should be fine.

if (stack_frame_size == 0) {
  ABISP abi_sp;
  if (process && (abi_sp = process->GetABI()))
    stack_frame_size = abi_sp->GetStackFrameSize();
  else
    stack_frame_size = 512 * 1024;
}

[kuilpd]

In D149262#4306820, @bulbazord wrote:

Sorry I should have brought this up earlier but I've noticed you don't have any tests with this change. Is it possible you could add something there? Something to make sure that these settings work as expected.

Should I do it also without a target?

Sorry for the churn btw, I really appreciate your patience here.

No worries, these are all valid points you raised :)

lldb/source/Expression/LLVMUserExpression.cpp

340–348

This worked, thanks!

[bulbazord]

In D149262#4306884, @kuilpd wrote:

In D149262#4306820, @bulbazord wrote:

Sorry I should have brought this up earlier but I've noticed you don't have any tests with this change. Is it possible you could add something there? Something to make sure that these settings work as expected.

Should I do it also without a target?

It's probably enough to just add a test with a target, the settings shouldn't mess with the non-target case I think.

[kuilpd]

Rebased and added a test.

Instead of just getting a variable's address from allocated inside an expression, I decided to check for memory allocations inside logs. Exact variable address could change with the compilation changes, so this should be more reliable.

[bulbazord]

Thanks for adding the test. The test itself doesn't seem portable and likely will fail on several platforms though I'm not sure which off the top of my head. After landing this, please watch the buildbots and be prepared to add the necessary decorators to the test to skip those platforms.

[kuilpd]

Rebased.

lldb/include/lldb/Target/ABI.h

public:
 
   llvm::MCRegisterInfo &GetMCRegisterInfo() { return *m_mc_register_info_up; }
 
   virtual void
   AugmentRegisterInfo(std::vector<DynamicRegisterInfo::Register> &regs) = 0;
 
   virtual bool GetPointerReturnRegister(const char *&name) { return false; }
 
+  virtual uint64_t GetStackFrameSize() { return 512 * 1024; }
+
   static lldb::ABISP FindPlugin(lldb::ProcessSP process_sp, const ArchSpec &arch);
 
 protected:
   ABI(lldb::ProcessSP process_sp, std::unique_ptr<llvm::MCRegisterInfo> info_up)
       : m_process_wp(process_sp), m_mc_register_info_up(std::move(info_up)) {
     assert(m_mc_register_info_up && "ABI must have MCRegisterInfo");
   }
 

lldb/include/lldb/Target/Target.h

public:
   bool GetBreakpointsConsultPlatformAvoidList();
 
   lldb::LanguageType GetLanguage() const;
 
   llvm::StringRef GetExpressionPrefixContents();
 
   uint64_t GetExprErrorLimit() const;
 
+  uint64_t GetExprAllocAddress() const;
+
+  uint64_t GetExprAllocSize() const;
+
+  uint64_t GetExprAllocAlign() const;
+
   bool GetUseHexImmediates() const;
 
   bool GetUseFastStepping() const;
 
   bool GetDisplayExpressionsInCrashlogs() const;
 
   LoadScriptFromSymFile GetLoadScriptFromSymbolFile() const;
 

lldb/source/Expression/IRMemoryMap.cpp

if (m_allocations.empty()) {
     ret = 0x0;
   } else {
     auto back = m_allocations.rbegin();
     lldb::addr_t addr = back->first;
     size_t alloc_size = back->second.m_size;
     ret = llvm::alignTo(addr + alloc_size, 4096);
   }
 
-  // Now, if it's possible to use the GetMemoryRegionInfo API to detect mapped
-  // regions, walk forward through memory until a region is found that has
-  // adequate space for our allocation.
-  if (process_is_alive) {
-    uint64_t end_of_memory;
-    switch (process_sp->GetAddressByteSize()) {
-    case 2:
-      end_of_memory = 0xffffull;
-      break;
-    case 4:
-      end_of_memory = 0xffffffffull;
-      break;
-    case 8:
-      end_of_memory = 0xffffffffffffffffull;
-      break;
-    default:
-      lldbassert(false && "Invalid address size.");
-      return LLDB_INVALID_ADDRESS;
-    }
+  switch (GetAddressByteSize()) {
 
+  // Now, if it's possible to use the GetMemoryRegionInfo API to detect mapped
+  // regions, walk forward through memory until a region is found that has
+  // adequate space for our allocation.
+  if (process_is_alive) {
     MemoryRegionInfo region_info;
     Status err = process_sp->GetMemoryRegionInfo(ret, region_info);
     if (err.Success()) {
       while (true) {
         if (region_info.GetReadable() != MemoryRegionInfo::OptionalBool::eNo ||
             region_info.GetWritable() != MemoryRegionInfo::OptionalBool::eNo ||
             region_info.GetExecutable() !=
                 MemoryRegionInfo::OptionalBool::eNo) {
if (err.Success()) {
       }
     }
   }
 
   // We've tried our algorithm, and it didn't work.  Now we have to reset back
   // to the end of the allocations we've already reported, or use a 'sensible'
   // default if this is our first allocation.
   if (m_allocations.empty()) {
+    uint64_t alloc_address = target_sp->GetExprAllocAddress();
+    if (alloc_address > 0) {
+      if (alloc_address >= end_of_memory) {
+        lldbassert(0 && "The allocation address for expression evaluation must "
+                        "be within process address space");
+        return LLDB_INVALID_ADDRESS;
+      }
+      ret = alloc_address;
+    } else {
       uint32_t address_byte_size = GetAddressByteSize();
       if (address_byte_size != UINT32_MAX) {
         switch (address_byte_size) {
         case 2:
           ret = 0x8000ull;
           break;
         case 4:
           ret = 0xee000000ull;
           break;
         case 8:
           ret = 0xdead0fff00000000ull;
           break;
         default:
           lldbassert(false && "Invalid address size.");
           return LLDB_INVALID_ADDRESS;
         }
       }
+    }
   } else {
     auto back = m_allocations.rbegin();
     lldb::addr_t addr = back->first;
     size_t alloc_size = back->second.m_size;
-    auto arch = target_sp->GetArchitecture().GetTriple().getArch();
-    auto align = arch == llvm::Triple::msp430 ? 512 : 4096;
+    uint64_t align = target_sp->GetExprAllocAlign();
+    if (align == 0)
+      align = 4096;
     ret = llvm::alignTo(addr + alloc_size, align);
   }
 
   return ret;
 }
 
 IRMemoryMap::AllocationMap::iterator
 IRMemoryMap::FindAllocation(lldb::addr_t addr, size_t size) {

lldb/source/Expression/LLVMUserExpression.cpp

 #include "lldb/Expression/Materializer.h"
 #include "lldb/Host/HostInfo.h"
 #include "lldb/Symbol/Block.h"
 #include "lldb/Symbol/Function.h"
 #include "lldb/Symbol/ObjectFile.h"
 #include "lldb/Symbol/SymbolVendor.h"
 #include "lldb/Symbol/Type.h"
 #include "lldb/Symbol/VariableList.h"
+#include "lldb/Target/ABI.h"
 #include "lldb/Target/ExecutionContext.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Target/StackFrame.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Target/ThreadPlan.h"
 #include "lldb/Target/ThreadPlanCallUserExpression.h"
 #include "lldb/Utility/ConstString.h"
 #include "lldb/Utility/LLDBLog.h"
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/StreamString.h"
 
+using namespace lldb;
 using namespace lldb_private;
 
 char LLVMUserExpression::ID;
 
 LLVMUserExpression::LLVMUserExpression(ExecutionContextScope &exe_scope,
                                        llvm::StringRef expr,
                                        llvm::StringRef prefix,
                                        lldb::LanguageType language,
if (m_materialized_address == LLDB_INVALID_ADDRESS) {
       }
     }
 
     struct_address = m_materialized_address;
 
     if (m_can_interpret && m_stack_frame_bottom == LLDB_INVALID_ADDRESS) {
       Status alloc_error;
 
-      auto arch = target->GetArchitecture().GetTriple().getArch();
-      const size_t stack_frame_size =
-          arch == llvm::Triple::msp430 ? 512 : 512 * 1024;
+      size_t stack_frame_size = target->GetExprAllocSize();
+      if (stack_frame_size == 0) {
+        ABISP abi_sp;
+        if (process && (abi_sp = process->GetABI()))
+          stack_frame_size = abi_sp->GetStackFrameSize();
+        else
+          stack_frame_size = 512 * 1024;
+      }
 
       const bool zero_memory = false;
 
       m_stack_frame_bottom = m_execution_unit_sp->Malloc(
           stack_frame_size, 8,
           lldb::ePermissionsReadable | lldb::ePermissionsWritable,
           IRMemoryMap::eAllocationPolicyHostOnly, zero_memory, alloc_error);
 
       m_stack_frame_top = m_stack_frame_bottom + stack_frame_size;
 
       if (!alloc_error.Success()) {

lldb/source/Plugins/ABI/MSP430/ABISysV_msp430.h

bool CallFrameAddressIsValid(lldb::addr_t cfa) override {
     return true;
   }
 
   bool CodeAddressIsValid(lldb::addr_t pc) override { return true; }
 
   const lldb_private::RegisterInfo *
   GetRegisterInfoArray(uint32_t &count) override;
 
+  uint64_t GetStackFrameSize() override { return 512; }
+
   //------------------------------------------------------------------
   // Static Functions
   //------------------------------------------------------------------
 
   static void Initialize();
 
   static void Terminate();
 

lldb/source/Target/Target.cpp

 }
 
 uint64_t TargetProperties::GetExprErrorLimit() const {
   const uint32_t idx = ePropertyExprErrorLimit;
   return m_collection_sp->GetPropertyAtIndexAsUInt64(idx).value_or(
       g_target_properties[idx].default_uint_value);
 }
 
+uint64_t TargetProperties::GetExprAllocAddress() const {
+  const uint32_t idx = ePropertyExprAllocAddress;
+  return m_collection_sp->GetPropertyAtIndexAsUInt64(
+      nullptr, idx, g_target_properties[idx].default_uint_value);
+}
+
+uint64_t TargetProperties::GetExprAllocSize() const {
+  const uint32_t idx = ePropertyExprAllocSize;
+  return m_collection_sp->GetPropertyAtIndexAsUInt64(
+      nullptr, idx, g_target_properties[idx].default_uint_value);
+}
+
+uint64_t TargetProperties::GetExprAllocAlign() const {
+  const uint32_t idx = ePropertyExprAllocAlign;
+  return m_collection_sp->GetPropertyAtIndexAsUInt64(
+      nullptr, idx, g_target_properties[idx].default_uint_value);
+}
+
 bool TargetProperties::GetBreakpointsConsultPlatformAvoidList() {
   const uint32_t idx = ePropertyBreakpointUseAvoidList;
   return m_collection_sp->GetPropertyAtIndexAsBoolean(idx).value_or(
       g_target_properties[idx].default_uint_value != 0);
 }
 
 bool TargetProperties::GetUseHexImmediates() const {
   const uint32_t idx = ePropertyUseHexImmediates;

lldb/source/Target/TargetProperties.td

def Language: Property<"language", "Language">,
     Desc<"The language to use when interpreting expressions entered in commands.">;
   def ExprPrefix: Property<"expr-prefix", "FileSpec">,
     DefaultStringValue<"">,
     Desc<"Path to a file containing expressions to be prepended to all expressions.">;
   def ExprErrorLimit: Property<"expr-error-limit", "UInt64">,
     DefaultUnsignedValue<5>,
     Desc<"The maximum amount of errors to emit while parsing an expression. "
          "A value of 0 means to always continue parsing if possible.">;
+  def ExprAllocAddress: Property<"expr-alloc-address", "UInt64">,
+    DefaultUnsignedValue<0>,
+    Desc<"Start address within the process address space of memory allocation for expression evaluation.">;
+  def ExprAllocSize: Property<"expr-alloc-size", "UInt64">,
+    DefaultUnsignedValue<0>,
+    Desc<"Amount of memory in bytes to allocate for expression evaluation.">;
+  def ExprAllocAlign: Property<"expr-alloc-align", "UInt64">,
+    DefaultUnsignedValue<0>,
+    Desc<"Alignment for each memory allocation for expression evaluation.">;
   def PreferDynamic: Property<"prefer-dynamic-value", "Enum">,
     DefaultEnumValue<"eDynamicDontRunTarget">,
     EnumValues<"OptionEnumValues(g_dynamic_value_types)">,
     Desc<"Should printed values be shown as their dynamic value.">;
   def EnableSynthetic: Property<"enable-synthetic-value", "Boolean">,
     DefaultTrue,
     Desc<"Should synthetic values be used by default whenever available.">;
   def SkipPrologue: Property<"skip-prologue", "Boolean">,

lldb/test/API/commands/expression/memory-allocation/Makefile

+CXX_SOURCES := main.cpp
+
+include Makefile.rules

lldb/test/API/commands/expression/memory-allocation/TestMemoryAllocSettings.py

+"""
+Test changing setting for expression memory allocation.
+"""
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestMemoryAllocSettings(TestBase):
+
+    def test(self):
+        """Test changing settings for expression memory allocation."""
+        self.build()
+        target = self.createTestTarget()
+
+        self.log_file = self.getBuildArtifact("log-expr.txt")
+
+        self.runCmd("settings set target.expr-alloc-address 0xdead0000")
+        self.runCmd("settings set target.expr-alloc-size 10000")
+        self.runCmd("settings set target.expr-alloc-align 0x1000")
+
+        self.runCmd("log enable lldb expr -f " + self.log_file)
+        self.runCmd("expression -- int foo; &foo")
+
+        self.assertTrue(os.path.isfile(self.log_file))
+        with open(self.log_file, 'r') as f:
+            log = f.read()
+
+        alloc0 = re.search('^.*IRMemoryMap::Malloc.+?0xdead0000.*$', log, re.MULTILINE)
+        # Malloc adds additional bytes to allocation size, hence 10007
+        alloc1 = re.search('^.*IRMemoryMap::Malloc\s*?\(10007.+?0xdead1000.*$', log, re.MULTILINE)
+        self.assertTrue(alloc0, "Couldn't find an allocation at a given address.")
+        self.assertTrue(alloc1, "Couldn't find an allocation of a given size at a given address.")
+

lldb/test/API/commands/expression/memory-allocation/main.cpp

+int main() {
+  return 0;
+}