[webkit-reviews] review denied: [Bug 46796] Extend constant pool to be able to store 16 bits instructions with a constant : [Attachment 83811] fix unaligned access address for SH4 platform

[bugzilla-daemon at webkit.org]

Gabor Loki <loki at webkit.org> has denied thouraya <thouraya.andolsi at st.com>'s
request for review:
Bug 46796: Extend constant pool to be able to store 16 bits instructions with a
constant
https://bugs.webkit.org/show_bug.cgi?id=46796

Attachment 83811: fix unaligned access address for SH4 platform
https://bugs.webkit.org/attachment.cgi?id=83811&action=review

------- Additional Comments from Gabor Loki <loki at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=83811&action=review

> Source/JavaScriptCore/assembler/AssemblerBufferWithConstantPool.h:269
> +	   // if the code buffer is notA32 bits aligned emit a nop...
> +	   if (useBarrier && !AssemblerBuffer::isAligned(4))
> +	       AssemblerBuffer::putShort(AssemblerType::padForAlign16);
> +

The issue here is the putShort inserts the padForAlign16 before the barrier
instruction. The padForAlign* should be an invalid instruction, and not NOP.
So, executing this instruction would cause an illegal instruction error. (Your
patch would cause an illegal instruction error if Thumb2 JIT were using the
constant pool.)

I suggest you to redesign or extend my patch for your needs. See the following
code:
 if (useBarrier)
-   AssemblerBuffer::putInt(AssemblerType::placeConstantPoolBarrier(m_numConsts
* sizeof(uint32_t) + alignPool));
+   putIntegral(AssemblerType::placeConstantPoolBarrier(m_numConsts *
sizeof(uint32_t) + alignPool));

Here I used putIntegral template function where the
AssemblerType::placeConstantPoolBarrier is called for the parameter. You can
use this infrastructure for SH4 as well. Create your own
AssemblerType::placeConstantPoolBarrier function which returns a structure of
two shorts, and implement the putIntegral template specialization which inserts
the shorts into the assembler buffer. I think this will help you out.