164893 2016-11-17 15:32:38 -0800 Inlining should be disallowed when JSC_alwaysUseShadowChicken=true. 2016-11-17 16:24:15 -0800 1 1 1 Unclassified WebKit JavaScriptCore WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 mark.lam mark.lam commit-queue keith_miller msaboff saam webkit-bug-importer oldest_to_newest 1252482 0 mark.lam 2016-11-17 15:32:38 -0800 The current shadow chicken implementation isn't written to work with inlining. 1252483 1 mark.lam 2016-11-17 15:33:03 -0800 <rdar://problem/29146436> 1252486 2 295094 mark.lam 2016-11-17 15:35:18 -0800 Created attachment 295094 proposed patch. 1252488 3 295094 saam 2016-11-17 15:37:43 -0800 Comment on attachment 295094 proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=295094&action=review > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1412 > + if (UNLIKELY(m_hasDebuggerEnabled || Options::alwaysUseShadowChicken())) { Why not set the inlining depth to 1 inside recomputeDependentOptions? 1252490 4 295094 saam 2016-11-17 15:38:15 -0800 Comment on attachment 295094 proposed patch. Also, do you have a test? 1252492 5 mark.lam 2016-11-17 15:41:40 -0800 (In reply to comment #3) > Comment on attachment 295094 [details] > proposed patch. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=295094&action=review > > > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1412 > > + if (UNLIKELY(m_hasDebuggerEnabled || Options::alwaysUseShadowChicken())) { > > Why not set the inlining depth to 1 inside recomputeDependentOptions? I like this solution better. I will change the patch. (In reply to comment #4) > Comment on attachment 295094 [details] > proposed patch. > > Also, do you have a test? Sorry, don't have a reduced one. 1252495 6 295097 mark.lam 2016-11-17 15:46:25 -0800 Created attachment 295097 proposed patch. 1252519 7 mark.lam 2016-11-17 16:24:15 -0800 Thanks for the review. Landed in r208866: <http://trac.webkit.org/r208866>. 295094 2016-11-17 15:35:18 -0800 2016-11-17 15:46:25 -0800 proposed patch. bug-164893.patch text/plain 1397 mark.lam SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMjA4ODYxKQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE0IEBA CisyMDE2LTExLTE3ICBNYXJrIExhbSAgPG1hcmsubGFtQGFwcGxlLmNvbT4KKworICAgICAgICBJ bmxpbmluZyBzaG91bGQgYmUgZGlzYWxsb3dlZCB3aGVuIEpTQ19hbHdheXNVc2VTaGFkb3dDaGlj a2VuPXRydWUuCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9p ZD0xNjQ4OTMKKyAgICAgICAgPHJkYXI6Ly9wcm9ibGVtLzI5MTQ2NDM2PgorCisgICAgICAgIFJl dmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgICogZGZnL0RGR0J5dGVDb2RlUGFy c2VyLmNwcDoKKyAgICAgICAgKEpTQzo6REZHOjpCeXRlQ29kZVBhcnNlcjo6aW5saW5pbmdDb3N0 KToKKwogMjAxNi0xMS0xNyAgRmlsaXAgUGl6bG8gIDxmcGl6bG9AYXBwbGUuY29tPgogCiAgICAg ICAgIFNwZWN1bGF0aXZlbHkgZGlzYWJsZSBlYWdlciBvYmplY3QgemVyby1maWxsIG9uIG5vdC14 ODYgdG8gbGV0IHRoZSBib3RzIGRlY2lkZSBpZiB0aGF0J3MgYSBwcm9ibGVtCkluZGV4OiBTb3Vy Y2UvSmF2YVNjcmlwdENvcmUvZGZnL0RGR0J5dGVDb2RlUGFyc2VyLmNwcAo9PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0t LSBTb3VyY2UvSmF2YVNjcmlwdENvcmUvZGZnL0RGR0J5dGVDb2RlUGFyc2VyLmNwcAkocmV2aXNp b24gMjA4ODYxKQorKysgU291cmNlL0phdmFTY3JpcHRDb3JlL2RmZy9ERkdCeXRlQ29kZVBhcnNl ci5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTE0MDksNyArMTQwOSw3IEBAIHVuc2lnbmVkIEJ5dGVD b2RlUGFyc2VyOjppbmxpbmluZ0Nvc3QoQ2EKICAgICBpZiAodmVyYm9zZSkKICAgICAgICAgZGF0 YUxvZygiQ29uc2lkZXJpbmcgaW5saW5pbmcgIiwgY2FsbGVlLCAiIGludG8gIiwgY3VycmVudENv ZGVPcmlnaW4oKSwgIlxuIik7CiAgICAgCi0gICAgaWYgKG1faGFzRGVidWdnZXJFbmFibGVkKSB7 CisgICAgaWYgKFVOTElLRUxZKG1faGFzRGVidWdnZXJFbmFibGVkIHx8IE9wdGlvbnM6OmFsd2F5 c1VzZVNoYWRvd0NoaWNrZW4oKSkpIHsKICAgICAgICAgaWYgKHZlcmJvc2UpCiAgICAgICAgICAg ICBkYXRhTG9nKCIgICAgRmFpbGluZyBiZWNhdXNlIHRoZSBkZWJ1Z2dlciBpcyBpbiB1c2UuXG4i KTsKICAgICAgICAgcmV0dXJuIFVJTlRfTUFYOwo= 295097 2016-11-17 15:46:25 -0800 2016-11-17 16:21:22 -0800 proposed patch. bug-164893.patch text/plain 1422 mark.lam SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMjA4ODYxKQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE0IEBA CisyMDE2LTExLTE3ICBNYXJrIExhbSAgPG1hcmsubGFtQGFwcGxlLmNvbT4KKworICAgICAgICBJ bmxpbmluZyBzaG91bGQgYmUgZGlzYWxsb3dlZCB3aGVuIEpTQ19hbHdheXNVc2VTaGFkb3dDaGlj a2VuPXRydWUuCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9p ZD0xNjQ4OTMKKyAgICAgICAgPHJkYXI6Ly9wcm9ibGVtLzI5MTQ2NDM2PgorCisgICAgICAgIFJl dmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgICogcnVudGltZS9PcHRpb25zLmNw cDoKKyAgICAgICAgKEpTQzo6cmVjb21wdXRlRGVwZW5kZW50T3B0aW9ucyk6CisKIDIwMTYtMTEt MTcgIEZpbGlwIFBpemxvICA8ZnBpemxvQGFwcGxlLmNvbT4KIAogICAgICAgICBTcGVjdWxhdGl2 ZWx5IGRpc2FibGUgZWFnZXIgb2JqZWN0IHplcm8tZmlsbCBvbiBub3QteDg2IHRvIGxldCB0aGUg Ym90cyBkZWNpZGUgaWYgdGhhdCdzIGEgcHJvYmxlbQpJbmRleDogU291cmNlL0phdmFTY3JpcHRD b3JlL3J1bnRpbWUvT3B0aW9ucy5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL0phdmFTY3JpcHRD b3JlL3J1bnRpbWUvT3B0aW9ucy5jcHAJKHJldmlzaW9uIDIwODg2MSkKKysrIFNvdXJjZS9KYXZh U2NyaXB0Q29yZS9ydW50aW1lL09wdGlvbnMuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC0zODQsNiAr Mzg0LDkgQEAgc3RhdGljIHZvaWQgcmVjb21wdXRlRGVwZW5kZW50T3B0aW9ucygpCiAgICAgT3B0 aW9uczo6dXNlU2VwYXJhdGVkV1hIZWFwKCkgPSB0cnVlOwogI2VuZGlmCiAKKyAgICBpZiAoT3B0 aW9uczo6YWx3YXlzVXNlU2hhZG93Q2hpY2tlbigpKQorICAgICAgICBPcHRpb25zOjptYXhpbXVt SW5saW5pbmdEZXB0aCgpID0gMTsKKwogICAgIC8vIENvbXB1dGUgdGhlIG1heGltdW0gdmFsdWUg b2YgdGhlIHJlb3B0aW1pemF0aW9uIHJldHJ5IGNvdW50ZXIuIFRoaXMgaXMgc2ltcGx5CiAgICAg Ly8gdGhlIGxhcmdlc3QgdmFsdWUgYXQgd2hpY2ggd2UgZG9uJ3Qgb3ZlcmZsb3cgdGhlIGV4ZWN1 dGUgY291bnRlciwgd2hlbiB1c2luZyBpdAogICAgIC8vIHRvIGxlZnQtc2hpZnQgdGhlIGV4ZWN1 dGlvbiBjb3VudGVyIGJ5IHRoaXMgYW1vdW50LiBDdXJyZW50bHkgdGhlIHZhbHVlIGVuZHMK