Bug 210037 – JSC shell shouldn't treat NUL as a terminator when printing a JS string

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Ross Kirsling]

JSC shell shouldn't treat NUL as a terminator when printing a JS string

[Ross Kirsling]

Created attachment 395525 [details]
Patch

[Ross Kirsling]

Hmm, I suppose this would be the right opportunity to remove $vm.print too -- it's hardly used, and it doesn't do what it says.

[Mark Lam]

(In reply to Ross Kirsling from comment #2)
> Hmm, I suppose this would be the right opportunity to remove $vm.print too
> -- it's hardly used, and it doesn't do what it says.

Do not remove $vm.print().  Its uses go beyond what it in the existing test scripts.  I use it for custom debugging tests all the time.  It comes in especially handy when I want to print from WebCore and don't want to go thru console.log().

[Ross Kirsling]

(In reply to Mark Lam from comment #3)
> (In reply to Ross Kirsling from comment #2)
> > Hmm, I suppose this would be the right opportunity to remove $vm.print too
> > -- it's hardly used, and it doesn't do what it says.
> 
> Do not remove $vm.print().  Its uses go beyond what it in the existing test
> scripts.  I use it for custom debugging tests all the time.  It comes in
> especially handy when I want to print from WebCore and don't want to go thru
> console.log().

Ah okay, thanks for the clarification. My concern is just because $vm.print is currently effectively dataLogLn, and I'm not sure whether this same change is appropriate to apply to dataLog.

[Mark Lam]

Why should the jsc shell treat NULL as a terminator when printing a JS string?  JS strings come with a length, and is not necessarily null terminated like a C string.  I fail to see why we would want to make this change.  In fact, it feels wrong to do so.

[Mark Lam]

(In reply to Mark Lam from comment #5)
> Why should the jsc shell treat NULL as a terminator when printing a JS
> string?  JS strings come with a length, and is not necessarily null
> terminated like a C string.  I fail to see why we would want to make this
> change.  In fact, it feels wrong to do so.

I talked with Ross offline and clarified things.  I was mistaken above and misread the change as the opposite of what it's doing.  Sorry for that.  Carry on.

No need to worry about $vm.print() because it is purely a WebKit developer debugging tool, not a user facing tool.  User (as in the developer) beware when using it.

[Ross Kirsling]

Created attachment 395533 [details]
Patch

[Darin Adler]

Comment on attachment 395533 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=395533&action=review

> Source/JavaScriptCore/jsc.cpp:1293
> +    fprintf(stderr, "--> ");

fputs?

> Source/JavaScriptCore/jsc.cpp:1295
> +    fprintf(stderr, "\n");

putc?

> Source/JavaScriptCore/jsc.cpp:2803
> +            printf("Exception: ");

puts?

> Source/JavaScriptCore/jsc.cpp:2808
> +        printf("\n");

putchar?

> JSTests/ChangeLog:11
> +        Update baseline, but ensure that \0 isn't printed to file, lest the file be treated as binary.

For testing purposes I often like the idea of replacing a NUL with something like "<NUL>" rather than just deleting it. Seems more likely to help us catch bugs and unlikely to cause confusion.

[Ross Kirsling]

(In reply to Darin Adler from comment #8)
> > JSTests/ChangeLog:11
> > +        Update baseline, but ensure that \0 isn't printed to file, lest the file be treated as binary.
> 
> For testing purposes I often like the idea of replacing a NUL with something
> like "<NUL>" rather than just deleting it. Seems more likely to help us
> catch bugs and unlikely to cause confusion.

That's a good idea, though it's probably inappropriate that I edit this test in the first place. :-/

Given that JSTests/ChakraCore is the only subdirectory that does file diffs and only one of its tests is having this problem, it might be simpler to just map its echo function to $vm.print instead...

[Ross Kirsling]

(In reply to Ross Kirsling from comment #9)
> Given that JSTests/ChakraCore is the only subdirectory that does file diffs
> and only one of its tests is having this problem, it might be simpler to
> just map its echo function to $vm.print instead...

Hrmm, but apparently $vm.print doesn't insert spaces between its arguments, so we'd need to either make it do so or update two dozen baselines.

That or I suppose we could just have .baseline-jsc _be_ binary for this test. It's gross, but the data is correct, and we'd avoid modifying files we shouldn't.

[Ross Kirsling]

Created attachment 395540 [details]
Patch for landing

[Ross Kirsling]

(In reply to Ross Kirsling from comment #10)
> That or I suppose we could just have .baseline-jsc _be_ binary for this
> test. It's gross, but the data is correct, and we'd avoid modifying files we
> shouldn't.

Seems like this works fine after all, so long as the file is marked diffable in .gitattributes.

[EWS]

Found 1 new test failure: storage/indexeddb/value-cursor-cycle.html

[EWS]

Committed r259564: <https://trac.webkit.org/changeset/259564>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 395540 [details].

[Radar WebKit Bug Importer]

<rdar://problem/61328543>