Page MenuHomePhabricator

Correct the start address for exported windows functions in arm
Needs ReviewPublic

Authored by sas on Oct 25 2017, 6:23 PM.

Details

Summary

This assumes that the environment is always Thumb

Change by Walter Erquinigo <wallace@fb.com>

Event Timeline

sas created this revision.Oct 25 2017, 6:23 PM
clayborg requested changes to this revision.Oct 26 2017, 9:54 AM
clayborg added inline comments.
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.

This revision now requires changes to proceed.Oct 26 2017, 9:54 AM
sas requested review of this revision.Oct 26 2017, 10:04 AM
sas edited edge metadata.

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 requested changes to this revision.Oct 26 2017, 10:09 AM

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.

This revision now requires changes to proceed.Oct 26 2017, 10:09 AM
davide requested changes to this revision.Oct 26 2017, 10:20 AM
davide added a subscriber: davide.

This change should be unit-testable, no?

sas updated this revision to Diff 120447.Oct 26 2017, 10:25 AM
sas edited edge metadata.

Address comments by @clayborg.

sas added a comment.Oct 26 2017, 10:50 AM

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

clayborg accepted this revision.Oct 26 2017, 10:51 AM
sas added a comment.Oct 26 2017, 11:09 AM

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 edited edge metadata.Oct 26 2017, 11:12 AM
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 :-/

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 added a comment.Oct 26 2017, 11:23 AM
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.

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?

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 added a subscriber: fjricci.Oct 26 2017, 1:20 PM

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 added a comment.Nov 6 2017, 4:20 PM

Friendly ping.