[all-commits] [llvm/llvm-project] 1653d1: Use `llvm::SmallVector` instead of `OwningArrayRef...

[David Stone via All-commits]

  Branch: refs/heads/users/davidstone/remove-owning-array-ref
  Home:   https://github.com/llvm/llvm-project
  Commit: 1653d14099cd722c00e575a4b5a9a9673f20e813
      https://github.com/llvm/llvm-project/commit/1653d14099cd722c00e575a4b5a9a9673f20e813
  Author: David Stone <davidfromonline at gmail.com>
  Date:   2025-11-21 (Fri, 21 Nov 2025)

  Changed paths:
    M clang/include/clang/AST/VTableBuilder.h
    M clang/lib/AST/VTableBuilder.cpp

  Log Message:
  -----------
  Use `llvm::SmallVector` instead of `OwningArrayRef` in `VTableLayout`.

This simplifies the code by removing the manual optimization for size == 1, and also gives us an optimization for other small sizes.

Accept a `llvm::SmallVector` by value for the constructor and move it into the destination, rather than accepting `ArrayRef` that we copy from. This also lets us not have to construct a reference to the elements of a `std::initializer_list`, which requires reading the implementation of the constructor to know whether it's safe.

Also explicitly document that the constructor requires the input indexes to have a size of at least 1.


  Commit: c3bc565c959ee690db386241d2c16d808b961faf
      https://github.com/llvm/llvm-project/commit/c3bc565c959ee690db386241d2c16d808b961faf
  Author: David Stone <davidfromonline at gmail.com>
  Date:   2025-11-21 (Fri, 21 Nov 2025)

  Changed paths:
    M llvm/include/llvm/DebugInfo/BTF/BTFParser.h
    M llvm/lib/DebugInfo/BTF/BTFParser.cpp

  Log Message:
  -----------
  [llvm] Replace `OwningArrayRef` with `std::vector` in `BTFParser`

`OwningArrayRef` requires that the size and the capacity are the same. This prevents reusing memory allocations unless the size happens to be exactly the same (which is rare enough we don't even try). Switch to `std::vector` instead so that we're not repeatedly calling `new[]` and `delete[]`.


  Commit: 01164dbcc1dcd5b8a1f09af225ed5582fbb0f3cb
      https://github.com/llvm/llvm-project/commit/01164dbcc1dcd5b8a1f09af225ed5582fbb0f3cb
  Author: David Stone <davidfromonline at gmail.com>
  Date:   2025-11-21 (Fri, 21 Nov 2025)

  Changed paths:
    M clang/include/clang/AST/VTableBuilder.h
    M clang/include/clang/Basic/LLVM.h
    M llvm/include/llvm/ADT/ArrayRef.h
    M llvm/include/llvm/CGData/CGDataPatchItem.h
    M llvm/include/llvm/CodeGen/PBQP/Math.h
    M llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
    M llvm/unittests/ADT/ArrayRefTest.cpp

  Log Message:
  -----------
  [llvm][clang] Remove `llvm::OwningArrayRef`

`OwningArrayRef` has several problems.

The naming is strange: `ArrayRef` is specifically a non-owning view, so the name means "owning non-owning view".

It has a const-correctness bug that is inherent to the interface. `OwningArrayRef<T>` publicly derives from `MutableArrayRef<T>`. This means that the following code compiles:

```c++
void const_incorrect(llvm::OwningArrayRef<int> const a) {
	a[0] = 5;
}
```

It's surprising for a non-reference type to allow modification of its elements even when it's declared `const`. However, the problems from this inheritance (which ultimately stem from the same issue as the weird name) are even worse. The following function compiles without warning but corrupts memory when called:

```c++
void memory_corruption(llvm::OwningArrayRef<int> a) {
	a.consume_front();
}
```

This happens because `MutableArrayRef::consume_front` modifies the internal data pointer to advance the referenced array forward. That's not an issue for `MutableArrayRef` because it's just a view. It is an issue for `OwningArrayRef` because that pointer is passed as the argument to `delete[]`, so when it's modified by advancing it forward it ceases to be valid to `delete[]`. From there, undefined behavior occurs.

It is mostly less convenient than `std::vector` for construction. By combining the `size` and the `capacity` together without going through `std::allocator` to get memory, it's not possible to fill in data with the correct value to begin with. Instead, the user must construct an `OwningArrayRef` of the appropriate size, then fill in the data. This has one of two consequences:

1. If `T` is a class type, we have to first default construct all of the elements when we construct `OwningArrayRef` and then in a second pass we can assign to those elements to give what we want. This wastes time and for some classes is not possible.
2. If `T` is a built-in type, the data starts out uninitialized. This easily forgotten step means we access uninitialized memory.

Using `std::vector`, by constrast, has well-known constructors that can fill in the data that we actually want on construction.

`OwningArrayRef` has slightly different performance characteristics than `std::vector`, but the difference is minimal.

The first difference is a theoretical negative for `OwningArrayRef`: by implementing in terms of `new[]` and `delete[]`, the implementation has less room to optimize these calls. However, I say this is theoretical because for clang, at least, the extra freedom of optimization given to `std::allocator` is not yet taken advantage of (see https://github.com/llvm/llvm-project/issues/68365)

The second difference is slightly in favor of `OwningArrayRef`: `sizeof(std::vector<T>) == sizeof(void *) 3` on pretty much any implementation, whereas `sizeof(OwningArrayRef) == sizeof(void *) * 2` which seems like a win. However, this is just a misdirection of the accounting costs: array-new sticks bookkeeping information in the allocated storage. There are some cases where this is beneficial to reduce stack usage, but that minor benefit doesn't seem worth the costs. If we actually need that optimization, we'd be better served by writing a `DynamicArray` type that implements a full vector-like feature set (except for operations that change the size of the container) while allocating through `std::allocator` to avoid the pitfalls outlined earlier.


Compare: https://github.com/llvm/llvm-project/compare/1653d14099cd%5E...01164dbcc1dc

To unsubscribe from these emails, change your notification settings at https://github.com/llvm/llvm-project/settings/notifications