clayborg requested changes to this revision.
clayborg added inline comments.
This revision now requires changes to proceed.

================
Comment at: source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:651-659
+          ArchSpec header_arch;
+          GetArchitecture(header_arch);
+          if (header_arch.GetMachine() == llvm::Triple::arm) {
+            if (function_rva & 1) {
+              // For Thumb we need the last bit to be 0 so that the address
+              // points to the right beginning of the symbol.
+              function_rva ^= 1;
----------------
Shouldn't this happen for all thumb symbols? Other object file parsers define 
something like:

```
#define THUMB_ADDRESS_BIT_MASK 0xfffffffffffffffeull
```
And do things like:

```
function_rva &=THUMB_ADDRESS_BIT_MASK;
```

Makes it a bit cleaner to read.

It would be better to create a  "bool is_arm = ...;" similar to other symbol 
file plug-ins. Determine that once before all symbols are being parsed. Here it 
is being done for each symbol in the loop. 

Be aware that you must answer the address class correctly later with:

```
AddressClass ObjectFilePECOFF::GetAddressClass(lldb::addr_t file_addr);
```

It must return "eAddressClassCode" for ARM address ranges, and 
"eAddressClassCodeAlternateISA" for Thumb address ranges. Check how 
"ObjectFileMachO::GetAddressClass()" or "ObjectFileELF::GetAddressClass()" does 
this. For mach-o we set a bit in each symbol's flags... I will comment below 
where you could do that. The address class map is used to set breakpoints 
correctly in Thumb or ARM code and must be done accurately.


https://reviews.llvm.org/D39315



_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to