[PATCH] D51317: Handle BumpPtrAllocator::Allocate(0) without undefined behavior

Brent Royal-Gordon via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Sep 6 13:01:32 PDT 2018


brentdax added a comment.

(Some emails I sent don't seem to have gotten through.)

In https://reviews.llvm.org/D51317#1222631, @hans wrote:

> In https://reviews.llvm.org/D51317#1221692, @brentdax wrote:
>
> > @hans So do you think it's okay to go at this point, or do you want more benchmarking?
>
>
> I'd like to see some kind of benchmark to verify this doesn't have negative performance impact.


This is a little rough because it uses time(1), but here's twenty compiles of oggenc.c (1.7 MB) at -O3 on macOS with as much extraneous stuff closed as possible:

Without change:

  103.81 real       101.95 user         1.48 sys

With change:

  101.31 real        99.55 user         1.46 sys

Additional runs under less controlled conditions (e.g. a web browser open in the background) didn't necessarily show the changed version being faster, but they were always quite close. Test runs the other day with bzip2.c (cited in the original commit) also looked pretty close, but it compiled so fast that I had a tough time getting solid numbers. I had trouble getting gcc.c to compile on macOS.

In https://reviews.llvm.org/D51317#1222695, @chandlerc wrote:

> FWIW, just because a lot of code hits the assert for non-zero size does not (to me) mean that isn't the correct approach.
>
> I actually think that the API is better if allocating zero size is disallowed. I think making clients think about how it makes sense in *their* case to handle this degenerate case is better than trying to do something inside the allocator. I think we'll constantly make one set of users of the API or the other unhappy: either we allocate when we shouldn't to avoid returning a nullptr, or we make clients deal with a potentially-null return.


I understand that perspective, but I'm not really sure there's much "dealing" for clients to do. With the patch in place, the allocator returns null only if it allocates zero bytes, meaning there's no space reserved for that pointer and it should never be dereferenced anyway. The best way to handle a zero-byte allocation is probably to leave null whatever variable you would have otherwise set to point to the buffer—and that's exactly what would happen with this patch. So I don't think the null solution will make a set of users unhappy unless they're being pedantic.

(And as a practical matter, as a first-time contributor I'm just not sure what steps I should take before making a breaking change of that magnitude. I assume you'd want to give Clang and other projects a heads-up, but I have no idea who should be involved.)


Repository:
  rL LLVM

https://reviews.llvm.org/D51317





More information about the llvm-commits mailing list