Closed Bug 1130166 Opened 9 years ago Closed 9 years ago

sanitize printing of line numbers

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla39
Tracking Flags:
Tracking Status
firefox39 --- fixed

People

(Reporter: tromey, Assigned: tromey)

References

Details

Attachments

(2 files, 4 obsolete files)

use SizePrintfMacros.h when printing line numbers
63.49 KB, patch
tromey
: review+
Details | Diff | Splinter Review
include IntegerPrintfMacros in PerfSpewer.cpp
861 bytes, patch
dougc
: review+
Details | Diff | Splinter Review 
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.
Attachment #8560092 - Attachment is obsolete: true
Attachment #8561556 - Flags: review?(jwalden+bmo)
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.
Attachment #8561556 - Flags: review?(jwalden+bmo) → review+
(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.
Attached patch review changes (obsolete) — — Splinter Review 
Changes from the review.
Attachment #8569232 - Flags: review?(jwalden+bmo)
Attachment #8561556 - Attachment is obsolete: true
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.
Attachment #8569232 - Flags: review?(jwalden+bmo) → review+
Rebased and squashed.  Carrying over Waldo's r+.
Attachment #8569231 - Attachment is obsolete: true
Attachment #8569232 - Attachment is obsolete: true
Attachment #8570527 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/63364da8765f
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
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?
Flags: needinfo?(ttromey)
(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.
Flags: needinfo?(ttromey)
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
Attachment #8573290 - Flags: review+
Sorry -- I forgot about this for a bit.  See bug 1145149.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: