[Lldb-commits] [PATCH] D35784: [LLD][MIPS] Fix Address::GetAddressClass() to return correct AddressClass based on the load address

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

Two issues we need to resolve:

- I don't think AddressClass should have Absolute as a value. Absolute values in symbol tables are just numbers, not necessarily addresses.
- This change won't work for ARM



================
Comment at: include/lldb/lldb-enumerations.h:829
+  eAddressClassRuntime,
+  eAddressClassTypeAbsoluteAddress
 };
----------------
I would suggest removing "Address" from the end of the enum name. It is already in an enum that starts with "eAddressClass". I also question why any address class should be absolute. Absolute symbols are usually not addresses, but just values.


================
Comment at: source/Core/Address.cpp:993-1003
+  // Get address class based on loaded Section type
+  SectionSP section_sp(GetSection());
+  if (section_sp) {
+    const SectionType section_type = section_sp->GetType();
+    AddressClass addr_class;
+    addr_class = ObjectFile::SectionTypeToAddressClass(section_type);
+    if (addr_class != eAddressClassTypeAbsoluteAddress)
----------------
This won't work correctly for ARM binaries. ".text" can be filled with ARM, Thumb and Data and there is a CPU map that can help unwind this. The above code will just ay "eAddressClassCode" for all ".text". So this won't work.

My guess is the right fix here is to check if the address has a valid section before calling the code below and removing all code above.

```
if (!GetSection())
  return eAddressClassInvalid;
```

GetFileAddress will just return the m_offset if the section isn't valid. One could argue that Address::GetFileAddress() should only return the file address if the section is valid though, perhaps that should be the change we make here. 


https://reviews.llvm.org/D35784