[libcxx-commits] [PATCH] D119628: [libc++] Don't return alloc_size - 1 in __recommend to add 1 when allocating

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added a comment.

In D119628#3356907 <https://reviews.llvm.org/D119628#3356907>, @philnik wrote:

> In D119628#3348916 <https://reviews.llvm.org/D119628#3348916>, @ldionne wrote:
>
>> it's possible that already compiled code has the old definition of e.g. `__init` inlined, and hence the capacity of the string is set to `N + 1`. If such a string is passed to code compiled with the new definition, the new code would think that the capacity represents `N`, not `N + 1`, and would most likely mis-behave. In other words, I think this is ABI breaking.
>
> I'm not changing what is saved in long cap. That has always been the capacity including the null terminator AFAICT. Or is there another potential ABI break that I'm not seeing?

I believe what Louis is getting at is that any time you change the //meaning// of a function's arguments (where the function is called across an ABI boundary), that can be an ABI break. For example, if `f` is calling `arctan(a, b)`, and then the maintainer changes the prototype of `arctan` from `arctan(int x, int y)` to `arctan(int y, int x)`, that's an ABI break. Or if `f` is calling `__set_long_cap(n+1)`, and then the maintainer changes the prototype of `__set_long_cap` from `__set_long_cap(int nplus1)` to `__set_long_cap(int n)`, that's an ABI break. //But// in this case I've double-checked that both `__set_long_cap` and `__recommend` are hide-from-abi, thus never called across an ABI boundary, so I //think// that's not a concern here; and if we were really really paranoid, we could even change the names of some of these helper functions.



================
Comment at: libcxx/include/string:3240-3242
+    if (__target_capacity == capacity() + 1) return;
 
+    __shrink_or_extend(__target_capacity - 1);
----------------
philnik wrote:
> ldionne wrote:
> > Quuxplusone wrote:
> > > This diff strikes me as questionable (which probably means the entire patch is questionable). The logic here previously was, "If the target capacity is equal to our current capacity, nothing to do, return. Otherwise, shrink or extend to the target capacity." //Now// the logic is, "If the target capacity is one greater than our current capacity, do nothing! Otherwise, shrink or extend to one //less// than the target capacity." Regardless of whether the arithmetic works out in practice, that logic seems very wrong. Maybe it can be saved by renaming some variables, e.g. maybe `__target_capacity` doesn't actually represent a capacity? Or maybe prior to line 3239 it's a capacity but after line 3239 it's a one-greater-than-a-capacity?
> > I agree, this looks fishy to me. Are you certain this wasn't a bug previously and that it should instead be `if (__target_capacity == capacity()) return;` *even* in the new diff? Do we have a test for that?
> IIUC `basic_string::capacity()` returns the number of characters without the null terminator while `__recommend()` (with this patch) returns the number of characters including the null terminator.
FWIW, I would bet that this is not a "real bug"; I would bet the machine code is correct. But I feel moderately strongly that we shouldn't ship //source// code that looks this confusing.

It is also //possible// that there is a bug in this vicinity after all. See https://llvm.org/pr51816 . 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119628