Bug 113656

Created attachment 195889 [details] work in progress

Created attachment 195900 [details] the patch

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.

(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...

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

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.

(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.

(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

(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