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)

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
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: