Closed
Bug 1314614
Opened 8 years ago
Closed 8 years ago
Don't use size_t for result of getpid()/GetCurrentProcessId()
Categories
(Core :: JavaScript Engine: JIT, defect, P3)
Tracking
()
RESOLVED
FIXED
mozilla53
Tracking | Status | |
---|---|---|
firefox53 | --- | fixed |
People
(Reporter: jesup, Unassigned)
Details
Attachments
(1 file, 1 obsolete file)
5.03 KB,
patch
|
h4writer
:
review+
|
Details | Diff | Splinter Review |
JitSpewer.cpp uses size_t for the result of getpid() (and GetCurrentProcessId()), which is wrong for both.
GetCurrentProcessId() returns DWORD.
getpid() returns pid_t (posix), or int (Windows). Try switching to getpid() and use pid_t -- likely on windows it's set to int. Also, to remove a deprecation warning on windows (which appears silly/wrong, but it's windows) you may need/want to use _getpid() on windows. But a lot of other things use getpid(). Note that _getpid()/getpid() on Windows just calls GetCurrentProcessId(), so you don't need conditional code here.
Comment 1•8 years ago
|
||
Forwarding to Nicolas, since he introduced that. Seems like an easy fix.
Flags: needinfo?(nicolas.b.pierron)
Priority: -- → P3
Comment 2•8 years ago
|
||
Comment 3•8 years ago
|
||
(In reply to Randell Jesup [:jesup] from comment #0)
> Try switching to getpid()
> and use pid_t -- likely on windows it's set to int.
This is apparently not defined, or I am missing the right header. So far I got the following error messages:
missing type specifier - int assumed. Note: C++ does not support default-int
So I can replace all the size_t by uint32_t, as done in the TraceLoggerGraph, but I do not see real value in it.
Comment 4•8 years ago
|
||
Attachment #8808737 -
Flags: review?(hv1989)
Comment 5•8 years ago
|
||
Comment on attachment 8808737 [details] [diff] [review]
Replace GetCurrentProcessId calls by getpid.
Review of attachment 8808737 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/jit/JitSpewer.cpp
@@ +172,5 @@
> const char *jsonFilename = JIT_SPEW_DIR "/ion.json";
>
> const char* usePid = getenv("ION_SPEW_BY_PID");
> if (usePid && *usePid != 0) {
> + uint32_t pid = getpid();
From what I read, pid is "32bit signed". Please use int32 here and below
@@ +177,2 @@
> size_t len;
> + len = snprintf(jsonBuffer, bufferLength, JIT_SPEW_DIR "/ion%" PRIu32 ".json", pid);
s/PRIu32/PRId32/ and below
Attachment #8808737 -
Flags: review?(hv1989) → review+
Comment 6•8 years ago
|
||
(In reply to Hannes Verschore [:h4writer] from comment #5)
> ::: js/src/jit/JitSpewer.cpp
> @@ +172,5 @@
> > const char *jsonFilename = JIT_SPEW_DIR "/ion.json";
> >
> > const char* usePid = getenv("ION_SPEW_BY_PID");
> > if (usePid && *usePid != 0) {
> > + uint32_t pid = getpid();
>
> From what I read, pid is "32bit signed". Please use int32 here and below
I am totally dubious about this fact, as I never heard of such beast, and this cannot be an error code as it getpid does not fail. [1][2]
As this is only for logging purpose this coercion should not matter, and this would be consistent with what is done in the TraceLogger and push the current patch as-is.
My guess would be that the negative part of pid_t is used to return the error notification from the "fork" function, which thus does not matter for getpid.
[1] https://www.gnu.org/software/libc/manual/html_node/POSIX-Safety-Concepts.html#POSIX-Safety-Concepts
[2] https://msdn.microsoft.com/en-us/library/t2y34y40.aspx
Flags: needinfo?(nicolas.b.pierron)
Updated•8 years ago
|
Attachment #8808234 -
Attachment is obsolete: true
Comment 7•8 years ago
|
||
Pushed by npierron@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/72eddba8641a
Replace GetCurrentProcessId calls by getpid. r=h4writer
Comment 9•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in
before you can comment on or make changes to this bug.
Description
•