[Lldb-commits] [PATCH] D68069: Explicitly set entry point arch when it's thumb

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Thanks for adding the lit test.



================
Comment at: lldb/lit/SymbolFile/dissassemble-entry-point.s:6
+# RUN: %lldb -x -b \
+# RUN:  -o 'settings set disassembly-format "{ <${function.concrete-only-addr-offset-no-padding}>}: "' \
+# RUN:  -o 'dis -s 0x8074 -e 0x8080' -- %t | FileCheck %s
----------------
Can you delete this? I'm pretty sure the nested quoting is going to cause problems when running the test on windows... If you need it, you can use regexes `{{.*}}` to match the filename portion in the check lines below.


================
Comment at: lldb/lit/SymbolFile/dissassemble-entry-point.s:12
+
+.global _Start
+.thumb_func
----------------
I don't think this has any effect, as it differs case from the symbol below, so you should be able to just delete it.


================
Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:2719
+    auto entry_point_addr = GetEntryPointAddress().GetFileAddress();
+    if (entry_point_addr != LLDB_INVALID_ADDRESS && (entry_point_addr & 1)) {
+      auto opcode_addr = entry_point_addr ^ 1;
----------------
You've removed the architecture check, but this check here means that the code only kicks in for entry points that happen to be at an odd address. That is definitely not right. If we're going to go through with making this stuff unconditional (and I still think we should) then this condition needs to go too. The only thing that needs to check for arm/thumb is the ` m_address_class_map[opcode_addr] = AddressClass::eCodeAlternateISA;` line down below.

Also, the long comment above needs to be redone to reflect the new reality. I'd just give a short comment that we're creating the entry point symbol if there isn't one, and that getting the address class right is important for expression evaluation on arm.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68069