This is an archive of the discontinued LLVM Phabricator instance.

[MIPS] Use qfThreadID if qC packet is not supported by target
ClosedPublic

Authored by jaydeep on Jul 26 2015, 11:12 PM.

Details

Reviewers
clayborg
jingham
Summary

The patch uses qfThreadID to get the thread IDs if qC packet is not supported by target.

Diff Detail

Repository
rL LLVM

Event Timeline

jaydeep updated this revision to Diff 30677.Jul 26 2015, 11:12 PM
jaydeep retitled this revision from to [MIPS] Use qfThreadID if qC packet is not supported by target.
jaydeep updated this object.
jaydeep added a reviewer: clayborg.
jaydeep set the repository for this revision to rL LLVM.
jaydeep added subscribers: lldb-commits, bhushan, slthakur and 2 others.
clayborg requested changes to this revision.Jul 27 2015, 11:15 AM
clayborg edited edge metadata.

Any other OSs other than Linux have process IDs being the same as thread IDs? The code added should be properly made conditional based on linux and any other OSs where this holds true.

source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
1462–1476

The threads ID is not the same as a process ID on any Apple based OS. We would need make this check conditional based on the OSs that have thread IDs being the same as process IDs. Not sure if that is only linux?

This revision now requires changes to proceed.Jul 27 2015, 11:15 AM
jaydeep added inline comments.Jul 28 2015, 8:51 PM
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
1462–1476

We are connected to a bare-iron target (IASim in this case). We need to check whether the target OS is Unknown.

That is fine for unknown or for any known OSs where the pid == tid for the main thread.

jaydeep updated this revision to Diff 31116.Jul 31 2015, 4:31 AM
jaydeep added a reviewer: jingham.

Addressed review comments.

jingham edited edge metadata.Jul 31 2015, 9:12 AM

That seems good, except take out the use of auto in the first diff. We try not to use "auto" for trivially nameable types like lldb::pid_t. When you are declaring a variable where you would have to scratch your head more than twice to figure out how to write the type of the thing, then auto is a real help. But for simple types it doesn't offer any benefit and makes reading the code a little bit harder...

jaydeep updated this revision to Diff 31213.Aug 2 2015, 8:23 PM
jaydeep edited edge metadata.

Address review comments

Could you please find some time to review this?
Thanks

clayborg requested changes to this revision.Aug 10 2015, 9:36 AM
clayborg edited edge metadata.

I don't see how passing the target architecture's OS helps here since you aren't checking it for linux or any of the other OS's where pid == tid. This arch is likely to be unset during many of these calls which makes the test just assume any time the OS is unknown it will just assume pid == tid.

source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
1463–1465

How does checking the "ostype" with unknown help us to determine if this is an OS where pid == tid? What if the user makes their target with:

(lldb) target create --arch x86_64-pc-linux ...

Then this code doesn't trigger?

This revision now requires changes to proceed.Aug 10 2015, 9:36 AM
jaydeep added inline comments.Aug 10 2015, 11:20 PM
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
1463–1465

In this case the target we are connected to is a bare-iron which does not support qC packet. Here pid == tid and only way to get this is using qfThreadID. If arch is likely to be unset during many of these calls then we need to find alternate way to implement this.

Lets just not send the OS then since it is very unreliable when the target if first attaching or launching and just always back up to using qfThreadID if first qProcessInfo and then qC are not implemented. We need some way to get the pid, so lets just do it.

source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
1101

Revert this and don't send the OS, see main comment below.

1428

Revert this and don't send the OS, see main comment below.

1464

remove the ostype check, see main comment below.

source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
112 ↗(On Diff #31213)

Revert this and don't send the OS, see main comment below.

source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
761–762 ↗(On Diff #31213)

Revert this and don't send the OS, see main comment below.

1016–1017 ↗(On Diff #31213)

Revert this and don't send the OS, see main comment below.

2323–2324 ↗(On Diff #31213)

Revert this and don't send the OS, see main comment below.

jaydeep updated this revision to Diff 31908.Aug 12 2015, 2:03 AM
jaydeep edited edge metadata.

Addressed review comments.

clayborg accepted this revision.Aug 12 2015, 10:01 AM
clayborg edited edge metadata.

Looks good.

This revision is now accepted and ready to land.Aug 12 2015, 10:01 AM
jaydeep closed this revision.Aug 13 2015, 10:07 PM

Closed by commit rL244866