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

RESOLVED FIXED in Firefox 53

Status

()

P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jesup, Unassigned)

Tracking

49 Branch
mozilla53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 years ago
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.
Created attachment 8808737 [details] [diff] [review]
Replace GetCurrentProcessId calls by getpid.
Attachment #8808737 - Flags: review?(hv1989)
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

Comment 8

2 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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/72eddba8641a
Status: NEW → RESOLVED
Last Resolved: 2 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.