5 ms·

I'm not sure that's sound. You can't just redirect an aligned new to the unaligned operator new as you may get unaligned result. It _sounds_ like what is happen

by olliej 4y ago

I'm not sure that's sound. You can't just redirect an aligned new to the unaligned operator new as you may get unaligned result. It _sounds_ like what is happening is a = ::operator new(some size, some alignment) ... ::operator delete(a); where delete is dropping the align_val_t parameter that would guarantee it hits the same allocator family. There are a variety of ways this can happen, and let's just take it as given that it is. The problem is that if operator new(size_t, align_val_t) is called then the struct has an alignment annotation. That can lead to codegen that reasonably assumes alignment, even without any source level decisions that depend on alignment. The result of having some equivalent of (either at runtime or link time) void * operator new(size_t sz, align_val_t a) { if (operator new(size_t) has been overridden) return ::operator new(sz); ... } could be an "aligned" allocation returning an unaligned value, causing crashes later on.

jenadine 4y ago
That's not sound in general, but it is "probably" going to work for this specific case because the previous build was build with allocator that did not support this alignment, meaning that they did not need extra alignment. This is pretty rare actually. And you had anyway to use a custom allocator already with previous C++ versions to make it work.
- olliej 4y ago
  While I do agree with you, and think it's probably worth seeing if detecting the override and falling back to unaligned allocation works, the problem is not that the code in TF is compiling assuming/requiring over aligned data. The problem is that there is system code that they are calling that is making using of over aligned allocation, so therefore could be generating code dependent on said alignment. The failure mode can very easily be someSystemLibrary.so`someFunction: alignedThing = ::operator new(size, alignment) ... i_dunno_dma_memcpy_or_something(a, somewhere else) ... ::operator delete(a) With no interaction with TF code at all. Except TF has replaced operator delete so that fails due to the allocator mismatch. If you make ::opeator new(size_t, align_val_t) redirect to ::operator new(size_t) if it detects an override then the aligned operation can fail. The above example is moderately difficult to induce so it's more likely that there's an explicit split with the system is doing one half of new/delete and TF is doing the other, but the important thing is that it implies the system code is built aware of alignment and it depends on the alignment even if TF does not.
viraptor 4y ago
If you don't mind wasting a bit of time, you could forward size+alignment to the allocator, return the aligned version and keep a record of aligned-to-allocation mapping. (For freeing later) But as the other comment mentioned - it should be a problem for tf2 in the first place since that's not the behaviour they're after.
- olliej 4y ago
  > If you don't mind wasting a bit of time, you could forward size+alignment to the allocator, return the aligned version and keep a record of aligned-to-allocation mapping. (For freeing later) I'm unsure what you're proposing here - the only methods you know in the replacement allocator are operator new(size_t) and operator delete(void). The two possible failure paths are: a = ::operator new(some size) ... ::operator delete(a, alignment) and a = ::operator new(some size, some alignment) ... ::operator delete(a) In the first case what you could do is say "if I did not allocator this pointer, optimistically forward it to operator delete(void)", in the latter case you can identify that a different operator new(size_t) exists but you have no idea how to make that allocator produce an aligned allocation. What I guess you could do is round the size up to a multiple of the specified alignment, and then just repeatedly allocate in the hope that you will eventually get a correctly aligned value out. But that would not be guaranteed.
  - viraptor 4y ago
    > and then just repeatedly allocate in the hope that you will eventually get a correctly aligned value out If you preload something that patches all the new/delete interfaces, you can do this without guesswork. new(size, alignment) -> res=alloc(size+alignment) res_aligned=res+... offsets[res_aligned] = res new(size) -> alloc(size) free(ptr) -> free(offsets[ptr] || ptr) offsets.del(ptr)
    - olliej 4y ago
      Haha, you've missed the issue. The question is what does the system do when someone overrides the builtin allocator functions, but does not override all of them. You are absolutely correct that as a developer you can have your process override the allocator functions, and that is in fact what TF has done. The problem is that they have not overridden all of the allocation functions, and so they're crashing due to mismatching allocators being used. TF2 can "easily" fix this crash by implementing the aligned new, new[], delete, and delete[] operators in their custom allocator, or by simply removing their custom allocator's override of the global new & delete operators and using a common base class to get their faster allocator. The question we're talking about is "how does the standard library respond to this scenario in a way that maximizes correctness?".
