Stop using PR_GetCurrentThread() in testThreadingExclusiveData.cpp

RESOLVED FIXED in Firefox 51

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: fitzgen, Assigned: fitzgen)

Tracking

unspecified
mozilla51
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

This also uncovered the fact that `platformData()` is marked inline, but defined
in a cpp file. I moved it to the header and shuffled around the static
assertions that needed to remain in the cpp files where `PlatformData` is fully
defined.
Created attachment 8781698 [details] [diff] [review]
Print js:ThisThread::GetId().platformData() instead of PR_GetCurrentThread() in testThreadingExclusiveData.cpp
Attachment #8781698 - Flags: review?(terrence)
Assignee: nobody → nfitzgerald
Blocks: 956899
Status: NEW → ASSIGNED
In case it isn't obvious: I'm printing js::ThisThread::GetId().platformData() because it can be formatted as a pointer, where as js::ThisThread::Id can't. Not that it really matters that much...
Comment on attachment 8781698 [details] [diff] [review]
Print js:ThisThread::GetId().platformData() instead of PR_GetCurrentThread() in testThreadingExclusiveData.cpp

Review of attachment 8781698 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/threading/Thread.h
@@ -56,5 @@
>      bool operator==(const Id& aOther) const;
>      bool operator!=(const Id& aOther) const { return !operator==(aOther); }
>  
> -    inline PlatformData* platformData();
> -    inline const PlatformData* platformData() const;

The intent of declaring these as inline with no definition is that it will only be possible to call them without a link error from the Translation Unit where we declare the bodys: in this case Thread.cpp. Ideally it would be a static method in Thread.cpp and not exposed at all, but it does need access to the platformData_, so that's not an option.

I think instead of exposing this we should add a |void printDiagnosticInfo(FILE* fp)| to class Thread. Then we can have a definition for both posix and win32 and print all the relevant information for each platform. On windows this would be the handle and thread id. On posix this data is opaque so maybe we could get the thread name and the platform data address? In any case, just printing &platformData_ seems pretty suboptimal as the values there can change.
Attachment #8781698 - Flags: review?(terrence)
Ok, let's back up a bit: this code is working just fine, and we shouldn't need to jump through hoops to print a little diagnostic info. We can get the same level of debug logging by printing "Thread N" where N is the bit this thread was assigned to set in the test. This way, we don't need to modify the js::Thread stuff at all.
Summary: Print js:ThisThread::GetId().platformData() instead of PR_GetCurrentThread() in testThreadingExclusiveData.cpp → Stop using PR_GetCurrentThread() in testThreadingExclusiveData.cpp
Created attachment 8783028 [details] [diff] [review]
Print the test thread's bit instead of PR_GetCurrentThread() in testThreadingExclusiveData.cpp
Attachment #8783028 - Flags: review?(terrence)
Attachment #8781698 - Attachment is obsolete: true
Attachment #8783028 - Flags: review?(terrence) → review+

Comment 6

2 years ago
Pushed by nfitzgerald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/82b764f1a1b1
Print the test thread's bit instead of PR_GetCurrentThread() in testThreadingExclusiveData.cpp; r=terrence

Comment 7

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/82b764f1a1b1
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.