Bug 135380

Summary: [ARM] Incorrect handling of Unicode characters
Product: WebKit Reporter: Dániel Bátyai <dbatyai.u-szeged>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: bunhere, commit-queue, darin, gyuyoung.kim, llango.u-szeged, mark.lam, ossy, rakuco, sergio
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 108645    
Attachments:
Description Flags
Patch
mark.lam: review-
Patch none

Dániel Bátyai
Reported 2014-07-29 04:36:16 PDT
In c++ the signedness of char can be implementation dependent. Unfortunately, this caused the incorrect handling of Unicode characters in JavaScriptCore on ARM, since the code in stringFromUTF8 in jsc.cpp (http://trac.webkit.org/browser/trunk/Source/JavaScriptCore/jsc.cpp#L658) assumes that char is signed, but that was not the case on ARM.
Attachments
Patch (1.35 KB, patch)
2014-07-29 04:39 PDT, Dániel Bátyai 		mark.lam: review-
DetailsFormatted DiffDiff
Patch (1.65 KB, patch)
2014-08-06 07:23 PDT, Dániel Bátyai 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Dániel Bátyai
Comment 1 2014-07-29 04:39:46 PDT
Created attachment 235676 [details] Patch Force GCC to use signed char for char
Mark Lam
Comment 2 2014-08-05 10:54:16 PDT
Comment on attachment 235676 [details] Patch I think the better fix is to make change stringFromUTF() in jsc.cpp to explicitly use a signed char since it is dependent on signed behavior for correctness. This ensures that the code is correct independent of build configurations. Are there other places where you’ve found the “sign”-ness of chars to be an issue?
Darin Adler
Comment 3 2014-08-05 16:33:14 PDT
Comment on attachment 235676 [details] Patch I think this change is OK, but I also think we should fix the code to not depend on char being signed. I don’t think we need to change the type to “signed char” — we can and should just remove the dependency.
Darin Adler
Comment 4 2014-08-05 16:36:11 PDT
Let me go further. We should remove the silly optimization in stringFromUTF. If we need a fast case for ASCII, that should be inside the fromUTF8WithLatin1Fallback function, not in the JSC tool. Please submit a patch that deletes the misguided “fast case” code from jsc.cpp.
Dániel Bátyai
Comment 5 2014-08-06 07:23:12 PDT
Created attachment 236100 [details] Patch Removed fast case, as suggested.
WebKit Commit Bot
Comment 6 2014-08-06 10:27:57 PDT
Comment on attachment 236100 [details] Patch Clearing flags on attachment: 236100 Committed r172152: <http://trac.webkit.org/changeset/172152>
WebKit Commit Bot
Comment 7 2014-08-06 10:28:01 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.