226401 2021-05-28 15:45:10 -0700 Non-unified build fixes, late-ish May 2021 edition redux 2021-06-03 08:04:44 -0700 1 1 1 Unclassified WebKit Tools / Tests WebKit Local Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 226088 1 aperez aperez don.olmstead ews-watchlist keith_miller mark.lam msaboff saam tzagallo webkit-bug-importer oldest_to_newest 1764970 0 aperez 2021-05-28 15:45:10 -0700 ☢️ 1764974 1 430064 aperez 2021-05-28 15:47:18 -0700 Created attachment 430064 Patch 1765071 2 ews-feeder 2021-05-29 03:09:26 -0700 Committed r278237 (238274@main): <https://commits.webkit.org/238274@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 430064. 1765072 3 webkit-bug-importer 2021-05-29 03:10:17 -0700 <rdar://problem/78646851> 1765086 4 430064 mark.lam 2021-05-29 08:05:05 -0700 Comment on attachment 430064 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=430064&action=review > Source/JavaScriptCore/jit/JITSizeStatistics.h:31 > +#include "CCallHelpers.h" This is the wrong thing to do because this header file doesn't really depend on CCallHelpers.h. The right thing to do is to #include "CCallHelpers.h" in the files that actually uses JITSizeStatistics::markStart and markEnd. AFAICT, the only files that need it are JIT.cpp and DFGSpeculativeJIT.cpp, and those files already #include CCallHelpers.h indirectly. So, what is motivating this change? 1765202 5 aperez 2021-05-30 03:16:06 -0700 (In reply to Mark Lam from comment #4) > Comment on attachment 430064 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=430064&action=review > > > Source/JavaScriptCore/jit/JITSizeStatistics.h:31 > > +#include "CCallHelpers.h" > > This is the wrong thing to do because this header file doesn't really depend > on CCallHelpers.h. > > The right thing to do is to #include "CCallHelpers.h" in the files that > actually uses JITSizeStatistics::markStart and markEnd. AFAICT, the only > files that need it are JIT.cpp and DFGSpeculativeJIT.cpp, and those files > already #include CCallHelpers.h indirectly. So, what is motivating this > change? In JITSizeStatistics.h there is an use of the CCallHelpers::Label type, which is defined all the way up the class inheritance chain in the AbstractMacroAssembler<T>. I gave it a small spin at trying to figure out the correct “T” to use but it ended up needing to know the particular type for the current target architecture and anyway needed including headers that CCallHelpers.h itself is already including… in the end it seemed easier and much more readable to keep the CCallHelpers::Label usage as-is and include CCallHelper.h If anybody has a better suggestion on how to get a definition for the CCallHelpers::Label type I can try to give it a try. 1766160 6 mark.lam 2021-06-02 14:25:11 -0700 (In reply to Adrian Perez from comment #5) > (In reply to Mark Lam from comment #4) > > Comment on attachment 430064 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=430064&action=review > > > > > Source/JavaScriptCore/jit/JITSizeStatistics.h:31 > > > +#include "CCallHelpers.h" > > > > This is the wrong thing to do because this header file doesn't really depend > > on CCallHelpers.h. > > > > The right thing to do is to #include "CCallHelpers.h" in the files that > > actually uses JITSizeStatistics::markStart and markEnd. AFAICT, the only > > files that need it are JIT.cpp and DFGSpeculativeJIT.cpp, and those files > > already #include CCallHelpers.h indirectly. So, what is motivating this > > change? > > In JITSizeStatistics.h there is an use of the CCallHelpers::Label > type, which is defined all the way up the class inheritance chain > in the AbstractMacroAssembler<T>. I gave it a small spin at trying > to figure out the correct “T” to use but it ended up needing to know > the particular type for the current target architecture and anyway > needed including headers that CCallHelpers.h itself is already > including… in the end it seemed easier and much more readable to > keep the CCallHelpers::Label usage as-is and include CCallHelper.h > > If anybody has a better suggestion on how to get a definition for > the CCallHelpers::Label type I can try to give it a try. I stand corrected. That's unfortunate. Thanks for explaining. We could have just used MacroAssembler::Label, but then, we'd still be #include'ing MacroAssembler.h. Given this, it's not worth thinking more about this issue. Your fix is correct. Sorry for the noise. 1766388 7 aperez 2021-06-03 08:04:44 -0700 (In reply to Mark Lam from comment #6) > (In reply to Adrian Perez from comment #5) > > (In reply to Mark Lam from comment #4) > > > Comment on attachment 430064 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=430064&action=review > > > > > > > Source/JavaScriptCore/jit/JITSizeStatistics.h:31 > > > > +#include "CCallHelpers.h" > > > > > > This is the wrong thing to do because this header file doesn't really depend > > > on CCallHelpers.h. > > > > > > The right thing to do is to #include "CCallHelpers.h" in the files that > > > actually uses JITSizeStatistics::markStart and markEnd. AFAICT, the only > > > files that need it are JIT.cpp and DFGSpeculativeJIT.cpp, and those files > > > already #include CCallHelpers.h indirectly. So, what is motivating this > > > change? > > > > In JITSizeStatistics.h there is an use of the CCallHelpers::Label > > type, which is defined all the way up the class inheritance chain > > in the AbstractMacroAssembler<T>. I gave it a small spin at trying > > to figure out the correct “T” to use but it ended up needing to know > > the particular type for the current target architecture and anyway > > needed including headers that CCallHelpers.h itself is already > > including… in the end it seemed easier and much more readable to > > keep the CCallHelpers::Label usage as-is and include CCallHelper.h > > > > If anybody has a better suggestion on how to get a definition for > > the CCallHelpers::Label type I can try to give it a try. > > I stand corrected. That's unfortunate. Thanks for explaining. We could > have just used MacroAssembler::Label, but then, we'd still be #include'ing > MacroAssembler.h. Given this, it's not worth thinking more about this > issue. Your fix is correct. Sorry for the noise. No problem at all, that's what we are here for—making WebKit as good as we can :) 430064 2021-05-28 15:47:18 -0700 2021-05-29 03:09:27 -0700 Patch bug-226401-20210529014717.patch text/plain 1950 aperez U3VidmVyc2lvbiBSZXZpc2lvbjogMjc4MjE3CmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCBj NGYzZjg5ZjRlZmViNWY5ZTZkM2EzMTUwNzdmNWYyY2NjZGI5MDk0Li4xYjFjMzZiNTE2ZjE1NTFi MzY3Mjc3YWNlYmYxODE1MDQ3YjJlOGJjIDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwxNCBAQAorMjAyMS0wNS0yOCAgQWRyaWFuIFBlcmV6IGRlIENhc3RybyAgPGFwZXJlekBp Z2FsaWEuY29tPgorCisgICAgICAgIE5vbi11bmlmaWVkIGJ1aWxkIGZpeGVzLCBsYXRlLWlzaCBN YXkgMjAyMSBlZGl0aW9uIHJlZHV4CisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3No b3dfYnVnLmNnaT9pZD0yMjY0MDEKKworICAgICAgICBVbnJldmlld2VkIG5vbi11bmlmaWVkIGJ1 aWxkIGZpeGVzLgorCisgICAgICAgICogaml0L0pJVFNpemVTdGF0aXN0aWNzLmNwcDogQWRkIG1p c3NpbmcgTGlua0J1ZmZlci5oIGhlYWRlci4KKyAgICAgICAgKiBqaXQvSklUU2l6ZVN0YXRpc3Rp Y3MuaDogQWRkIG1pc3NpbmcgQ0NhbGxIZWxwZXJzLmggYW5kIHd0Zi90ZXh0L1dURlN0cmluZy5o IGhlYWRlcnM7CisgICAgICAgIHJlbW92ZSB1bm5lZWRlZCBDQ2FsbEhlbHBlcnMgZm9yd2FyZCBk ZWNsYXJhdGlvbi4KKwogMjAyMS0wNS0yOCAgU2FhbSBCYXJhdGkgIDxzYmFyYXRpQGFwcGxlLmNv bT4KIAogICAgICAgICBBZGQgdGhlIGFiaWxpdHkgdG8gZHVtcCBzdGF0aXN0aWNzIGFib3V0IGN1 bXVsYXRpdmUgY291bnRzIGFuZCBjb2RlIHNpemVzIG9mIEJhc2VsaW5lIEpJVCBvcGNvZGVzIGFu ZCBERkcgbm9kZXMKZGlmZiAtLWdpdCBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9qaXQvSklUU2l6 ZVN0YXRpc3RpY3MuY3BwIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL2ppdC9KSVRTaXplU3RhdGlz dGljcy5jcHAKaW5kZXggOGVmMmM5MTYzMWVhMjcxOWNiMzM5MTkxZjExMTE5ZGUyYjJkMjc4YS4u OWFmOWY2ZDcyNmUxNmY0YTZlNjM0OTE5ZjllNWJjNzIwMTZjMTQ0ZSAxMDA2NDQKLS0tIGEvU291 cmNlL0phdmFTY3JpcHRDb3JlL2ppdC9KSVRTaXplU3RhdGlzdGljcy5jcHAKKysrIGIvU291cmNl L0phdmFTY3JpcHRDb3JlL2ppdC9KSVRTaXplU3RhdGlzdGljcy5jcHAKQEAgLTI5LDYgKzI5LDcg QEAKICNpZiBFTkFCTEUoSklUKQogCiAjaW5jbHVkZSAiQ0NhbGxIZWxwZXJzLmgiCisjaW5jbHVk ZSAiTGlua0J1ZmZlci5oIgogI2luY2x1ZGUgPHd0Zi9CdWJibGVTb3J0Lmg+CiAKIG5hbWVzcGFj ZSBKU0MgewpkaWZmIC0tZ2l0IGEvU291cmNlL0phdmFTY3JpcHRDb3JlL2ppdC9KSVRTaXplU3Rh dGlzdGljcy5oIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL2ppdC9KSVRTaXplU3RhdGlzdGljcy5o CmluZGV4IGI1OGY0ZmY2YTg1YTMzOGQxZDYyYTY0YjIyNjg2YTc3NzhmMzVlYTIuLmM3NGIyYzQz OTZlZmYxMzRmZWRjYjQzNDNhZDUxZWI4MTFiODI0MjcgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9KYXZh U2NyaXB0Q29yZS9qaXQvSklUU2l6ZVN0YXRpc3RpY3MuaAorKysgYi9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvaml0L0pJVFNpemVTdGF0aXN0aWNzLmgKQEAgLTI4LDEzICsyOCwxMyBAQAogCiAjaWYg RU5BQkxFKEpJVCkKIAorI2luY2x1ZGUgIkNDYWxsSGVscGVycy5oIgogI2luY2x1ZGUgPHd0Zi9I YXNoTWFwLmg+CiAjaW5jbHVkZSA8d3RmL1ByaW50U3RyZWFtLmg+CisjaW5jbHVkZSA8d3RmL3Rl eHQvV1RGU3RyaW5nLmg+CiAKIG5hbWVzcGFjZSBKU0MgewogCi1jbGFzcyBDQ2FsbEhlbHBlcnM7 Ci0KIGNsYXNzIEpJVFNpemVTdGF0aXN0aWNzIHsKICAgICBXVEZfTUFLRV9GQVNUX0FMTE9DQVRF RDsKIHB1YmxpYzoK