Re: Patch: Remove unneeded double operation in libstdc++-v3/src/c++17/fs_path.cc

Re: Patch: Remove unneeded double operation in libstdc++-v3/src/c++17/fs_path.cc

[Jonathan Wakely]

On 18/12/23 09:36 +0100, Martin Küttler wrote:
>This is a small change to libstdc++ which does not change any behavior.

Please CC the libstdc++@gcc.gnu.org list on all libstdc++ patches, as
documented at https://gcc.gnu.org/lists.html

Otherwise I won't see the patches unless I happen to glance at the
gcc-patches archive by chance.

>This change has two, ihmo positive, implications:
>
> - The implicit conversion from double to int is avoided (Avoiding a
>   warning).

I don't see any warning here. What do you see?

> - No floating point number is used at all, which could be significant
>   in some scenarios.

Yes, it seems worth doing for this reason. I'll test+push the patch,
thanks.

Looking at path::_List::reserve now, we probably also want to avoid
overflow. Although a path with INT_MAX/1.5 components seems
implausible for 32-bit and 64-bit targets, it could be a problem for
16-bit targets. I'll take care of that too.

>------------
>
>diff --git a/libstdc++-v3/src/c++17/fs_path.cc b/libstdc++-v3/src/c++17/fs_path.cc
>index d65b5482e8b..b47ed0aa7aa 100644
>--- a/libstdc++-v3/src/c++17/fs_path.cc
>+++ b/libstdc++-v3/src/c++17/fs_path.cc
>@@ -447,8 +447,9 @@ path::_List::reserve(int newcap, bool exact = false)
>
>   if (curcap < newcap)
>     {
>-      if (!exact && newcap < int(1.5 * curcap))
>-       newcap = 1.5 * curcap;
>+      const int nextcap = curcap + curcap / 2;
>+      if (!exact && newcap < nextcap)
>+       newcap = nextcap;
>
>       void* p = ::operator new(sizeof(_Impl) + newcap * sizeof(value_type));
>       std::unique_ptr<_Impl, _Impl_deleter> newptr(::new(p) _Impl{newcap});

Re: Patch: Remove unneeded double operation in libstdc++-v3/src/c++17/fs_path.cc

[Martin Küttler]

>>This is a small change to libstdc++ which does not change any behavior.
>
> Please CC the libstdc++@gcc.gnu.org list on all libstdc++ patches, as
> documented at https://gcc.gnu.org/lists.html

Acknowledged. Sorry.

>>This change has two, ihmo positive, implications:
>>
>> - The implicit conversion from double to int is avoided (Avoiding a
>>   warning).
>
> I don't see any warning here. What do you see?

I see "warning: conversion from ‘double’ to ‘int’ may change value
[-Wfloat-conversion]" This appears to be a specifically enabled warning.

> Looking at path::_List::reserve now, we probably also want to avoid
> overflow. Although a path with INT_MAX/1.5 components seems
> implausible for 32-bit and 64-bit targets, it could be a problem for
> 16-bit targets. I'll take care of that too.

Nice catch.

Martin

--
Kernkonzept GmbH at Dresden, Germany, HRB 31129, CEO Dr.-Ing. Michael Hohmuth

Re: Patch: Remove unneeded double operation in libstdc++-v3/src/c++17/fs_path.cc

[Jonathan Wakely]

On Fri, 5 Jan 2024 at 13:00, Martin Küttler
<martin.kuettler@kernkonzept.com> wrote:
>
>
> >>This is a small change to libstdc++ which does not change any behavior.
> >
> > Please CC the libstdc++@gcc.gnu.org list on all libstdc++ patches, as
> > documented at https://gcc.gnu.org/lists.html
>
> Acknowledged. Sorry.
>
> >>This change has two, ihmo positive, implications:
> >>
> >> - The implicit conversion from double to int is avoided (Avoiding a
> >>   warning).
> >
> > I don't see any warning here. What do you see?
>
> I see "warning: conversion from ‘double’ to ‘int’ may change value
> [-Wfloat-conversion]" This appears to be a specifically enabled warning.
>
> > Looking at path::_List::reserve now, we probably also want to avoid
> > overflow. Although a path with INT_MAX/1.5 components seems
> > implausible for 32-bit and 64-bit targets, it could be a problem for
> > 16-bit targets. I'll take care of that too.
>
> Nice catch.


We also have some redundant code in path::operator/= which can just be
removed, because _List::reserve does it anyway:

  if (orig_type == _Type::_Multi)
    {
      const int curcap = _M_cmpts._M_impl->capacity();
      if (capacity > curcap)
        capacity = std::max(capacity, (int) (curcap * 1.5));
    }

[committed] libstdc++: Avoid overflow when appending to std::filesystem::path

[Jonathan Wakely]

Tested x86_64-linux. Pushed to trunk.

-- >8 --

This prevents a std::filesystem::path from exceeding INT_MAX/4
components (which is unlikely to ever be a problem except on 16-bit
targets). That limit ensures that the capacity*1.5 calculation doesn't
overflow. We should also check that we don't exceed SIZE_MAX when
calculating how many bytes to allocate. That only needs to be checked
when int is at least as large as size_t, because otherwise we know that
the product INT_MAX/4 * sizeof(value_type) will fit in SIZE_MAX. For
targets where size_t is twice as wide as int this obviously holds. For
msp430-elf we have 16-bit int and 20-bit size_t, so the condition holds
as long as sizeof(value_type) fits in 7 bits, which it does.

We can also remove some floating-point arithmetic in operator/= which
ensures exponential growth of the buffer. That's redundant because
path::_List::reserve does that anyway (and does so more efficiently
since the commit immediately before this one).

libstdc++-v3/ChangeLog:

	* src/c++17/fs_path.cc (path::_List::reserve): Limit maximum
	size and check for overflows in arithmetic.
	(path::operator/=(const path&)): Remove redundant exponential
	growth calculation.
---
 libstdc++-v3/src/c++17/fs_path.cc | 35 +++++++++++++++++++++----------
 1 file changed, 24 insertions(+), 11 deletions(-)

diff --git a/libstdc++-v3/src/c++17/fs_path.cc b/libstdc++-v3/src/c++17/fs_path.cc
index a2d3c54a88a..d33b8d96663 100644
--- a/libstdc++-v3/src/c++17/fs_path.cc
+++ b/libstdc++-v3/src/c++17/fs_path.cc
@@ -35,6 +35,7 @@
 #include <algorithm>
 #include <array>
 #include <bits/stl_uninitialized.h>
+#include <ext/numeric_traits.h> // __gnu_cxx::__int_traits
 
 namespace fs = std::filesystem;
 using fs::path;
@@ -447,11 +448,30 @@ path::_List::reserve(int newcap, bool exact = false)
 
   if (curcap < newcap)
     {
-      const int nextcap = curcap + curcap / 2;
-      if (!exact && newcap < nextcap)
-	newcap = nextcap;
+      if (!exact)
+	{
+	  const int nextcap = curcap + curcap / 2;
+	  if (newcap < nextcap)
+	    newcap = nextcap;
+	}
 
-      void* p = ::operator new(sizeof(_Impl) + newcap * sizeof(value_type));
+      using __gnu_cxx::__int_traits;
+      // Nobody should need paths with this many components.
+      if (newcap >= __int_traits<int>::__max / 4)
+	std::__throw_bad_alloc();
+
+      size_t bytes;
+      if constexpr (__int_traits<int>::__max >= __int_traits<size_t>::__max)
+	{
+	  size_t components;
+	  if (__builtin_mul_overflow(newcap, sizeof(value_type), &components)
+		|| __builtin_add_overflow(sizeof(_Impl), components, &bytes))
+	    std::__throw_bad_alloc();
+	}
+      else // This won't overflow, even for 20-bit size_t on msp430.
+	bytes = sizeof(_Impl) + newcap * sizeof(value_type);
+
+      void* p = ::operator new(bytes);
       std::unique_ptr<_Impl, _Impl_deleter> newptr(::new(p) _Impl{newcap});
       const int cursize = curptr ? curptr->size() : 0;
       if (cursize)
@@ -588,13 +608,6 @@ path::operator/=(const path& __p)
     ++capacity; // Need to insert root-directory after root-name
 #endif
 
-  if (orig_type == _Type::_Multi)
-    {
-      const int curcap = _M_cmpts._M_impl->capacity();
-      if (capacity > curcap)
-	capacity = std::max(capacity, (int) (curcap * 1.5));
-    }
-
   _M_pathname.reserve(_M_pathname.length() + sep.length()
 		      + __p._M_pathname.length());
 
-- 
2.43.0