198096 2019-05-21 17:10:43 -0700 WHLSL: Parsing negative int literals parses the positive value instead 2019-05-21 20:06:18 -0700 1 1 1 Unclassified WebKit WebGPU WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 saam saam commit-queue dino fpizlo jonlee justin_fan mmaxfield rmorisset webkit-bug-importer oldest_to_newest 1538061 0 saam 2019-05-21 17:10:43 -0700 ... 1538079 1 370365 saam 2019-05-21 18:25:18 -0700 Created attachment 370365 patch 1538080 2 370365 mmaxfield 2019-05-21 18:32:21 -0700 Comment on attachment 370365 patch View in context: https://bugs.webkit.org/attachment.cgi?id=370365&action=review > Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:233 > + int64_t intResult = -static_cast<int64_t>(result); Don't we still need a static assert, though? C++ doesn't guarantee int64_t is wider than int. 1538082 3 370365 saam 2019-05-21 18:35:49 -0700 Comment on attachment 370365 patch View in context: https://bugs.webkit.org/attachment.cgi?id=370365&action=review >> Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:233 >> + int64_t intResult = -static_cast<int64_t>(result); > > Don't we still need a static assert, though? C++ doesn't guarantee int64_t is wider than int. Few reasons I removed it: 1. This is not the only place relying on sizeof(int) == 4. All of WK relies on this. It's not super helpful to just have it here. 2. We could also take this further. I don't believe sizeof(unsigned) is guaranteed to be 4. So let's say sizeof(unsigned) == 8, then the static_cast from unsigned to int64_t is also broken. I could add another static_assert, but the same as above: lots of things in WK would be broken if sizeof(unsigned) != 4 1538083 4 saam 2019-05-21 18:36:40 -0700 (In reply to Saam Barati from comment #3) > Comment on attachment 370365 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=370365&action=review > > >> Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:233 > >> + int64_t intResult = -static_cast<int64_t>(result); > > > > Don't we still need a static assert, though? C++ doesn't guarantee int64_t is wider than int. > > Few reasons I removed it: > 1. This is not the only place relying on sizeof(int) == 4. All of WK relies > on this. It's not super helpful to just have it here. > 2. We could also take this further. I don't believe sizeof(unsigned) is > guaranteed to be 4. So let's say sizeof(unsigned) == 8, then the static_cast > from unsigned to int64_t is also broken. I could add another static_assert, > but the same as above: lots of things in WK would be broken if > sizeof(unsigned) != 4 The nicer thing could be to switch to uint32_t/int32_t/int64_t everywhere. 1538088 5 mmaxfield 2019-05-21 18:48:53 -0700 > > Few reasons I removed it: > > 1. This is not the only place relying on sizeof(int) == 4. All of WK relies > > on this. Don't we want this code to be easily separable from WebKit? 1538089 6 saam 2019-05-21 18:58:16 -0700 (In reply to Myles C. Maxfield from comment #5) > > > Few reasons I removed it: > > > 1. This is not the only place relying on sizeof(int) == 4. All of WK relies > > > on this. > Don't we want this code to be easily separable from WebKit? Not sure. I’ll defer to you on that. Anyways, this isn’t a super interesting thing to worry about. It sounds like we should use int32_t since that is probably what’s specified by WHLSL. (And also use uint32_t/int64_t, etc, in surrounding code.) 1538096 7 saam 2019-05-21 19:17:58 -0700 (In reply to Saam Barati from comment #6) > (In reply to Myles C. Maxfield from comment #5) > > > > Few reasons I removed it: > > > > 1. This is not the only place relying on sizeof(int) == 4. All of WK relies > > > > on this. > > Don't we want this code to be easily separable from WebKit? > > Not sure. I’ll defer to you on that. > > Anyways, this isn’t a super interesting thing to worry about. It sounds like > we should use int32_t since that is probably what’s specified by WHLSL. (And > also use uint32_t/int64_t, etc, in surrounding code.) This is somewhat annoying to do. I'll just add back a similar static_assert. Anyways, we should consider using types that specify width in the areas that's specified by the specification. This isn't really that interesting because no one is going to compile C programs with sizeof(int)/sizeof(unsigned) not equal to 4. 1538097 8 370372 saam 2019-05-21 19:18:32 -0700 Created attachment 370372 patch for landing 1538105 9 370372 commit-queue 2019-05-21 20:05:41 -0700 Comment on attachment 370372 patch for landing Clearing flags on attachment: 370372 Committed r245612: <https://trac.webkit.org/changeset/245612> 1538106 10 commit-queue 2019-05-21 20:05:43 -0700 All reviewed patches have been landed. Closing bug. 1538107 11 webkit-bug-importer 2019-05-21 20:06:18 -0700 <rdar://problem/51012964> 370365 2019-05-21 18:25:18 -0700 2019-05-21 19:18:32 -0700 patch c-backup.diff text/plain 2166 saam SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDI0NTYwOCkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE4IEBACisyMDE5LTA1LTIxICBTYWFtIGJh cmF0aSAgPHNiYXJhdGlAYXBwbGUuY29tPgorCisgICAgICAgIFdITFNMOiBQYXJzaW5nIG5lZ2F0 aXZlIGludCBsaXRlcmFscyBwYXJzZXMgdGhlIHBvc2l0aXZlIHZhbHVlIGluc3RlYWQKKyAgICAg ICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE5ODA5NgorCisgICAg ICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIEkgYWxzbyBtYWRlIHRo ZSBjb2RlIGFyb3VuZCA8IElOVF9NSU4gYSBiaXQgZWFzaWVyIHRvIGZvbGxvdyBhbG9uZyB3aXRo LgorCisgICAgICAgIE5vIG5ldyB0ZXN0cyBiZWNhdXNlIHdlIGhhdmVuJ3QgaW1wb3J0ZWQgV0hM U0wgdGVzdCBzdWl0ZSB5ZXQuCisgICAgICAgIFZlcmlmaWVkIHRoaXMgd29ya3MgdXNpbmcgdGhl IEFTVCBkdW1wZXIuCisKKyAgICAgICAgKiBNb2R1bGVzL3dlYmdwdS9XSExTTC9XSExTTFBhcnNl ci5jcHA6CisgICAgICAgIChXZWJDb3JlOjpXSExTTDo6aW50TGl0ZXJhbFRvSW50KToKKwogMjAx OS0wNS0yMSAgTXlsZXMgQy4gTWF4ZmllbGQgIDxtbWF4ZmllbGRAYXBwbGUuY29tPgogCiAgICAg ICAgIGZvbnQtb3B0aWNhbC1zaXppbmcgYXBwbGllcyB0aGUgd3JvbmcgdmFyaWF0aW9uIHZhbHVl CkluZGV4OiBTb3VyY2UvV2ViQ29yZS9Nb2R1bGVzL3dlYmdwdS9XSExTTC9XSExTTFBhcnNlci5j cHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PQotLS0gU291cmNlL1dlYkNvcmUvTW9kdWxlcy93ZWJncHUvV0hMU0wvV0hM U0xQYXJzZXIuY3BwCShyZXZpc2lvbiAyNDU2MDgpCisrKyBTb3VyY2UvV2ViQ29yZS9Nb2R1bGVz L3dlYmdwdS9XSExTTC9XSExTTFBhcnNlci5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTIzMCwxMCAr MjMwLDEwIEBAIHN0YXRpYyBFeHBlY3RlZDxpbnQsIFBhcnNlcjo6RXJyb3I+IGludEwKICAgICAg ICAgICAgIHJldHVybiBVbmV4cGVjdGVkPFBhcnNlcjo6RXJyb3I+KFBhcnNlcjo6RXJyb3IobWFr ZVN0cmluZygiaW50IGxpdGVyYWwgIiwgdGV4dCwgIiBpcyBvdXQgb2YgYm91bmRzIikpKTsKICAg ICB9CiAgICAgaWYgKG5lZ2F0ZSkgewotICAgICAgICBzdGF0aWNfYXNzZXJ0KHN0ZDo6bnVtZXJp Y19saW1pdHM8bG9uZyBsb25nIGludD46Om1pbigpIDwgc3RkOjpudW1lcmljX2xpbWl0czxpbnQ+ OjptaW4oKSwgImxvbmcgbG9uZyBuZWVkcyB0byBiZSBiaWdnZXIgdGhhbiBhbiBpbnQiKTsKLSAg ICAgICAgaWYgKHN0YXRpY19jYXN0PGxvbmcgbG9uZz4ocmVzdWx0KSA+IHN0ZDo6YWJzKHN0YXRp Y19jYXN0PGxvbmcgbG9uZz4oc3RkOjpudW1lcmljX2xpbWl0czxpbnQ+OjptaW4oKSkpKQorICAg ICAgICBpbnQ2NF90IGludFJlc3VsdCA9IC1zdGF0aWNfY2FzdDxpbnQ2NF90PihyZXN1bHQpOwor ICAgICAgICBpZiAoaW50UmVzdWx0IDwgc3RhdGljX2Nhc3Q8aW50NjRfdD4oc3RkOjpudW1lcmlj X2xpbWl0czxpbnQ+OjptaW4oKSkpCiAgICAgICAgICAgICByZXR1cm4gVW5leHBlY3RlZDxQYXJz ZXI6OkVycm9yPihQYXJzZXI6OkVycm9yKG1ha2VTdHJpbmcoImludCBsaXRlcmFsICIsIHRleHQs ICIgaXMgb3V0IG9mIGJvdW5kcyIpKSk7Ci0gICAgICAgIHJldHVybiB7IHN0YXRpY19jYXN0PGlu dD4oc3RhdGljX2Nhc3Q8bG9uZyBsb25nPihyZXN1bHQpICogMSkgfTsKKyAgICAgICAgcmV0dXJu IHsgc3RhdGljX2Nhc3Q8aW50PihpbnRSZXN1bHQpIH07CiAgICAgfQogICAgIGlmIChyZXN1bHQg PiBzdGF0aWNfY2FzdDx1bnNpZ25lZD4oc3RkOjpudW1lcmljX2xpbWl0czxpbnQ+OjptYXgoKSkp CiAgICAgICAgIHJldHVybiBVbmV4cGVjdGVkPFBhcnNlcjo6RXJyb3I+KFBhcnNlcjo6RXJyb3Io bWFrZVN0cmluZygiaW50IGxpdGVyYWwgIiwgdGV4dCwgIiBpcyBvdXQgb2YgYm91bmRzIikpKTsK 370372 2019-05-21 19:18:32 -0700 2019-05-21 20:05:41 -0700 patch for landing c-backup.diff text/plain 2295 saam SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDI0NTYwOCkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE4IEBACisyMDE5LTA1LTIxICBTYWFtIGJh cmF0aSAgPHNiYXJhdGlAYXBwbGUuY29tPgorCisgICAgICAgIFdITFNMOiBQYXJzaW5nIG5lZ2F0 aXZlIGludCBsaXRlcmFscyBwYXJzZXMgdGhlIHBvc2l0aXZlIHZhbHVlIGluc3RlYWQKKyAgICAg ICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE5ODA5NgorCisgICAg ICAgIFJldmlld2VkIGJ5IERlYW4gSmFja3Nvbi4KKworICAgICAgICBJIGFsc28gbWFkZSB0aGUg Y29kZSBhcm91bmQgPCBJTlRfTUlOIGEgYml0IGVhc2llciB0byBmb2xsb3cgYWxvbmcgd2l0aC4K KworICAgICAgICBObyBuZXcgdGVzdHMgYmVjYXVzZSB3ZSBoYXZlbid0IGltcG9ydGVkIFdITFNM IHRlc3Qgc3VpdGUgeWV0LgorICAgICAgICBWZXJpZmllZCB0aGlzIHdvcmtzIHVzaW5nIHRoZSBB U1QgZHVtcGVyLgorCisgICAgICAgICogTW9kdWxlcy93ZWJncHUvV0hMU0wvV0hMU0xQYXJzZXIu Y3BwOgorICAgICAgICAoV2ViQ29yZTo6V0hMU0w6OmludExpdGVyYWxUb0ludCk6CisKIDIwMTkt MDUtMjEgIE15bGVzIEMuIE1heGZpZWxkICA8bW1heGZpZWxkQGFwcGxlLmNvbT4KIAogICAgICAg ICBmb250LW9wdGljYWwtc2l6aW5nIGFwcGxpZXMgdGhlIHdyb25nIHZhcmlhdGlvbiB2YWx1ZQpJ bmRleDogU291cmNlL1dlYkNvcmUvTW9kdWxlcy93ZWJncHUvV0hMU0wvV0hMU0xQYXJzZXIuY3Bw Cj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT0KLS0tIFNvdXJjZS9XZWJDb3JlL01vZHVsZXMvd2ViZ3B1L1dITFNML1dITFNM UGFyc2VyLmNwcAkocmV2aXNpb24gMjQ1NjA4KQorKysgU291cmNlL1dlYkNvcmUvTW9kdWxlcy93 ZWJncHUvV0hMU0wvV0hMU0xQYXJzZXIuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC0yMzAsMTAgKzIz MCwxMSBAQCBzdGF0aWMgRXhwZWN0ZWQ8aW50LCBQYXJzZXI6OkVycm9yPiBpbnRMCiAgICAgICAg ICAgICByZXR1cm4gVW5leHBlY3RlZDxQYXJzZXI6OkVycm9yPihQYXJzZXI6OkVycm9yKG1ha2VT dHJpbmcoImludCBsaXRlcmFsICIsIHRleHQsICIgaXMgb3V0IG9mIGJvdW5kcyIpKSk7CiAgICAg fQogICAgIGlmIChuZWdhdGUpIHsKLSAgICAgICAgc3RhdGljX2Fzc2VydChzdGQ6Om51bWVyaWNf bGltaXRzPGxvbmcgbG9uZyBpbnQ+OjptaW4oKSA8IHN0ZDo6bnVtZXJpY19saW1pdHM8aW50Pjo6 bWluKCksICJsb25nIGxvbmcgbmVlZHMgdG8gYmUgYmlnZ2VyIHRoYW4gYW4gaW50Iik7Ci0gICAg ICAgIGlmIChzdGF0aWNfY2FzdDxsb25nIGxvbmc+KHJlc3VsdCkgPiBzdGQ6OmFicyhzdGF0aWNf Y2FzdDxsb25nIGxvbmc+KHN0ZDo6bnVtZXJpY19saW1pdHM8aW50Pjo6bWluKCkpKSkKKyAgICAg ICAgc3RhdGljX2Fzc2VydChzaXplb2YoaW50NjRfdCkgPiBzaXplb2YodW5zaWduZWQpICYmIHNp emVvZihpbnQ2NF90KSA+IHNpemVvZihpbnQpLCAiVGhpcyBjb2RlIHdvdWxkIGJlIHdyb25nIG90 aGVyd2lzZSIpOworICAgICAgICBpbnQ2NF90IGludFJlc3VsdCA9IC1zdGF0aWNfY2FzdDxpbnQ2 NF90PihyZXN1bHQpOworICAgICAgICBpZiAoaW50UmVzdWx0IDwgc3RhdGljX2Nhc3Q8aW50NjRf dD4oc3RkOjpudW1lcmljX2xpbWl0czxpbnQ+OjptaW4oKSkpCiAgICAgICAgICAgICByZXR1cm4g VW5leHBlY3RlZDxQYXJzZXI6OkVycm9yPihQYXJzZXI6OkVycm9yKG1ha2VTdHJpbmcoImludCBs aXRlcmFsICIsIHRleHQsICIgaXMgb3V0IG9mIGJvdW5kcyIpKSk7Ci0gICAgICAgIHJldHVybiB7 IHN0YXRpY19jYXN0PGludD4oc3RhdGljX2Nhc3Q8bG9uZyBsb25nPihyZXN1bHQpICogMSkgfTsK KyAgICAgICAgcmV0dXJuIHsgc3RhdGljX2Nhc3Q8aW50PihpbnRSZXN1bHQpIH07CiAgICAgfQog ICAgIGlmIChyZXN1bHQgPiBzdGF0aWNfY2FzdDx1bnNpZ25lZD4oc3RkOjpudW1lcmljX2xpbWl0 czxpbnQ+OjptYXgoKSkpCiAgICAgICAgIHJldHVybiBVbmV4cGVjdGVkPFBhcnNlcjo6RXJyb3I+ KFBhcnNlcjo6RXJyb3IobWFrZVN0cmluZygiaW50IGxpdGVyYWwgIiwgdGV4dCwgIiBpcyBvdXQg b2YgYm91bmRzIikpKTsK