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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
