[Lldb-commits] [PATCH] D146965: [lldb] Add support for MSP430 in LLDB.

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

I'm not too familiar with MSP430 but the general idea looks fine. Others may have comments about areas I'm less familiar with.

One concern I have is that there are no tests. I can understand that it may be difficult to get automated tests running testing the debugging of an architecture like MSP430 but there are things we can test to sanity test MSP430 support... At the very least, adding a unit test for the new ArchSpec support would be good -- `llvm-project/lldb/unittests/Utility/ArchSpecTest.cpp`.



================
Comment at: lldb/include/lldb/Utility/DataExtractor.h:846-848
 #ifdef LLDB_CONFIGURATION_DEBUG
-    assert(addr_size == 4 || addr_size == 8);
+    assert(addr_size == 2 || addr_size == 4 || addr_size == 8);
 #endif
----------------
I know this is not your code but maybe we should deconditionalize this assertion?


================
Comment at: lldb/source/Expression/IRMemoryMap.cpp:100-101
+    uint64_t end_of_memory;
+    uint32_t address_byte_size = process_sp->GetAddressByteSize();
+    switch (address_byte_size) {
+    case 2:
----------------
switch on the expression directly instead of creating a local?


================
Comment at: lldb/source/Expression/IRMemoryMap.cpp:102-110
+    case 2:
+      end_of_memory = 0xffffull;
+      break;
+    case 4:
+      end_of_memory = 0xffffffffull;
+      break;
+    default:
----------------
Instead of making 8 the default, maybe we can assert on `default` or something? This can prevent future bugs where perhaps address_byte_size is not 8 for some reason and we do the wrong thing.


================
Comment at: lldb/source/Expression/IRMemoryMap.cpp:113-114
 
     lldbassert(process_sp->GetAddressByteSize() == 4 ||
                end_of_memory != 0xffffffffull);
 
----------------
I think with the change above we should probably make this assertion more useful.


================
Comment at: lldb/source/Expression/IRMemoryMap.cpp:160-161
         break;
       default:
+        ret = 0xdead0fff00000000ull;
         break;
----------------
Same here, let's not assume 8 if we hit default.


================
Comment at: lldb/source/Expression/IRMemoryMap.cpp:169-170
     size_t alloc_size = back->second.m_size;
-    ret = llvm::alignTo(addr + alloc_size, 4096);
+    auto align = GetAddressByteSize() == 2 ? 512 : 4096;
+    ret = llvm::alignTo(addr + alloc_size, align);
   }
----------------
Is this true for all machines where the address byte size is 2? Seems like 512 and 4096 should be based on something other than the address byte size. Platform/ABI perhaps?


================
Comment at: lldb/source/Expression/LLVMUserExpression.cpp:336-338
+      const size_t stack_frame_size =
+          target->GetArchitecture().GetAddressByteSize() == 2 ? 512
+                                                              : 512 * 1024;
----------------
Same here, this seems dependent on Platform/ABI.


================
Comment at: lldb/source/Plugins/ABI/MSP430/ABISysV_msp430.h:13-16
+// C Includes
+// C++ Includes
+// Other libraries and framework includes
+// Project includes
----------------
You can remove these comments, I don't think we track that kind of thing anymore... I think?


================
Comment at: lldb/source/Plugins/ABI/MSP430/ABISysV_msp430.h:51-54
+    if (cfa & 0x01)
+      return false; // Not 2 byte aligned
+    if (cfa == 0)
+      return false; // Zero is not a valid stack address
----------------
nit: you can merge these


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146965