Fix alignment in data section allocator.

• Component: JavaScriptCore
• Status: NEW ---

[Peter Molnar]

Fix alignment in data section allocator.

[Peter Molnar]

Created attachment 220741 [details]
patch

[Filip Pizlo]

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.

[Peter Molnar]

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?

[Filip Pizlo]

(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.

[Peter Molnar]

(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?

[Filip Pizlo]

(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