218201 2020-10-26 13:14:17 -0700 [WPE][GTK] Ensure either ENABLE_JIT or ENABLE_C_LOOP is enabled(?) 2020-10-27 07:30:45 -0700 1 1 1 Unclassified WebKit WebKitGTK WebKit Nightly Build PC Linux RESOLVED WONTFIX P2 Normal --- 1 mcatanzaro mcatanzaro annulen bugs-noreply clopez ews-watchlist guijemont gyuyoung.kim mcatanzaro ryuan.choi sergio xan.lopez oldest_to_newest 1701582 0 mcatanzaro 2020-10-26 13:14:17 -0700 In bug #217989, I mentioned that Red Hat and SUSE accidentally shipped WebKit builds with both JIT and cloop disabled. i.e. JSC was using assembler-based llint interpreter rather than cloop. Carlos Lopez requested we add a check to prevent this from happening: (In reply to Carlos Alberto Lopez Perez from comment #12) > I think we should add a check in cmake to ensure that one (and only one) of > JIT or C_LOOP is enabled, so that we don't allow building with both options > disabled (or with both enabled) That makes sense to me. I would do this only in OptionsGTK.cmake and OptionsWPE.cmake, though, because I know Apple does not want that restriction for its ports, and we don't want to make transitioning to CMake any harder. But because ENABLE_C_LOOP and ENABLE_JIT are both PRIVATE settings, and WebKitFeatures.cmake normally ensures exactly one or the other is always enabled, checking to ensure it hasn't somehow happened probably makes sense for us. We could change that and remove this restriction if Igalia JSC developers ever want to do so, but that's the status quo currently, and enforcing it would avoid mistakes where we accidentally ship with a different JSC than everybody else, with our own special bugs. Counterargument: we don't support changing the values of PRIVATE settings, and this error will never happen if you don't do that. If bug #217989 is fixed, then distros will no longer have to mess with PRIVATE settings, so there is less value in adding that check. Alternative: maybe you really do know what you're doing and intentionally want to experiment with both JIT and cloop disabled? If so, then removing this check manually will be annoying. Anyway, I'll submit a patch, and we can see what people think. 1701596 1 mcatanzaro 2020-10-26 13:37:04 -0700 The more I think about this, the more I kinda don't want to do it. If you mess with private options, you should know what you're doing, right? And I imagine this configuration could be useful for JSC developers? Anyway, no strong opinion from me. Patch is incoming regardless. 1701597 2 412350 mcatanzaro 2020-10-26 13:39:30 -0700 Created attachment 412350 Patch 1701626 3 clopez 2020-10-26 14:27:21 -0700 (In reply to Michael Catanzaro from comment #0) > In bug #217989, I mentioned that Red Hat and SUSE accidentally shipped > WebKit builds with both JIT and cloop disabled. i.e. JSC was using > assembler-based llint interpreter rather than cloop. I'm curious.. Does JSC work fine with this combination (both JIT and C_LOOP disabled) ? I thought this was not supported, but maybe it is? 1701640 4 mcatanzaro 2020-10-26 14:43:34 -0700 (In reply to Carlos Alberto Lopez Perez from comment #3) > I'm curious.. > > Does JSC work fine with this combination (both JIT and C_LOOP disabled) ? I > thought this was not supported, but maybe it is? I think it works fine? It ought to work, but I'm not certain if it actually does. It's "not supported" at the GTK/WPE public build options level. It *is* supported at the JSC level. (JSC devs would know way more about this than me, but I'm pretty sure Apple ships this configuration on some device. Maybe watchOS?) That's why I kinda think we should not commit this. As long as we don't *have* to use private options to make WebKit work (as we currently must, bug #217989), then people probably won't use the private options unless they know what they're doing... hopefully. 1701777 5 xan.lopez 2020-10-26 23:29:51 -0700 Just to clarify, since I feel there could be a slight confusion here: CLOOP is another variant of LLInt. The C++, portable one, instead of the native implementations (x86_64, various flavors of ARM, MIPS, etc). Logically it sits at the same level than LLInt in this conversation. That being said... CLOOP and JIT disabled is a reasonable combination that is used by people in the real world, and is used when developing and debugging the engine itself (although in that case we typically disable tiers at runtime). It just means you only have the interpreter available and you are using the native implementations instead of CLOOP, the C++/portable one. CLOOP and JIT enabled, as far as I know, will not work, because there's an assumption in JSC about LLInt/baseline JIT sharing stack representations, among other things. In practice I also don't know who would bother making the baseline JIT work but leave LLInt with only the CLOOP variant. So it *could* make sense to prevent the previous combination to be enabled, but not sure this makes much sense. The way I see it distros should just get good defaults for their architectures, and not mess too much with these things because they are unlikely to get it right unless they have a pretty decent understanding of JSC, which seems overkill for a distributor. 1701809 6 guijemont 2020-10-27 03:23:09 -0700 (In reply to Xan Lopez from comment #5) > Just to clarify, since I feel there could be a slight confusion here: CLOOP > is another variant of LLInt. The C++, portable one, instead of the native > implementations (x86_64, various flavors of ARM, MIPS, etc). Logically it > sits at the same level than LLInt in this conversation. > > That being said... > > CLOOP and JIT disabled is a reasonable combination that is used by people in > the real world, and is used when developing and debugging the engine itself > (although in that case we typically disable tiers at runtime). It just means > you only have the interpreter available and you are using the native > implementations instead of CLOOP, the C++/portable one. To add to that point: this configuration (LLInt with asm backend, no JIT) is also likely to be the fastest combination available for environment where a JIT is not possible (e.g. due to a security policy not allowing JITs, I think that might be the case on the Playstation for instance). > > So it *could* make sense to prevent the previous combination to be enabled, > but not sure this makes much sense. The way I see it distros should just get > good defaults for their architectures, and not mess too much with these > things because they are unlikely to get it right unless they have a pretty > decent understanding of JSC, which seems overkill for a distributor. +1 to that. I am also thinking that this issue could be better addressed by providing better documentation on the subject, which hopefully would avoid confusion in the future. 1701814 7 412350 guijemont 2020-10-27 03:32:30 -0700 Comment on attachment 412350 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=412350&action=review Not a reviewer, but I disagree with this change, as it's useful to allow this configuration, as explained in other comments. > ChangeLog:13 > + different JSC than everybody else, with special codepaths and special bugs. There aren't much specific codepaths that asm LLInt+no JIT would take, compared to when the JIT is enabled. All the code goes through the LLInt before being JIT'ed, so this code path is well tested. Also, EWS and buildbots run tests with `--useJIT=false` (and LLInt with asm backend), which has very similar codepaths to the case you want to forbid. Are there specific bugs you encountered that happened only in this configuration? 1701861 8 mcatanzaro 2020-10-27 07:29:33 -0700 (In reply to Guillaume Emont from comment #7) > Not a reviewer, but I disagree with this change, as it's useful to allow > this configuration, as explained in other comments. OK, I'll close this then. (In reply to Guillaume Emont from comment #7) > Are there specific bugs you encountered that happened only in this > configuration? Nope. Either it works fine, or nobody has tested it and complained that it doesn't. ;) 1701862 9 mcatanzaro 2020-10-27 07:30:45 -0700 (In reply to Xan Lopez from comment #5) > The way I see it distros should just get > good defaults for their architectures, and not mess too much with these > things because they are unlikely to get it right unless they have a pretty > decent understanding of JSC, which seems overkill for a distributor. Currently we do the right thing for all architectures except aarch64, bug #217989. Any comment in that issue would be great. 412350 2020-10-26 13:39:30 -0700 2020-10-27 07:29:40 -0700 Patch bug-218201-20201026153932.patch text/plain 2444 mcatanzaro U3VidmVyc2lvbiBSZXZpc2lvbjogMjY4NzcyCmRpZmYgLS1naXQgYS9Tb3VyY2UvY21ha2UvT3B0 aW9uc0dUSy5jbWFrZSBiL1NvdXJjZS9jbWFrZS9PcHRpb25zR1RLLmNtYWtlCmluZGV4IGEwOWUy MTNiODdmMmIzNTI1ZDliYTFmN2I0NDczZWEwMjI5YjQ3NTguLjcxNjZkOWJmMTBkYzJhNzc0NWEz OTgwMDc0ZWU0MjNjMmYwODE3YWIgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9jbWFrZS9PcHRpb25zR1RL LmNtYWtlCisrKyBiL1NvdXJjZS9jbWFrZS9PcHRpb25zR1RLLmNtYWtlCkBAIC0zOTksNiArMzk5 LDEwIEBAIGlmIChFTkFCTEVfRU5DUllQVEVEX01FRElBIEFORCBFTkFCTEVfVEhVTkRFUikKICAg ZmluZF9wYWNrYWdlKFRodW5kZXIgUkVRVUlSRUQpCiBlbmRpZiAoKQogCitpZiAoTk9UIEVOQUJM RV9DX0xPT1AgQU5EIE5PVCBFTkFCTEVfSklUKQorICAgIG1lc3NhZ2UoRkFUQUxfRVJST1IgIkRp c2FibGluZyBib3RoIEpJVCBhbmQgY2xvb3AgaXMgbm90IHN1cHBvcnRlZCIpCitlbmRpZiAoKQor CiAjIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xODIyNDcKIGlmIChF TkFCTEVEX0NPTVBJTEVSX1NBTklUSVpFUlMpCiAgICAgc2V0KEVOQUJMRV9JTlRST1NQRUNUSU9O IE9GRikKZGlmZiAtLWdpdCBhL1NvdXJjZS9jbWFrZS9PcHRpb25zV1BFLmNtYWtlIGIvU291cmNl L2NtYWtlL09wdGlvbnNXUEUuY21ha2UKaW5kZXggMWVkMTM3YzQ5MjM2MGQ1ZDdjMmU0YzI0NTZj ZTY1OWNkMmEyYjc2Ni4uZWYyMTc4YmNiY2M5N2I0YTkzZWQ2ODJlZjU5YzczMmU1MTliZWU1YyAx MDA2NDQKLS0tIGEvU291cmNlL2NtYWtlL09wdGlvbnNXUEUuY21ha2UKKysrIGIvU291cmNlL2Nt YWtlL09wdGlvbnNXUEUuY21ha2UKQEAgLTIwMCw2ICsyMDAsMTAgQEAgaWYgKEVOQUJMRV9FTkNS WVBURURfTUVESUEgQU5EIEVOQUJMRV9USFVOREVSKQogICBmaW5kX3BhY2thZ2UoVGh1bmRlciBS RVFVSVJFRCkKIGVuZGlmICgpCiAKK2lmIChOT1QgRU5BQkxFX0NfTE9PUCBBTkQgTk9UIEVOQUJM RV9KSVQpCisgICAgbWVzc2FnZShGQVRBTF9FUlJPUiAiRGlzYWJsaW5nIGJvdGggSklUIGFuZCBj bG9vcCBpcyBub3Qgc3VwcG9ydGVkIikKK2VuZGlmICgpCisKIGFkZF9kZWZpbml0aW9ucygtREJV SUxESU5HX1dQRV9fPTEpCiBhZGRfZGVmaW5pdGlvbnMoLURHRVRURVhUX1BBQ0tBR0U9IldQRSIp CiBhZGRfZGVmaW5pdGlvbnMoLURKU0NfR0xJQl9BUElfRU5BQkxFRCkKZGlmZiAtLWdpdCBhL0No YW5nZUxvZyBiL0NoYW5nZUxvZwppbmRleCA5MGM2ZDE1YzE1NjUxYjUxNDI3MzgyZjk3NTVhM2Nj ODY3ZTNmMWM3Li5lYzUxZGIyZmJlZGFiYmViNTkwNzNlMWQ0MTEzM2Q1Yjk4NzVlYzZkIDEwMDY0 NAotLS0gYS9DaGFuZ2VMb2cKKysrIGIvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMjAgQEAKKzIwMjAt MTAtMjYgIE1pY2hhZWwgQ2F0YW56YXJvICA8bWNhdGFuemFyb0Bnbm9tZS5vcmc+CisKKyAgICAg ICAgW1dQRV1bR1RLXSBFbnN1cmUgZWl0aGVyIEVOQUJMRV9KSVQgb3IgRU5BQkxFX0NfTE9PUCBp cyBlbmFibGVkCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9p ZD0yMTgyMDEKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAg ICBCZWNhdXNlIEVOQUJMRV9DX0xPT1AgYW5kIEVOQUJMRV9KSVQgYXJlIGJvdGggUFJJVkFURSBz ZXR0aW5ncywgYW5kIFdlYktpdEZlYXR1cmVzLmNtYWtlCisgICAgICAgIG5vcm1hbGx5IGVuc3Vy ZXMgZXhhY3RseSBvbmUgb3IgdGhlIG90aGVyIGlzIGFsd2F5cyBlbmFibGVkLCBjaGVja2luZyB0 byBlbnN1cmUgaXQgaGFzbid0CisgICAgICAgIHNvbWVob3cgaGFwcGVuZWQgcHJvYmFibHkgbWFr ZXMgc2Vuc2UgZm9yIHVzLiBXZSBjb3VsZCBjaGFuZ2UgdGhhdCBhbmQgcmVtb3ZlIHRoaXMKKyAg ICAgICAgcmVzdHJpY3Rpb24gaWYgSWdhbGlhIEpTQyBkZXZlbG9wZXJzIGV2ZXIgd2FudCB0byBk byBzbywgYnV0IHRoYXQncyB0aGUgc3RhdHVzIHF1bworICAgICAgICBjdXJyZW50bHksIGFuZCBl bmZvcmNpbmcgaXQgd291bGQgYXZvaWQgbWlzdGFrZXMgd2hlcmUgZG93bnN0cmVhbSBhY2NpZGVu dGFsbHkgc2hpcHMgYQorICAgICAgICBkaWZmZXJlbnQgSlNDIHRoYW4gZXZlcnlib2R5IGVsc2Us IHdpdGggc3BlY2lhbCBjb2RlcGF0aHMgYW5kIHNwZWNpYWwgYnVncy4KKworICAgICAgICAqIFNv dXJjZS9jbWFrZS9PcHRpb25zR1RLLmNtYWtlOgorICAgICAgICAqIFNvdXJjZS9jbWFrZS9PcHRp b25zV1BFLmNtYWtlOgorCiAyMDIwLTEwLTIwICBNaWNoYWVsIENhdGFuemFybyAgPG1jYXRhbnph cm9AZ25vbWUub3JnPgogCiAgICAgICAgIFtHVEtdIE1vdmUgRU5BQkxFX0FTWU5DX1NDUk9MTElO RyBidWlsZCBvcHRpb24gdG8gcmlnaHQgcGxhY2UgaW4gT3B0aW9uc0dUSy5jbWFrZQo=