97008 2012-09-18 06:19:10 -0700 [BlackBerry] Allow denormal floats in ARM VFP 2012-09-27 00:00:57 -0700 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) Other Other RESOLVED FIXED P3 Minor --- 1 ctruta webkit-unassigned barraclough benjamin fpizlo webkit.review.bot yong.li.webkit oldest_to_newest 722720 0 ctruta 2012-09-18 06:19:10 -0700 By default, the ARM FPU runs in FastZero mode, which is not IEEE-compliant. In this mode, mathematical routines that handle denormal floats do not work correctly. By clearing the FastZero flag, the IEEE mode will be enabled. 726339 1 165301 ctruta 2012-09-23 19:29:40 -0700 Created attachment 165301 Patch 726340 2 webkit.review.bot 2012-09-23 19:32:45 -0700 Attachment 165301 did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1 Source/JavaScriptCore/runtime/JSGlobalData.cpp:133: Omit int when using unsigned [runtime/unsigned] [1] Source/JavaScriptCore/runtime/JSGlobalData.cpp:134: __volatile__ is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/JavaScriptCore/runtime/JSGlobalData.cpp:136: __volatile__ is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 3 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style. 726353 3 165301 fpizlo 2012-09-23 20:23:37 -0700 Comment on attachment 165301 Patch I don't like the way you've done this. 1) If you need to do things to your CPU to make it conform, then wouldn't it be better to do those things in the code that uses JavaScriptCore rather than in JavaScriptCore itself? The way you've implemented it, a program that uses JavaScriptCore will start out having one kind of FP semantics, until the first JSGlobalData is instantiated, at which point it will henceforth have a different kind of FP semantics. That's weird. Wouldn't it be better to have the client program set up the FP state that it wants at the beginning (equivalent of main())? 2) If you really must put this into JavaScriptCore, say because you don't want the client program to care about what kinds of FP settings JavaScriptCore wants, then wouldn't it be infinitely better to put it someplace like initializeThreading()? Changing the FP state should have nothing to do with creating a new JSC instance, especially since this is by definition a one-way state change - JSGlobalData::~JSGlobalData does not reset the FP state, and you don't flip it in any way on entry into this particular JSGlobalData instance. Please consider doing either (1) or (2) instead of this current approach. 726484 4 barraclough 2012-09-24 02:22:55 -0700 +1 to Filip's comment. 726637 5 yong.li.webkit 2012-09-24 07:25:03 -0700 (In reply to comment #3) > (From update of attachment 165301 [details]) > I don't like the way you've done this. > > 1) If you need to do things to your CPU to make it conform, then wouldn't it be better to do those things in the code that uses JavaScriptCore rather than in JavaScriptCore itself? The way you've implemented it, a program that uses JavaScriptCore will start out having one kind of FP semantics, until the first JSGlobalData is instantiated, at which point it will henceforth have a different kind of FP semantics. That's weird. Wouldn't it be better to have the client program set up the FP state that it wants at the beginning (equivalent of main())? > > 2) If you really must put this into JavaScriptCore, say because you don't want the client program to care about what kinds of FP settings JavaScriptCore wants, then wouldn't it be infinitely better to put it someplace like initializeThreading()? Changing the FP state should have nothing to do with creating a new JSC instance, especially since this is by definition a one-way state change - JSGlobalData::~JSGlobalData does not reset the FP state, and you don't flip it in any way on entry into this particular JSGlobalData instance. > > Please consider doing either (1) or (2) instead of this current approach. We thought about using initializeThreading() or doing it in the caller. However it doesn't cover Web Worker threads. So either we need to add the same code to wtf/ThreadingPosix.cpp, or to WebCore web worker code. Which way do you think is better? 727041 6 fpizlo 2012-09-24 15:22:29 -0700 (In reply to comment #5) > (In reply to comment #3) > > (From update of attachment 165301 [details] [details]) > > I don't like the way you've done this. > > > > 1) If you need to do things to your CPU to make it conform, then wouldn't it be better to do those things in the code that uses JavaScriptCore rather than in JavaScriptCore itself? The way you've implemented it, a program that uses JavaScriptCore will start out having one kind of FP semantics, until the first JSGlobalData is instantiated, at which point it will henceforth have a different kind of FP semantics. That's weird. Wouldn't it be better to have the client program set up the FP state that it wants at the beginning (equivalent of main())? > > > > 2) If you really must put this into JavaScriptCore, say because you don't want the client program to care about what kinds of FP settings JavaScriptCore wants, then wouldn't it be infinitely better to put it someplace like initializeThreading()? Changing the FP state should have nothing to do with creating a new JSC instance, especially since this is by definition a one-way state change - JSGlobalData::~JSGlobalData does not reset the FP state, and you don't flip it in any way on entry into this particular JSGlobalData instance. > > > > Please consider doing either (1) or (2) instead of this current approach. > > We thought about using initializeThreading() or doing it in the caller. However it doesn't cover Web Worker threads. So either we need to add the same code to wtf/ThreadingPosix.cpp, or to WebCore web worker code. Which way do you think is better? Why not do (1)? Or is it the case that your OS doesn't have the notion of setting the FP mode for the entire process? If it's the case that your OS doesn't allow you to set this for the entire process, then it seems like wtf/ThreadingPosix.cpp is the way to go. 727192 7 ctruta 2012-09-24 19:04:47 -0700 (In reply to comment #6) > (In reply to comment #5) > > We thought about using initializeThreading() or doing it in the caller. However it doesn't cover Web Worker threads. So either we need to add the same code to wtf/ThreadingPosix.cpp, or to WebCore web worker code. Which way do you think is better? > > Why not do (1)? > > Or is it the case that your OS doesn't have the notion of setting the FP mode for the entire process? > > If it's the case that your OS doesn't allow you to set this for the entire process, then it seems like wtf/ThreadingPosix.cpp is the way to go. We need to set the FP mode per thread, and we will do this initialization in wtf/ThreadingPthreads.cpp. Thank you very much. 729196 8 165925 ctruta 2012-09-26 21:40:11 -0700 Created attachment 165925 Patch (In reply to comment #6) > Or is it the case that your OS doesn't have the notion of setting the FP mode for the entire process? We spoke with the QNX developers, who confirmed that, on ARM, every new thread starts with a default FP mode in which the FZ flag is on by default. We must clear FZ per-thread, we can't do it per-process. 729254 9 165925 fpizlo 2012-09-26 23:50:42 -0700 Comment on attachment 165925 Patch I like this approach much better! 729256 10 fpizlo 2012-09-26 23:51:06 -0700 (In reply to comment #8) > Created an attachment (id=165925) [details] > Patch > > (In reply to comment #6) > > Or is it the case that your OS doesn't have the notion of setting the FP mode for the entire process? > > We spoke with the QNX developers, who confirmed that, on ARM, every new thread starts with a default FP mode in which the FZ flag is on by default. We must clear FZ per-thread, we can't do it per-process. Got it. R=me. 729262 11 165925 webkit.review.bot 2012-09-27 00:00:52 -0700 Comment on attachment 165925 Patch Clearing flags on attachment: 165925 Committed r129730: <http://trac.webkit.org/changeset/129730> 729263 12 webkit.review.bot 2012-09-27 00:00:57 -0700 All reviewed patches have been landed. Closing bug. 165301 2012-09-23 19:29:40 -0700 2012-09-26 21:40:11 -0700 Patch patch.diff text/plain 1944 ctruta ZGlmZiAtLWdpdCBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cgYi9Tb3VyY2UvSmF2 YVNjcmlwdENvcmUvQ2hhbmdlTG9nCmluZGV4IDZmYWVkYjQuLjA0YjQ0ZTMgMTAwNjQ0Ci0tLSBh L1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL0phdmFTY3JpcHRD b3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIwIEBACisyMDEyLTA5LTIzICBDb3NtaW4gVHJ1dGEg IDxjdHJ1dGFAcmltLmNvbT4KKworICAgICAgICBbQmxhY2tCZXJyeV0gQWxsb3cgZGVub3JtYWwg ZmxvYXRzIGluIEFSTSBWRlAKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19i dWcuY2dpP2lkPTk3MDA4CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisK KyAgICAgICAgUklNIFBSIDIwMjY2MQorICAgICAgICBSZXZpZXdlZCBpbnRlcm5hbGx5IGJ5IEdl b3JnZSBTdGFpa29zLgorCisgICAgICAgIEVuYWJsZSB0aGUgSUVFRS03NTQgZGVub3JtYWwgZmxv YXRzIGJ5IGNsZWFyaW5nIHRoZSBGYXN0WmVybyBmbGFnIGluIHRoZSBBUk0gRlBVLgorCisgICAg ICAgICogcnVudGltZS9KU0dsb2JhbERhdGEuY3BwOgorICAgICAgICAoSlNDKToKKyAgICAgICAg KEpTQzo6ZW5hYmxlSUVFRTc1NERlbm9ybWFsKToKKyAgICAgICAgKEpTQzo6SlNHbG9iYWxEYXRh OjpKU0dsb2JhbERhdGEpOgorCiAyMDEyLTA5LTIzICBHYXZpbiBCYXJyYWNsb3VnaCAgPGJhcnJh Y2xvdWdoQGFwcGxlLmNvbT4KIAogICAgICAgICBTb3J0aW5nIGEgbm9uLWFycmF5IGNyZWF0ZXMg cHJvcHJldGllcyAoc3BlYy12aW9sYXRpb24pCmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvcnVudGltZS9KU0dsb2JhbERhdGEuY3BwIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL3J1 bnRpbWUvSlNHbG9iYWxEYXRhLmNwcAppbmRleCAyNmYyYjg2Li4wMDlmYmRjIDEwMDY0NAotLS0g YS9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvcnVudGltZS9KU0dsb2JhbERhdGEuY3BwCisrKyBiL1Nv dXJjZS9KYXZhU2NyaXB0Q29yZS9ydW50aW1lL0pTR2xvYmFsRGF0YS5jcHAKQEAgLTEyNiw2ICsx MjYsMTcgQEAgc3RhdGljIGJvb2wgZW5hYmxlQXNzZW1ibGVyKEV4ZWN1dGFibGVBbGxvY2F0b3Im IGV4ZWN1dGFibGVBbGxvY2F0b3IpCiB9CiAjZW5kaWYKIAorI2lmIFBMQVRGT1JNKEJMQUNLQkVS UlkpICYmIENQVShBUk1fVEhVTUIyKQorc3RhdGljIHZvaWQgZW5hYmxlSUVFRTc1NERlbm9ybWFs KCkKK3sKKyAgICAvLyBDbGVhciB0aGUgQVJNX1ZGUF9GUFNDUl9GWiBmbGFnIGluIEZQU0NSLgor ICAgIHVuc2lnbmVkIGludCBmcHNjcjsKKyAgICBfX2FzbV9fIF9fdm9sYXRpbGVfXygidm1ycyAl MCwgZnBzY3IiIDogIj1yIihmcHNjcikpOworICAgIGZwc2NyICY9IH4weDAxMDAwMDAwdTsKKyAg ICBfX2FzbV9fIF9fdm9sYXRpbGVfXygidm1zciBmcHNjciwgJTAiIDogOiAiciIoZnBzY3IpKTsK K30KKyNlbmRpZgorCiBKU0dsb2JhbERhdGE6OkpTR2xvYmFsRGF0YShHbG9iYWxEYXRhVHlwZSBn bG9iYWxEYXRhVHlwZSwgVGhyZWFkU3RhY2tUeXBlIHRocmVhZFN0YWNrVHlwZSwgSGVhcFR5cGUg aGVhcFR5cGUpCiAgICAgOgogI2lmIEVOQUJMRShBU1NFTUJMRVIpCkBAIC0yMjgsNiArMjM5LDEw IEBAIEpTR2xvYmFsRGF0YTo6SlNHbG9iYWxEYXRhKEdsb2JhbERhdGFUeXBlIGdsb2JhbERhdGFU eXBlLCBUaHJlYWRTdGFja1R5cGUgdGhyZWFkCiAgICAgaGVhcC5ub3RpZnlJc1NhZmVUb0NvbGxl Y3QoKTsKICAgICAKICAgICBMTEludDo6RGF0YTo6cGVyZm9ybUFzc2VydGlvbnMoKnRoaXMpOwor CisjaWYgUExBVEZPUk0oQkxBQ0tCRVJSWSkgJiYgQ1BVKEFSTV9USFVNQjIpCisgICAgZW5hYmxl SUVFRTc1NERlbm9ybWFsKCk7CisjZW5kaWYKIH0KIAogSlNHbG9iYWxEYXRhOjp+SlNHbG9iYWxE YXRhKCkK 165925 2012-09-26 21:40:11 -0700 2012-09-27 00:00:52 -0700 Patch patch.diff text/plain 2014 ctruta ZGlmZiAtLWdpdCBhL1NvdXJjZS9XVEYvQ2hhbmdlTG9nIGIvU291cmNlL1dURi9DaGFuZ2VMb2cK aW5kZXggNTFjNjgzYi4uNTVmMTg3YyAxMDA2NDQKLS0tIGEvU291cmNlL1dURi9DaGFuZ2VMb2cK KysrIGIvU291cmNlL1dURi9DaGFuZ2VMb2cKQEAgLTEsMyArMSwxOCBAQAorMjAxMi0wOS0yNiAg Q29zbWluIFRydXRhICA8Y3RydXRhQHJpbS5jb20+CisKKyAgICAgICAgW0JsYWNrQmVycnldIEFs bG93IGRlbm9ybWFsIGZsb2F0cyBpbiBBUk0gVkZQCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJr aXQub3JnL3Nob3dfYnVnLmNnaT9pZD05NzAwOAorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9E WSAoT09QUyEpLgorICAgICAgICBSZXZpZXdlZCBpbnRlcm5hbGx5IGJ5IFlvbmcgTGkuCisKKyAg ICAgICAgQ2xlYXJlZCB0aGUgRmx1c2gtdG8tWmVybyBmbGFnIGluIHRoZSBBUk0gRlBTQ1IgcmVn aXN0ZXIgb24gUU5YLgorCisgICAgICAgICogd3RmL1RocmVhZGluZ1B0aHJlYWRzLmNwcDoKKyAg ICAgICAgKFdURjo6ZW5hYmxlSUVFRTc1NERlbm9ybWFsKTogQWRkZWQuCisgICAgICAgIChXVEY6 OmluaXRpYWxpemVUaHJlYWRpbmcpOgorICAgICAgICAoV1RGOjppbml0aWFsaXplQ3VycmVudFRo cmVhZEludGVybmFsKToKKwogMjAxMi0wOS0yNiAgTWljaGFlbCBTYWJvZmYgIDxtc2Fib2ZmQGFw cGxlLmNvbT4KIAogICAgICAgICBVcGRhdGUgQ29tcGxleFRleHRDb250cm9sbGVyIGZvciA4IGJp dCBUZXh0UnVuIGNoYW5nZXMKZGlmZiAtLWdpdCBhL1NvdXJjZS9XVEYvd3RmL1RocmVhZGluZ1B0 aHJlYWRzLmNwcCBiL1NvdXJjZS9XVEYvd3RmL1RocmVhZGluZ1B0aHJlYWRzLmNwcAppbmRleCAw YTI1NDhjLi44OTkxZTI0IDEwMDY0NAotLS0gYS9Tb3VyY2UvV1RGL3d0Zi9UaHJlYWRpbmdQdGhy ZWFkcy5jcHAKKysrIGIvU291cmNlL1dURi93dGYvVGhyZWFkaW5nUHRocmVhZHMuY3BwCkBAIC0x MTAsMTEgKzExMCwyNiBAQCBzdGF0aWMgTXV0ZXgmIHRocmVhZE1hcE11dGV4KCkKICAgICByZXR1 cm4gbXV0ZXg7CiB9CiAKKyNpZiBPUyhRTlgpICYmIENQVShBUk1fVEhVTUIyKQorc3RhdGljIHZv aWQgZW5hYmxlSUVFRTc1NERlbm9ybWFsKCkKK3sKKyAgICAvLyBDbGVhciB0aGUgQVJNX1ZGUF9G UFNDUl9GWiBmbGFnIGluIEZQU0NSLgorICAgIHVuc2lnbmVkIGZwc2NyOworICAgIGFzbSB2b2xh dGlsZSgidm1ycyAlMCwgZnBzY3IiIDogIj1yIihmcHNjcikpOworICAgIGZwc2NyICY9IH4weDAx MDAwMDAwdTsKKyAgICBhc20gdm9sYXRpbGUoInZtc3IgZnBzY3IsICUwIiA6IDogInIiKGZwc2Ny KSk7Cit9CisjZW5kaWYKKwogdm9pZCBpbml0aWFsaXplVGhyZWFkaW5nKCkKIHsKICAgICBpZiAo YXRvbWljYWxseUluaXRpYWxpemVkU3RhdGljTXV0ZXgpCiAgICAgICAgIHJldHVybjsKIAorI2lm IE9TKFFOWCkgJiYgQ1BVKEFSTV9USFVNQjIpCisgICAgZW5hYmxlSUVFRTc1NERlbm9ybWFsKCk7 CisjZW5kaWYKKwogICAgIFdURjo6ZG91YmxlX2NvbnZlcnNpb246OmluaXRpYWxpemUoKTsKICAg ICAvLyBTdHJpbmdJbXBsOjplbXB0eSgpIGRvZXMgbm90IGNvbnN0cnVjdCBpdHMgc3RhdGljIHN0 cmluZyBpbiBhIHRocmVhZHNhZmUgZmFzaGlvbiwKICAgICAvLyBzbyBlbnN1cmUgaXQgaGFzIGJl ZW4gaW5pdGlhbGl6ZWQgZnJvbSBoZXJlLgpAQCAtMjEyLDYgKzIyNywxMCBAQCB2b2lkIGluaXRp YWxpemVDdXJyZW50VGhyZWFkSW50ZXJuYWwoY29uc3QgY2hhciogdGhyZWFkTmFtZSkKICAgICBv YmpjX3JlZ2lzdGVyVGhyZWFkV2l0aENvbGxlY3RvcigpOwogI2VuZGlmCiAKKyNpZiBPUyhRTlgp ICYmIENQVShBUk1fVEhVTUIyKQorICAgIGVuYWJsZUlFRUU3NTREZW5vcm1hbCgpOworI2VuZGlm CisKICAgICBUaHJlYWRJZGVudGlmaWVyIGlkID0gaWRlbnRpZmllckJ5UHRocmVhZEhhbmRsZShw dGhyZWFkX3NlbGYoKSk7CiAgICAgQVNTRVJUKGlkKTsKICAgICBUaHJlYWRJZGVudGlmaWVyRGF0 YTo6aW5pdGlhbGl6ZShpZCk7Cg==