[webkit-reviews] review denied: [Bug 24986] ARM JIT port : [Attachment 32046] Constant pool for AssemblerBuffer (v2)

[bugzilla-daemon at webkit.org]

Gavin Barraclough <barraclough at apple.com> has denied Gabor Loki
<loki at inf.u-szeged.hu>'s request for review:
Bug 24986: ARM JIT port
https://bugs.webkit.org/show_bug.cgi?id=24986

Attachment 32046: Constant pool for AssemblerBuffer (v2)
https://bugs.webkit.org/attachment.cgi?id=32046&action=review

------- Additional Comments from Gavin Barraclough <barraclough at apple.com>
> Constant pool for AssemblerBuffer (v2)

This basically looks fine as is, r- for a handful of small fixes, but no big
issues at all.

The comment on the class states that the constant pool can store 4 or 8 bytes
values, however the code currently appears to be very 32-bit specific, only
handling entries of size (sizeof(uint32_t)) - so unless I'm missing something
the comment is currently a little misleading.  It may be good to fix this to
say that the pool can hold 32-bit values, and that we intend to extend the
class to be able to hold 64-bit values in the pool too, at some point in the
future.

When you align the pool, I'd suggest you may want to consider replacing the
magic number '0x0badf00d' with a value that will be interpreted as a
breakpoint, or illegal instruction on ARM (just as an extra guard against an
erroneously linked branch jumping into the padding.  To make this effective you
would also have to reverse the order in which it checks & plants the alignment
(byte, then short, then int) in order that the int value is 32-bit aligned.  On
x86 we use HLTs to pad (we use these since they're illegal in user space, cause
a trap, and as such clearly indicate an error case), and we'd probably want to
do the same in the constant pool too.  At minimum I'd suggest you should move
these values into the Assembler (AssemblerType::padForAlign8, 16, 32) so that
these values can be provided in a platform-specific fashion (we also try to
avoid "magic numbers" inline in the code, and prefer defining constant values
or declaring static const variables – making this change would fix this problem
too).  I think it could be even nicer if you just called
AssemblerType::align(int) to do the alignment ... except I imagine that could
be problematic (since you would end up recursing, at Assembler::align tried to
plant a byte, caused a recursive flush, called Assembler::align, tried to plant
a byte ... etc.).  You could of course add a new specific method to Assembler
to alignConstantPool(int), and then provide methods to put the pad values
directly to the assembler buffer.  But then you have a danger that these
methods could be misused.  I'd probably ignore this last suggestion myself. 
I'd probably just add a set of constants to Assembler
(AssemblerType::padForAlign8, 16, 32).

> if (10 * m_numConsts > 6 * maxPoolSize / sizeof(uint32_t))
I think this check is prone to overflow.  It is probably not a practical
concern right now, since m_pool is allocated with a single malloc, so a huge
value for maxPoolSize would not be sensible anyway.  However in order to
support large constant pools on x86[_64], the obvious thing to do will probably
be to replace these arrays with some form of vector, and when we do so this
test would become potentially unsafe.  'maxPoolSize' is defined as a signed
int, so if (maxPoolSize > 0x15555555) then (6 * maxPoolSize) will overflow to a
negative walue, and the test will always pass.	Due to the first multiply (*
10) there are also boundary condition problems that could cause the left had
side of this compare to overflow, too.	If you enforce that maxPoolSize is no
larger than 0x15555555 then the limit (6 * maxPoolSize) could be 0x7FFFFFFE,
however as m_numConsts rolls over from 0x0CCCCCCC -> 0x0CCCCCCD (m_numConsts *
10) goes from 0x7FFFFFF8 -> 0x80000002, which, again interpreted as signed
values are both less than 0x7FFFFFFE, so the limit being exceeded will not be
detected.  I'd suggest adding an "ASSERT(maxPoolSize < 0x10000000);" above this
line to make sure we stay well away from any potential overflow issues here
would be a good idea.

Finally, a quick find & replace code style issue – it is not WebKit coding
style to use abbreviations, we use full words.	AssemblerBufferWithCP is
certainly against style, placeConstPoolBarrier etc really should probably be
placeConstantPoolBarrier - but I leave it at your discretion as to exactly how
you change these names to bring these names in line with coding style.

But other than these little style issues, the mechanism looks great, and yes,
looks like it should be fairly straightforward to take this and roll it out to
the other platforms when we hit a point that we need to add constant-pool
support, which is really good news.