[Support] Remove incorrect noalias return attribute in BumpPtrAllocator

[nikic]

Summary

The memory returned by the Allocate() function is also otherwise accessible -- and is indeed accessed by the DestroyAll() method of SpecificBumpPtrAlloactor. This is a violation of the noalias return contract.

This should address the issue reported in https://reviews.llvm.org/D116728#3252464.

[reames]

There's a couple more occurrences of noalias in the same file. I think if we remove one, we need to remove all. Please also add a comment explaining that these are *not* noalias and why. (So we don't add the same bug back.)

Removing this is unfortunate just to allow destruction, but it does seem to be required with the current code structure. I think we could add Allocate wrappers on the standard bump pointer typedef (i.e turn it into a subclass) and preserve noalias there, but I'm fine landing the removal and then restructing if we see a regression.

[nikic]

Add comment.

[nikic]

In D117664#3254874, @reames wrote:

There's a couple more occurrences of noalias in the same file.

I don't see any other references apart from these two. We have another in MemAlloc.h, but that's unrelated (and legit).

Removing this is unfortunate just to allow destruction, but it does seem to be required with the current code structure. I think we could add Allocate wrappers on the standard bump pointer typedef (i.e turn it into a subclass) and preserve noalias there, but I'm fine landing the removal and then restructing if we see a regression.

Yeah, that might be possible. I think it wouldn't be strictly speaking correct, but likely wouldn't cause issues as long as SpecificBumpPtrAllocator is split out.

[reames]

LGTM

Sorry for the confusing comment on the extra call site. I was thinking of the second one in your original diff, but somehow didn't see it. Not enough coffee yet apparently. :)

[aganea]

There might more to it. Unfortunately I don't see any difference after the patch, either applied on rG3cef3cf02f09e397c471cf008060c89b34951959 or on ToT. The repro still don't emit the movs into the target buffer. Is anyone able to confirm?

[tschuett]

Maybe add a macro LLVM_ATTRIBUTE_RETURNS_ALIAS for documentation.

[nikic]

In D117664#3255143, @aganea wrote:

There might more to it. Unfortunately I don't see any difference after the patch, either applied on rG3cef3cf02f09e397c471cf008060c89b34951959 or on ToT. The repro still don't emit the movs into the target buffer. Is anyone able to confirm?

When I tested this I dropped the __attribute__((malloc)) from the preprocessed source, and that did it for me. Note that this patch needs to be in the source code for the stage2 lld build, not the stage1 clang build.

[reames]

This patch is a correctness fix on it's own and should land.

On the original regression, can I ask we file a bug and move discussion there? It's already fragmented across two reviews and getting increasingly hard to follow.

[aganea]

In D117664#3255376, @nikic wrote:

In D117664#3255143, @aganea wrote:

There might more to it. Unfortunately I don't see any difference after the patch, either applied on rG3cef3cf02f09e397c471cf008060c89b34951959 or on ToT. The repro still don't emit the movs into the target buffer. Is anyone able to confirm?

When I tested this I dropped the __attribute__((malloc)) from the preprocessed source, and that did it for me. Note that this patch needs to be in the source code for the stage2 lld build, not the stage1 clang build.

You're right, I don't know what I was thinking :-/ Apologies @nikic, it works now. Many thanks for your time!

llvm/include/llvm/Support/Allocator.h

void Reset() {
     End = CurPtr + SlabSize;
 
     __asan_poison_memory_region(*Slabs.begin(), computeSlabSize(0));
     DeallocateSlabs(std::next(Slabs.begin()), Slabs.end());
     Slabs.erase(std::next(Slabs.begin()), Slabs.end());
   }
 
   /// Allocate space at the specified alignment.
-  LLVM_ATTRIBUTE_RETURNS_NONNULL LLVM_ATTRIBUTE_RETURNS_NOALIAS void *
-  Allocate(size_t Size, Align Alignment) {
+  // This method is *not* marked noalias, because
+  // SpecificBumpPtrAllocator::DestroyAll() loops over all allocations, and
+  // that loop is not based on the Allocate() return value.
+  LLVM_ATTRIBUTE_RETURNS_NONNULL void *Allocate(size_t Size, Align Alignment) {
     // Keep track of how many bytes we've allocated.
     BytesAllocated += Size;
 
     size_t Adjustment = offsetToAlignedAddr(CurPtr, Alignment);
     assert(Adjustment + Size >= Size && "Adjustment + Size must not overflow");
 
     size_t SizeToAllocate = Size;
 #if LLVM_ADDRESS_SANITIZER_BUILD
assert(AlignedAddr + SizeToAllocate <= (uintptr_t)End &&
            "Unable to allocate memory!");
     char *AlignedPtr = (char*)AlignedAddr;
     CurPtr = AlignedPtr + SizeToAllocate;
     __msan_allocated_memory(AlignedPtr, Size);
     __asan_unpoison_memory_region(AlignedPtr, Size);
     return AlignedPtr;
   }
 
-  inline LLVM_ATTRIBUTE_RETURNS_NONNULL LLVM_ATTRIBUTE_RETURNS_NOALIAS void *
+  inline LLVM_ATTRIBUTE_RETURNS_NONNULL void *
   Allocate(size_t Size, size_t Alignment) {
     assert(Alignment > 0 && "0-byte alignment is not allowed. Use 1 instead.");
     return Allocate(Size, Align(Alignment));
   }
 
   // Pull in base class overloads.
   using AllocatorBase<BumpPtrAllocatorImpl>::Allocate;