[Webkit-unassigned] [Bug 100566] JSC: a JSC printf (support for %J+s and %b)

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Oct 30 20:56:10 PDT 2012


--- Comment #3 from Mark Lam <mark.lam at apple.com>  2012-10-30 20:57:28 PST ---
Responses below.  New patch to follow shortly.

(In reply to comment #2)
> (From update of attachment 171028 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171028&action=review
> What about tests?

Tests added to Tools/TestWebKitAPI/Tests/JavaScriptCore/VMInspector.cpp.

> Overall it looks good, but I'd like to see these comments addressed.
> > Source/JavaScriptCore/ChangeLog:13
> > +        - %J+s prints the WTF::String in verbose mode.
> > +          The + means verbose.
> Consider switching the order of these two lines.  '+' is a modifier and %J+s is an example of it's use.


> > Source/JavaScriptCore/ChangeLog:16
> > +        (JSC):
> The prepare-ChangeLog script put this in but messed up what it is for.  Change it to the right item or remove.


> > Source/JavaScriptCore/interpreter/VMInspector.cpp:112
> > +
> > Source/JavaScriptCore/interpreter/VMInspector.cpp:142
> > +
> Remove, only need one blank line between declarations.

Per our offline discussion, we're leaving these as is for better readability since the coding-style doesn't say anything about blank lines.

> > Source/JavaScriptCore/interpreter/VMInspector.cpp:139
> > +
> Remove this empty line.


> > Source/JavaScriptCore/interpreter/VMInspector.cpp:154
> > +    char buffer[128];
> Seems like the buffer might be too small.

This buffer is only for:
1. To temporarily hold a copy of the normal chars (not needing formatting)  to be passed to printArg() and printed.

    The incoming format string may contain a string of normal chars much longer than 128, but we handle this by breaking them out to 128 chars fragments and printing each fragment before re-using the buffer to load up the next fragment.

2. To hold a single "%..." format to be passed to printArg() to process a single va_arg.

    We also verify in this case that we never copy a format string greater than 128 chars.  I'm assuming that a 128 char format string is meaningless and if the user specifies such a string, then we'll treat it as an error.

Given this usage, I felt that 128 chars would be a good buffer size without using too much stack space.  BTW, I bumped the buffer size up to 129 chars so that we can have 128 chars not including the null terminator.   I've also added the above usage details as a comment in the code.

Are you seeing a flaw somewhere in my reasoning?

> > Source/JavaScriptCore/interpreter/VMInspector.cpp:227
> > +            // verbose mode prints: WTF::String "<your string>"
> This comment isn't needed here since you have the same comment above printWTFString.


> > Source/JavaScriptCore/interpreter/VMInspector.cpp:237
> > +            p--;
> This is not good enough.  Consider the handling of the unsupported %J+z.

Fixed.  I now cache the startOfFormatSpecifier at the beginning, and simply reset to that if we failed to parse the format.  This also takes care of future formats that can be more complex or of variable length.

> > Source/JavaScriptCore/interpreter/VMInspector.cpp:251
> > +        char* argFormat = buffer;
> Do you need argFormat or could you just use buffer?  Using buffer could eliminate future confusion that buffer is available for use between here and the end of the while loop.

I wanted to use argFormat to improve readability (and conveyance of the intent of the code) and I had thought of the issues you brought up.  I've changed my mind now.  I use buffer everywhere, and added comments to indicate that buffer contains an argFormat instead.

> > Source/JavaScriptCore/interpreter/VMInspector.cpp:254
> > +        if (UNLIKELY(curr >= end))
> > +            goto formatTooLong;
> Don't think the overflow check is needed here, since curr was just set to buffer.

This was needed, when I had argFormat separate from buffer, in case someone mess up argFormat in the future.  Since I got rid of argFormat, this is no longer needed, and is deleted.

> Also, you could create a buffer overflow macro instead of putting the if ... goto everywhere.


> > Source/JavaScriptCore/interpreter/VMInspector.cpp:263
> > +        if (c == 'l' || c == 'h') {
> What about hh, ll, L and the more obscure j, t and z length modifiers?

I was going to say "tough luck" and not support every variation of the standard formats.  But on second thought, I decided to make at least a effort to make a good faith effort to make it more compatible (to the extent possible without rat-holing in handling exotic formats).  The code has been updated to support more formats variations.

> > Source/JavaScriptCore/interpreter/VMInspector.cpp:275
> > +            ASSERT(curr[-1] == '\0');
> Seems a little unnecessary given the assignment and if.

Agreed. I was being a little too paranoid when the code was still in flux. It's unnecessary now and is deleted.

> > Source/JavaScriptCore/interpreter/VMInspector.cpp:295
> > +    p -= 5;
> Why subtract 5?  Is this to move before the current format?  Why not just use format for the error message?

This was a little heuristic so that we get some context on the current format that we're attempting to parse.  Otherwise, the error message can print pass the % and be meaningless.  But now that I cache startOfFormatSpecifier, I can set p to that and be guaranteed a meaningful error message.

> > Source/JavaScriptCore/interpreter/VMInspector.cpp:302
> > +    // We've got an error. Can't do any more work. Print and error message if
> Should be "an" instead of "and".


> > Source/JavaScriptCore/interpreter/VMInspector.cpp:357
> > +            const LChar* chars = str->characters8();
> > +            printArg("%s", reinterpret_cast<const char*>(chars));
> Don't think you need the temp.

Not needed.  Just wanted it for readability and in case I want to inspect chars in gdb.  Since this doesn't bloat the code too much, I'd rather keep it.

> > Source/JavaScriptCore/interpreter/VMInspector.cpp:360
> > +            const UChar* chars = str->characters16();
> > +            printArg("%S", reinterpret_cast<const wchar_t*>(chars));
> Ditto.


> > Source/JavaScriptCore/interpreter/VMInspector.cpp:445
> > +        int count = ::vsnprintf(m_buffer, m_size, format, args);
> I don't think it matters too much, but count doesn't include the terminating '\0'.

Yes, I am aware of that and am actually counting on it here (in StringNFormatPrinter::printArg()):

        // Adjust the buffer to what's left if appropriate:
        if (success) {
            m_buffer += count;
            m_size -= count;

Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

More information about the webkit-unassigned mailing list