Closed Bug 1314614 Opened 5 years ago Closed 5 years ago

Don't use size_t for result of getpid()/GetCurrentProcessId()

Categories

(Core :: JavaScript Engine: JIT, defect, P3)

49 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: jesup, Unassigned)

Details

Attachments

(1 file, 1 obsolete file)

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.
Forwarding to Nicolas, since he introduced that. Seems like an easy fix.
Flags: needinfo?(nicolas.b.pierron)
Priority: -- → P3
(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 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+
(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)
Attachment #8808234 - Attachment is obsolete: true
Pushed by npierron@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/72eddba8641a
Replace GetCurrentProcessId calls by getpid. r=h4writer
https://hg.mozilla.org/mozilla-central/rev/72eddba8641a
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.