ARM crash due to non-aligned memory access
Created attachment 13431 [details] ARM crash due to non-aligned memory access On some ARM systems accessing 32-bit values at non-4-aligned addresses crashes or silently reads incorrect data (depending on how kernel handles non-aligned access trap). This patch ensures that if any of the strings are not 4-byte aligned, we do the slower, but working, 16-bit access.
Comment on attachment 13431 [details] ARM crash due to non-aligned memory access This will break the build on Mac OS X, because we will get a warning (treated as an error) about the aligned4 function unused but having internal linkage. Is it really worthwhile to optimize the aligned case? Instead we should probably just always use the character loop on ARM and ignore the faster loop entirely.
Created attachment 13432 [details] Updated per comments I assume it would be faster to do 32-bit at a time vs. 16-bit at a time, but I have no data to back it up so updated patch gets rid of aligned4() function and always does 16-bit access.
Created attachment 13433 [details] Updated per comments I assume it would be faster to do 32-bit at a time vs. 16-bit at a time, but I have no data to back it up so updated patch gets rid of aligned4() function and always does 16-bit access.
Comment on attachment 13433 [details] Updated per comments Looks fine. + const uint16_t* strChars = reinterpret_cast<const uint16_t*>(str->characters()); + const uint16_t* bufChars = reinterpret_cast<const uint16_t*>(buf.s); These should just be const UChar* instead of const uint16_t*, and then you won't need reinterpret_cast. I think in the future we might want to reverse the if and enable the fast 32-bit-at-a-time path only on certain platforms. The ARM case can be the normal case. In fact, I don't think the ARM side needs a comment. Instead the comment should be on the 32-bit side. Something like: // On platforms where it's safe to do so, it's faster to go 4 bytes at a time. review- because of the unneeded reinterpret_cast.
Created attachment 13443 [details] Updated per review comments Removed unnecessary reinterpret_cast, move comment from ARM part to the other part.
Comment on attachment 13443 [details] Updated per review comments r=me
landed in r19930