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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne 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?

Ahhhhhh, that's actually what I missed. I saw the change from `__set_long_cap(__cap+1);` to `__set_long_cap(__cap);`, and I went "hmm this changes the **meaning** of the capacity held inside the string, so that's an ABI break". But in reality, it doesn't, because `__recommend` returns 1 more than it used to after applying your patch. So in both cases we are setting the capacity to the same value. Please double-check my reasoning, and if we're on the same page, then thanks for correcting me -- I don't think there's any ABI break in this patch.



================
Comment at: libcxx/include/string:3240-3242
+    if (__target_capacity == capacity() + 1) return;
 
+    __shrink_or_extend(__target_capacity - 1);
----------------
Quuxplusone wrote:
> 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 . 
Let me reformulate my ask: @philnik (or someone else), can you please explain why this is correct:

```
if (__target_capacity == capacity() + 1)
  return;
```

It seems to me that it should instead be

```
if (__target_capacity == capacity())
  return;
```

I suspect I'll be much happier shipping this patch once I understand it (and it will probably mandate a comment since it's not trivial to understand).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119628