126703 2014-01-09 08:34:15 -0800 Fix alignment in data section allocator. 2014-01-13 08:15:30 -0800 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) Unspecified Unspecified NEW P2 Normal --- 1 pmolnar.u-szeged webkit-unassigned fpizlo oldest_to_newest 965916 0 pmolnar.u-szeged 2014-01-09 08:34:15 -0800 Fix alignment in data section allocator. 965917 1 220741 pmolnar.u-szeged 2014-01-09 08:36:01 -0800 Created attachment 220741 patch 965937 2 fpizlo 2014-01-09 09:13:10 -0800 This doesn't actually fix any bugs. Your new code still fails to align allocations to the requested alignment except if that alignment is LSectionWord. You so removed the assertion to that effect so this patch is just a pure regression. 966384 3 pmolnar.u-szeged 2014-01-10 08:56:22 -0800 I got into this one when trying to allocate with alignment of 16. In this case, both the old, and the suggested new version allocates 2 LSectionWords, so in this case the code works OK, but the assertion is triggered. But, suppose we wanted to allocate 15 or 17 with alignment of 16. In case of 15, both calculations yield 2 LSectionWords, which is good, but in case of 17, the old one yields 3 LSectionWords, which is not aligned to 16, but the suggested new calculation yields 4 LSectionWords which is the correct version I think. It can't see why the new code "fails to align allocations to the requested alignment except if that alignment is LSectionWord". Can you please explain this, or give an example? 966754 4 fpizlo 2014-01-10 22:25:48 -0800 (In reply to comment #3) > I got into this one when trying to allocate with alignment of 16. In this case, both the old, and the suggested new version allocates 2 LSectionWords, so in this case the code works OK, but the assertion is triggered. Right, because there is no guarantee that the returned pointer is actually 16 byte aligned. That's what the assertion is asserting. It has nothing to do with allocation size. > > But, suppose we wanted to allocate 15 or 17 with alignment of 16. > > In case of 15, both calculations yield 2 LSectionWords, which is good, but in case of 17, the old one yields 3 LSectionWords, which is not aligned to 16, but the suggested new calculation yields 4 LSectionWords which is the correct version I think. Nobody castes if you allocate the right number of section words. The relevant part is the alignment of the returned pointer. > > It can't see why the new code "fails to align allocations to the requested alignment except if that alignment is LSectionWord". Can you please explain this, or give an example? Do you understand how memory allocation works? The OS allocator, or the language allocator like C++ "new", will provide a hard guarantee that the returned pointer is aligned to some machine word amount. We conservatively assume that it's sizeof(LSectionWord). The OS allocator isn't going to magically align allocations to the thing that LLVM wanted via the "alignment" parameter. We just asset that LLVM will never ask for alignment greater than sizeof(LSectionWord), which is true on Darwin. 967314 5 pmolnar.u-szeged 2014-01-13 07:52:52 -0800 (In reply to comment #4) > (In reply to comment #3) > > I got into this one when trying to allocate with alignment of 16. In this case, both the old, and the suggested new version allocates 2 LSectionWords, so in this case the code works OK, but the assertion is triggered. > > Right, because there is no guarantee that the returned pointer is actually 16 byte aligned. That's what the assertion is asserting. It has nothing to do with allocation size. > > > > > But, suppose we wanted to allocate 15 or 17 with alignment of 16. > > > > In case of 15, both calculations yield 2 LSectionWords, which is good, but in case of 17, the old one yields 3 LSectionWords, which is not aligned to 16, but the suggested new calculation yields 4 LSectionWords which is the correct version I think. > > Nobody castes if you allocate the right number of section words. The relevant part is the alignment of the returned pointer. > > > > > It can't see why the new code "fails to align allocations to the requested alignment except if that alignment is LSectionWord". Can you please explain this, or give an example? > > Do you understand how memory allocation works? The OS allocator, or the language allocator like C++ "new", will provide a hard guarantee that the returned pointer is aligned to some machine word amount. We conservatively assume that it's sizeof(LSectionWord). The OS allocator isn't going to magically align allocations to the thing that LLVM wanted via the "alignment" parameter. We just asset that LLVM will never ask for alignment greater than sizeof(LSectionWord), which is true on Darwin. Ok, I'm getting your point now. It seems, on Linux, LLVM does ask for alignment greater than sizeof(LSectionWord), 16 bytes actually, when sizeof(LSectionWord) is 8 bytes, so in this case we can't have that assumption that we can have on Darwin. Do you have any idea, how this could be handled to get in to work on Linux, without affecting Darwin? 967321 6 fpizlo 2014-01-13 08:15:30 -0800 (In reply to comment #5) > (In reply to comment #4) > > (In reply to comment #3) > > > I got into this one when trying to allocate with alignment of 16. In this case, both the old, and the suggested new version allocates 2 LSectionWords, so in this case the code works OK, but the assertion is triggered. > > > > Right, because there is no guarantee that the returned pointer is actually 16 byte aligned. That's what the assertion is asserting. It has nothing to do with allocation size. > > > > > > > > But, suppose we wanted to allocate 15 or 17 with alignment of 16. > > > > > > In case of 15, both calculations yield 2 LSectionWords, which is good, but in case of 17, the old one yields 3 LSectionWords, which is not aligned to 16, but the suggested new calculation yields 4 LSectionWords which is the correct version I think. > > > > Nobody castes if you allocate the right number of section words. The relevant part is the alignment of the returned pointer. > > > > > > > > It can't see why the new code "fails to align allocations to the requested alignment except if that alignment is LSectionWord". Can you please explain this, or give an example? > > > > Do you understand how memory allocation works? The OS allocator, or the language allocator like C++ "new", will provide a hard guarantee that the returned pointer is aligned to some machine word amount. We conservatively assume that it's sizeof(LSectionWord). The OS allocator isn't going to magically align allocations to the thing that LLVM wanted via the "alignment" parameter. We just asset that LLVM will never ask for alignment greater than sizeof(LSectionWord), which is true on Darwin. > > Ok, I'm getting your point now. It seems, on Linux, LLVM does ask for alignment greater than sizeof(LSectionWord), 16 bytes actually, when sizeof(LSectionWord) is 8 bytes, so in this case we can't have that assumption that we can have on Darwin. > Do you have any idea, how this could be handled to get in to work on Linux, without affecting Darwin? man memalign 220741 2014-01-09 08:36:01 -0800 2014-01-09 09:12:06 -0800 patch ftl-allocator.patch text/plain 1662 pmolnar.u-szeged ZGlmZiAtLWdpdCBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cgYi9Tb3VyY2UvSmF2 YVNjcmlwdENvcmUvQ2hhbmdlTG9nCmluZGV4IDM1ZmU4OTguLmQ5MzU2YTAgMTAwNjQ0Ci0tLSBh L1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL0phdmFTY3JpcHRD b3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE2IEBACisyMDE0LTAxLTA5ICBQZXRlciBNb2xuYXIg IDxwbW9sbmFyLnUtc3plZ2VkQHBhcnRuZXIuc2Ftc3VuZy5jb20+CisKKyAgICAgICAgW0ZUTF0g Rml4IGFsaWdubWVudCBpbiBkYXRhIHNlY3Rpb24gYWxsb2NhdG9yLgorICAgICAgICBodHRwczov L2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTI2NzAzCisKKyAgICAgICAgUmV2aWV3 ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgKiBmdGwvRlRMQ29tcGlsZS5jcHA6Cisg ICAgICAgIChKU0M6OkZUTDo6bW1BbGxvY2F0ZURhdGFTZWN0aW9uKToKKyAgICAgICAgVGhlIGNh bGN1bGF0aW9uIG9mIHRoZSBzZWN0aW9uIHNpemUgb25seSB3b3JrZWQgcHJvcGVybHkgaWYgYWxp Z25tZW50IHdhcyBsZXNzIHRoYW4gc2l6ZW9mKExTZWN0aW9uV29yZCkuCisgICAgICAgIElmIHRo ZSBhbGlnbm1lbnQgaXMgZm9yIGV4YW1wbGUgMTYsIHRoZSBhbGxvY2F0aW9uIHdhcyB3cm9uZywg dGhpcyBjYXNlIHdhcyBwcm90ZWN0ZWQgYnkgdGhlIFJFTEVBU0VfQVNTRVJULgorICAgICAgICBU aGUgZml4ZWQgY2FsY3VsYXRpb24gd29ya3MgZm9yIGFueSBhbGlnbm1lbnQsIHRoZXJlZm9yZSB0 aGUgUkVMRUFTRV9BU1NFUlQgY2FuIGFsc28gYmUgcmVtb3ZlZC4KKwogMjAxNC0wMS0wOCAgTWFy ayBIYWhuZW5iZXJnICA8bWhhaG5lbmJlcmdAYXBwbGUuY29tPgogCiAgICAgICAgIFJldmVydGlu ZyBhY2NpZGVudGFsIEdDIGxvZ2dpbmcKZGlmZiAtLWdpdCBhL1NvdXJjZS9KYXZhU2NyaXB0Q29y ZS9mdGwvRlRMQ29tcGlsZS5jcHAgYi9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvZnRsL0ZUTENvbXBp bGUuY3BwCmluZGV4IDZjMDFmNGYuLjQyM2E3YTkgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9KYXZhU2Ny aXB0Q29yZS9mdGwvRlRMQ29tcGlsZS5jcHAKKysrIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL2Z0 bC9GVExDb21waWxlLmNwcApAQCAtNzMsMTAgKzczLDcgQEAgc3RhdGljIHVpbnQ4X3QqIG1tQWxs b2NhdGVEYXRhU2VjdGlvbigKIAogICAgIFN0YXRlJiBzdGF0ZSA9ICpzdGF0aWNfY2FzdDxTdGF0 ZSo+KG9wYXF1ZVN0YXRlKTsKICAgICAKLSAgICBSRUxFQVNFX0FTU0VSVChhbGlnbm1lbnQgPD0g c2l6ZW9mKExTZWN0aW9uV29yZCkpOwotICAgIAotICAgIFJlZkNvdW50ZWRBcnJheTxMU2VjdGlv bldvcmQ+IHNlY3Rpb24oCi0gICAgICAgIChzaXplICsgc2l6ZW9mKExTZWN0aW9uV29yZCkgLSAx KSAvIHNpemVvZihMU2VjdGlvbldvcmQpKTsKKyAgICBSZWZDb3VudGVkQXJyYXk8TFNlY3Rpb25X b3JkPiBzZWN0aW9uKFdURjo6cm91bmRVcFRvTXVsdGlwbGVPZihhbGlnbm1lbnQsIHNpemUpIC8g c2l6ZW9mKExTZWN0aW9uV29yZCkpOwogICAgIAogICAgIGlmICghc3RyY21wKHNlY3Rpb25OYW1l LCAiX19sbHZtX3N0YWNrbWFwcyIpKQogICAgICAgICBzdGF0ZS5zdGFja21hcHNTZWN0aW9uID0g c2VjdGlvbjsK