1295740 - Stop using PR_GetCurrentThread() in testThreadingExclusiveData.cpp

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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.

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Print js:ThisThread::GetId().platformData() instead of PR_GetCurrentThread() in testThreadingExclusiveData.cpp

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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 3

[Terrence Cole [:terrence]]

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.

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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.

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Print the test thread's bit instead of PR_GetCurrentThread() in testThreadingExclusiveData.cpp

Comment 6

[Pulsebot]

Pushed by [e-mail]:
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

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/82b764f1a1b1