In bug 987069, Waldo requested a number of fixes to debugging prints to use the SizePrintfMacros.h defines rather than either %d with a cast or the unportable %z. I'm splitting this into a separate bug to preserve our sanity.

This fixes all the line-number-printing code in js/. It adds "%z" and "%I" support to jsprf.cpp. This ensures that the PRI*SIZE macros work universally. It changes all prints referring directly to ->lineno() to use the appropriate macro from SizePrintfMacros.h. As a bonus it also changes the existing uses of %z in js/ to use the macros.

Now I know about check_spider_monkey_style.py.

Comment on attachment 8561556 [details] [diff] [review] use SizePrintfMacros.h when printing line numbers Review of attachment 8561556 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/BaselineBailouts.cpp @@ +949,2 @@ > resumeAfter ? "after" : "at", (int) pcOff, js_CodeName[op], > + PCToLineNumber(script, pc), script->filename(), script->lineno()); Does PCToLineNumber really not return size_t? @@ +1107,4 @@ > BailoutKindString(bailoutKind), > resumeAfter ? "after" : "at", > js_CodeName[op], > int(PCToLineNumber(script, pc)), Probably remove int(), use PRIuSIZE? Or the right corresponding thing. ::: js/src/jit/BaselineIC.cpp @@ +63,3 @@ > script->filename(), > script->lineno(), > (int) script->pcToOffset(pc), Don't cast to int, use PRIuSIZE for this. Same for numOptimizedStubs below. @@ +63,5 @@ > script->filename(), > script->lineno(), > (int) script->pcToOffset(pc), > PCToLineNumber(script, pc), > script->getWarmUpCount(), getWarmupCount is uint32_t, pass PRIu32. @@ +88,3 @@ > script->filename(), > script->lineno(), > (int) script->pcToOffset(pc), Kill (int) here, pass PRIuSIZE, and for numOptimizedStubs. @@ +88,5 @@ > script->filename(), > script->lineno(), > (int) script->pcToOffset(pc), > PCToLineNumber(script, pc), > script->getWarmUpCount(), getWarmupCount is uint32_t, pass PRIu32. @@ +944,1 @@ > script->filename(), script->lineno(), (int) script->getWarmUpCount(), (void *) pc); getWarmupCount is uint32_t, pass PRIu32. @@ +3756,5 @@ > "(obj=%p, shape=%p, holder=%p, holderShape=%p)", > (obj == holder) ? "direct" : "prototype", > needsAtomize ? " atomizing" : "", > getter->nonLazyScript()->filename(), getter->nonLazyScript()->lineno(), > obj.get(), obj->lastProperty(), holder.get(), holder->lastProperty()); Head spinning yet? :-( ::: js/src/jit/CodeGenerator.cpp @@ +3836,5 @@ > > #ifdef DEBUG > const char *filename = nullptr; > + size_t lineNumber = 0; > + unsigned columnNumber = 0; In this just below here: JitSpew(JitSpew_Codegen, "# block%lu %s:%u:%u%s:", i, filename ? filename : "?", lineNumber, columnNumber, current->mir()->isLoopHeader() ? " (loop header)" : ""); %lu should be "%" PRIuSIZE, %u should be PRIuSIZE now. And bump |i| to the next line, it's horribly misleading there. ::: js/src/jit/IonCaches.cpp @@ +419,1 @@ > this, script_->filename(), script_->lineno(), script_->pcToOffset(pc_), I think you want "/%" PRIuSIZE here, as pcToOffset returns size_t. ::: js/src/jit/JitFrames.cpp @@ +2713,5 @@ > MOZ_ASSERT(location.length() == depth); > > JitSpew(JitSpew_Profiling, "Found bytecode location of depth %d:", depth); > for (size_t i = 0; i < location.length(); i++) { > + JitSpew(JitSpew_Profiling, " %s:%d - %" PRIuSIZE, Shouldn't this be " %s:%" PRIuSIZE " - %d" as the lineno is the second variadic argument, and the last argument is int which matches %d exactly? Tho, probably they should both be PRIuSIZE, and the current (int) cast should be a (size_t) cast. @@ +2730,4 @@ > (int)idx, > inlineFrames.script()->filename(), > inlineFrames.script()->lineno(), > inlineFrames.pc() - inlineFrames.script()->code(), This should probably should be cast to size_t and another PRIuSIZE inserted, for consistency with above. Also the %d(%d) at the end should be PRIuSIZEs with a size_t cast on the last argument. ::: js/src/jit/JitcodeMap.cpp @@ +879,2 @@ > scriptList[0]->filename(), scriptList[0]->lineno(), > int(end - start)); It'd be nice to convert the int(...) to a mozilla::PointerRangeSize(start, end) and use PRIuSIZE there too. @@ +881,5 @@ > > JitSpew(JitSpew_Profiling, " ScriptList of size %d", int(scriptListSize)); > for (uint32_t i = 0; i < scriptListSize; i++) { > + JitSpew(JitSpew_Profiling, " Script %d - %s:%" PRIuSIZE, > + int(i), scriptList[i]->filename(), scriptList[i]->lineno()); Don't pas int(i), just pass i and pass PRIu32. ::: js/src/jit/PerfSpewer.cpp @@ +233,1 @@ > funcStart, prologueSize, script->filename(), script->lineno(), thisFunctionIndex); funcStart should be PRIxPTR. Same for prologueSize. @@ +246,2 @@ > static_cast<uintptr_t>(cur), > static_cast<uintptr_t>(blockStart - cur), These static_cast<>s are unnecessary. And the corresponding things should be PRIxPTR, excepting the last one really is validly PRIuSIZE. @@ +257,1 @@ > static_cast<uintptr_t>(blockStart), size, Pointless cast, should be PRIxPTR. @@ +261,5 @@ > } > > MOZ_ASSERT(cur <= funcEndInlineCode); > if (cur < funcEndInlineCode) { > + fprintf(PerfFilePtr, "%" PRIxSIZE " %" PRIxSIZE " %s:%" PRIuSIZE ": Func%02d-Epilogue\n", PRIxPTR for the first two PRIxSIZEs here. @@ +274,1 @@ > funcEndInlineCode, funcEnd - funcEndInlineCode, Again same thing. @@ +296,1 @@ > reinterpret_cast<uintptr_t>(code->raw()), And PRIxPTR again for the first, but other two PRIxSIZEs are fine. @@ +315,1 @@ > reinterpret_cast<uintptr_t>(code->raw()), PRIxPTR for the first. @@ +329,5 @@ > > if (!lockPerfMap()) > return; > > + fprintf(PerfFilePtr, "%" PRIxSIZE " %" PRIxSIZE " %s:%d:%d: Function %s\n", base, size, filename, lineno, colIndex, funcName); Mind moving the non-format args to the next line? All blends together right now. And those should be PRIxPTR, and the %d:%d should be %u for both. @@ +403,2 @@ > blockStart, size, > filename, r.lineNumber, r.columnNumber, Aren't r.lineNumber/columnNumber size_t, needing PRIuSIZE, probably? @@ +428,5 @@ > > if (!lockPerfMap()) > return; > > + fprintf(PerfFilePtr, "%" PRIxSIZE " %" PRIxSIZE " AsmJS Entries and Exits (0x%" PRIxSIZE " 0x%" PRIxSIZE ")\n", base, size, base, size); Again move to new line, base should be PRIxPTR for both instances. ::: js/src/jsprf.cpp @@ +477,5 @@ > + if (sizeof(size_t) == sizeof(int64_t)) { > + nas[cn].type = TYPE_INT32; > + } else { > + nas[cn].type = TYPE_INT64; > + } More compact: nas[cn].type = sizeof(size_t) == sizeof(int64_t) ? TYPE_INT64 : TYPE_INT32; Aren't those two values flipped? Don't you want 32 if size_t is 64-bits? @@ +715,5 @@ > + if (sizeof(size_t) == sizeof(int64_t)) { > + type = TYPE_INT32; > + } else { > + type = TYPE_INT64; > + } Again compact.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #5) > ::: js/src/jit/BaselineBailouts.cpp > @@ +949,2 @@ > > resumeAfter ? "after" : "at", (int) pcOff, js_CodeName[op], > > + PCToLineNumber(script, pc), script->filename(), script->lineno()); > > Does PCToLineNumber really not return size_t? Yeah. It is a buglet but I didn't want to tackle it here. https://dxr.mozilla.org/mozilla-central/source/js/src/jsscript.h#2137 > @@ +63,5 @@ > > script->filename(), > > script->lineno(), > > (int) script->pcToOffset(pc), > > PCToLineNumber(script, pc), > > script->getWarmUpCount(), > > getWarmupCount is uint32_t, pass PRIu32. This seems a little dangerous given that jsprf and the system printf disagree on the meanings of some formats. PRIu32 is probably safe but wider types may yield bugs. This is why I only dealt with PRI*SIZE, which is a local invention. Also, note that this was just an audit of the line-number-printing code. I'm sure there are many more printf bugs in js, because nothing checks these, and they are mostly for logging. For these reasons I left out the uint32_t changes. If you really want them, let me know, and I'll go through the review again. > @@ +88,3 @@ > > script->filename(), > > script->lineno(), > > (int) script->pcToOffset(pc), > > Kill (int) here, pass PRIuSIZE, and for numOptimizedStubs. I omitted the numOptimizedStubs change as it is another uint32_t. > ::: js/src/jit/JitFrames.cpp > @@ +2713,5 @@ > > MOZ_ASSERT(location.length() == depth); > > > > JitSpew(JitSpew_Profiling, "Found bytecode location of depth %d:", depth); > > for (size_t i = 0; i < location.length(); i++) { > > + JitSpew(JitSpew_Profiling, " %s:%d - %" PRIuSIZE, > > Shouldn't this be > > " %s:%" PRIuSIZE " - %d" > > as the lineno is the second variadic argument, and the last argument is int > which matches %d exactly? Tho, probably they should both be PRIuSIZE, and > the current (int) cast should be a (size_t) cast. Yeah, I agree. I changed it this way. > @@ +403,2 @@ > > blockStart, size, > > filename, r.lineNumber, r.columnNumber, > > Aren't r.lineNumber/columnNumber size_t, needing PRIuSIZE, probably? 'r' is a struct Record (PerfSpewer.h) which uses "unsigned": struct Record { const char *filename; unsigned lineNumber; unsigned columnNumber; > ::: js/src/jsprf.cpp > @@ +477,5 @@ > > + if (sizeof(size_t) == sizeof(int64_t)) { > > + nas[cn].type = TYPE_INT32; > > + } else { > > + nas[cn].type = TYPE_INT64; > > + } > > More compact: > > nas[cn].type = sizeof(size_t) == sizeof(int64_t) ? TYPE_INT64 : TYPE_INT32; > > Aren't those two values flipped? Don't you want 32 if size_t is 64-bits? Yes, barf.

Rebased. Just one conflict, due to a context change.

Changes from the review.

Comment on attachment 8569232 [details] [diff] [review] review changes Review of attachment 8569232 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/JitFrames.cpp @@ +2751,2 @@ > location[i].script->filename(), location[i].script->lineno(), > + (size_t) (location[i].pc - location[i].script->code())); Make it a size_t(...) C++-style cast, and on similar places.

Rebased and squashed. Carrying over Waldo's r+.

js/src/jit/PerfSpewer.c makes use of PRIxPTR yet this does not seem to be defined in SizePrintfMacros.h. An i686 cross compile is failing here due to this. It appears that %zx would have been appropriate. Could I ask what an appropriate fix would be? Also does the SizePrintfMacros.h header file need to be included in all files using these definitions, for the case of non-unified builds?

(In reply to Douglas Crosher [:dougc] from comment #14) > js/src/jit/PerfSpewer.c makes use of PRIxPTR yet this does not seem to be > defined in SizePrintfMacros.h. An i686 cross compile is failing here due to > this. It appears that %zx would have been appropriate. Could I ask what an > appropriate fix would be? Include IntegerPrintfMacros.h. I'm sorry about the breakage. I've attached a patch which I think should fix it. If it works for you I'll make a new bug and whatnot to get it in. > Also does the SizePrintfMacros.h header file need to be included in all > files using these definitions, for the case of non-unified builds? My understanding is that non-unified builds don't exist any more. If you can tell me how to make one, though, I will fix any problems in this area.

Comment on attachment 8573290 [details] [diff] [review] include IntegerPrintfMacros in PerfSpewer.cpp Review of attachment 8573290 [details] [diff] [review]: ----------------------------------------------------------------- Thank you, this fixes the compilation failure. It does look like there is no way to disable unified compilation now, there was: ac_add_options --disable-unified-compilation

Sorry -- I forgot about this for a bit. See bug 1145149.