Correct the start address for exported windows functions in arm

[sas]

Summary

This assumes that the environment is always Thumb

Change by Walter Erquinigo <[e-mail]>

[clayborg]

source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp

656–664

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.

[sas]

I saw the bool is_arm = pattern only in ObjectFileMachO.cpp. I'm not sure about the specifics for Darwin targets, but having an object file with some functions in ARM and some functions in Thumb is valid, so I think checking for each symbol is the right thing to do.

As for the GetAddressClass function, I have another diff that does exactly what you are talking about. I'll upload it when this one goes in.

[clayborg]

ok, just move the ARM detection out of the loop and use a THUMB_ADDRESS_BIT_MASK for ARM symbols and this is good to go.

[davide]

This change should be unit-testable, no?

[sas]

Address comments by @clayborg.

[sas]

@clayborg: the follow-up to this change is in D39337, where we implement GetAddressClass().

[sas]

Same thing here, I have no idea how to go about testing something specific like this. Given that this is Windows Phone, it's even worse than simply Windows Desktop because the only way to test is by doing remote debugging. I suppose checking in binaries to the tree so we can analyze the symbols and ensure correctness is the only way to do it?

[zturner]

In D39315#908246, @sas wrote:

Same thing here, I have no idea how to go about testing something specific like this. Given that this is Windows Phone, it's even worse than simply Windows Desktop because the only way to test is by doing remote debugging. I suppose checking in binaries to the tree so we can analyze the symbols and ensure correctness is the only way to do it?

Do you guys not have access to Windows Desktops? It sounds like we need to get remote debugging of Windows targets working. I don't think I'm gonna be the one to do that in the near future, but if it's going to be a frequent thing that you're improving windows support for remote targets like Windows Phone, I imagine you're going to want the code to be tested, so hopefully you can convince someone to give you cycles to make remote debugging work properly, otherwise you're shipping untested code :-/

[davide]

In D39315#908246, @sas wrote:

Same thing here, I have no idea how to go about testing something specific like this. Given that this is Windows Phone, it's even worse than simply Windows Desktop because the only way to test is by doing remote debugging. I suppose checking in binaries to the tree so we can analyze the symbols and ensure correctness is the only way to do it?

@zturner is probably the most expert in this area.

[sas]

In D39315#908251, @zturner wrote:

In D39315#908246, @sas wrote:

Same thing here, I have no idea how to go about testing something specific like this. Given that this is Windows Phone, it's even worse than simply Windows Desktop because the only way to test is by doing remote debugging. I suppose checking in binaries to the tree so we can analyze the symbols and ensure correctness is the only way to do it?

Do you guys not have access to Windows Desktops? It sounds like we need to get remote debugging of Windows targets working. I don't think I'm gonna be the one to do that in the near future, but if it's going to be a frequent thing that you're improving windows support for remote targets like Windows Phone, I imagine you're going to want the code to be tested, so hopefully you can convince someone to give you cycles to make remote debugging work properly, otherwise you're shipping untested code :-/

Well as far as I can tell, remote debugging for Windows does work, assuming you use ds2 on the remote, and that you have a few of our local patches (which I'm trying to send upstream currently). Remote Windows debugging is the only way we debug on Windows for now and the testing we have is just a combination of people using our tools internally as well as some end-to-end tests we run internally with full debug session setup + breakpoints + execution control, etc, with a remote session.

Given that this works for us, I'm gonna have a hard time justifying assigning people to bringing up windows remote debugging in lldb without ds2, and that leaves me in a state where testing stuff in lldb is hard/impossible for now.

[zturner]

So when you're using ds2 as the remote, are you still using the normal lldb test suite? dotest.py? If so, then we could have a test decorator that says @expectedFailure(not(debugserver=ds2)) or something. Then, even though you're the only one that can run it, at least YOU are sure it works. Some coverage is better than nothing. Is something like this possible?

[clayborg]

The test suite can be run remotely if ds2 supports the "lldb-server platform" packets. I'll be happy to help you get this going Stephane. Ping me internally if interested.

[sas]

In D39315#908284, @zturner wrote:

So when you're using ds2 as the remote, are you still using the normal lldb test suite? dotest.py? If so, then we could have a test decorator that says @expectedFailure(not(debugserver=ds2)) or something. Then, even though you're the only one that can run it, at least YOU are sure it works. Some coverage is better than nothing. Is something like this possible?

So there are two ways we test our stuff.

First part is centered around ds2 testing, and you can see it here: https://travis-ci.org/facebook/ds2/builds/292216534. We build ds2 for a bunch of different targets, and for some of them, we run dotest.py with LLDB_DEBUGSERVER_PATH=/path/to/ds2. This gives us a lot of coverage for non-Windows targets.

For Windows targets, we can only build ds2 (see https://ci.appveyor.com/project/a20012251/ds2/branch/master) and we are unable to run tests because we need a binary of lldb for Windows, but the builds available on http://releases.llvm.org/ were historically broken, and when @fjricci tried to fix them it didn't lead anywhere IIRC.

The second way we have to test our debugging tools is to simply run our debugging scripts with a known target, internally. That doesn't use dotest.py. It simply launches lldb and ds2, tries to set breakpoints, control the execution of the process, etc. The changes I am currently sending upstream work with that type of internal testing, but that's of no use for you because you can't run it yourself, nor can you see the results of our runs. :/

[sas]

Friendly ping.

source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp

 
 #include "llvm/Support/MemoryBuffer.h"
 
 #define IMAGE_DOS_SIGNATURE 0x5A4D    // MZ
 #define IMAGE_NT_SIGNATURE 0x00004550 // PE00
 #define OPT_HEADER_MAGIC_PE32 0x010b
 #define OPT_HEADER_MAGIC_PE32_PLUS 0x020b
 
+#define THUMB_ADDRESS_BIT_MASK 0xfffffffffffffffeull
 using namespace lldb;
 using namespace lldb_private;
 
 void ObjectFilePECOFF::Initialize() {
   PluginManager::RegisterPlugin(
       GetPluginNameStatic(), GetPluginDescriptionStatic(), CreateInstance,
       CreateMemoryInstance, GetModuleSpecifications, SaveCore);
 }
if (m_symtab_ap.get() == NULL) {
         lldb::offset_t name_offset = export_table.address_of_names - data_start;
         lldb::offset_t name_ordinal_offset =
             export_table.address_of_name_ordinals - data_start;
 
         Symbol *symbols = m_symtab_ap->Resize(export_table.number_of_names);
 
         std::string symbol_name;
 
+        ArchSpec header_arch;
+        GetArchitecture(header_arch);
+        bool is_arm = header_arch.GetMachine() == llvm::Triple::arm;
+
         // Read each export table entry
         for (size_t i = 0; i < export_table.number_of_names; ++i) {
           uint32_t name_ordinal =
               has_ordinal ? symtab_data.GetU16(&name_ordinal_offset) : i;
           uint32_t name_address = symtab_data.GetU32(&name_offset);
 
           const char *symbol_name_cstr =
               symtab_data.PeekCStr(name_address - data_start);
           symbol_name.assign(symbol_name_cstr);
 
           lldb::offset_t function_offset = export_table.address_of_functions -
                                            data_start +
                                            sizeof(uint32_t) * name_ordinal;
           uint32_t function_rva = symtab_data.GetU32(&function_offset);
 
+          if (is_arm && 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 &= THUMB_ADDRESS_BIT_MASK;
+          }
+
           Address symbol_addr(m_coff_header_opt.image_base + function_rva,
                               sect_list);
           symbols[i].GetMangled().SetValue(ConstString(symbol_name.c_str()));
           symbols[i].GetAddressRef() = symbol_addr;
           symbols[i].SetType(lldb::eSymbolTypeCode);
           symbols[i].SetDebug(true);
         }
       }
       m_symtab_ap->CalculateSymbolSizes();
     }
   }