[PATCH] D59269: ELF: Use bump pointer allocator for uncompressed section buffers. NFCI.

[Rui Ueyama via Phabricator via llvm-commits]

ruiu accepted this revision.
ruiu added a comment.
This revision is now accepted and ready to land.

In D59269#1426696 <https://reviews.llvm.org/D59269#1426696>, @pcc wrote:

> In D59269#1426677 <https://reviews.llvm.org/D59269#1426677>, @ruiu wrote:
>
> > I thought that we could protect `BumpPtrAllocator::Allocate` with a mutex, but since StringSaver directly calls that function, there's no way to acquire a lock before calling that function. Maybe we should protect `StringSaver::save`?
>
>
> We could, I suppose? But then that code would be effectively dead since we don't save any strings on threads other than the main thread right now. I'm more inclined to let TSan find any mistakes involving `BumpPtrAllocator` here.
>
> > Or, we could also make BAlloc and StringSaver thread-local variables. That might be a better approach.
>
> I'm not sure if that will work. Imagine that we spin up a thread that decompresses a section and then terminates. When the thread terminates, the allocator is destroyed, so we are left with a dangling pointer in the section object.
>
> We could work around that by having the thread local variables be pointers to objects allocated on the heap, and arrange for them to be destroyed later in `lld::freeArena`. But now we're back to the original problem of thread-safely adding a "thing to deallocate later" to a list.


Hmm, you are right. My approach won't work. The other thing I can think of is to make BumpPtrAllocator thread-safe and mostly lock-free using compare-and-exchange. That doesn't seem technically not too hard, but that's probably too much. I think I'm fine with your original approach.

LGTM


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59269/new/

https://reviews.llvm.org/D59269