Bug 113656 – fourthTier: FTL JIT should supply TBAA meta-data to LLVM

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

[Filip Pizlo]

We should get this right early, so that we don't have a bad time trying to put it in later.

[Filip Pizlo]

Created attachment 195889 [details]
work in progress

[Filip Pizlo]

Created attachment 195900 [details]
the patch

[Oliver Hunt]

Comment on attachment 195900 [details]
the patch

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

> Source/JavaScriptCore/ftl/FTLAbstractHeap.cpp:117
> +        

static const char* negSplit = "_neg_";

> Source/JavaScriptCore/ftl/FTLAbstractHeap.cpp:118
> +        size_t stringLength = m_heapNameLength + (negative ? 5 : 1) + numHexlets;

negative ? sizeof(negSplit) : 1

> Source/JavaScriptCore/ftl/FTLAbstractHeap.cpp:124
> +            memcpy(characters + m_heapNameLength, "_neg_", 5);

memcpy(characters + m_heapNameLength, negSplit, sizeof(negSplit))

feel free to choose a better name than negsplit though :D

> Source/JavaScriptCore/ftl/FTLAbstractHeap.cpp:130
> +            characters[stringLength - i - 1] = ("0123456789ABCDEF")[accumulator & 0xF];

Slightly concerned about this leaking address space layout info, but this would be trivial to fix if it were a problem, and i'm not sure if it is a problem without some bug already being present.

[Filip Pizlo]

