Bug 171101 – Refactor MASM probe to allow printing of custom types.

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

[Mark Lam]

This allows us to add printing of CodeBlock* and Air::Args amongst other things.

[Mark Lam]

Created attachment 307701 [details]
proposed patch.

[Mark Lam]

Created attachment 307702 [details]
proposed patch: fix EWS complaint.

[Mark Lam]

Created attachment 307704 [details]
proposed patch: fix 32-bit, and guess fix for Win.

[JF Bastien]

Comment on attachment 307704 [details]
proposed patch: fix 32-bit, and guess fix for Win.

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

Needs a 🖨 somewhere.

A few comments, but this looks good. r=me

> Source/JavaScriptCore/assembler/MacroAssemblerPrinter.cpp:47
> +            out.print(" ");

out.print(' '); instead?

> Source/JavaScriptCore/assembler/MacroAssemblerPrinter.cpp:119
> +    out.printf("%s:<0x%016llx %.13g>", name, u.uint64Value, u.doubleValue);

Use bitwise_cast instead of the union(here and below).

> Source/JavaScriptCore/assembler/MacroAssemblerPrinter.h:71
> +//      jit.print(AllRegisters(4)); // The 4 here is for an indentation of 4 characters.

Instead of comment-comment:
size_t indentation = 4;
jit.print(AllRegisters(indentation));

> Source/JavaScriptCore/assembler/MacroAssemblerPrinter.h:94
> +    AllRegisters(int charsToIndent = 0)

`explicit` here and other constructors below (especially the ones that are non-class enum like Printer).

> Source/JavaScriptCore/assembler/Printer.h:41
> +    Data() { }

is this useful? Can we = delete it, or at least memset the entire content to zero?

> Source/JavaScriptCore/assembler/Printer.h:51
> +        memcpy(buffer, src, size);

OK so this is great for a union, because "active member" is what matters in unions. You're only allowed to load back the active member of a union, and the active member is the last one you stored... When you memcpy into a union *all* the members become active, so it's well-defined behavior to load from *any* member!

You should therefore use the same memcpy trick (or bitwise_cast) in the above 3 constructors.

> Source/JavaScriptCore/assembler/Printer.h:61
> +    const T as(int = 0) const

Why the unused default param here?

> Source/JavaScriptCore/assembler/Printer.h:78
> +    uintptr_t buffer[6];

The buffer should be `char` because you cast around it and `char` has magical pixie-dust TBAA properties. Or std::aligned_storage, that's much nicer actually.

> Source/JavaScriptCore/assembler/Printer.h:134
> +inline PrintRecordList* makePrintRecordList(Arguments&&... arguments)

The lifetime of the pointer is unclear here. It's both appended and returned. Who owns it? Should this be some smart pointer?

> Source/JavaScriptCore/assembler/Printer.h:174
> +void setPrinter(PrintRecord& record, T value, intptr_t = 0)

The defaults param is to distinguish signedness? Can you make an enum instead, and pass it as a defaulted template parameter after T?

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:1145
> +        Printer::appendAirArgs(inst, std::forward<Arguments>(arguments)...);

You're forwarding the arguments twice. Can't do that.

> Source/JavaScriptCore/b3/air/AirPrintSpecial.h:124
> +    numSpecialArgs + numCalleeArgs + numReturnGPArgs + numReturnFPArgs;

constexpr instead?

[Mark Lam]

Comment on attachment 307704 [details]
proposed patch: fix 32-bit, and guess fix for Win.

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

>> Source/JavaScriptCore/assembler/MacroAssemblerPrinter.cpp:47
>> +            out.print(" ");
> 
> out.print(' '); instead?

Can't so that.  That would print the ascii value of ' ' as a number.  See printInternal(PrintStream& out, signed char value) and printInternal(PrintStream& out, unsigned char value) in PrintStream.cpp.

>> Source/JavaScriptCore/assembler/MacroAssemblerPrinter.cpp:119
>> +    out.printf("%s:<0x%016llx %.13g>", name, u.uint64Value, u.doubleValue);
> 
> Use bitwise_cast instead of the union(here and below).

Fixed.

>> Source/JavaScriptCore/assembler/MacroAssemblerPrinter.h:71
>> +//      jit.print(AllRegisters(4)); // The 4 here is for an indentation of 4 characters.
> 
> Instead of comment-comment:
> size_t indentation = 4;
> jit.print(AllRegisters(indentation));

Fixed.

>> Source/JavaScriptCore/assembler/MacroAssemblerPrinter.h:94
>> +    AllRegisters(int charsToIndent = 0)
> 
> `explicit` here and other constructors below (especially the ones that are non-class enum like Printer).