Asooka 4y ago
The C interface for aligned memory allocation is aligned_alloc(). The returned pointers are always freed with free(). So what is probably happening is that aligned new calls aligned_alloc(), and then aligned delete simply calls the regular delete, expecting to end up in free(), which by design should work with both kinds of pointers. I think the problem here is partly with the implementation of aligned new/delete. Since one is free to override only the old versions, the ones supplied by the standard library should make sure not to fall back to functions that may be partially overriden.
amluto 4y ago
As pure speculation, one could forward to aligned_alloc and still free with ::delete. I haven’t tested this, nor have I looked at the code.

by olliej 4y ago

jenadine 4y ago

That's not sound in general, but it is "probably" going to work for this specific case because the previous build was build with allocator that did not support this alignment, meaning that they did not need extra alignment. This is pretty rare actually. And you had anyway to use a custom allocator already with previous C++ versions to make it work.

olliej 4y ago
While I do agree with you, and think it's probably worth seeing if detecting the override and falling back to unaligned allocation works, the problem is not that the code in TF is compiling assuming/requiring over aligned data. The problem is that there is system code that they are calling that is making using of over aligned allocation, so therefore could be generating code dependent on said alignment. The failure mode can very easily be someSystemLibrary.so`someFunction: alignedThing = ::operator new(size, alignment) ... i_dunno_dma_memcpy_or_something(a, somewhere else) ... ::operator delete(a) With no interaction with TF code at all. Except TF has replaced operator delete so that fails due to the allocator mismatch. If you make ::opeator new(size_t, align_val_t) redirect to ::operator new(size_t) if it detects an override then the aligned operation can fail. The above example is moderately difficult to induce so it's more likely that there's an explicit split with the system is doing one half of new/delete and TF is doing the other, but the important thing is that it implies the system code is built aware of alignment and it depends on the alignment even if TF does not.

viraptor 4y ago

If you don't mind wasting a bit of time, you could forward size+alignment to the allocator, return the aligned version and keep a record of aligned-to-allocation mapping. (For freeing later) But as the other comment mentioned - it should be a problem for tf2 in the first place since that's not the behaviour they're after.

olliej 4y ago
> If you don't mind wasting a bit of time, you could forward size+alignment to the allocator, return the aligned version and keep a record of aligned-to-allocation mapping. (For freeing later) I'm unsure what you're proposing here - the only methods you know in the replacement allocator are operator new(size_t) and operator delete(void). The two possible failure paths are: a = ::operator new(some size) ... ::operator delete(a, alignment) and a = ::operator new(some size, some alignment) ... ::operator delete(a) In the first case what you could do is say "if I did not allocator this pointer, optimistically forward it to operator delete(void)", in the latter case you can identify that a different operator new(size_t) exists but you have no idea how to make that allocator produce an aligned allocation. What I guess you could do is round the size up to a multiple of the specified alignment, and then just repeatedly allocate in the hope that you will eventually get a correctly aligned value out. But that would not be guaranteed.
- viraptor 4y ago
  > and then just repeatedly allocate in the hope that you will eventually get a correctly aligned value out If you preload something that patches all the new/delete interfaces, you can do this without guesswork. new(size, alignment) -> res=alloc(size+alignment) res_aligned=res+... offsets[res_aligned] = res new(size) -> alloc(size) free(ptr) -> free(offsets[ptr] || ptr) offsets.del(ptr)
  - olliej 4y ago
    Haha, you've missed the issue. The question is what does the system do when someone overrides the builtin allocator functions, but does not override all of them. You are absolutely correct that as a developer you can have your process override the allocator functions, and that is in fact what TF has done. The problem is that they have not overridden all of the allocation functions, and so they're crashing due to mismatching allocators being used. TF2 can "easily" fix this crash by implementing the aligned new, new[], delete, and delete[] operators in their custom allocator, or by simply removing their custom allocator's override of the global new & delete operators and using a common base class to get their faster allocator. The question we're talking about is "how does the standard library respond to this scenario in a way that maximizes correctness?".

Asooka 4y ago

The C interface for aligned memory allocation is aligned_alloc(). The returned pointers are always freed with free(). So what is probably happening is that aligned new calls aligned_alloc(), and then aligned delete simply calls the regular delete, expecting to end up in free(), which by design should work with both kinds of pointers. I think the problem here is partly with the implementation of aligned new/delete. Since one is free to override only the old versions, the ones supplied by the standard library should make sure not to fall back to functions that may be partially overriden.

amluto 4y ago

As pure speculation, one could forward to aligned_alloc and still free with ::delete. I haven’t tested this, nor have I looked at the code.