(In reply to comment #3)
> (From update of attachment 195900 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=195900&action=review
> 
> > Source/JavaScriptCore/ftl/FTLAbstractHeap.cpp:117
> > +        
> 
> static const char* negSplit = "_neg_";

Will do this and all of those other things.  Can't think of a better name than negSplit so I'll use that.

> 
> > Source/JavaScriptCore/ftl/FTLAbstractHeap.cpp:118
> > +        size_t stringLength = m_heapNameLength + (negative ? 5 : 1) + numHexlets;
> 
> negative ? sizeof(negSplit) : 1
> 
> > Source/JavaScriptCore/ftl/FTLAbstractHeap.cpp:124
> > +            memcpy(characters + m_heapNameLength, "_neg_", 5);
> 
> memcpy(characters + m_heapNameLength, negSplit, sizeof(negSplit))
> 
> feel free to choose a better name than negsplit though :D
> 
> > Source/JavaScriptCore/ftl/FTLAbstractHeap.cpp:130
> > +            characters[stringLength - i - 1] = ("0123456789ABCDEF")[accumulator & 0xF];
> 
> Slightly concerned about this leaking address space layout info, but this would be trivial to fix if it were a problem, and i'm not sure if it is a problem without some bug already being present.

Interesting point.  My thinking is that pointers to globals can already be accessed through other means.  I think of this nasty code as being, from an information flow standpoint, just like saying:

class MyAwesomeClass {
     MyAwesomeClass(void* foo) : m_foo(foo) { }
    void* m_foo;
}

new MyAwesomeClass(addressOfGlobal(blah))

We do analogous equivalents to MyAwesomeClass all over the place in the compiler already.  DFG::Node is an example of an awesome class that has a pointer to globals for GetGlobalVar and PutGlobalVar...

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/147292

[Sam Weinig]

Comment on attachment 195900 [details]
the patch

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

> Source/JavaScriptCore/ftl/FTLAbstractHeap.cpp:113
> +        // The crazy way of testing of index >= 2 ** power
> +        // (where I use ** to denote pow()).
> +        if ((index >> 1) >> (power - 1))

This seems like it would prefer to be in a helper function.

>> Source/JavaScriptCore/ftl/FTLAbstractHeap.cpp:130
>> +            characters[stringLength - i - 1] = ("0123456789ABCDEF")[accumulator & 0xF];
> 
> Slightly concerned about this leaking address space layout info, but this would be trivial to fix if it were a problem, and i'm not sure if it is a problem without some bug already being present.

There is toASCIIHexValue that you could use here.

> Source/JavaScriptCore/ftl/FTLAbstractHeap.h:29
> +#include <wtf/Platform.h>

You shouldn't have to include this.

> Source/JavaScriptCore/ftl/FTLAbstractHeap.h:169
> +    struct MyHashTraits : WTF::GenericHashTraits<ptrdiff_t> {

This is not the best name in the world.

> Source/JavaScriptCore/ftl/FTLAbstractHeap.h:216
> +private:
> +    
> +    // The trick here is that the indexed heap is "indexed" by a pointer-width

Extra newline.

> Source/JavaScriptCore/ftl/FTLAbstractHeapRepository.h:95
> +    unsigned m_tbaaKind;
> +    
> +};

Extra newline.

[Filip Pizlo]

(In reply to comment #6)
> (From update of attachment 195900 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=195900&action=review
> 
> > Source/JavaScriptCore/ftl/FTLAbstractHeap.cpp:113
> > +        // The crazy way of testing of index >= 2 ** power
> > +        // (where I use ** to denote pow()).
> > +        if ((index >> 1) >> (power - 1))
> 
> This seems like it would prefer to be in a helper function.

Well, sort of; this relies on power never being 0.  I'll add WTF::isGreaterThanNonZeroPowerOfTwo().

> 
> >> Source/JavaScriptCore/ftl/FTLAbstractHeap.cpp:130
> >> +            characters[stringLength - i - 1] = ("0123456789ABCDEF")[accumulator & 0xF];
> > 
> > Slightly concerned about this leaking address space layout info, but this would be trivial to fix if it were a problem, and i'm not sure if it is a problem without some bug already being present.
> 
> There is toASCIIHexValue that you could use here.

Will do.

> 
> > Source/JavaScriptCore/ftl/FTLAbstractHeap.h:29
> > +#include <wtf/Platform.h>
> 
> You shouldn't have to include this.

OK.  Stay tuned for a big patch to remove those.  This is a pattern we used in the DFG.

> 
> > Source/JavaScriptCore/ftl/FTLAbstractHeap.h:169
> > +    struct MyHashTraits : WTF::GenericHashTraits<ptrdiff_t> {
> 
> This is not the best name in the world.

Will rename to WithoutZeroOrOneHashTraits.

> 
> > Source/JavaScriptCore/ftl/FTLAbstractHeap.h:216
> > +private:
> > +    
> > +    // The trick here is that the indexed heap is "indexed" by a pointer-width
> 
> Extra newline.

OK.

> 
> > Source/JavaScriptCore/ftl/FTLAbstractHeapRepository.h:95
> > +    unsigned m_tbaaKind;
> > +    
> > +};
> 
> Extra newline.

OK.

[Filip Pizlo]

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 195900 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=195900&action=review
> > 
> > > Source/JavaScriptCore/ftl/FTLAbstractHeap.cpp:113
> > > +        // The crazy way of testing of index >= 2 ** power
> > > +        // (where I use ** to denote pow()).
> > > +        if ((index >> 1) >> (power - 1))
> > 
> > This seems like it would prefer to be in a helper function.
> 
> Well, sort of; this relies on power never being 0.  I'll add WTF::isGreaterThanNonZeroPowerOfTwo().
> 
> > 
> > >> Source/JavaScriptCore/ftl/FTLAbstractHeap.cpp:130
> > >> +            characters[stringLength - i - 1] = ("0123456789ABCDEF")[accumulator & 0xF];
> > > 
> > > Slightly concerned about this leaking address space layout info, but this would be trivial to fix if it were a problem, and i'm not sure if it is a problem without some bug already being present.
> > 
> > There is toASCIIHexValue that you could use here.
> 
> Will do.
> 
> > 
> > > Source/JavaScriptCore/ftl/FTLAbstractHeap.h:29
> > > +#include <wtf/Platform.h>
> > 
> > You shouldn't have to include this.
> 
> OK.  Stay tuned for a big patch to remove those.  This is a pattern we used in the DFG.
> 
> > 
> > > Source/JavaScriptCore/ftl/FTLAbstractHeap.h:169
> > > +    struct MyHashTraits : WTF::GenericHashTraits<ptrdiff_t> {
> > 
> > This is not the best name in the world.
> 
> Will rename to WithoutZeroOrOneHashTraits.
> 
> > 
> > > Source/JavaScriptCore/ftl/FTLAbstractHeap.h:216
> > > +private:
> > > +    
> > > +    // The trick here is that the indexed heap is "indexed" by a pointer-width
> > 
> > Extra newline.
> 
> OK.
> 
> > 
> > > Source/JavaScriptCore/ftl/FTLAbstractHeapRepository.h:95
> > > +    unsigned m_tbaaKind;
> > > +    
> > > +};
> > 
> > Extra newline.
> 
> OK.

Landed everything except the #include <wtf/Platform.h> thingy in http://trac.webkit.org/changeset/147299

[Filip Pizlo]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > (From update of attachment 195900 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=195900&action=review
> > > 
> > > > Source/JavaScriptCore/ftl/FTLAbstractHeap.cpp:113
> > > > +        // The crazy way of testing of index >= 2 ** power
> > > > +        // (where I use ** to denote pow()).
> > > > +        if ((index >> 1) >> (power - 1))
> > > 
> > > This seems like it would prefer to be in a helper function.
> > 
> > Well, sort of; this relies on power never being 0.  I'll add WTF::isGreaterThanNonZeroPowerOfTwo().
> > 
> > > 
> > > >> Source/JavaScriptCore/ftl/FTLAbstractHeap.cpp:130
> > > >> +            characters[stringLength - i - 1] = ("0123456789ABCDEF")[accumulator & 0xF];
> > > > 
> > > > Slightly concerned about this leaking address space layout info, but this would be trivial to fix if it were a problem, and i'm not sure if it is a problem without some bug already being present.
> > > 
> > > There is toASCIIHexValue that you could use here.
> > 
> > Will do.
> > 
> > > 
> > > > Source/JavaScriptCore/ftl/FTLAbstractHeap.h:29
> > > > +#include <wtf/Platform.h>
> > > 
> > > You shouldn't have to include this.
> > 
> > OK.  Stay tuned for a big patch to remove those.  This is a pattern we used in the DFG.
> > 
> > > 
> > > > Source/JavaScriptCore/ftl/FTLAbstractHeap.h:169
> > > > +    struct MyHashTraits : WTF::GenericHashTraits<ptrdiff_t> {
> > > 
> > > This is not the best name in the world.
> > 
> > Will rename to WithoutZeroOrOneHashTraits.
> > 
> > > 
> > > > Source/JavaScriptCore/ftl/FTLAbstractHeap.h:216
> > > > +private:
> > > > +    
> > > > +    // The trick here is that the indexed heap is "indexed" by a pointer-width
> > > 
> > > Extra newline.
> > 
> > OK.
> > 
> > > 
> > > > Source/JavaScriptCore/ftl/FTLAbstractHeapRepository.h:95
> > > > +    unsigned m_tbaaKind;
> > > > +    
> > > > +};
> > > 
> > > Extra newline.
> > 
> > OK.
> 
> Landed everything except the #include <wtf/Platform.h> thingy in http://trac.webkit.org/changeset/147299

And really landed in http://trac.webkit.org/changeset/147300