I made AllRegisters, Memory, and MemWord constructors explicit.

I want Printer to be implicit.  This way we can do:
    jit.print("cb = ", codeBlock, "\n");
instead of:
    jit.print(Printer("cb = "), Printer(codeBlock), Printer("\n"));

And it is safe to have the Printer constructors be implicit because there's only one consumer of Printer instances (i.e. the MASM print mechanism).  It won't get auto-substituted elsewhere (or should not).

>> Source/JavaScriptCore/assembler/Printer.h:41
>> +    Data() { }
> 
> is this useful? Can we = delete it, or at least memset the entire content to zero?

Yes, this is needed because we can instantiate PrintRecord with an uninitialized Data.  I'll memcpy 0xdeadb0d0 into it to mark it as uninitialized.

>> Source/JavaScriptCore/assembler/Printer.h:51
>> +        memcpy(buffer, src, size);
> 
> OK so this is great for a union, because "active member" is what matters in unions. You're only allowed to load back the active member of a union, and the active member is the last one you stored... When you memcpy into a union *all* the members become active, so it's well-defined behavior to load from *any* member!
> 
> You should therefore use the same memcpy trick (or bitwise_cast) in the above 3 constructors.

Fixed.

>> Source/JavaScriptCore/assembler/Printer.h:61
>> +    const T as(int = 0) const
> 
> Why the unused default param here?

Because VC on Win is not smart enough.  This is a workaround for VC not being able to see that this does not conflict with the as() above.

>> Source/JavaScriptCore/assembler/Printer.h:78
>> +    uintptr_t buffer[6];
> 
> The buffer should be `char` because you cast around it and `char` has magical pixie-dust TBAA properties. Or std::aligned_storage, that's much nicer actually.

Fixed to use std::aligned_storage.

>> Source/JavaScriptCore/assembler/Printer.h:134
>> +inline PrintRecordList* makePrintRecordList(Arguments&&... arguments)
> 
> The lifetime of the pointer is unclear here. It's both appended and returned. Who owns it? Should this be some smart pointer?

The MASM print mechanism is debugging code and this list is intentionally being leaked.  I'll add a comment indicating that.

>> Source/JavaScriptCore/assembler/Printer.h:174
>> +void setPrinter(PrintRecord& record, T value, intptr_t = 0)
> 
> The defaults param is to distinguish signedness? Can you make an enum instead, and pass it as a defaulted template parameter after T?

The default parameter is not for distinguishing sign-ness.  I could have used "uintptr_t = 0, uintptr_t = 0" here instead.  It is just to give this setPrinter template a different signature than the one one below, because of template magic reasons.  The compiler (or maybe the language?) doesn't like it otherwise.

>> Source/JavaScriptCore/b3/B3LowerToAir.cpp:1145
>> +        Printer::appendAirArgs(inst, std::forward<Arguments>(arguments)...);
> 
> You're forwarding the arguments twice. Can't do that.

Fixed.

>> Source/JavaScriptCore/b3/air/AirPrintSpecial.h:124
>> +    numSpecialArgs + numCalleeArgs + numReturnGPArgs + numReturnFPArgs;
> 
> constexpr instead?

Fixed.

[Mark Lam]

Comment on attachment 307704 [details]
proposed patch: fix 32-bit, and guess fix for Win.

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

>>> Source/JavaScriptCore/assembler/Printer.h:78
>>> +    uintptr_t buffer[6];
>> 
>> The buffer should be `char` because you cast around it and `char` has magical pixie-dust TBAA properties. Or std::aligned_storage, that's much nicer actually.
> 
> Fixed to use std::aligned_storage.

Actually, std::aligned_storage<N> appears to always gives me a size of 1 regardless of what I specify for N.  I'm reverting to uintptr_t arrays so that I'll get uintptr_t alignment.

[Mark Lam]

Created attachment 307769 [details]
updated patch w/ JF's feedback applied.

[JF Bastien]

Comment on attachment 307769 [details]
updated patch w/ JF's feedback applied.

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

r=me

> Source/JavaScriptCore/assembler/MacroAssemblerPrinter.h:117
> +    explicit Memory(RegisterID& reg, size_t bytes, DumpStyle style = GenericDump)

FWIW I only meant for the single-parameter ctors. This is fine though: pre-C++11 it didn't mean anything, but now it applies the same rules for initialization with curlies.

[Mark Lam]

Thanks for the review.  Landed in r215642: <http://trac.webkit.org/r215642>.