Page MenuHomePhabricator

Prevent client from querying each thread's PC at each stop.

Authored by jmajors on Jan 18 2017, 5:10 PM.



The server was no longer sending the thread PCs the way the client
expected them.
I changed the server to send them back as a threadstop info field,
similar to the Apple version of the server.
I also changed the client to look for them there, before querying the
I added a test to ensure the server doesn't stop sending them.

Diff Detail


Event Timeline

jmajors created this revision.Jan 18 2017, 5:10 PM
labath edited edge metadata.Jan 19 2017, 2:43 AM

Thank you for writing the test. I have a couple of cleanup suggestions. Let me know what you think.

Also, for the future, please upload a new version of the patch under the same differential revision as the previous one (so one can compare the changes, keep all the subscribers, etc.). If you're submitting using arcanist, it should amend your git commit to include the "differential revision" blurb. Then subsequent "arc diff"s should update automatically.

86 ↗(On Diff #84912)

This makes the figuring out the return type of the function quite confusing. How about just keeping the "tuple" branch, adjusting callers (there's only 2 of them) to call the function with a tuple (possibly containing only one element)?
Maybe also rename the function to _field*s*, as it can return multiple items.

101 ↗(On Diff #84912)

this would become:

stop_reply_threads_text = self.gather_stop_reply_fields( post_startup_log_lines, thread_count, ("threads"))["threads"]
119 ↗(On Diff #84912)

Shouldn't we just assert here?

298 ↗(On Diff #84912)

This should be @debugserver_test

1774 ↗(On Diff #84912)
auto iter = std::find(m_thread_ids.begin(), m_thread_ids.end(), tid);
if (iter != m_thread_ids.end())
  SetThreadPc(thread_sp, iter - m_thread_ids.begin());
jmajors updated this revision to Diff 85075.Jan 19 2017, 5:40 PM

Addressed feedback.

labath accepted this revision.Jan 20 2017, 5:05 AM

Looks good. Thank you.

I am going to do the array conversion and commit this (if I am not mistaken, you don't have commit access).

98 ↗(On Diff #85075)

Ok, this should probably be an array instead of a tuple ["threads"]. Then we can avoid the funny-looking comma.

This revision is now accepted and ready to land.Jan 20 2017, 5:05 AM
This revision was automatically updated to reflect the committed changes.