* [devel] [PATCH for apt 1/2] Add option for debugging DynamicMMap::Allocate
@ 2019-12-06 13:16 Aleksei Nikiforov
2019-12-06 13:16 ` [devel] [PATCH for apt 2/2] Fix pointer arithmetics Aleksei Nikiforov
2019-12-08 22:50 ` [devel] [PATCH for apt 1/2] Add option for debugging DynamicMMap::Allocate Dmitry V. Levin
0 siblings, 2 replies; 34+ messages in thread
From: Aleksei Nikiforov @ 2019-12-06 13:16 UTC (permalink / raw)
To: devel; +Cc: Aleksei Nikiforov
---
apt/apt-pkg/contrib/mmap.cc | 15 +++++++++++++++
apt/doc/apt.conf.5.sgml | 2 +-
2 files changed, 16 insertions(+), 1 deletion(-)
diff --git a/apt/apt-pkg/contrib/mmap.cc b/apt/apt-pkg/contrib/mmap.cc
index a3b06cc..cf01be9 100644
--- a/apt/apt-pkg/contrib/mmap.cc
+++ b/apt/apt-pkg/contrib/mmap.cc
@@ -265,6 +265,21 @@ std::experimental::optional<map_ptrloc> DynamicMMap::Allocate(unsigned long Item
// No pool is allocated, use an unallocated one
if (I == Pools + PoolCount)
{
+ static const bool debug_grow = _config->FindB("Debug::DynamicMMap::Allocate", false);
+
+ if (debug_grow)
+ {
+ Pool *pool_iter = Pools;
+ size_t pool_idx = 0;
+
+ _error->Warning(_("DynamicMMap::Allocate: allocating item of size %lu"), ItemSize);
+
+ for (; pool_idx < PoolCount; ++pool_iter, ++pool_idx)
+ {
+ _error->Warning(_("DynamicMMap::Allocate: Pool %zu, item size: %lu, start: %lu, count: %lu"), pool_idx, pool_iter->ItemSize, pool_iter->Start, pool_iter->Count);
+ }
+ }
+
// Woops, we ran out, the calling code should allocate more.
if (Empty == 0)
{
diff --git a/apt/doc/apt.conf.5.sgml b/apt/doc/apt.conf.5.sgml
index 0a72e45..72fc0c3 100644
--- a/apt/doc/apt.conf.5.sgml
+++ b/apt/doc/apt.conf.5.sgml
@@ -526,7 +526,7 @@ DPkg::Pre-Install-Pkgs {"/usr/sbin/dpkg-preconfigure --apt";};
disable the inclusion of statfs data in CDROM IDs.
</para><para>
To debug issues related to dynamic memory allocation, an option
- <literal/Debug::DynamicMMap::Grow/ may be used.
+ <literal/Debug::DynamicMMap::Grow/ and <literal/Debug::DynamicMMap::Allocate/ may be used.
</para>
</RefSect1>
--
2.24.0
^ permalink raw reply [flat|nested] 34+ messages in thread
* [devel] [PATCH for apt 2/2] Fix pointer arithmetics
2019-12-06 13:16 [devel] [PATCH for apt 1/2] Add option for debugging DynamicMMap::Allocate Aleksei Nikiforov
@ 2019-12-06 13:16 ` Aleksei Nikiforov
2019-12-06 13:36 ` Ivan A. Melnikov
` (2 more replies)
2019-12-08 22:50 ` [devel] [PATCH for apt 1/2] Add option for debugging DynamicMMap::Allocate Dmitry V. Levin
1 sibling, 3 replies; 34+ messages in thread
From: Aleksei Nikiforov @ 2019-12-06 13:16 UTC (permalink / raw)
To: devel; +Cc: Aleksei Nikiforov
This change should fix pointer arithmetic issues for e2k.
---
apt/apt-pkg/Makefile.am | 1 +
apt/apt-pkg/cacheiterators.h | 14 ++++++++------
apt/apt-pkg/contrib/mmap.cc | 8 +++++---
apt/apt-pkg/pkgcachegen.cc | 15 ++++++++-------
apt/apt-pkg/rebase_pointer.h | 16 ++++++++++++++++
apt/apt-pkg/rpm/rpmlistparser.cc | 3 ++-
6 files changed, 40 insertions(+), 17 deletions(-)
create mode 100644 apt/apt-pkg/rebase_pointer.h
diff --git a/apt/apt-pkg/Makefile.am b/apt/apt-pkg/Makefile.am
index 4c0d234..d038d01 100644
--- a/apt/apt-pkg/Makefile.am
+++ b/apt/apt-pkg/Makefile.am
@@ -94,6 +94,7 @@ libapt_pkg_la_SOURCES = \
pkgsystem.h \
policy.cc \
policy.h \
+ rebase_pointer.h \
repository.cc \
repository.h \
scopeexit.h \
diff --git a/apt/apt-pkg/cacheiterators.h b/apt/apt-pkg/cacheiterators.h
index a4bf670..51f70c1 100644
--- a/apt/apt-pkg/cacheiterators.h
+++ b/apt/apt-pkg/cacheiterators.h
@@ -34,6 +34,8 @@
#pragma interface "apt-pkg/cacheiterators.h"
#endif
+#include <apt-pkg/rebase_pointer.h>
+
#include <set>
// Package Iterator
@@ -89,7 +91,7 @@ class pkgCache::PkgIterator
{
if (Owner == 0 || Pkg == 0)
return;
- Pkg += static_cast<Package const *>(newMap) - static_cast<Package const *>(oldMap);
+ Pkg = RebasePointer(Pkg, oldMap, newMap);
}
// Constructors
@@ -151,7 +153,7 @@ class pkgCache::VerIterator
{
if (Owner == 0 || Ver == 0)
return;
- Ver += static_cast<Version const *>(newMap) - static_cast<Version const *>(oldMap);
+ Ver = RebasePointer(Ver, oldMap, newMap);
}
inline VerIterator() : Ver(0), Owner(0) {};
@@ -224,7 +226,7 @@ class pkgCache::DepIterator
{
if (Owner == 0 || Dep == 0)
return;
- Dep += static_cast<Dependency const *>(newMap) - static_cast<Dependency const *>(oldMap);
+ Dep = RebasePointer(Dep, oldMap, newMap);
}
inline DepIterator(pkgCache &Owner,Dependency *Trg,Version * = 0) :
@@ -283,7 +285,7 @@ class pkgCache::PrvIterator
{
if (Owner == 0 || Prv == 0)
return;
- Prv += static_cast<Provides const *>(newMap) - static_cast<Provides const *>(oldMap);
+ Prv = RebasePointer(Prv, oldMap, newMap);
}
inline PrvIterator() : Prv(0), Type(PrvVer), Owner(0) {};
@@ -346,7 +348,7 @@ class pkgCache::PkgFileIterator
{
if (Owner == 0 || File == 0)
return;
- File += static_cast<PackageFile const *>(newMap) - static_cast<PackageFile const *>(oldMap);
+ File = RebasePointer(File, oldMap, newMap);
}
// Constructors
@@ -387,7 +389,7 @@ class pkgCache::VerFileIterator
{
if (Owner == 0 || FileP == 0)
return;
- FileP += static_cast<VerFile const *>(newMap) - static_cast<VerFile const *>(oldMap);
+ FileP = RebasePointer(FileP, oldMap, newMap);
}
inline VerFileIterator() : Owner(0), FileP(0) {};
diff --git a/apt/apt-pkg/contrib/mmap.cc b/apt/apt-pkg/contrib/mmap.cc
index cf01be9..ddae2ff 100644
--- a/apt/apt-pkg/contrib/mmap.cc
+++ b/apt/apt-pkg/contrib/mmap.cc
@@ -30,6 +30,7 @@
#include <apt-pkg/configuration.h>
#include <apt-pkg/mmap.h>
#include <apt-pkg/error.h>
+#include <apt-pkg/rebase_pointer.h>
#include <apti18n.h>
@@ -301,7 +302,7 @@ std::experimental::optional<map_ptrloc> DynamicMMap::Allocate(unsigned long Item
Pool* oldPools = Pools;
auto idxResult = RawAllocate(I->Count*ItemSize,ItemSize);
if (Pools != oldPools)
- I += Pools - oldPools;
+ I = RebasePointer(I, oldPools, Pools);
// Does the allocation failed ?
if (!idxResult)
@@ -371,7 +372,7 @@ bool DynamicMMap::Grow(unsigned long long size)
Fd->Write(&C,sizeof(C));
}
- unsigned long const poolOffset = Pools - ((Pool*) Base);
+ const void * const old_base = Base;
if (Fd != 0)
{
@@ -408,7 +409,8 @@ bool DynamicMMap::Grow(unsigned long long size)
memset((char*)Base + WorkSpace, 0, newSize - WorkSpace);
}
- Pools = (Pool*) Base + poolOffset;
+ if (Base != old_base)
+ Pools = RebasePointer(Pools, old_base, Base);
WorkSpace = newSize;
return true;
diff --git a/apt/apt-pkg/pkgcachegen.cc b/apt/apt-pkg/pkgcachegen.cc
index 654c81c..10a0fd7 100644
--- a/apt/apt-pkg/pkgcachegen.cc
+++ b/apt/apt-pkg/pkgcachegen.cc
@@ -26,6 +26,7 @@
#include <apt-pkg/strutl.h>
#include <apt-pkg/sptr.h>
#include <apt-pkg/pkgsystem.h>
+#include <apt-pkg/rebase_pointer.h>
#include <apti18n.h>
@@ -116,11 +117,11 @@ void pkgCacheGenerator::ReMap(void const * const oldMap, void const * const newM
Cache.ReMap(false);
- CurrentFile += (pkgCache::PackageFile*) newMap - (pkgCache::PackageFile*) oldMap;
+ CurrentFile = RebasePointer(CurrentFile, oldMap, newMap);
for (size_t i = 0; i < _count(UniqHash); ++i)
if (UniqHash[i] != 0)
- UniqHash[i] += (pkgCache::StringItem*) newMap - (pkgCache::StringItem*) oldMap;
+ UniqHash[i] = RebasePointer(UniqHash[i], oldMap, newMap);
for (auto i = Dynamic<pkgCache::PkgIterator>::toReMap.begin();
i != Dynamic<pkgCache::PkgIterator>::toReMap.end(); ++i)
@@ -271,7 +272,7 @@ bool pkgCacheGenerator::MergeList(ListParser &List,
if (oldMap != Map.Data())
{
- Last += (map_ptrloc*) Map.Data() - (map_ptrloc*) oldMap;
+ Last = RebasePointer(Last, oldMap, Map.Data());
oldMap = Map.Data();
}
@@ -297,7 +298,7 @@ bool pkgCacheGenerator::MergeList(ListParser &List,
PackageName.c_str(), 1);
if (oldMap != Map.Data())
- Last += (map_ptrloc*) Map.Data() - (map_ptrloc*) oldMap;
+ Last = RebasePointer(Last, oldMap, Map.Data());
*Last = *verindex;
Ver->ParentPkg = Pkg.Index();
@@ -605,7 +606,7 @@ bool pkgCacheGenerator::ListParser::NewDepends(pkgCache::VerIterator &Ver,
OldDepLast = &D->NextDepends;
OldDepVer = Ver;
} else if (oldMap != Owner->Map.Data())
- OldDepLast += (map_ptrloc*) Owner->Map.Data() - (map_ptrloc*) oldMap;
+ OldDepLast = RebasePointer(OldDepLast, oldMap, Owner->Map.Data());
// Is it a file dependency?
if (PackageName[0] == '/')
@@ -747,8 +748,8 @@ std::experimental::optional<map_ptrloc> pkgCacheGenerator::WriteUniqString(const
if (oldMap != Map.Data())
{
- Last += (map_ptrloc*) Map.Data() - (map_ptrloc*) oldMap;
- I += (pkgCache::StringItem*) Map.Data() - (pkgCache::StringItem*) oldMap;
+ Last = RebasePointer(Last, oldMap, Map.Data());
+ I = RebasePointer(I, oldMap, Map.Data());
}
*Last = *Item;
diff --git a/apt/apt-pkg/rebase_pointer.h b/apt/apt-pkg/rebase_pointer.h
new file mode 100644
index 0000000..efc4074
--- /dev/null
+++ b/apt/apt-pkg/rebase_pointer.h
@@ -0,0 +1,16 @@
+#ifndef PKGLIB_REBASE_POINTER_H
+#define PKGLIB_REBASE_POINTER_H
+
+template <typename T>
+static inline T* RebasePointer(T *ptr, const void * const old_base, const void * const new_base)
+{
+ return reinterpret_cast<T*>(reinterpret_cast<char*>(ptr) + (reinterpret_cast<char const * const>(new_base) - reinterpret_cast<char const * const>(old_base)));
+}
+
+template <typename T>
+static inline const T* RebasePointer(const T *ptr, const void * const old_base, const void * const new_base)
+{
+ return reinterpret_cast<const T*>(reinterpret_cast<const char*>(ptr) + (reinterpret_cast<char const * const>(new_base) - reinterpret_cast<char const * const>(old_base)));
+}
+
+#endif
diff --git a/apt/apt-pkg/rpm/rpmlistparser.cc b/apt/apt-pkg/rpm/rpmlistparser.cc
index 9b2e9ad..84b6b8d 100644
--- a/apt/apt-pkg/rpm/rpmlistparser.cc
+++ b/apt/apt-pkg/rpm/rpmlistparser.cc
@@ -25,6 +25,7 @@
#include <apt-pkg/strutl.h>
#include <apt-pkg/crc-16.h>
#include <apt-pkg/tagfile.h>
+#include <apt-pkg/rebase_pointer.h>
#include <apti18n.h>
@@ -56,7 +57,7 @@ rpmListParser::rpmListParser(RPMHandler *Handler)
for (auto iter: *SeenPackages)
{
- tmp.insert(iter + (static_cast<const char *>(newMap) - static_cast<const char *>(oldMap)));
+ tmp.insert(RebasePointer(iter, oldMap, newMap));
}
SeenPackages->swap(tmp);
--
2.24.0
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [devel] [PATCH for apt 2/2] Fix pointer arithmetics
2019-12-06 13:16 ` [devel] [PATCH for apt 2/2] Fix pointer arithmetics Aleksei Nikiforov
@ 2019-12-06 13:36 ` Ivan A. Melnikov
2019-12-06 15:32 ` Aleksei Nikiforov
2019-12-06 15:36 ` [devel] [PATCH for apt 2/2 v2] " Aleksei Nikiforov
2019-12-08 23:31 ` [devel] [PATCH for apt 2/2] " Dmitry V. Levin
2019-12-09 23:54 ` [devel] [PATCH apt 0/3] Fix 0.5.15lorg2-alt70~9 fallout Dmitry V. Levin
2 siblings, 2 replies; 34+ messages in thread
From: Ivan A. Melnikov @ 2019-12-06 13:36 UTC (permalink / raw)
To: ALT Linux Team development discussions; +Cc: Aleksei Nikiforov
On Fri, Dec 06, 2019 at 04:16:06PM +0300, Aleksei Nikiforov wrote:
> This change should fix pointer arithmetic issues for e2k.
> ---
> apt/apt-pkg/Makefile.am | 1 +
> apt/apt-pkg/cacheiterators.h | 14 ++++++++------
> apt/apt-pkg/contrib/mmap.cc | 8 +++++---
> apt/apt-pkg/pkgcachegen.cc | 15 ++++++++-------
> apt/apt-pkg/rebase_pointer.h | 16 ++++++++++++++++
> apt/apt-pkg/rpm/rpmlistparser.cc | 3 ++-
> 6 files changed, 40 insertions(+), 17 deletions(-)
> create mode 100644 apt/apt-pkg/rebase_pointer.h
>
> diff --git a/apt/apt-pkg/Makefile.am b/apt/apt-pkg/Makefile.am
> index 4c0d234..d038d01 100644
> --- a/apt/apt-pkg/Makefile.am
> +++ b/apt/apt-pkg/Makefile.am
> @@ -94,6 +94,7 @@ libapt_pkg_la_SOURCES = \
> pkgsystem.h \
> policy.cc \
> policy.h \
> + rebase_pointer.h \
> repository.cc \
> repository.h \
> scopeexit.h \
> diff --git a/apt/apt-pkg/cacheiterators.h b/apt/apt-pkg/cacheiterators.h
> index a4bf670..51f70c1 100644
> --- a/apt/apt-pkg/cacheiterators.h
> +++ b/apt/apt-pkg/cacheiterators.h
> @@ -34,6 +34,8 @@
> #pragma interface "apt-pkg/cacheiterators.h"
> #endif
>
> +#include <apt-pkg/rebase_pointer.h>
> +
> #include <set>
>
> // Package Iterator
> @@ -89,7 +91,7 @@ class pkgCache::PkgIterator
> {
> if (Owner == 0 || Pkg == 0)
> return;
> - Pkg += static_cast<Package const *>(newMap) - static_cast<Package const *>(oldMap);
> + Pkg = RebasePointer(Pkg, oldMap, newMap);
> }
>
> // Constructors
> @@ -151,7 +153,7 @@ class pkgCache::VerIterator
> {
> if (Owner == 0 || Ver == 0)
> return;
> - Ver += static_cast<Version const *>(newMap) - static_cast<Version const *>(oldMap);
> + Ver = RebasePointer(Ver, oldMap, newMap);
> }
>
> inline VerIterator() : Ver(0), Owner(0) {};
> @@ -224,7 +226,7 @@ class pkgCache::DepIterator
> {
> if (Owner == 0 || Dep == 0)
> return;
> - Dep += static_cast<Dependency const *>(newMap) - static_cast<Dependency const *>(oldMap);
> + Dep = RebasePointer(Dep, oldMap, newMap);
> }
>
> inline DepIterator(pkgCache &Owner,Dependency *Trg,Version * = 0) :
> @@ -283,7 +285,7 @@ class pkgCache::PrvIterator
> {
> if (Owner == 0 || Prv == 0)
> return;
> - Prv += static_cast<Provides const *>(newMap) - static_cast<Provides const *>(oldMap);
> + Prv = RebasePointer(Prv, oldMap, newMap);
> }
>
> inline PrvIterator() : Prv(0), Type(PrvVer), Owner(0) {};
> @@ -346,7 +348,7 @@ class pkgCache::PkgFileIterator
> {
> if (Owner == 0 || File == 0)
> return;
> - File += static_cast<PackageFile const *>(newMap) - static_cast<PackageFile const *>(oldMap);
> + File = RebasePointer(File, oldMap, newMap);
> }
>
> // Constructors
> @@ -387,7 +389,7 @@ class pkgCache::VerFileIterator
> {
> if (Owner == 0 || FileP == 0)
> return;
> - FileP += static_cast<VerFile const *>(newMap) - static_cast<VerFile const *>(oldMap);
> + FileP = RebasePointer(FileP, oldMap, newMap);
> }
>
> inline VerFileIterator() : Owner(0), FileP(0) {};
> diff --git a/apt/apt-pkg/contrib/mmap.cc b/apt/apt-pkg/contrib/mmap.cc
> index cf01be9..ddae2ff 100644
> --- a/apt/apt-pkg/contrib/mmap.cc
> +++ b/apt/apt-pkg/contrib/mmap.cc
> @@ -30,6 +30,7 @@
> #include <apt-pkg/configuration.h>
> #include <apt-pkg/mmap.h>
> #include <apt-pkg/error.h>
> +#include <apt-pkg/rebase_pointer.h>
>
> #include <apti18n.h>
>
> @@ -301,7 +302,7 @@ std::experimental::optional<map_ptrloc> DynamicMMap::Allocate(unsigned long Item
> Pool* oldPools = Pools;
> auto idxResult = RawAllocate(I->Count*ItemSize,ItemSize);
> if (Pools != oldPools)
> - I += Pools - oldPools;
> + I = RebasePointer(I, oldPools, Pools);
>
> // Does the allocation failed ?
> if (!idxResult)
> @@ -371,7 +372,7 @@ bool DynamicMMap::Grow(unsigned long long size)
> Fd->Write(&C,sizeof(C));
> }
>
> - unsigned long const poolOffset = Pools - ((Pool*) Base);
> + const void * const old_base = Base;
>
> if (Fd != 0)
> {
> @@ -408,7 +409,8 @@ bool DynamicMMap::Grow(unsigned long long size)
> memset((char*)Base + WorkSpace, 0, newSize - WorkSpace);
> }
>
> - Pools = (Pool*) Base + poolOffset;
> + if (Base != old_base)
> + Pools = RebasePointer(Pools, old_base, Base);
> WorkSpace = newSize;
>
> return true;
> diff --git a/apt/apt-pkg/pkgcachegen.cc b/apt/apt-pkg/pkgcachegen.cc
> index 654c81c..10a0fd7 100644
> --- a/apt/apt-pkg/pkgcachegen.cc
> +++ b/apt/apt-pkg/pkgcachegen.cc
> @@ -26,6 +26,7 @@
> #include <apt-pkg/strutl.h>
> #include <apt-pkg/sptr.h>
> #include <apt-pkg/pkgsystem.h>
> +#include <apt-pkg/rebase_pointer.h>
>
> #include <apti18n.h>
>
> @@ -116,11 +117,11 @@ void pkgCacheGenerator::ReMap(void const * const oldMap, void const * const newM
>
> Cache.ReMap(false);
>
> - CurrentFile += (pkgCache::PackageFile*) newMap - (pkgCache::PackageFile*) oldMap;
> + CurrentFile = RebasePointer(CurrentFile, oldMap, newMap);
>
> for (size_t i = 0; i < _count(UniqHash); ++i)
> if (UniqHash[i] != 0)
> - UniqHash[i] += (pkgCache::StringItem*) newMap - (pkgCache::StringItem*) oldMap;
> + UniqHash[i] = RebasePointer(UniqHash[i], oldMap, newMap);
>
> for (auto i = Dynamic<pkgCache::PkgIterator>::toReMap.begin();
> i != Dynamic<pkgCache::PkgIterator>::toReMap.end(); ++i)
> @@ -271,7 +272,7 @@ bool pkgCacheGenerator::MergeList(ListParser &List,
>
> if (oldMap != Map.Data())
> {
> - Last += (map_ptrloc*) Map.Data() - (map_ptrloc*) oldMap;
> + Last = RebasePointer(Last, oldMap, Map.Data());
> oldMap = Map.Data();
> }
>
> @@ -297,7 +298,7 @@ bool pkgCacheGenerator::MergeList(ListParser &List,
> PackageName.c_str(), 1);
>
> if (oldMap != Map.Data())
> - Last += (map_ptrloc*) Map.Data() - (map_ptrloc*) oldMap;
> + Last = RebasePointer(Last, oldMap, Map.Data());
> *Last = *verindex;
>
> Ver->ParentPkg = Pkg.Index();
> @@ -605,7 +606,7 @@ bool pkgCacheGenerator::ListParser::NewDepends(pkgCache::VerIterator &Ver,
> OldDepLast = &D->NextDepends;
> OldDepVer = Ver;
> } else if (oldMap != Owner->Map.Data())
> - OldDepLast += (map_ptrloc*) Owner->Map.Data() - (map_ptrloc*) oldMap;
> + OldDepLast = RebasePointer(OldDepLast, oldMap, Owner->Map.Data());
>
> // Is it a file dependency?
> if (PackageName[0] == '/')
> @@ -747,8 +748,8 @@ std::experimental::optional<map_ptrloc> pkgCacheGenerator::WriteUniqString(const
>
> if (oldMap != Map.Data())
> {
> - Last += (map_ptrloc*) Map.Data() - (map_ptrloc*) oldMap;
> - I += (pkgCache::StringItem*) Map.Data() - (pkgCache::StringItem*) oldMap;
> + Last = RebasePointer(Last, oldMap, Map.Data());
> + I = RebasePointer(I, oldMap, Map.Data());
> }
>
> *Last = *Item;
> diff --git a/apt/apt-pkg/rebase_pointer.h b/apt/apt-pkg/rebase_pointer.h
> new file mode 100644
> index 0000000..efc4074
> --- /dev/null
> +++ b/apt/apt-pkg/rebase_pointer.h
> @@ -0,0 +1,16 @@
> +#ifndef PKGLIB_REBASE_POINTER_H
> +#define PKGLIB_REBASE_POINTER_H
> +
> +template <typename T>
> +static inline T* RebasePointer(T *ptr, const void * const old_base, const void * const new_base)
> +{
> + return reinterpret_cast<T*>(reinterpret_cast<char*>(ptr) + (reinterpret_cast<char const * const>(new_base) - reinterpret_cast<char const * const>(old_base)));
AFAIR standard only allows to substract pointers to elements of the same
array. Wouldn't it be safer to first compute the offset of ptr with
respect to old_base and then add it to new_base?
> +}
> +
> +template <typename T>
> +static inline const T* RebasePointer(const T *ptr, const void * const old_base, const void * const new_base)
> +{
> + return reinterpret_cast<const T*>(reinterpret_cast<const char*>(ptr) + (reinterpret_cast<char const * const>(new_base) - reinterpret_cast<char const * const>(old_base)));
> +}
> +
> +#endif
> diff --git a/apt/apt-pkg/rpm/rpmlistparser.cc b/apt/apt-pkg/rpm/rpmlistparser.cc
> index 9b2e9ad..84b6b8d 100644
> --- a/apt/apt-pkg/rpm/rpmlistparser.cc
> +++ b/apt/apt-pkg/rpm/rpmlistparser.cc
> @@ -25,6 +25,7 @@
> #include <apt-pkg/strutl.h>
> #include <apt-pkg/crc-16.h>
> #include <apt-pkg/tagfile.h>
> +#include <apt-pkg/rebase_pointer.h>
>
> #include <apti18n.h>
>
> @@ -56,7 +57,7 @@ rpmListParser::rpmListParser(RPMHandler *Handler)
>
> for (auto iter: *SeenPackages)
> {
> - tmp.insert(iter + (static_cast<const char *>(newMap) - static_cast<const char *>(oldMap)));
> + tmp.insert(RebasePointer(iter, oldMap, newMap));
> }
>
> SeenPackages->swap(tmp);
> --
> 2.24.0
>
> _______________________________________________
> Devel mailing list
> Devel@lists.altlinux.org
> https://lists.altlinux.org/mailman/listinfo/devel
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [devel] [PATCH for apt 2/2] Fix pointer arithmetics
2019-12-06 13:36 ` Ivan A. Melnikov
@ 2019-12-06 15:32 ` Aleksei Nikiforov
2019-12-06 15:36 ` [devel] [PATCH for apt 2/2 v2] " Aleksei Nikiforov
1 sibling, 0 replies; 34+ messages in thread
From: Aleksei Nikiforov @ 2019-12-06 15:32 UTC (permalink / raw)
To: devel
06.12.2019 16:36, Ivan A. Melnikov пишет:
> On Fri, Dec 06, 2019 at 04:16:06PM +0300, Aleksei Nikiforov wrote:
>> This change should fix pointer arithmetic issues for e2k.
>> ---
>> apt/apt-pkg/Makefile.am | 1 +
>> apt/apt-pkg/cacheiterators.h | 14 ++++++++------
>> apt/apt-pkg/contrib/mmap.cc | 8 +++++---
>> apt/apt-pkg/pkgcachegen.cc | 15 ++++++++-------
>> apt/apt-pkg/rebase_pointer.h | 16 ++++++++++++++++
>> apt/apt-pkg/rpm/rpmlistparser.cc | 3 ++-
>> 6 files changed, 40 insertions(+), 17 deletions(-)
>> create mode 100644 apt/apt-pkg/rebase_pointer.h
>>
>> diff --git a/apt/apt-pkg/Makefile.am b/apt/apt-pkg/Makefile.am
>> index 4c0d234..d038d01 100644
>> --- a/apt/apt-pkg/Makefile.am
>> +++ b/apt/apt-pkg/Makefile.am
>> @@ -94,6 +94,7 @@ libapt_pkg_la_SOURCES = \
>> pkgsystem.h \
>> policy.cc \
>> policy.h \
>> + rebase_pointer.h \
>> repository.cc \
>> repository.h \
>> scopeexit.h \
>> diff --git a/apt/apt-pkg/cacheiterators.h b/apt/apt-pkg/cacheiterators.h
>> index a4bf670..51f70c1 100644
>> --- a/apt/apt-pkg/cacheiterators.h
>> +++ b/apt/apt-pkg/cacheiterators.h
>> @@ -34,6 +34,8 @@
>> #pragma interface "apt-pkg/cacheiterators.h"
>> #endif
>>
>> +#include <apt-pkg/rebase_pointer.h>
>> +
>> #include <set>
>>
>> // Package Iterator
>> @@ -89,7 +91,7 @@ class pkgCache::PkgIterator
>> {
>> if (Owner == 0 || Pkg == 0)
>> return;
>> - Pkg += static_cast<Package const *>(newMap) - static_cast<Package const *>(oldMap);
>> + Pkg = RebasePointer(Pkg, oldMap, newMap);
>> }
>>
>> // Constructors
>> @@ -151,7 +153,7 @@ class pkgCache::VerIterator
>> {
>> if (Owner == 0 || Ver == 0)
>> return;
>> - Ver += static_cast<Version const *>(newMap) - static_cast<Version const *>(oldMap);
>> + Ver = RebasePointer(Ver, oldMap, newMap);
>> }
>>
>> inline VerIterator() : Ver(0), Owner(0) {};
>> @@ -224,7 +226,7 @@ class pkgCache::DepIterator
>> {
>> if (Owner == 0 || Dep == 0)
>> return;
>> - Dep += static_cast<Dependency const *>(newMap) - static_cast<Dependency const *>(oldMap);
>> + Dep = RebasePointer(Dep, oldMap, newMap);
>> }
>>
>> inline DepIterator(pkgCache &Owner,Dependency *Trg,Version * = 0) :
>> @@ -283,7 +285,7 @@ class pkgCache::PrvIterator
>> {
>> if (Owner == 0 || Prv == 0)
>> return;
>> - Prv += static_cast<Provides const *>(newMap) - static_cast<Provides const *>(oldMap);
>> + Prv = RebasePointer(Prv, oldMap, newMap);
>> }
>>
>> inline PrvIterator() : Prv(0), Type(PrvVer), Owner(0) {};
>> @@ -346,7 +348,7 @@ class pkgCache::PkgFileIterator
>> {
>> if (Owner == 0 || File == 0)
>> return;
>> - File += static_cast<PackageFile const *>(newMap) - static_cast<PackageFile const *>(oldMap);
>> + File = RebasePointer(File, oldMap, newMap);
>> }
>>
>> // Constructors
>> @@ -387,7 +389,7 @@ class pkgCache::VerFileIterator
>> {
>> if (Owner == 0 || FileP == 0)
>> return;
>> - FileP += static_cast<VerFile const *>(newMap) - static_cast<VerFile const *>(oldMap);
>> + FileP = RebasePointer(FileP, oldMap, newMap);
>> }
>>
>> inline VerFileIterator() : Owner(0), FileP(0) {};
>> diff --git a/apt/apt-pkg/contrib/mmap.cc b/apt/apt-pkg/contrib/mmap.cc
>> index cf01be9..ddae2ff 100644
>> --- a/apt/apt-pkg/contrib/mmap.cc
>> +++ b/apt/apt-pkg/contrib/mmap.cc
>> @@ -30,6 +30,7 @@
>> #include <apt-pkg/configuration.h>
>> #include <apt-pkg/mmap.h>
>> #include <apt-pkg/error.h>
>> +#include <apt-pkg/rebase_pointer.h>
>>
>> #include <apti18n.h>
>>
>> @@ -301,7 +302,7 @@ std::experimental::optional<map_ptrloc> DynamicMMap::Allocate(unsigned long Item
>> Pool* oldPools = Pools;
>> auto idxResult = RawAllocate(I->Count*ItemSize,ItemSize);
>> if (Pools != oldPools)
>> - I += Pools - oldPools;
>> + I = RebasePointer(I, oldPools, Pools);
>>
>> // Does the allocation failed ?
>> if (!idxResult)
>> @@ -371,7 +372,7 @@ bool DynamicMMap::Grow(unsigned long long size)
>> Fd->Write(&C,sizeof(C));
>> }
>>
>> - unsigned long const poolOffset = Pools - ((Pool*) Base);
>> + const void * const old_base = Base;
>>
>> if (Fd != 0)
>> {
>> @@ -408,7 +409,8 @@ bool DynamicMMap::Grow(unsigned long long size)
>> memset((char*)Base + WorkSpace, 0, newSize - WorkSpace);
>> }
>>
>> - Pools = (Pool*) Base + poolOffset;
>> + if (Base != old_base)
>> + Pools = RebasePointer(Pools, old_base, Base);
>> WorkSpace = newSize;
>>
>> return true;
>> diff --git a/apt/apt-pkg/pkgcachegen.cc b/apt/apt-pkg/pkgcachegen.cc
>> index 654c81c..10a0fd7 100644
>> --- a/apt/apt-pkg/pkgcachegen.cc
>> +++ b/apt/apt-pkg/pkgcachegen.cc
>> @@ -26,6 +26,7 @@
>> #include <apt-pkg/strutl.h>
>> #include <apt-pkg/sptr.h>
>> #include <apt-pkg/pkgsystem.h>
>> +#include <apt-pkg/rebase_pointer.h>
>>
>> #include <apti18n.h>
>>
>> @@ -116,11 +117,11 @@ void pkgCacheGenerator::ReMap(void const * const oldMap, void const * const newM
>>
>> Cache.ReMap(false);
>>
>> - CurrentFile += (pkgCache::PackageFile*) newMap - (pkgCache::PackageFile*) oldMap;
>> + CurrentFile = RebasePointer(CurrentFile, oldMap, newMap);
>>
>> for (size_t i = 0; i < _count(UniqHash); ++i)
>> if (UniqHash[i] != 0)
>> - UniqHash[i] += (pkgCache::StringItem*) newMap - (pkgCache::StringItem*) oldMap;
>> + UniqHash[i] = RebasePointer(UniqHash[i], oldMap, newMap);
>>
>> for (auto i = Dynamic<pkgCache::PkgIterator>::toReMap.begin();
>> i != Dynamic<pkgCache::PkgIterator>::toReMap.end(); ++i)
>> @@ -271,7 +272,7 @@ bool pkgCacheGenerator::MergeList(ListParser &List,
>>
>> if (oldMap != Map.Data())
>> {
>> - Last += (map_ptrloc*) Map.Data() - (map_ptrloc*) oldMap;
>> + Last = RebasePointer(Last, oldMap, Map.Data());
>> oldMap = Map.Data();
>> }
>>
>> @@ -297,7 +298,7 @@ bool pkgCacheGenerator::MergeList(ListParser &List,
>> PackageName.c_str(), 1);
>>
>> if (oldMap != Map.Data())
>> - Last += (map_ptrloc*) Map.Data() - (map_ptrloc*) oldMap;
>> + Last = RebasePointer(Last, oldMap, Map.Data());
>> *Last = *verindex;
>>
>> Ver->ParentPkg = Pkg.Index();
>> @@ -605,7 +606,7 @@ bool pkgCacheGenerator::ListParser::NewDepends(pkgCache::VerIterator &Ver,
>> OldDepLast = &D->NextDepends;
>> OldDepVer = Ver;
>> } else if (oldMap != Owner->Map.Data())
>> - OldDepLast += (map_ptrloc*) Owner->Map.Data() - (map_ptrloc*) oldMap;
>> + OldDepLast = RebasePointer(OldDepLast, oldMap, Owner->Map.Data());
>>
>> // Is it a file dependency?
>> if (PackageName[0] == '/')
>> @@ -747,8 +748,8 @@ std::experimental::optional<map_ptrloc> pkgCacheGenerator::WriteUniqString(const
>>
>> if (oldMap != Map.Data())
>> {
>> - Last += (map_ptrloc*) Map.Data() - (map_ptrloc*) oldMap;
>> - I += (pkgCache::StringItem*) Map.Data() - (pkgCache::StringItem*) oldMap;
>> + Last = RebasePointer(Last, oldMap, Map.Data());
>> + I = RebasePointer(I, oldMap, Map.Data());
>> }
>>
>> *Last = *Item;
>> diff --git a/apt/apt-pkg/rebase_pointer.h b/apt/apt-pkg/rebase_pointer.h
>> new file mode 100644
>> index 0000000..efc4074
>> --- /dev/null
>> +++ b/apt/apt-pkg/rebase_pointer.h
>> @@ -0,0 +1,16 @@
>> +#ifndef PKGLIB_REBASE_POINTER_H
>> +#define PKGLIB_REBASE_POINTER_H
>> +
>> +template <typename T>
>> +static inline T* RebasePointer(T *ptr, const void * const old_base, const void * const new_base)
>> +{
>> + return reinterpret_cast<T*>(reinterpret_cast<char*>(ptr) + (reinterpret_cast<char const * const>(new_base) - reinterpret_cast<char const * const>(old_base)));
>
>
> AFAIR standard only allows to substract pointers to elements of the same
> array. Wouldn't it be safer to first compute the offset of ptr with
> respect to old_base and then add it to new_base?
>
>
It can be done. Arithmetically it should be same result, but it'd also
take some changes to get rid of 'const' specifiers since they'd clash
with non-const variables (ptr).
- return reinterpret_cast<T*>(reinterpret_cast<char*>(ptr) +
(reinterpret_cast<char const * const>(new_base) - reinterpret_cast<char
const * const>(old_base)));
+ return reinterpret_cast<T*>(reinterpret_cast<char*>(new_base) +
(reinterpret_cast<char*>(ptr) - reinterpret_cast<char*>(old_base)));
>> +}
>> +
>> +template <typename T>
>> +static inline const T* RebasePointer(const T *ptr, const void * const old_base, const void * const new_base)
>> +{
>> + return reinterpret_cast<const T*>(reinterpret_cast<const char*>(ptr) + (reinterpret_cast<char const * const>(new_base) - reinterpret_cast<char const * const>(old_base)));
>> +}
>> +
>> +#endif
>> diff --git a/apt/apt-pkg/rpm/rpmlistparser.cc b/apt/apt-pkg/rpm/rpmlistparser.cc
>> index 9b2e9ad..84b6b8d 100644
>> --- a/apt/apt-pkg/rpm/rpmlistparser.cc
>> +++ b/apt/apt-pkg/rpm/rpmlistparser.cc
>> @@ -25,6 +25,7 @@
>> #include <apt-pkg/strutl.h>
>> #include <apt-pkg/crc-16.h>
>> #include <apt-pkg/tagfile.h>
>> +#include <apt-pkg/rebase_pointer.h>
>>
>> #include <apti18n.h>
>>
>> @@ -56,7 +57,7 @@ rpmListParser::rpmListParser(RPMHandler *Handler)
>>
>> for (auto iter: *SeenPackages)
>> {
>> - tmp.insert(iter + (static_cast<const char *>(newMap) - static_cast<const char *>(oldMap)));
>> + tmp.insert(RebasePointer(iter, oldMap, newMap));
>> }
>>
>> SeenPackages->swap(tmp);
>> --
>> 2.24.0
>>
>> _______________________________________________
>> Devel mailing list
>> Devel@lists.altlinux.org
>> https://lists.altlinux.org/mailman/listinfo/devel
> _______________________________________________
> Devel mailing list
> Devel@lists.altlinux.org
> https://lists.altlinux.org/mailman/listinfo/devel
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* [devel] [PATCH for apt 2/2 v2] Fix pointer arithmetics
2019-12-06 13:36 ` Ivan A. Melnikov
2019-12-06 15:32 ` Aleksei Nikiforov
@ 2019-12-06 15:36 ` Aleksei Nikiforov
2019-12-07 14:52 ` Andrey Savchenko
2019-12-08 23:21 ` Dmitry V. Levin
1 sibling, 2 replies; 34+ messages in thread
From: Aleksei Nikiforov @ 2019-12-06 15:36 UTC (permalink / raw)
To: devel; +Cc: Aleksei Nikiforov
This change should fix pointer arithmetic issues for e2k.
---
apt/apt-pkg/Makefile.am | 1 +
apt/apt-pkg/cacheiterators.h | 26 ++++++++++++++------------
apt/apt-pkg/contrib/mmap.cc | 8 +++++---
apt/apt-pkg/pkgcachegen.cc | 29 +++++++++++++++--------------
apt/apt-pkg/pkgcachegen.h | 6 +++---
apt/apt-pkg/rebase_pointer.h | 16 ++++++++++++++++
apt/apt-pkg/rpm/rpmlistparser.cc | 5 +++--
apt/apt-pkg/rpm/rpmpackagedata.cc | 2 +-
8 files changed, 58 insertions(+), 35 deletions(-)
create mode 100644 apt/apt-pkg/rebase_pointer.h
diff --git a/apt/apt-pkg/Makefile.am b/apt/apt-pkg/Makefile.am
index 4c0d234..d038d01 100644
--- a/apt/apt-pkg/Makefile.am
+++ b/apt/apt-pkg/Makefile.am
@@ -94,6 +94,7 @@ libapt_pkg_la_SOURCES = \
pkgsystem.h \
policy.cc \
policy.h \
+ rebase_pointer.h \
repository.cc \
repository.h \
scopeexit.h \
diff --git a/apt/apt-pkg/cacheiterators.h b/apt/apt-pkg/cacheiterators.h
index a4bf670..118838e 100644
--- a/apt/apt-pkg/cacheiterators.h
+++ b/apt/apt-pkg/cacheiterators.h
@@ -34,6 +34,8 @@
#pragma interface "apt-pkg/cacheiterators.h"
#endif
+#include <apt-pkg/rebase_pointer.h>
+
#include <set>
// Package Iterator
@@ -85,11 +87,11 @@ class pkgCache::PkgIterator
inline unsigned long long Index() const {return Pkg - Owner->PkgP;};
OkState State() const;
- void ReMap(void const * const oldMap, void const * const newMap)
+ void ReMap(void *oldMap, void *newMap)
{
if (Owner == 0 || Pkg == 0)
return;
- Pkg += static_cast<Package const *>(newMap) - static_cast<Package const *>(oldMap);
+ Pkg = RebasePointer(Pkg, oldMap, newMap);
}
// Constructors
@@ -147,11 +149,11 @@ class pkgCache::VerIterator
bool Automatic() const;
VerFileIterator NewestFile() const;
- void ReMap(void const * const oldMap, void const * const newMap)
+ void ReMap(void *oldMap, void *newMap)
{
if (Owner == 0 || Ver == 0)
return;
- Ver += static_cast<Version const *>(newMap) - static_cast<Version const *>(oldMap);
+ Ver = RebasePointer(Ver, oldMap, newMap);
}
inline VerIterator() : Ver(0), Owner(0) {};
@@ -220,11 +222,11 @@ class pkgCache::DepIterator
inline const char *CompType() {return Owner->CompType(Dep->CompareOp);};
inline const char *DepType() {return Owner->DepType(Dep->Type);};
- void ReMap(void const * const oldMap, void const * const newMap)
+ void ReMap(void *oldMap, void *newMap)
{
if (Owner == 0 || Dep == 0)
return;
- Dep += static_cast<Dependency const *>(newMap) - static_cast<Dependency const *>(oldMap);
+ Dep = RebasePointer(Dep, oldMap, newMap);
}
inline DepIterator(pkgCache &Owner,Dependency *Trg,Version * = 0) :
@@ -279,11 +281,11 @@ class pkgCache::PrvIterator
inline PkgIterator OwnerPkg() const {return PkgIterator(*Owner,Owner->PkgP + Owner->VerP[Prv->Version].ParentPkg);};
inline unsigned long long Index() const {return Prv - Owner->ProvideP;};
- void ReMap(void const * const oldMap, void const * const newMap)
+ void ReMap(void *oldMap, void *newMap)
{
if (Owner == 0 || Prv == 0)
return;
- Prv += static_cast<Provides const *>(newMap) - static_cast<Provides const *>(oldMap);
+ Prv = RebasePointer(Prv, oldMap, newMap);
}
inline PrvIterator() : Prv(0), Type(PrvVer), Owner(0) {};
@@ -342,11 +344,11 @@ class pkgCache::PkgFileIterator
bool IsOk();
string RelStr();
- void ReMap(void const * const oldMap, void const * const newMap)
+ void ReMap(void *oldMap, void *newMap)
{
if (Owner == 0 || File == 0)
return;
- File += static_cast<PackageFile const *>(newMap) - static_cast<PackageFile const *>(oldMap);
+ File = RebasePointer(File, oldMap, newMap);
}
// Constructors
@@ -383,11 +385,11 @@ class pkgCache::VerFileIterator
inline PkgFileIterator File() const {return PkgFileIterator(*Owner,FileP->File + Owner->PkgFileP);};
inline unsigned long long Index() const {return FileP - Owner->VerFileP;};
- void ReMap(void const * const oldMap, void const * const newMap)
+ void ReMap(void *oldMap, void *newMap)
{
if (Owner == 0 || FileP == 0)
return;
- FileP += static_cast<VerFile const *>(newMap) - static_cast<VerFile const *>(oldMap);
+ FileP = RebasePointer(FileP, oldMap, newMap);
}
inline VerFileIterator() : Owner(0), FileP(0) {};
diff --git a/apt/apt-pkg/contrib/mmap.cc b/apt/apt-pkg/contrib/mmap.cc
index cf01be9..35f1a25 100644
--- a/apt/apt-pkg/contrib/mmap.cc
+++ b/apt/apt-pkg/contrib/mmap.cc
@@ -30,6 +30,7 @@
#include <apt-pkg/configuration.h>
#include <apt-pkg/mmap.h>
#include <apt-pkg/error.h>
+#include <apt-pkg/rebase_pointer.h>
#include <apti18n.h>
@@ -301,7 +302,7 @@ std::experimental::optional<map_ptrloc> DynamicMMap::Allocate(unsigned long Item
Pool* oldPools = Pools;
auto idxResult = RawAllocate(I->Count*ItemSize,ItemSize);
if (Pools != oldPools)
- I += Pools - oldPools;
+ I = RebasePointer(I, oldPools, Pools);
// Does the allocation failed ?
if (!idxResult)
@@ -371,7 +372,7 @@ bool DynamicMMap::Grow(unsigned long long size)
Fd->Write(&C,sizeof(C));
}
- unsigned long const poolOffset = Pools - ((Pool*) Base);
+ void *old_base = Base;
if (Fd != 0)
{
@@ -408,7 +409,8 @@ bool DynamicMMap::Grow(unsigned long long size)
memset((char*)Base + WorkSpace, 0, newSize - WorkSpace);
}
- Pools = (Pool*) Base + poolOffset;
+ if (Base != old_base)
+ Pools = RebasePointer(Pools, old_base, Base);
WorkSpace = newSize;
return true;
diff --git a/apt/apt-pkg/pkgcachegen.cc b/apt/apt-pkg/pkgcachegen.cc
index 654c81c..37364e9 100644
--- a/apt/apt-pkg/pkgcachegen.cc
+++ b/apt/apt-pkg/pkgcachegen.cc
@@ -26,6 +26,7 @@
#include <apt-pkg/strutl.h>
#include <apt-pkg/sptr.h>
#include <apt-pkg/pkgsystem.h>
+#include <apt-pkg/rebase_pointer.h>
#include <apti18n.h>
@@ -109,18 +110,18 @@ pkgCacheGenerator::~pkgCacheGenerator()
Map.Sync(0,sizeof(pkgCache::Header));
}
/*}}}*/
-void pkgCacheGenerator::ReMap(void const * const oldMap, void const * const newMap)
+void pkgCacheGenerator::ReMap(void *oldMap, void *newMap)
{
if (oldMap == newMap)
return;
Cache.ReMap(false);
- CurrentFile += (pkgCache::PackageFile*) newMap - (pkgCache::PackageFile*) oldMap;
+ CurrentFile = RebasePointer(CurrentFile, oldMap, newMap);
for (size_t i = 0; i < _count(UniqHash); ++i)
if (UniqHash[i] != 0)
- UniqHash[i] += (pkgCache::StringItem*) newMap - (pkgCache::StringItem*) oldMap;
+ UniqHash[i] = RebasePointer(UniqHash[i], oldMap, newMap);
for (auto i = Dynamic<pkgCache::PkgIterator>::toReMap.begin();
i != Dynamic<pkgCache::PkgIterator>::toReMap.end(); ++i)
@@ -147,7 +148,7 @@ void pkgCacheGenerator::ReMap(void const * const oldMap, void const * const newM
// CacheGenerator::WriteStringInMap /*{{{*/
std::experimental::optional<map_ptrloc> pkgCacheGenerator::WriteStringInMap(const char *String,
unsigned long Len) {
- void const * const oldMap = Map.Data();
+ void *oldMap = Map.Data();
const auto index = Map.WriteString(String, Len);
if (index)
ReMap(oldMap, Map.Data());
@@ -156,7 +157,7 @@ std::experimental::optional<map_ptrloc> pkgCacheGenerator::WriteStringInMap(cons
/*}}}*/
// CacheGenerator::WriteStringInMap /*{{{*/
std::experimental::optional<map_ptrloc> pkgCacheGenerator::WriteStringInMap(const char *String) {
- void const * const oldMap = Map.Data();
+ void *oldMap = Map.Data();
const auto index = Map.WriteString(String);
if (index)
ReMap(oldMap, Map.Data());
@@ -164,7 +165,7 @@ std::experimental::optional<map_ptrloc> pkgCacheGenerator::WriteStringInMap(cons
}
/*}}}*/
std::experimental::optional<map_ptrloc> pkgCacheGenerator::AllocateInMap(unsigned long size) {/*{{{*/
- void const * const oldMap = Map.Data();
+ void *oldMap = Map.Data();
const auto index = Map.Allocate(size);
if (index)
ReMap(oldMap, Map.Data());
@@ -229,7 +230,7 @@ bool pkgCacheGenerator::MergeList(ListParser &List,
pkgCache::VerIterator Ver = Pkg.VersionList();
Dynamic<pkgCache::VerIterator> DynVer(Ver);
map_ptrloc *Last = &Pkg->VersionList;
- const void * oldMap = Map.Data();
+ void *oldMap = Map.Data();
int Res = 1;
for (; Ver.end() == false; Last = &Ver->NextVer, Ver++)
{
@@ -271,7 +272,7 @@ bool pkgCacheGenerator::MergeList(ListParser &List,
if (oldMap != Map.Data())
{
- Last += (map_ptrloc*) Map.Data() - (map_ptrloc*) oldMap;
+ Last = RebasePointer(Last, oldMap, Map.Data());
oldMap = Map.Data();
}
@@ -297,7 +298,7 @@ bool pkgCacheGenerator::MergeList(ListParser &List,
PackageName.c_str(), 1);
if (oldMap != Map.Data())
- Last += (map_ptrloc*) Map.Data() - (map_ptrloc*) oldMap;
+ Last = RebasePointer(Last, oldMap, Map.Data());
*Last = *verindex;
Ver->ParentPkg = Pkg.Index();
@@ -553,7 +554,7 @@ bool pkgCacheGenerator::ListParser::NewDepends(pkgCache::VerIterator &Ver,
unsigned int Op,
unsigned int Type)
{
- void const * const oldMap = Owner->Map.Data();
+ void *oldMap = Owner->Map.Data();
pkgCache &Cache = Owner->Cache;
// Get a structure
@@ -605,7 +606,7 @@ bool pkgCacheGenerator::ListParser::NewDepends(pkgCache::VerIterator &Ver,
OldDepLast = &D->NextDepends;
OldDepVer = Ver;
} else if (oldMap != Owner->Map.Data())
- OldDepLast += (map_ptrloc*) Owner->Map.Data() - (map_ptrloc*) oldMap;
+ OldDepLast = RebasePointer(OldDepLast, oldMap, Owner->Map.Data());
// Is it a file dependency?
if (PackageName[0] == '/')
@@ -739,7 +740,7 @@ std::experimental::optional<map_ptrloc> pkgCacheGenerator::WriteUniqString(const
}
// Get a structure
- void const * const oldMap = Map.Data();
+ void *oldMap = Map.Data();
const auto Item = AllocateInMap(sizeof(pkgCache::StringItem));
const auto idxString = WriteStringInMap(S, Size);
if ((!Item) || (!idxString))
@@ -747,8 +748,8 @@ std::experimental::optional<map_ptrloc> pkgCacheGenerator::WriteUniqString(const
if (oldMap != Map.Data())
{
- Last += (map_ptrloc*) Map.Data() - (map_ptrloc*) oldMap;
- I += (pkgCache::StringItem*) Map.Data() - (pkgCache::StringItem*) oldMap;
+ Last = RebasePointer(Last, oldMap, Map.Data());
+ I = RebasePointer(I, oldMap, Map.Data());
}
*Last = *Item;
diff --git a/apt/apt-pkg/pkgcachegen.h b/apt/apt-pkg/pkgcachegen.h
index 77075a6..0faf6e1 100644
--- a/apt/apt-pkg/pkgcachegen.h
+++ b/apt/apt-pkg/pkgcachegen.h
@@ -72,7 +72,7 @@ class pkgCacheGenerator
class DynamicFunction
{
public:
- typedef std::function<void(const void *, const void *)> function;
+ typedef std::function<void(void *, void *)> function;
static std::set<DynamicFunction*> toReMap;
@@ -93,7 +93,7 @@ class pkgCacheGenerator
toReMap.erase(this);
}
- void call(const void *oldMap, const void *newMap)
+ void call(void *oldMap, void *newMap)
{
if (m_function)
m_function(oldMap, newMap);
@@ -140,7 +140,7 @@ class pkgCacheGenerator
// CNC:2003-03-18
inline void ResetFileDeps() {FoundFileDeps = false;};
- void ReMap(void const * const oldMap, void const * const newMap);
+ void ReMap(void *oldMap, void *newMap);
pkgCacheGenerator(DynamicMMap *Map,OpProgress *Progress);
~pkgCacheGenerator();
diff --git a/apt/apt-pkg/rebase_pointer.h b/apt/apt-pkg/rebase_pointer.h
new file mode 100644
index 0000000..f6b3c15
--- /dev/null
+++ b/apt/apt-pkg/rebase_pointer.h
@@ -0,0 +1,16 @@
+#ifndef PKGLIB_REBASE_POINTER_H
+#define PKGLIB_REBASE_POINTER_H
+
+template <typename T>
+static inline T* RebasePointer(T *ptr, void *old_base, void *new_base)
+{
+ return reinterpret_cast<T*>(reinterpret_cast<char*>(new_base) + (reinterpret_cast<char*>(ptr) - reinterpret_cast<char*>(old_base)));
+}
+
+template <typename T>
+static inline const T* RebasePointer(const T *ptr, void *old_base, void *new_base)
+{
+ return reinterpret_cast<const T*>(reinterpret_cast<char*>(new_base) + (reinterpret_cast<const char*>(ptr) - reinterpret_cast<char*>(old_base)));
+}
+
+#endif
diff --git a/apt/apt-pkg/rpm/rpmlistparser.cc b/apt/apt-pkg/rpm/rpmlistparser.cc
index 9b2e9ad..bd13b99 100644
--- a/apt/apt-pkg/rpm/rpmlistparser.cc
+++ b/apt/apt-pkg/rpm/rpmlistparser.cc
@@ -25,6 +25,7 @@
#include <apt-pkg/strutl.h>
#include <apt-pkg/crc-16.h>
#include <apt-pkg/tagfile.h>
+#include <apt-pkg/rebase_pointer.h>
#include <apti18n.h>
@@ -50,13 +51,13 @@ rpmListParser::rpmListParser(RPMHandler *Handler)
{
SeenPackages.reset(new SeenPackagesType);
m_SeenPackagesRealloc.reset(new pkgCacheGenerator::DynamicFunction(
- [this] (void const *oldMap, void const *newMap)
+ [this] (void *oldMap, void *newMap)
{
SeenPackagesType tmp;
for (auto iter: *SeenPackages)
{
- tmp.insert(iter + (static_cast<const char *>(newMap) - static_cast<const char *>(oldMap)));
+ tmp.insert(RebasePointer(iter, oldMap, newMap));
}
SeenPackages->swap(tmp);
diff --git a/apt/apt-pkg/rpm/rpmpackagedata.cc b/apt/apt-pkg/rpm/rpmpackagedata.cc
index 61c15d5..d6c677b 100644
--- a/apt/apt-pkg/rpm/rpmpackagedata.cc
+++ b/apt/apt-pkg/rpm/rpmpackagedata.cc
@@ -23,7 +23,7 @@ RPMPackageData::RPMPackageData()
#endif
{
m_VerMapRealloc.reset(new pkgCacheGenerator::DynamicFunction(
- [this] (void const *oldMap, void const *newMap)
+ [this] (void *oldMap, void *newMap)
{
for (auto iter1 = VerMap.begin(); iter1 != VerMap.end(); ++iter1)
{
--
2.24.0
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [devel] [PATCH for apt 2/2 v2] Fix pointer arithmetics
2019-12-06 15:36 ` [devel] [PATCH for apt 2/2 v2] " Aleksei Nikiforov
@ 2019-12-07 14:52 ` Andrey Savchenko
2019-12-08 22:56 ` Dmitry V. Levin
2019-12-09 6:54 ` Aleksei Nikiforov
2019-12-08 23:21 ` Dmitry V. Levin
1 sibling, 2 replies; 34+ messages in thread
From: Andrey Savchenko @ 2019-12-07 14:52 UTC (permalink / raw)
To: ALT Linux Team development discussions
[-- Attachment #1: Type: text/plain, Size: 15582 bytes --]
Hi all!
On Fri, 6 Dec 2019 18:36:55 +0300 Aleksei Nikiforov wrote:
> This change should fix pointer arithmetic issues for e2k.
This commit message is misleading. Pointer arithmetic is broken on
all architectures, but by pure chance in happens to work correctly
when gcc is used. On e2k the lcc compiler is used, so problem
manifests itself. But this doesn't change the fact that pointer
arithmetic was flawed on all architectures.
> ---
> apt/apt-pkg/Makefile.am | 1 +
> apt/apt-pkg/cacheiterators.h | 26 ++++++++++++++------------
> apt/apt-pkg/contrib/mmap.cc | 8 +++++---
> apt/apt-pkg/pkgcachegen.cc | 29 +++++++++++++++--------------
> apt/apt-pkg/pkgcachegen.h | 6 +++---
> apt/apt-pkg/rebase_pointer.h | 16 ++++++++++++++++
> apt/apt-pkg/rpm/rpmlistparser.cc | 5 +++--
> apt/apt-pkg/rpm/rpmpackagedata.cc | 2 +-
> 8 files changed, 58 insertions(+), 35 deletions(-)
> create mode 100644 apt/apt-pkg/rebase_pointer.h
>
> diff --git a/apt/apt-pkg/Makefile.am b/apt/apt-pkg/Makefile.am
> index 4c0d234..d038d01 100644
> --- a/apt/apt-pkg/Makefile.am
> +++ b/apt/apt-pkg/Makefile.am
> @@ -94,6 +94,7 @@ libapt_pkg_la_SOURCES = \
> pkgsystem.h \
> policy.cc \
> policy.h \
> + rebase_pointer.h \
> repository.cc \
> repository.h \
> scopeexit.h \
> diff --git a/apt/apt-pkg/cacheiterators.h b/apt/apt-pkg/cacheiterators.h
> index a4bf670..118838e 100644
> --- a/apt/apt-pkg/cacheiterators.h
> +++ b/apt/apt-pkg/cacheiterators.h
> @@ -34,6 +34,8 @@
> #pragma interface "apt-pkg/cacheiterators.h"
> #endif
>
> +#include <apt-pkg/rebase_pointer.h>
> +
> #include <set>
>
> // Package Iterator
> @@ -85,11 +87,11 @@ class pkgCache::PkgIterator
> inline unsigned long long Index() const {return Pkg - Owner->PkgP;};
> OkState State() const;
>
> - void ReMap(void const * const oldMap, void const * const newMap)
> + void ReMap(void *oldMap, void *newMap)
> {
> if (Owner == 0 || Pkg == 0)
> return;
> - Pkg += static_cast<Package const *>(newMap) - static_cast<Package const *>(oldMap);
> + Pkg = RebasePointer(Pkg, oldMap, newMap);
> }
>
> // Constructors
> @@ -147,11 +149,11 @@ class pkgCache::VerIterator
> bool Automatic() const;
> VerFileIterator NewestFile() const;
>
> - void ReMap(void const * const oldMap, void const * const newMap)
> + void ReMap(void *oldMap, void *newMap)
> {
> if (Owner == 0 || Ver == 0)
> return;
> - Ver += static_cast<Version const *>(newMap) - static_cast<Version const *>(oldMap);
> + Ver = RebasePointer(Ver, oldMap, newMap);
> }
>
> inline VerIterator() : Ver(0), Owner(0) {};
> @@ -220,11 +222,11 @@ class pkgCache::DepIterator
> inline const char *CompType() {return Owner->CompType(Dep->CompareOp);};
> inline const char *DepType() {return Owner->DepType(Dep->Type);};
>
> - void ReMap(void const * const oldMap, void const * const newMap)
> + void ReMap(void *oldMap, void *newMap)
> {
> if (Owner == 0 || Dep == 0)
> return;
> - Dep += static_cast<Dependency const *>(newMap) - static_cast<Dependency const *>(oldMap);
> + Dep = RebasePointer(Dep, oldMap, newMap);
> }
>
> inline DepIterator(pkgCache &Owner,Dependency *Trg,Version * = 0) :
> @@ -279,11 +281,11 @@ class pkgCache::PrvIterator
> inline PkgIterator OwnerPkg() const {return PkgIterator(*Owner,Owner->PkgP + Owner->VerP[Prv->Version].ParentPkg);};
> inline unsigned long long Index() const {return Prv - Owner->ProvideP;};
>
> - void ReMap(void const * const oldMap, void const * const newMap)
> + void ReMap(void *oldMap, void *newMap)
> {
> if (Owner == 0 || Prv == 0)
> return;
> - Prv += static_cast<Provides const *>(newMap) - static_cast<Provides const *>(oldMap);
> + Prv = RebasePointer(Prv, oldMap, newMap);
> }
>
> inline PrvIterator() : Prv(0), Type(PrvVer), Owner(0) {};
> @@ -342,11 +344,11 @@ class pkgCache::PkgFileIterator
> bool IsOk();
> string RelStr();
>
> - void ReMap(void const * const oldMap, void const * const newMap)
> + void ReMap(void *oldMap, void *newMap)
> {
> if (Owner == 0 || File == 0)
> return;
> - File += static_cast<PackageFile const *>(newMap) - static_cast<PackageFile const *>(oldMap);
> + File = RebasePointer(File, oldMap, newMap);
> }
>
> // Constructors
> @@ -383,11 +385,11 @@ class pkgCache::VerFileIterator
> inline PkgFileIterator File() const {return PkgFileIterator(*Owner,FileP->File + Owner->PkgFileP);};
> inline unsigned long long Index() const {return FileP - Owner->VerFileP;};
>
> - void ReMap(void const * const oldMap, void const * const newMap)
> + void ReMap(void *oldMap, void *newMap)
> {
> if (Owner == 0 || FileP == 0)
> return;
> - FileP += static_cast<VerFile const *>(newMap) - static_cast<VerFile const *>(oldMap);
> + FileP = RebasePointer(FileP, oldMap, newMap);
> }
>
> inline VerFileIterator() : Owner(0), FileP(0) {};
> diff --git a/apt/apt-pkg/contrib/mmap.cc b/apt/apt-pkg/contrib/mmap.cc
> index cf01be9..35f1a25 100644
> --- a/apt/apt-pkg/contrib/mmap.cc
> +++ b/apt/apt-pkg/contrib/mmap.cc
> @@ -30,6 +30,7 @@
> #include <apt-pkg/configuration.h>
> #include <apt-pkg/mmap.h>
> #include <apt-pkg/error.h>
> +#include <apt-pkg/rebase_pointer.h>
>
> #include <apti18n.h>
>
> @@ -301,7 +302,7 @@ std::experimental::optional<map_ptrloc> DynamicMMap::Allocate(unsigned long Item
> Pool* oldPools = Pools;
> auto idxResult = RawAllocate(I->Count*ItemSize,ItemSize);
> if (Pools != oldPools)
> - I += Pools - oldPools;
> + I = RebasePointer(I, oldPools, Pools);
>
> // Does the allocation failed ?
> if (!idxResult)
> @@ -371,7 +372,7 @@ bool DynamicMMap::Grow(unsigned long long size)
> Fd->Write(&C,sizeof(C));
> }
>
> - unsigned long const poolOffset = Pools - ((Pool*) Base);
> + void *old_base = Base;
>
> if (Fd != 0)
> {
> @@ -408,7 +409,8 @@ bool DynamicMMap::Grow(unsigned long long size)
> memset((char*)Base + WorkSpace, 0, newSize - WorkSpace);
> }
>
> - Pools = (Pool*) Base + poolOffset;
> + if (Base != old_base)
> + Pools = RebasePointer(Pools, old_base, Base);
> WorkSpace = newSize;
>
> return true;
> diff --git a/apt/apt-pkg/pkgcachegen.cc b/apt/apt-pkg/pkgcachegen.cc
> index 654c81c..37364e9 100644
> --- a/apt/apt-pkg/pkgcachegen.cc
> +++ b/apt/apt-pkg/pkgcachegen.cc
> @@ -26,6 +26,7 @@
> #include <apt-pkg/strutl.h>
> #include <apt-pkg/sptr.h>
> #include <apt-pkg/pkgsystem.h>
> +#include <apt-pkg/rebase_pointer.h>
>
> #include <apti18n.h>
>
> @@ -109,18 +110,18 @@ pkgCacheGenerator::~pkgCacheGenerator()
> Map.Sync(0,sizeof(pkgCache::Header));
> }
> /*}}}*/
> -void pkgCacheGenerator::ReMap(void const * const oldMap, void const * const newMap)
> +void pkgCacheGenerator::ReMap(void *oldMap, void *newMap)
> {
> if (oldMap == newMap)
> return;
>
> Cache.ReMap(false);
>
> - CurrentFile += (pkgCache::PackageFile*) newMap - (pkgCache::PackageFile*) oldMap;
> + CurrentFile = RebasePointer(CurrentFile, oldMap, newMap);
>
> for (size_t i = 0; i < _count(UniqHash); ++i)
> if (UniqHash[i] != 0)
> - UniqHash[i] += (pkgCache::StringItem*) newMap - (pkgCache::StringItem*) oldMap;
> + UniqHash[i] = RebasePointer(UniqHash[i], oldMap, newMap);
>
> for (auto i = Dynamic<pkgCache::PkgIterator>::toReMap.begin();
> i != Dynamic<pkgCache::PkgIterator>::toReMap.end(); ++i)
> @@ -147,7 +148,7 @@ void pkgCacheGenerator::ReMap(void const * const oldMap, void const * const newM
> // CacheGenerator::WriteStringInMap /*{{{*/
> std::experimental::optional<map_ptrloc> pkgCacheGenerator::WriteStringInMap(const char *String,
> unsigned long Len) {
> - void const * const oldMap = Map.Data();
> + void *oldMap = Map.Data();
> const auto index = Map.WriteString(String, Len);
> if (index)
> ReMap(oldMap, Map.Data());
> @@ -156,7 +157,7 @@ std::experimental::optional<map_ptrloc> pkgCacheGenerator::WriteStringInMap(cons
> /*}}}*/
> // CacheGenerator::WriteStringInMap /*{{{*/
> std::experimental::optional<map_ptrloc> pkgCacheGenerator::WriteStringInMap(const char *String) {
> - void const * const oldMap = Map.Data();
> + void *oldMap = Map.Data();
> const auto index = Map.WriteString(String);
> if (index)
> ReMap(oldMap, Map.Data());
> @@ -164,7 +165,7 @@ std::experimental::optional<map_ptrloc> pkgCacheGenerator::WriteStringInMap(cons
> }
> /*}}}*/
> std::experimental::optional<map_ptrloc> pkgCacheGenerator::AllocateInMap(unsigned long size) {/*{{{*/
> - void const * const oldMap = Map.Data();
> + void *oldMap = Map.Data();
> const auto index = Map.Allocate(size);
> if (index)
> ReMap(oldMap, Map.Data());
> @@ -229,7 +230,7 @@ bool pkgCacheGenerator::MergeList(ListParser &List,
> pkgCache::VerIterator Ver = Pkg.VersionList();
> Dynamic<pkgCache::VerIterator> DynVer(Ver);
> map_ptrloc *Last = &Pkg->VersionList;
> - const void * oldMap = Map.Data();
> + void *oldMap = Map.Data();
> int Res = 1;
> for (; Ver.end() == false; Last = &Ver->NextVer, Ver++)
> {
> @@ -271,7 +272,7 @@ bool pkgCacheGenerator::MergeList(ListParser &List,
>
> if (oldMap != Map.Data())
> {
> - Last += (map_ptrloc*) Map.Data() - (map_ptrloc*) oldMap;
> + Last = RebasePointer(Last, oldMap, Map.Data());
> oldMap = Map.Data();
> }
>
> @@ -297,7 +298,7 @@ bool pkgCacheGenerator::MergeList(ListParser &List,
> PackageName.c_str(), 1);
>
> if (oldMap != Map.Data())
> - Last += (map_ptrloc*) Map.Data() - (map_ptrloc*) oldMap;
> + Last = RebasePointer(Last, oldMap, Map.Data());
> *Last = *verindex;
>
> Ver->ParentPkg = Pkg.Index();
> @@ -553,7 +554,7 @@ bool pkgCacheGenerator::ListParser::NewDepends(pkgCache::VerIterator &Ver,
> unsigned int Op,
> unsigned int Type)
> {
> - void const * const oldMap = Owner->Map.Data();
> + void *oldMap = Owner->Map.Data();
> pkgCache &Cache = Owner->Cache;
>
> // Get a structure
> @@ -605,7 +606,7 @@ bool pkgCacheGenerator::ListParser::NewDepends(pkgCache::VerIterator &Ver,
> OldDepLast = &D->NextDepends;
> OldDepVer = Ver;
> } else if (oldMap != Owner->Map.Data())
> - OldDepLast += (map_ptrloc*) Owner->Map.Data() - (map_ptrloc*) oldMap;
> + OldDepLast = RebasePointer(OldDepLast, oldMap, Owner->Map.Data());
>
> // Is it a file dependency?
> if (PackageName[0] == '/')
> @@ -739,7 +740,7 @@ std::experimental::optional<map_ptrloc> pkgCacheGenerator::WriteUniqString(const
> }
>
> // Get a structure
> - void const * const oldMap = Map.Data();
> + void *oldMap = Map.Data();
> const auto Item = AllocateInMap(sizeof(pkgCache::StringItem));
> const auto idxString = WriteStringInMap(S, Size);
> if ((!Item) || (!idxString))
> @@ -747,8 +748,8 @@ std::experimental::optional<map_ptrloc> pkgCacheGenerator::WriteUniqString(const
>
> if (oldMap != Map.Data())
> {
> - Last += (map_ptrloc*) Map.Data() - (map_ptrloc*) oldMap;
> - I += (pkgCache::StringItem*) Map.Data() - (pkgCache::StringItem*) oldMap;
> + Last = RebasePointer(Last, oldMap, Map.Data());
> + I = RebasePointer(I, oldMap, Map.Data());
> }
>
> *Last = *Item;
> diff --git a/apt/apt-pkg/pkgcachegen.h b/apt/apt-pkg/pkgcachegen.h
> index 77075a6..0faf6e1 100644
> --- a/apt/apt-pkg/pkgcachegen.h
> +++ b/apt/apt-pkg/pkgcachegen.h
> @@ -72,7 +72,7 @@ class pkgCacheGenerator
> class DynamicFunction
> {
> public:
> - typedef std::function<void(const void *, const void *)> function;
> + typedef std::function<void(void *, void *)> function;
>
> static std::set<DynamicFunction*> toReMap;
>
> @@ -93,7 +93,7 @@ class pkgCacheGenerator
> toReMap.erase(this);
> }
>
> - void call(const void *oldMap, const void *newMap)
> + void call(void *oldMap, void *newMap)
> {
> if (m_function)
> m_function(oldMap, newMap);
> @@ -140,7 +140,7 @@ class pkgCacheGenerator
> // CNC:2003-03-18
> inline void ResetFileDeps() {FoundFileDeps = false;};
>
> - void ReMap(void const * const oldMap, void const * const newMap);
> + void ReMap(void *oldMap, void *newMap);
>
> pkgCacheGenerator(DynamicMMap *Map,OpProgress *Progress);
> ~pkgCacheGenerator();
> diff --git a/apt/apt-pkg/rebase_pointer.h b/apt/apt-pkg/rebase_pointer.h
> new file mode 100644
> index 0000000..f6b3c15
> --- /dev/null
> +++ b/apt/apt-pkg/rebase_pointer.h
> @@ -0,0 +1,16 @@
> +#ifndef PKGLIB_REBASE_POINTER_H
> +#define PKGLIB_REBASE_POINTER_H
> +
> +template <typename T>
> +static inline T* RebasePointer(T *ptr, void *old_base, void *new_base)
> +{
> + return reinterpret_cast<T*>(reinterpret_cast<char*>(new_base) + (reinterpret_cast<char*>(ptr) - reinterpret_cast<char*>(old_base)));
> +}
> +
> +template <typename T>
> +static inline const T* RebasePointer(const T *ptr, void *old_base, void *new_base)
> +{
> + return reinterpret_cast<const T*>(reinterpret_cast<char*>(new_base) + (reinterpret_cast<const char*>(ptr) - reinterpret_cast<char*>(old_base)));
> +}
> +
> +#endif
> diff --git a/apt/apt-pkg/rpm/rpmlistparser.cc b/apt/apt-pkg/rpm/rpmlistparser.cc
> index 9b2e9ad..bd13b99 100644
> --- a/apt/apt-pkg/rpm/rpmlistparser.cc
> +++ b/apt/apt-pkg/rpm/rpmlistparser.cc
> @@ -25,6 +25,7 @@
> #include <apt-pkg/strutl.h>
> #include <apt-pkg/crc-16.h>
> #include <apt-pkg/tagfile.h>
> +#include <apt-pkg/rebase_pointer.h>
>
> #include <apti18n.h>
>
> @@ -50,13 +51,13 @@ rpmListParser::rpmListParser(RPMHandler *Handler)
> {
> SeenPackages.reset(new SeenPackagesType);
> m_SeenPackagesRealloc.reset(new pkgCacheGenerator::DynamicFunction(
> - [this] (void const *oldMap, void const *newMap)
> + [this] (void *oldMap, void *newMap)
> {
> SeenPackagesType tmp;
>
> for (auto iter: *SeenPackages)
> {
> - tmp.insert(iter + (static_cast<const char *>(newMap) - static_cast<const char *>(oldMap)));
> + tmp.insert(RebasePointer(iter, oldMap, newMap));
> }
>
> SeenPackages->swap(tmp);
> diff --git a/apt/apt-pkg/rpm/rpmpackagedata.cc b/apt/apt-pkg/rpm/rpmpackagedata.cc
> index 61c15d5..d6c677b 100644
> --- a/apt/apt-pkg/rpm/rpmpackagedata.cc
> +++ b/apt/apt-pkg/rpm/rpmpackagedata.cc
> @@ -23,7 +23,7 @@ RPMPackageData::RPMPackageData()
> #endif
> {
> m_VerMapRealloc.reset(new pkgCacheGenerator::DynamicFunction(
> - [this] (void const *oldMap, void const *newMap)
> + [this] (void *oldMap, void *newMap)
> {
> for (auto iter1 = VerMap.begin(); iter1 != VerMap.end(); ++iter1)
> {
Best regards,
Andrew Savchenko
[-- Attachment #2: Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [devel] [PATCH for apt 1/2] Add option for debugging DynamicMMap::Allocate
2019-12-06 13:16 [devel] [PATCH for apt 1/2] Add option for debugging DynamicMMap::Allocate Aleksei Nikiforov
2019-12-06 13:16 ` [devel] [PATCH for apt 2/2] Fix pointer arithmetics Aleksei Nikiforov
@ 2019-12-08 22:50 ` Dmitry V. Levin
2019-12-09 6:58 ` Aleksei Nikiforov
2019-12-09 7:00 ` [devel] [PATCH for apt 1/2 v2] Add option for debugging Debug::DynamicMMap::Allocate Aleksei Nikiforov
1 sibling, 2 replies; 34+ messages in thread
From: Dmitry V. Levin @ 2019-12-08 22:50 UTC (permalink / raw)
To: Aleksei Nikiforov; +Cc: ALT Devel discussion list
[-- Attachment #1: Type: text/plain, Size: 2978 bytes --]
On Fri, Dec 06, 2019 at 04:16:05PM +0300, Aleksei Nikiforov wrote:
> ---
> apt/apt-pkg/contrib/mmap.cc | 15 +++++++++++++++
> apt/doc/apt.conf.5.sgml | 2 +-
> 2 files changed, 16 insertions(+), 1 deletion(-)
I suggest adding the name of the new option to the commit message
so it would be git-grep'able, for example:
Add Debug::DynamicMMap::Allocate option
Add a new option for debugging DynamicMMap::Allocate.
> diff --git a/apt/apt-pkg/contrib/mmap.cc b/apt/apt-pkg/contrib/mmap.cc
> index a3b06cc..cf01be9 100644
> --- a/apt/apt-pkg/contrib/mmap.cc
> +++ b/apt/apt-pkg/contrib/mmap.cc
> @@ -265,6 +265,21 @@ std::experimental::optional<map_ptrloc> DynamicMMap::Allocate(unsigned long Item
> // No pool is allocated, use an unallocated one
> if (I == Pools + PoolCount)
> {
> + static const bool debug_grow = _config->FindB("Debug::DynamicMMap::Allocate", false);
> +
> + if (debug_grow)
> + {
> + Pool *pool_iter = Pools;
> + size_t pool_idx = 0;
> +
> + _error->Warning(_("DynamicMMap::Allocate: allocating item of size %lu"), ItemSize);
> +
> + for (; pool_idx < PoolCount; ++pool_iter, ++pool_idx)
> + {
> + _error->Warning(_("DynamicMMap::Allocate: Pool %zu, item size: %lu, start: %lu, count: %lu"), pool_idx, pool_iter->ItemSize, pool_iter->Start, pool_iter->Count);
> + }
> + }
Nitpicking:
- this is debugging, there is no need to optimize, so there is no need for
pool_iter;
- let's use "for" loop initial declarations, they are widely used in apt
already;
- the type of iterator should be the same as the type of PoolCount;
- lines are too long.
Consider something like this (untested):
if (debug_grow) {
_error->Warning(_("DynamicMMap::Allocate: allocating item of size %lu"),
ItemSize);
for (unsigned int i = 0; i < PoolCount; ++i) {
_error->Warning(_("DynamicMMap::Allocate: Pool %u, item size: %lu"
", start: %lu, count: %lu"),
i, Pools[i].ItemSize,
Pools[i].Start, Pools[i].Count);
}
}
> // Woops, we ran out, the calling code should allocate more.
> if (Empty == 0)
> {
> diff --git a/apt/doc/apt.conf.5.sgml b/apt/doc/apt.conf.5.sgml
> index 0a72e45..72fc0c3 100644
> --- a/apt/doc/apt.conf.5.sgml
> +++ b/apt/doc/apt.conf.5.sgml
> @@ -526,7 +526,7 @@ DPkg::Pre-Install-Pkgs {"/usr/sbin/dpkg-preconfigure --apt";};
> disable the inclusion of statfs data in CDROM IDs.
> </para><para>
> To debug issues related to dynamic memory allocation, an option
> - <literal/Debug::DynamicMMap::Grow/ may be used.
> + <literal/Debug::DynamicMMap::Grow/ and <literal/Debug::DynamicMMap::Allocate/ may be used.
As we've got two options now, I suggest s/an option/options/ .
--
ldv
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [devel] [PATCH for apt 2/2 v2] Fix pointer arithmetics
2019-12-07 14:52 ` Andrey Savchenko
@ 2019-12-08 22:56 ` Dmitry V. Levin
2019-12-09 6:54 ` Aleksei Nikiforov
1 sibling, 0 replies; 34+ messages in thread
From: Dmitry V. Levin @ 2019-12-08 22:56 UTC (permalink / raw)
To: ALT Devel discussion list
[-- Attachment #1: Type: text/plain, Size: 912 bytes --]
On Sat, Dec 07, 2019 at 05:52:01PM +0300, Andrey Savchenko wrote:
> Hi all!
>
> On Fri, 6 Dec 2019 18:36:55 +0300 Aleksei Nikiforov wrote:
> > This change should fix pointer arithmetic issues for e2k.
>
> This commit message is misleading. Pointer arithmetic is broken on
> all architectures, but by pure chance in happens to work correctly
> when gcc is used. On e2k the lcc compiler is used, so problem
> manifests itself. But this doesn't change the fact that pointer
> arithmetic was flawed on all architectures.
Strictly speaking, it's not "broken", it's "UB", but I agree
it is not specific to e2k.
My original commit message for this change was the following:
"Fix UB in pointer arithmetic
Commit 6d5e6a689d07de8feef2cbecb24bc42d5994861b aka 0.5.15lorg2-alt70~9
among other changes introduced UB in pointer arithmetic by casting raw
pointers to specific types."
--
ldv
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [devel] [PATCH for apt 2/2 v2] Fix pointer arithmetics
2019-12-06 15:36 ` [devel] [PATCH for apt 2/2 v2] " Aleksei Nikiforov
2019-12-07 14:52 ` Andrey Savchenko
@ 2019-12-08 23:21 ` Dmitry V. Levin
2019-12-09 7:08 ` Aleksei Nikiforov
1 sibling, 1 reply; 34+ messages in thread
From: Dmitry V. Levin @ 2019-12-08 23:21 UTC (permalink / raw)
To: Aleksei Nikiforov; +Cc: ALT Devel discussion list
[-- Attachment #1: Type: text/plain, Size: 2012 bytes --]
On Fri, Dec 06, 2019 at 06:36:55PM +0300, Aleksei Nikiforov wrote:
[...]
> @@ -85,11 +87,11 @@ class pkgCache::PkgIterator
> inline unsigned long long Index() const {return Pkg - Owner->PkgP;};
> OkState State() const;
>
> - void ReMap(void const * const oldMap, void const * const newMap)
> + void ReMap(void *oldMap, void *newMap)
Is there any particular reason for stripping const here and in other
similar places?
[...]
> @@ -301,7 +302,7 @@ std::experimental::optional<map_ptrloc> DynamicMMap::Allocate(unsigned long Item
> Pool* oldPools = Pools;
> auto idxResult = RawAllocate(I->Count*ItemSize,ItemSize);
> if (Pools != oldPools)
> - I += Pools - oldPools;
> + I = RebasePointer(I, oldPools, Pools);
>
> // Does the allocation failed ?
> if (!idxResult)
In my patch RebasePointer invocation was after the idxResult check,
not before the check.
By the way, in this and other similar cases,
is there any reason for "Pools != oldPools" check?
Is RebasePointer incapable of handling this, or is it an optimization?
[...]
> diff --git a/apt/apt-pkg/rebase_pointer.h b/apt/apt-pkg/rebase_pointer.h
> new file mode 100644
> index 0000000..f6b3c15
> --- /dev/null
> +++ b/apt/apt-pkg/rebase_pointer.h
> @@ -0,0 +1,16 @@
> +#ifndef PKGLIB_REBASE_POINTER_H
> +#define PKGLIB_REBASE_POINTER_H
> +
> +template <typename T>
> +static inline T* RebasePointer(T *ptr, void *old_base, void *new_base)
> +{
> + return reinterpret_cast<T*>(reinterpret_cast<char*>(new_base) + (reinterpret_cast<char*>(ptr) - reinterpret_cast<char*>(old_base)));
> +}
> +
> +template <typename T>
> +static inline const T* RebasePointer(const T *ptr, void *old_base, void *new_base)
> +{
> + return reinterpret_cast<const T*>(reinterpret_cast<char*>(new_base) + (reinterpret_cast<const char*>(ptr) - reinterpret_cast<char*>(old_base)));
> +}
> +
> +#endif
Do we really need two templates here?
--
ldv
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [devel] [PATCH for apt 2/2] Fix pointer arithmetics
2019-12-06 13:16 ` [devel] [PATCH for apt 2/2] Fix pointer arithmetics Aleksei Nikiforov
2019-12-06 13:36 ` Ivan A. Melnikov
@ 2019-12-08 23:31 ` Dmitry V. Levin
2019-12-09 7:09 ` Aleksei Nikiforov
2019-12-09 7:16 ` [devel] [PATCH for apt 2/2 v3] " Aleksei Nikiforov
2019-12-09 23:54 ` [devel] [PATCH apt 0/3] Fix 0.5.15lorg2-alt70~9 fallout Dmitry V. Levin
2 siblings, 2 replies; 34+ messages in thread
From: Dmitry V. Levin @ 2019-12-08 23:31 UTC (permalink / raw)
To: Aleksei Nikiforov; +Cc: ALT Devel discussion list
[-- Attachment #1: Type: text/plain, Size: 1215 bytes --]
On Fri, Dec 06, 2019 at 04:16:06PM +0300, Aleksei Nikiforov wrote:
[...]
> diff --git a/apt/apt-pkg/rebase_pointer.h b/apt/apt-pkg/rebase_pointer.h
> new file mode 100644
> index 0000000..efc4074
> --- /dev/null
> +++ b/apt/apt-pkg/rebase_pointer.h
> @@ -0,0 +1,16 @@
> +#ifndef PKGLIB_REBASE_POINTER_H
> +#define PKGLIB_REBASE_POINTER_H
> +
> +template <typename T>
> +static inline T* RebasePointer(T *ptr, const void * const old_base, const void * const new_base)
> +{
> + return reinterpret_cast<T*>(reinterpret_cast<char*>(ptr) + (reinterpret_cast<char const * const>(new_base) - reinterpret_cast<char const * const>(old_base)));
> +}
> +
> +template <typename T>
> +static inline const T* RebasePointer(const T *ptr, const void * const old_base, const void * const new_base)
> +{
> + return reinterpret_cast<const T*>(reinterpret_cast<const char*>(ptr) + (reinterpret_cast<char const * const>(new_base) - reinterpret_cast<char const * const>(old_base)));
This line is way too long - about twice longer than a normal line of code.
Please break long lines.
My edition of rebase_pointer.h had the maximum length of all lines within
the traditional 80-symbol limit.
--
ldv
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [devel] [PATCH for apt 2/2 v2] Fix pointer arithmetics
2019-12-07 14:52 ` Andrey Savchenko
2019-12-08 22:56 ` Dmitry V. Levin
@ 2019-12-09 6:54 ` Aleksei Nikiforov
2019-12-12 19:20 ` Andrey Savchenko
1 sibling, 1 reply; 34+ messages in thread
From: Aleksei Nikiforov @ 2019-12-09 6:54 UTC (permalink / raw)
To: devel
07.12.2019 17:52, Andrey Savchenko пишет:
> Hi all!
>
> On Fri, 6 Dec 2019 18:36:55 +0300 Aleksei Nikiforov wrote:
>> This change should fix pointer arithmetic issues for e2k.
>
> This commit message is misleading. Pointer arithmetic is broken on
> all architectures, but by pure chance in happens to work correctly
> when gcc is used. On e2k the lcc compiler is used, so problem
> manifests itself. But this doesn't change the fact that pointer
> arithmetic was flawed on all architectures.
>
Your first sentence contradicts later ones. You just repeated my
sentence about issue manifesting on e2k, but you did it more verbosely.
Yes, there is a flaw for all architectures, but it manifests only for
e2k in practice, thus change fixes it for e2k. So, what's misleading there?
>> ---
>> apt/apt-pkg/Makefile.am | 1 +
>> apt/apt-pkg/cacheiterators.h | 26 ++++++++++++++------------
>> apt/apt-pkg/contrib/mmap.cc | 8 +++++---
>> apt/apt-pkg/pkgcachegen.cc | 29 +++++++++++++++--------------
>> apt/apt-pkg/pkgcachegen.h | 6 +++---
>> apt/apt-pkg/rebase_pointer.h | 16 ++++++++++++++++
>> apt/apt-pkg/rpm/rpmlistparser.cc | 5 +++--
>> apt/apt-pkg/rpm/rpmpackagedata.cc | 2 +-
>> 8 files changed, 58 insertions(+), 35 deletions(-)
>> create mode 100644 apt/apt-pkg/rebase_pointer.h
>>
>> diff --git a/apt/apt-pkg/Makefile.am b/apt/apt-pkg/Makefile.am
>> index 4c0d234..d038d01 100644
>> --- a/apt/apt-pkg/Makefile.am
>> +++ b/apt/apt-pkg/Makefile.am
>> @@ -94,6 +94,7 @@ libapt_pkg_la_SOURCES = \
>> pkgsystem.h \
>> policy.cc \
>> policy.h \
>> + rebase_pointer.h \
>> repository.cc \
>> repository.h \
>> scopeexit.h \
>> diff --git a/apt/apt-pkg/cacheiterators.h b/apt/apt-pkg/cacheiterators.h
>> index a4bf670..118838e 100644
>> --- a/apt/apt-pkg/cacheiterators.h
>> +++ b/apt/apt-pkg/cacheiterators.h
>> @@ -34,6 +34,8 @@
>> #pragma interface "apt-pkg/cacheiterators.h"
>> #endif
>>
>> +#include <apt-pkg/rebase_pointer.h>
>> +
>> #include <set>
>>
>> // Package Iterator
>> @@ -85,11 +87,11 @@ class pkgCache::PkgIterator
>> inline unsigned long long Index() const {return Pkg - Owner->PkgP;};
>> OkState State() const;
>>
>> - void ReMap(void const * const oldMap, void const * const newMap)
>> + void ReMap(void *oldMap, void *newMap)
>> {
>> if (Owner == 0 || Pkg == 0)
>> return;
>> - Pkg += static_cast<Package const *>(newMap) - static_cast<Package const *>(oldMap);
>> + Pkg = RebasePointer(Pkg, oldMap, newMap);
>> }
>>
>> // Constructors
>> @@ -147,11 +149,11 @@ class pkgCache::VerIterator
>> bool Automatic() const;
>> VerFileIterator NewestFile() const;
>>
>> - void ReMap(void const * const oldMap, void const * const newMap)
>> + void ReMap(void *oldMap, void *newMap)
>> {
>> if (Owner == 0 || Ver == 0)
>> return;
>> - Ver += static_cast<Version const *>(newMap) - static_cast<Version const *>(oldMap);
>> + Ver = RebasePointer(Ver, oldMap, newMap);
>> }
>>
>> inline VerIterator() : Ver(0), Owner(0) {};
>> @@ -220,11 +222,11 @@ class pkgCache::DepIterator
>> inline const char *CompType() {return Owner->CompType(Dep->CompareOp);};
>> inline const char *DepType() {return Owner->DepType(Dep->Type);};
>>
>> - void ReMap(void const * const oldMap, void const * const newMap)
>> + void ReMap(void *oldMap, void *newMap)
>> {
>> if (Owner == 0 || Dep == 0)
>> return;
>> - Dep += static_cast<Dependency const *>(newMap) - static_cast<Dependency const *>(oldMap);
>> + Dep = RebasePointer(Dep, oldMap, newMap);
>> }
>>
>> inline DepIterator(pkgCache &Owner,Dependency *Trg,Version * = 0) :
>> @@ -279,11 +281,11 @@ class pkgCache::PrvIterator
>> inline PkgIterator OwnerPkg() const {return PkgIterator(*Owner,Owner->PkgP + Owner->VerP[Prv->Version].ParentPkg);};
>> inline unsigned long long Index() const {return Prv - Owner->ProvideP;};
>>
>> - void ReMap(void const * const oldMap, void const * const newMap)
>> + void ReMap(void *oldMap, void *newMap)
>> {
>> if (Owner == 0 || Prv == 0)
>> return;
>> - Prv += static_cast<Provides const *>(newMap) - static_cast<Provides const *>(oldMap);
>> + Prv = RebasePointer(Prv, oldMap, newMap);
>> }
>>
>> inline PrvIterator() : Prv(0), Type(PrvVer), Owner(0) {};
>> @@ -342,11 +344,11 @@ class pkgCache::PkgFileIterator
>> bool IsOk();
>> string RelStr();
>>
>> - void ReMap(void const * const oldMap, void const * const newMap)
>> + void ReMap(void *oldMap, void *newMap)
>> {
>> if (Owner == 0 || File == 0)
>> return;
>> - File += static_cast<PackageFile const *>(newMap) - static_cast<PackageFile const *>(oldMap);
>> + File = RebasePointer(File, oldMap, newMap);
>> }
>>
>> // Constructors
>> @@ -383,11 +385,11 @@ class pkgCache::VerFileIterator
>> inline PkgFileIterator File() const {return PkgFileIterator(*Owner,FileP->File + Owner->PkgFileP);};
>> inline unsigned long long Index() const {return FileP - Owner->VerFileP;};
>>
>> - void ReMap(void const * const oldMap, void const * const newMap)
>> + void ReMap(void *oldMap, void *newMap)
>> {
>> if (Owner == 0 || FileP == 0)
>> return;
>> - FileP += static_cast<VerFile const *>(newMap) - static_cast<VerFile const *>(oldMap);
>> + FileP = RebasePointer(FileP, oldMap, newMap);
>> }
>>
>> inline VerFileIterator() : Owner(0), FileP(0) {};
>> diff --git a/apt/apt-pkg/contrib/mmap.cc b/apt/apt-pkg/contrib/mmap.cc
>> index cf01be9..35f1a25 100644
>> --- a/apt/apt-pkg/contrib/mmap.cc
>> +++ b/apt/apt-pkg/contrib/mmap.cc
>> @@ -30,6 +30,7 @@
>> #include <apt-pkg/configuration.h>
>> #include <apt-pkg/mmap.h>
>> #include <apt-pkg/error.h>
>> +#include <apt-pkg/rebase_pointer.h>
>>
>> #include <apti18n.h>
>>
>> @@ -301,7 +302,7 @@ std::experimental::optional<map_ptrloc> DynamicMMap::Allocate(unsigned long Item
>> Pool* oldPools = Pools;
>> auto idxResult = RawAllocate(I->Count*ItemSize,ItemSize);
>> if (Pools != oldPools)
>> - I += Pools - oldPools;
>> + I = RebasePointer(I, oldPools, Pools);
>>
>> // Does the allocation failed ?
>> if (!idxResult)
>> @@ -371,7 +372,7 @@ bool DynamicMMap::Grow(unsigned long long size)
>> Fd->Write(&C,sizeof(C));
>> }
>>
>> - unsigned long const poolOffset = Pools - ((Pool*) Base);
>> + void *old_base = Base;
>>
>> if (Fd != 0)
>> {
>> @@ -408,7 +409,8 @@ bool DynamicMMap::Grow(unsigned long long size)
>> memset((char*)Base + WorkSpace, 0, newSize - WorkSpace);
>> }
>>
>> - Pools = (Pool*) Base + poolOffset;
>> + if (Base != old_base)
>> + Pools = RebasePointer(Pools, old_base, Base);
>> WorkSpace = newSize;
>>
>> return true;
>> diff --git a/apt/apt-pkg/pkgcachegen.cc b/apt/apt-pkg/pkgcachegen.cc
>> index 654c81c..37364e9 100644
>> --- a/apt/apt-pkg/pkgcachegen.cc
>> +++ b/apt/apt-pkg/pkgcachegen.cc
>> @@ -26,6 +26,7 @@
>> #include <apt-pkg/strutl.h>
>> #include <apt-pkg/sptr.h>
>> #include <apt-pkg/pkgsystem.h>
>> +#include <apt-pkg/rebase_pointer.h>
>>
>> #include <apti18n.h>
>>
>> @@ -109,18 +110,18 @@ pkgCacheGenerator::~pkgCacheGenerator()
>> Map.Sync(0,sizeof(pkgCache::Header));
>> }
>> /*}}}*/
>> -void pkgCacheGenerator::ReMap(void const * const oldMap, void const * const newMap)
>> +void pkgCacheGenerator::ReMap(void *oldMap, void *newMap)
>> {
>> if (oldMap == newMap)
>> return;
>>
>> Cache.ReMap(false);
>>
>> - CurrentFile += (pkgCache::PackageFile*) newMap - (pkgCache::PackageFile*) oldMap;
>> + CurrentFile = RebasePointer(CurrentFile, oldMap, newMap);
>>
>> for (size_t i = 0; i < _count(UniqHash); ++i)
>> if (UniqHash[i] != 0)
>> - UniqHash[i] += (pkgCache::StringItem*) newMap - (pkgCache::StringItem*) oldMap;
>> + UniqHash[i] = RebasePointer(UniqHash[i], oldMap, newMap);
>>
>> for (auto i = Dynamic<pkgCache::PkgIterator>::toReMap.begin();
>> i != Dynamic<pkgCache::PkgIterator>::toReMap.end(); ++i)
>> @@ -147,7 +148,7 @@ void pkgCacheGenerator::ReMap(void const * const oldMap, void const * const newM
>> // CacheGenerator::WriteStringInMap /*{{{*/
>> std::experimental::optional<map_ptrloc> pkgCacheGenerator::WriteStringInMap(const char *String,
>> unsigned long Len) {
>> - void const * const oldMap = Map.Data();
>> + void *oldMap = Map.Data();
>> const auto index = Map.WriteString(String, Len);
>> if (index)
>> ReMap(oldMap, Map.Data());
>> @@ -156,7 +157,7 @@ std::experimental::optional<map_ptrloc> pkgCacheGenerator::WriteStringInMap(cons
>> /*}}}*/
>> // CacheGenerator::WriteStringInMap /*{{{*/
>> std::experimental::optional<map_ptrloc> pkgCacheGenerator::WriteStringInMap(const char *String) {
>> - void const * const oldMap = Map.Data();
>> + void *oldMap = Map.Data();
>> const auto index = Map.WriteString(String);
>> if (index)
>> ReMap(oldMap, Map.Data());
>> @@ -164,7 +165,7 @@ std::experimental::optional<map_ptrloc> pkgCacheGenerator::WriteStringInMap(cons
>> }
>> /*}}}*/
>> std::experimental::optional<map_ptrloc> pkgCacheGenerator::AllocateInMap(unsigned long size) {/*{{{*/
>> - void const * const oldMap = Map.Data();
>> + void *oldMap = Map.Data();
>> const auto index = Map.Allocate(size);
>> if (index)
>> ReMap(oldMap, Map.Data());
>> @@ -229,7 +230,7 @@ bool pkgCacheGenerator::MergeList(ListParser &List,
>> pkgCache::VerIterator Ver = Pkg.VersionList();
>> Dynamic<pkgCache::VerIterator> DynVer(Ver);
>> map_ptrloc *Last = &Pkg->VersionList;
>> - const void * oldMap = Map.Data();
>> + void *oldMap = Map.Data();
>> int Res = 1;
>> for (; Ver.end() == false; Last = &Ver->NextVer, Ver++)
>> {
>> @@ -271,7 +272,7 @@ bool pkgCacheGenerator::MergeList(ListParser &List,
>>
>> if (oldMap != Map.Data())
>> {
>> - Last += (map_ptrloc*) Map.Data() - (map_ptrloc*) oldMap;
>> + Last = RebasePointer(Last, oldMap, Map.Data());
>> oldMap = Map.Data();
>> }
>>
>> @@ -297,7 +298,7 @@ bool pkgCacheGenerator::MergeList(ListParser &List,
>> PackageName.c_str(), 1);
>>
>> if (oldMap != Map.Data())
>> - Last += (map_ptrloc*) Map.Data() - (map_ptrloc*) oldMap;
>> + Last = RebasePointer(Last, oldMap, Map.Data());
>> *Last = *verindex;
>>
>> Ver->ParentPkg = Pkg.Index();
>> @@ -553,7 +554,7 @@ bool pkgCacheGenerator::ListParser::NewDepends(pkgCache::VerIterator &Ver,
>> unsigned int Op,
>> unsigned int Type)
>> {
>> - void const * const oldMap = Owner->Map.Data();
>> + void *oldMap = Owner->Map.Data();
>> pkgCache &Cache = Owner->Cache;
>>
>> // Get a structure
>> @@ -605,7 +606,7 @@ bool pkgCacheGenerator::ListParser::NewDepends(pkgCache::VerIterator &Ver,
>> OldDepLast = &D->NextDepends;
>> OldDepVer = Ver;
>> } else if (oldMap != Owner->Map.Data())
>> - OldDepLast += (map_ptrloc*) Owner->Map.Data() - (map_ptrloc*) oldMap;
>> + OldDepLast = RebasePointer(OldDepLast, oldMap, Owner->Map.Data());
>>
>> // Is it a file dependency?
>> if (PackageName[0] == '/')
>> @@ -739,7 +740,7 @@ std::experimental::optional<map_ptrloc> pkgCacheGenerator::WriteUniqString(const
>> }
>>
>> // Get a structure
>> - void const * const oldMap = Map.Data();
>> + void *oldMap = Map.Data();
>> const auto Item = AllocateInMap(sizeof(pkgCache::StringItem));
>> const auto idxString = WriteStringInMap(S, Size);
>> if ((!Item) || (!idxString))
>> @@ -747,8 +748,8 @@ std::experimental::optional<map_ptrloc> pkgCacheGenerator::WriteUniqString(const
>>
>> if (oldMap != Map.Data())
>> {
>> - Last += (map_ptrloc*) Map.Data() - (map_ptrloc*) oldMap;
>> - I += (pkgCache::StringItem*) Map.Data() - (pkgCache::StringItem*) oldMap;
>> + Last = RebasePointer(Last, oldMap, Map.Data());
>> + I = RebasePointer(I, oldMap, Map.Data());
>> }
>>
>> *Last = *Item;
>> diff --git a/apt/apt-pkg/pkgcachegen.h b/apt/apt-pkg/pkgcachegen.h
>> index 77075a6..0faf6e1 100644
>> --- a/apt/apt-pkg/pkgcachegen.h
>> +++ b/apt/apt-pkg/pkgcachegen.h
>> @@ -72,7 +72,7 @@ class pkgCacheGenerator
>> class DynamicFunction
>> {
>> public:
>> - typedef std::function<void(const void *, const void *)> function;
>> + typedef std::function<void(void *, void *)> function;
>>
>> static std::set<DynamicFunction*> toReMap;
>>
>> @@ -93,7 +93,7 @@ class pkgCacheGenerator
>> toReMap.erase(this);
>> }
>>
>> - void call(const void *oldMap, const void *newMap)
>> + void call(void *oldMap, void *newMap)
>> {
>> if (m_function)
>> m_function(oldMap, newMap);
>> @@ -140,7 +140,7 @@ class pkgCacheGenerator
>> // CNC:2003-03-18
>> inline void ResetFileDeps() {FoundFileDeps = false;};
>>
>> - void ReMap(void const * const oldMap, void const * const newMap);
>> + void ReMap(void *oldMap, void *newMap);
>>
>> pkgCacheGenerator(DynamicMMap *Map,OpProgress *Progress);
>> ~pkgCacheGenerator();
>> diff --git a/apt/apt-pkg/rebase_pointer.h b/apt/apt-pkg/rebase_pointer.h
>> new file mode 100644
>> index 0000000..f6b3c15
>> --- /dev/null
>> +++ b/apt/apt-pkg/rebase_pointer.h
>> @@ -0,0 +1,16 @@
>> +#ifndef PKGLIB_REBASE_POINTER_H
>> +#define PKGLIB_REBASE_POINTER_H
>> +
>> +template <typename T>
>> +static inline T* RebasePointer(T *ptr, void *old_base, void *new_base)
>> +{
>> + return reinterpret_cast<T*>(reinterpret_cast<char*>(new_base) + (reinterpret_cast<char*>(ptr) - reinterpret_cast<char*>(old_base)));
>> +}
>> +
>> +template <typename T>
>> +static inline const T* RebasePointer(const T *ptr, void *old_base, void *new_base)
>> +{
>> + return reinterpret_cast<const T*>(reinterpret_cast<char*>(new_base) + (reinterpret_cast<const char*>(ptr) - reinterpret_cast<char*>(old_base)));
>> +}
>> +
>> +#endif
>> diff --git a/apt/apt-pkg/rpm/rpmlistparser.cc b/apt/apt-pkg/rpm/rpmlistparser.cc
>> index 9b2e9ad..bd13b99 100644
>> --- a/apt/apt-pkg/rpm/rpmlistparser.cc
>> +++ b/apt/apt-pkg/rpm/rpmlistparser.cc
>> @@ -25,6 +25,7 @@
>> #include <apt-pkg/strutl.h>
>> #include <apt-pkg/crc-16.h>
>> #include <apt-pkg/tagfile.h>
>> +#include <apt-pkg/rebase_pointer.h>
>>
>> #include <apti18n.h>
>>
>> @@ -50,13 +51,13 @@ rpmListParser::rpmListParser(RPMHandler *Handler)
>> {
>> SeenPackages.reset(new SeenPackagesType);
>> m_SeenPackagesRealloc.reset(new pkgCacheGenerator::DynamicFunction(
>> - [this] (void const *oldMap, void const *newMap)
>> + [this] (void *oldMap, void *newMap)
>> {
>> SeenPackagesType tmp;
>>
>> for (auto iter: *SeenPackages)
>> {
>> - tmp.insert(iter + (static_cast<const char *>(newMap) - static_cast<const char *>(oldMap)));
>> + tmp.insert(RebasePointer(iter, oldMap, newMap));
>> }
>>
>> SeenPackages->swap(tmp);
>> diff --git a/apt/apt-pkg/rpm/rpmpackagedata.cc b/apt/apt-pkg/rpm/rpmpackagedata.cc
>> index 61c15d5..d6c677b 100644
>> --- a/apt/apt-pkg/rpm/rpmpackagedata.cc
>> +++ b/apt/apt-pkg/rpm/rpmpackagedata.cc
>> @@ -23,7 +23,7 @@ RPMPackageData::RPMPackageData()
>> #endif
>> {
>> m_VerMapRealloc.reset(new pkgCacheGenerator::DynamicFunction(
>> - [this] (void const *oldMap, void const *newMap)
>> + [this] (void *oldMap, void *newMap)
>> {
>> for (auto iter1 = VerMap.begin(); iter1 != VerMap.end(); ++iter1)
>> {
>
>
> Best regards,
> Andrew Savchenko
>
>
> _______________________________________________
> Devel mailing list
> Devel@lists.altlinux.org
> https://lists.altlinux.org/mailman/listinfo/devel
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [devel] [PATCH for apt 1/2] Add option for debugging DynamicMMap::Allocate
2019-12-08 22:50 ` [devel] [PATCH for apt 1/2] Add option for debugging DynamicMMap::Allocate Dmitry V. Levin
@ 2019-12-09 6:58 ` Aleksei Nikiforov
2019-12-09 10:24 ` Dmitry V. Levin
2019-12-09 7:00 ` [devel] [PATCH for apt 1/2 v2] Add option for debugging Debug::DynamicMMap::Allocate Aleksei Nikiforov
1 sibling, 1 reply; 34+ messages in thread
From: Aleksei Nikiforov @ 2019-12-09 6:58 UTC (permalink / raw)
To: devel
09.12.2019 1:50, Dmitry V. Levin пишет:
> On Fri, Dec 06, 2019 at 04:16:05PM +0300, Aleksei Nikiforov wrote:
>> ---
>> apt/apt-pkg/contrib/mmap.cc | 15 +++++++++++++++
>> apt/doc/apt.conf.5.sgml | 2 +-
>> 2 files changed, 16 insertions(+), 1 deletion(-)
>
> I suggest adding the name of the new option to the commit message
> so it would be git-grep'able, for example:
>
> Add Debug::DynamicMMap::Allocate option
>
> Add a new option for debugging DynamicMMap::Allocate.
>
But option name is already in commit message. See the email subject
which is also a part of commit message. Why is it needed to duplicate it?
I'll update option name in subject to completely match full option name.
>> diff --git a/apt/apt-pkg/contrib/mmap.cc b/apt/apt-pkg/contrib/mmap.cc
>> index a3b06cc..cf01be9 100644
>> --- a/apt/apt-pkg/contrib/mmap.cc
>> +++ b/apt/apt-pkg/contrib/mmap.cc
>> @@ -265,6 +265,21 @@ std::experimental::optional<map_ptrloc> DynamicMMap::Allocate(unsigned long Item
>> // No pool is allocated, use an unallocated one
>> if (I == Pools + PoolCount)
>> {
>> + static const bool debug_grow = _config->FindB("Debug::DynamicMMap::Allocate", false);
>> +
>> + if (debug_grow)
>> + {
>> + Pool *pool_iter = Pools;
>> + size_t pool_idx = 0;
>> +
>> + _error->Warning(_("DynamicMMap::Allocate: allocating item of size %lu"), ItemSize);
>> +
>> + for (; pool_idx < PoolCount; ++pool_iter, ++pool_idx)
>> + {
>> + _error->Warning(_("DynamicMMap::Allocate: Pool %zu, item size: %lu, start: %lu, count: %lu"), pool_idx, pool_iter->ItemSize, pool_iter->Start, pool_iter->Count);
>> + }
>> + }
>
> Nitpicking:
> - this is debugging, there is no need to optimize, so there is no need for
> pool_iter;
> - let's use "for" loop initial declarations, they are widely used in apt
> already;
> - the type of iterator should be the same as the type of PoolCount;
> - lines are too long.
>
> Consider something like this (untested):
>
> if (debug_grow) {
> _error->Warning(_("DynamicMMap::Allocate: allocating item of size %lu"),
> ItemSize);
>
> for (unsigned int i = 0; i < PoolCount; ++i) {
> _error->Warning(_("DynamicMMap::Allocate: Pool %u, item size: %lu"
> ", start: %lu, count: %lu"),
> i, Pools[i].ItemSize,
> Pools[i].Start, Pools[i].Count);
> }
> }
>
>> // Woops, we ran out, the calling code should allocate more.
>> if (Empty == 0)
>> {
>> diff --git a/apt/doc/apt.conf.5.sgml b/apt/doc/apt.conf.5.sgml
>> index 0a72e45..72fc0c3 100644
>> --- a/apt/doc/apt.conf.5.sgml
>> +++ b/apt/doc/apt.conf.5.sgml
>> @@ -526,7 +526,7 @@ DPkg::Pre-Install-Pkgs {"/usr/sbin/dpkg-preconfigure --apt";};
>> disable the inclusion of statfs data in CDROM IDs.
>> </para><para>
>> To debug issues related to dynamic memory allocation, an option
>> - <literal/Debug::DynamicMMap::Grow/ may be used.
>> + <literal/Debug::DynamicMMap::Grow/ and <literal/Debug::DynamicMMap::Allocate/ may be used.
>
> As we've got two options now, I suggest s/an option/options/ .
>
>
Thanks, I'll update this text.
>
> _______________________________________________
> Devel mailing list
> Devel@lists.altlinux.org
> https://lists.altlinux.org/mailman/listinfo/devel
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* [devel] [PATCH for apt 1/2 v2] Add option for debugging Debug::DynamicMMap::Allocate
2019-12-08 22:50 ` [devel] [PATCH for apt 1/2] Add option for debugging DynamicMMap::Allocate Dmitry V. Levin
2019-12-09 6:58 ` Aleksei Nikiforov
@ 2019-12-09 7:00 ` Aleksei Nikiforov
1 sibling, 0 replies; 34+ messages in thread
From: Aleksei Nikiforov @ 2019-12-09 7:00 UTC (permalink / raw)
To: devel; +Cc: Aleksei Nikiforov
---
apt/apt-pkg/contrib/mmap.cc | 15 +++++++++++++++
apt/doc/apt.conf.5.sgml | 4 ++--
2 files changed, 17 insertions(+), 2 deletions(-)
diff --git a/apt/apt-pkg/contrib/mmap.cc b/apt/apt-pkg/contrib/mmap.cc
index a3b06cc..cf01be9 100644
--- a/apt/apt-pkg/contrib/mmap.cc
+++ b/apt/apt-pkg/contrib/mmap.cc
@@ -265,6 +265,21 @@ std::experimental::optional<map_ptrloc> DynamicMMap::Allocate(unsigned long Item
// No pool is allocated, use an unallocated one
if (I == Pools + PoolCount)
{
+ static const bool debug_grow = _config->FindB("Debug::DynamicMMap::Allocate", false);
+
+ if (debug_grow)
+ {
+ Pool *pool_iter = Pools;
+ size_t pool_idx = 0;
+
+ _error->Warning(_("DynamicMMap::Allocate: allocating item of size %lu"), ItemSize);
+
+ for (; pool_idx < PoolCount; ++pool_iter, ++pool_idx)
+ {
+ _error->Warning(_("DynamicMMap::Allocate: Pool %zu, item size: %lu, start: %lu, count: %lu"), pool_idx, pool_iter->ItemSize, pool_iter->Start, pool_iter->Count);
+ }
+ }
+
// Woops, we ran out, the calling code should allocate more.
if (Empty == 0)
{
diff --git a/apt/doc/apt.conf.5.sgml b/apt/doc/apt.conf.5.sgml
index 0a72e45..0026359 100644
--- a/apt/doc/apt.conf.5.sgml
+++ b/apt/doc/apt.conf.5.sgml
@@ -525,8 +525,8 @@ DPkg::Pre-Install-Pkgs {"/usr/sbin/dpkg-preconfigure --apt";};
command line for each dpkg invokation. <literal/Debug::IdentCdrom/ will
disable the inclusion of statfs data in CDROM IDs.
</para><para>
- To debug issues related to dynamic memory allocation, an option
- <literal/Debug::DynamicMMap::Grow/ may be used.
+ To debug issues related to dynamic memory allocation, options
+ <literal/Debug::DynamicMMap::Grow/ and <literal/Debug::DynamicMMap::Allocate/ may be used.
</para>
</RefSect1>
--
2.24.0
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [devel] [PATCH for apt 2/2 v2] Fix pointer arithmetics
2019-12-08 23:21 ` Dmitry V. Levin
@ 2019-12-09 7:08 ` Aleksei Nikiforov
2019-12-10 0:07 ` Dmitry V. Levin
0 siblings, 1 reply; 34+ messages in thread
From: Aleksei Nikiforov @ 2019-12-09 7:08 UTC (permalink / raw)
To: devel
09.12.2019 2:21, Dmitry V. Levin пишет:
> On Fri, Dec 06, 2019 at 06:36:55PM +0300, Aleksei Nikiforov wrote:
> [...]
>> @@ -85,11 +87,11 @@ class pkgCache::PkgIterator
>> inline unsigned long long Index() const {return Pkg - Owner->PkgP;};
>> OkState State() const;
>>
>> - void ReMap(void const * const oldMap, void const * const newMap)
>> + void ReMap(void *oldMap, void *newMap)
>
> Is there any particular reason for stripping const here and in other
> similar places?
>
Yes, it's needed due to issues emerging from mixing const and non-const
pointers with new and allegedly more proper way of calculating rebased
pointers.
> [...]
>> @@ -301,7 +302,7 @@ std::experimental::optional<map_ptrloc> DynamicMMap::Allocate(unsigned long Item
>> Pool* oldPools = Pools;
>> auto idxResult = RawAllocate(I->Count*ItemSize,ItemSize);
>> if (Pools != oldPools)
>> - I += Pools - oldPools;
>> + I = RebasePointer(I, oldPools, Pools);
>>
>> // Does the allocation failed ?
>> if (!idxResult)
>
> In my patch RebasePointer invocation was after the idxResult check,
> not before the check.
>
Theoretically, order here might be important. In practice, it doesn't
matter.
> By the way, in this and other similar cases,
> is there any reason for "Pools != oldPools" check?
> Is RebasePointer incapable of handling this, or is it an optimization?
>
It's just an optimization, it may be removed.
> [...]
>> diff --git a/apt/apt-pkg/rebase_pointer.h b/apt/apt-pkg/rebase_pointer.h
>> new file mode 100644
>> index 0000000..f6b3c15
>> --- /dev/null
>> +++ b/apt/apt-pkg/rebase_pointer.h
>> @@ -0,0 +1,16 @@
>> +#ifndef PKGLIB_REBASE_POINTER_H
>> +#define PKGLIB_REBASE_POINTER_H
>> +
>> +template <typename T>
>> +static inline T* RebasePointer(T *ptr, void *old_base, void *new_base)
>> +{
>> + return reinterpret_cast<T*>(reinterpret_cast<char*>(new_base) + (reinterpret_cast<char*>(ptr) - reinterpret_cast<char*>(old_base)));
>> +}
>> +
>> +template <typename T>
>> +static inline const T* RebasePointer(const T *ptr, void *old_base, void *new_base)
>> +{
>> + return reinterpret_cast<const T*>(reinterpret_cast<char*>(new_base) + (reinterpret_cast<const char*>(ptr) - reinterpret_cast<char*>(old_base)));
>> +}
>> +
>> +#endif
>
> Do we really need two templates here?
>
>
Yes, second template with const ptr is needed for
rpmListParser::rpmListParser from rpmlistparser.cc.
Variable SeenPackages has type SeenPackagesType, which is a typedef to
std::set<const char*,cstr_lt_pred>. Thus, elements are 'const char*',
and either it should be const-casted to 'char*', which is ugly, or
const-correctness should be achieved some other way, for example by
getting rid of unimportant const qualifiers like in my changes.
And first template is needed for every other case with non-const ptr.
>
> _______________________________________________
> Devel mailing list
> Devel@lists.altlinux.org
> https://lists.altlinux.org/mailman/listinfo/devel
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [devel] [PATCH for apt 2/2] Fix pointer arithmetics
2019-12-08 23:31 ` [devel] [PATCH for apt 2/2] " Dmitry V. Levin
@ 2019-12-09 7:09 ` Aleksei Nikiforov
2019-12-09 7:16 ` [devel] [PATCH for apt 2/2 v3] " Aleksei Nikiforov
1 sibling, 0 replies; 34+ messages in thread
From: Aleksei Nikiforov @ 2019-12-09 7:09 UTC (permalink / raw)
To: devel
09.12.2019 2:31, Dmitry V. Levin пишет:
> On Fri, Dec 06, 2019 at 04:16:06PM +0300, Aleksei Nikiforov wrote:
> [...]
>> diff --git a/apt/apt-pkg/rebase_pointer.h b/apt/apt-pkg/rebase_pointer.h
>> new file mode 100644
>> index 0000000..efc4074
>> --- /dev/null
>> +++ b/apt/apt-pkg/rebase_pointer.h
>> @@ -0,0 +1,16 @@
>> +#ifndef PKGLIB_REBASE_POINTER_H
>> +#define PKGLIB_REBASE_POINTER_H
>> +
>> +template <typename T>
>> +static inline T* RebasePointer(T *ptr, const void * const old_base, const void * const new_base)
>> +{
>> + return reinterpret_cast<T*>(reinterpret_cast<char*>(ptr) + (reinterpret_cast<char const * const>(new_base) - reinterpret_cast<char const * const>(old_base)));
>> +}
>> +
>> +template <typename T>
>> +static inline const T* RebasePointer(const T *ptr, const void * const old_base, const void * const new_base)
>> +{
>> + return reinterpret_cast<const T*>(reinterpret_cast<const char*>(ptr) + (reinterpret_cast<char const * const>(new_base) - reinterpret_cast<char const * const>(old_base)));
>
> This line is way too long - about twice longer than a normal line of code.
> Please break long lines.
>
> My edition of rebase_pointer.h had the maximum length of all lines within
> the traditional 80-symbol limit.
>
>
Sure, I'll update this code.
>
> _______________________________________________
> Devel mailing list
> Devel@lists.altlinux.org
> https://lists.altlinux.org/mailman/listinfo/devel
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* [devel] [PATCH for apt 2/2 v3] Fix pointer arithmetics
2019-12-08 23:31 ` [devel] [PATCH for apt 2/2] " Dmitry V. Levin
2019-12-09 7:09 ` Aleksei Nikiforov
@ 2019-12-09 7:16 ` Aleksei Nikiforov
1 sibling, 0 replies; 34+ messages in thread
From: Aleksei Nikiforov @ 2019-12-09 7:16 UTC (permalink / raw)
To: devel; +Cc: Aleksei Nikiforov
This change should fix pointer arithmetic issues
which manifest on e2k architecture.
---
apt/apt-pkg/Makefile.am | 1 +
apt/apt-pkg/cacheiterators.h | 26 ++++++++++++++------------
apt/apt-pkg/contrib/mmap.cc | 8 +++++---
apt/apt-pkg/pkgcachegen.cc | 29 +++++++++++++++--------------
apt/apt-pkg/pkgcachegen.h | 6 +++---
apt/apt-pkg/rebase_pointer.h | 22 ++++++++++++++++++++++
apt/apt-pkg/rpm/rpmlistparser.cc | 5 +++--
apt/apt-pkg/rpm/rpmpackagedata.cc | 2 +-
8 files changed, 64 insertions(+), 35 deletions(-)
create mode 100644 apt/apt-pkg/rebase_pointer.h
diff --git a/apt/apt-pkg/Makefile.am b/apt/apt-pkg/Makefile.am
index 4c0d234..d038d01 100644
--- a/apt/apt-pkg/Makefile.am
+++ b/apt/apt-pkg/Makefile.am
@@ -94,6 +94,7 @@ libapt_pkg_la_SOURCES = \
pkgsystem.h \
policy.cc \
policy.h \
+ rebase_pointer.h \
repository.cc \
repository.h \
scopeexit.h \
diff --git a/apt/apt-pkg/cacheiterators.h b/apt/apt-pkg/cacheiterators.h
index a4bf670..118838e 100644
--- a/apt/apt-pkg/cacheiterators.h
+++ b/apt/apt-pkg/cacheiterators.h
@@ -34,6 +34,8 @@
#pragma interface "apt-pkg/cacheiterators.h"
#endif
+#include <apt-pkg/rebase_pointer.h>
+
#include <set>
// Package Iterator
@@ -85,11 +87,11 @@ class pkgCache::PkgIterator
inline unsigned long long Index() const {return Pkg - Owner->PkgP;};
OkState State() const;
- void ReMap(void const * const oldMap, void const * const newMap)
+ void ReMap(void *oldMap, void *newMap)
{
if (Owner == 0 || Pkg == 0)
return;
- Pkg += static_cast<Package const *>(newMap) - static_cast<Package const *>(oldMap);
+ Pkg = RebasePointer(Pkg, oldMap, newMap);
}
// Constructors
@@ -147,11 +149,11 @@ class pkgCache::VerIterator
bool Automatic() const;
VerFileIterator NewestFile() const;
- void ReMap(void const * const oldMap, void const * const newMap)
+ void ReMap(void *oldMap, void *newMap)
{
if (Owner == 0 || Ver == 0)
return;
- Ver += static_cast<Version const *>(newMap) - static_cast<Version const *>(oldMap);
+ Ver = RebasePointer(Ver, oldMap, newMap);
}
inline VerIterator() : Ver(0), Owner(0) {};
@@ -220,11 +222,11 @@ class pkgCache::DepIterator
inline const char *CompType() {return Owner->CompType(Dep->CompareOp);};
inline const char *DepType() {return Owner->DepType(Dep->Type);};
- void ReMap(void const * const oldMap, void const * const newMap)
+ void ReMap(void *oldMap, void *newMap)
{
if (Owner == 0 || Dep == 0)
return;
- Dep += static_cast<Dependency const *>(newMap) - static_cast<Dependency const *>(oldMap);
+ Dep = RebasePointer(Dep, oldMap, newMap);
}
inline DepIterator(pkgCache &Owner,Dependency *Trg,Version * = 0) :
@@ -279,11 +281,11 @@ class pkgCache::PrvIterator
inline PkgIterator OwnerPkg() const {return PkgIterator(*Owner,Owner->PkgP + Owner->VerP[Prv->Version].ParentPkg);};
inline unsigned long long Index() const {return Prv - Owner->ProvideP;};
- void ReMap(void const * const oldMap, void const * const newMap)
+ void ReMap(void *oldMap, void *newMap)
{
if (Owner == 0 || Prv == 0)
return;
- Prv += static_cast<Provides const *>(newMap) - static_cast<Provides const *>(oldMap);
+ Prv = RebasePointer(Prv, oldMap, newMap);
}
inline PrvIterator() : Prv(0), Type(PrvVer), Owner(0) {};
@@ -342,11 +344,11 @@ class pkgCache::PkgFileIterator
bool IsOk();
string RelStr();
- void ReMap(void const * const oldMap, void const * const newMap)
+ void ReMap(void *oldMap, void *newMap)
{
if (Owner == 0 || File == 0)
return;
- File += static_cast<PackageFile const *>(newMap) - static_cast<PackageFile const *>(oldMap);
+ File = RebasePointer(File, oldMap, newMap);
}
// Constructors
@@ -383,11 +385,11 @@ class pkgCache::VerFileIterator
inline PkgFileIterator File() const {return PkgFileIterator(*Owner,FileP->File + Owner->PkgFileP);};
inline unsigned long long Index() const {return FileP - Owner->VerFileP;};
- void ReMap(void const * const oldMap, void const * const newMap)
+ void ReMap(void *oldMap, void *newMap)
{
if (Owner == 0 || FileP == 0)
return;
- FileP += static_cast<VerFile const *>(newMap) - static_cast<VerFile const *>(oldMap);
+ FileP = RebasePointer(FileP, oldMap, newMap);
}
inline VerFileIterator() : Owner(0), FileP(0) {};
diff --git a/apt/apt-pkg/contrib/mmap.cc b/apt/apt-pkg/contrib/mmap.cc
index cf01be9..35f1a25 100644
--- a/apt/apt-pkg/contrib/mmap.cc
+++ b/apt/apt-pkg/contrib/mmap.cc
@@ -30,6 +30,7 @@
#include <apt-pkg/configuration.h>
#include <apt-pkg/mmap.h>
#include <apt-pkg/error.h>
+#include <apt-pkg/rebase_pointer.h>
#include <apti18n.h>
@@ -301,7 +302,7 @@ std::experimental::optional<map_ptrloc> DynamicMMap::Allocate(unsigned long Item
Pool* oldPools = Pools;
auto idxResult = RawAllocate(I->Count*ItemSize,ItemSize);
if (Pools != oldPools)
- I += Pools - oldPools;
+ I = RebasePointer(I, oldPools, Pools);
// Does the allocation failed ?
if (!idxResult)
@@ -371,7 +372,7 @@ bool DynamicMMap::Grow(unsigned long long size)
Fd->Write(&C,sizeof(C));
}
- unsigned long const poolOffset = Pools - ((Pool*) Base);
+ void *old_base = Base;
if (Fd != 0)
{
@@ -408,7 +409,8 @@ bool DynamicMMap::Grow(unsigned long long size)
memset((char*)Base + WorkSpace, 0, newSize - WorkSpace);
}
- Pools = (Pool*) Base + poolOffset;
+ if (Base != old_base)
+ Pools = RebasePointer(Pools, old_base, Base);
WorkSpace = newSize;
return true;
diff --git a/apt/apt-pkg/pkgcachegen.cc b/apt/apt-pkg/pkgcachegen.cc
index 654c81c..37364e9 100644
--- a/apt/apt-pkg/pkgcachegen.cc
+++ b/apt/apt-pkg/pkgcachegen.cc
@@ -26,6 +26,7 @@
#include <apt-pkg/strutl.h>
#include <apt-pkg/sptr.h>
#include <apt-pkg/pkgsystem.h>
+#include <apt-pkg/rebase_pointer.h>
#include <apti18n.h>
@@ -109,18 +110,18 @@ pkgCacheGenerator::~pkgCacheGenerator()
Map.Sync(0,sizeof(pkgCache::Header));
}
/*}}}*/
-void pkgCacheGenerator::ReMap(void const * const oldMap, void const * const newMap)
+void pkgCacheGenerator::ReMap(void *oldMap, void *newMap)
{
if (oldMap == newMap)
return;
Cache.ReMap(false);
- CurrentFile += (pkgCache::PackageFile*) newMap - (pkgCache::PackageFile*) oldMap;
+ CurrentFile = RebasePointer(CurrentFile, oldMap, newMap);
for (size_t i = 0; i < _count(UniqHash); ++i)
if (UniqHash[i] != 0)
- UniqHash[i] += (pkgCache::StringItem*) newMap - (pkgCache::StringItem*) oldMap;
+ UniqHash[i] = RebasePointer(UniqHash[i], oldMap, newMap);
for (auto i = Dynamic<pkgCache::PkgIterator>::toReMap.begin();
i != Dynamic<pkgCache::PkgIterator>::toReMap.end(); ++i)
@@ -147,7 +148,7 @@ void pkgCacheGenerator::ReMap(void const * const oldMap, void const * const newM
// CacheGenerator::WriteStringInMap /*{{{*/
std::experimental::optional<map_ptrloc> pkgCacheGenerator::WriteStringInMap(const char *String,
unsigned long Len) {
- void const * const oldMap = Map.Data();
+ void *oldMap = Map.Data();
const auto index = Map.WriteString(String, Len);
if (index)
ReMap(oldMap, Map.Data());
@@ -156,7 +157,7 @@ std::experimental::optional<map_ptrloc> pkgCacheGenerator::WriteStringInMap(cons
/*}}}*/
// CacheGenerator::WriteStringInMap /*{{{*/
std::experimental::optional<map_ptrloc> pkgCacheGenerator::WriteStringInMap(const char *String) {
- void const * const oldMap = Map.Data();
+ void *oldMap = Map.Data();
const auto index = Map.WriteString(String);
if (index)
ReMap(oldMap, Map.Data());
@@ -164,7 +165,7 @@ std::experimental::optional<map_ptrloc> pkgCacheGenerator::WriteStringInMap(cons
}
/*}}}*/
std::experimental::optional<map_ptrloc> pkgCacheGenerator::AllocateInMap(unsigned long size) {/*{{{*/
- void const * const oldMap = Map.Data();
+ void *oldMap = Map.Data();
const auto index = Map.Allocate(size);
if (index)
ReMap(oldMap, Map.Data());
@@ -229,7 +230,7 @@ bool pkgCacheGenerator::MergeList(ListParser &List,
pkgCache::VerIterator Ver = Pkg.VersionList();
Dynamic<pkgCache::VerIterator> DynVer(Ver);
map_ptrloc *Last = &Pkg->VersionList;
- const void * oldMap = Map.Data();
+ void *oldMap = Map.Data();
int Res = 1;
for (; Ver.end() == false; Last = &Ver->NextVer, Ver++)
{
@@ -271,7 +272,7 @@ bool pkgCacheGenerator::MergeList(ListParser &List,
if (oldMap != Map.Data())
{
- Last += (map_ptrloc*) Map.Data() - (map_ptrloc*) oldMap;
+ Last = RebasePointer(Last, oldMap, Map.Data());
oldMap = Map.Data();
}
@@ -297,7 +298,7 @@ bool pkgCacheGenerator::MergeList(ListParser &List,
PackageName.c_str(), 1);
if (oldMap != Map.Data())
- Last += (map_ptrloc*) Map.Data() - (map_ptrloc*) oldMap;
+ Last = RebasePointer(Last, oldMap, Map.Data());
*Last = *verindex;
Ver->ParentPkg = Pkg.Index();
@@ -553,7 +554,7 @@ bool pkgCacheGenerator::ListParser::NewDepends(pkgCache::VerIterator &Ver,
unsigned int Op,
unsigned int Type)
{
- void const * const oldMap = Owner->Map.Data();
+ void *oldMap = Owner->Map.Data();
pkgCache &Cache = Owner->Cache;
// Get a structure
@@ -605,7 +606,7 @@ bool pkgCacheGenerator::ListParser::NewDepends(pkgCache::VerIterator &Ver,
OldDepLast = &D->NextDepends;
OldDepVer = Ver;
} else if (oldMap != Owner->Map.Data())
- OldDepLast += (map_ptrloc*) Owner->Map.Data() - (map_ptrloc*) oldMap;
+ OldDepLast = RebasePointer(OldDepLast, oldMap, Owner->Map.Data());
// Is it a file dependency?
if (PackageName[0] == '/')
@@ -739,7 +740,7 @@ std::experimental::optional<map_ptrloc> pkgCacheGenerator::WriteUniqString(const
}
// Get a structure
- void const * const oldMap = Map.Data();
+ void *oldMap = Map.Data();
const auto Item = AllocateInMap(sizeof(pkgCache::StringItem));
const auto idxString = WriteStringInMap(S, Size);
if ((!Item) || (!idxString))
@@ -747,8 +748,8 @@ std::experimental::optional<map_ptrloc> pkgCacheGenerator::WriteUniqString(const
if (oldMap != Map.Data())
{
- Last += (map_ptrloc*) Map.Data() - (map_ptrloc*) oldMap;
- I += (pkgCache::StringItem*) Map.Data() - (pkgCache::StringItem*) oldMap;
+ Last = RebasePointer(Last, oldMap, Map.Data());
+ I = RebasePointer(I, oldMap, Map.Data());
}
*Last = *Item;
diff --git a/apt/apt-pkg/pkgcachegen.h b/apt/apt-pkg/pkgcachegen.h
index 77075a6..0faf6e1 100644
--- a/apt/apt-pkg/pkgcachegen.h
+++ b/apt/apt-pkg/pkgcachegen.h
@@ -72,7 +72,7 @@ class pkgCacheGenerator
class DynamicFunction
{
public:
- typedef std::function<void(const void *, const void *)> function;
+ typedef std::function<void(void *, void *)> function;
static std::set<DynamicFunction*> toReMap;
@@ -93,7 +93,7 @@ class pkgCacheGenerator
toReMap.erase(this);
}
- void call(const void *oldMap, const void *newMap)
+ void call(void *oldMap, void *newMap)
{
if (m_function)
m_function(oldMap, newMap);
@@ -140,7 +140,7 @@ class pkgCacheGenerator
// CNC:2003-03-18
inline void ResetFileDeps() {FoundFileDeps = false;};
- void ReMap(void const * const oldMap, void const * const newMap);
+ void ReMap(void *oldMap, void *newMap);
pkgCacheGenerator(DynamicMMap *Map,OpProgress *Progress);
~pkgCacheGenerator();
diff --git a/apt/apt-pkg/rebase_pointer.h b/apt/apt-pkg/rebase_pointer.h
new file mode 100644
index 0000000..3b66b98
--- /dev/null
+++ b/apt/apt-pkg/rebase_pointer.h
@@ -0,0 +1,22 @@
+#ifndef PKGLIB_REBASE_POINTER_H
+#define PKGLIB_REBASE_POINTER_H
+
+template <typename T>
+static inline T* RebasePointer(T *ptr,
+ void *old_base, void *new_base)
+{
+ return reinterpret_cast<T*>(reinterpret_cast<char*>(new_base)
+ + (reinterpret_cast<char*>(ptr)
+ - reinterpret_cast<char*>(old_base)));
+}
+
+template <typename T>
+static inline const T* RebasePointer(const T *ptr,
+ void *old_base, void *new_base)
+{
+ return reinterpret_cast<const T*>(reinterpret_cast<char*>(new_base)
+ + (reinterpret_cast<const char*>(ptr)
+ - reinterpret_cast<char*>(old_base)));
+}
+
+#endif
diff --git a/apt/apt-pkg/rpm/rpmlistparser.cc b/apt/apt-pkg/rpm/rpmlistparser.cc
index 9b2e9ad..bd13b99 100644
--- a/apt/apt-pkg/rpm/rpmlistparser.cc
+++ b/apt/apt-pkg/rpm/rpmlistparser.cc
@@ -25,6 +25,7 @@
#include <apt-pkg/strutl.h>
#include <apt-pkg/crc-16.h>
#include <apt-pkg/tagfile.h>
+#include <apt-pkg/rebase_pointer.h>
#include <apti18n.h>
@@ -50,13 +51,13 @@ rpmListParser::rpmListParser(RPMHandler *Handler)
{
SeenPackages.reset(new SeenPackagesType);
m_SeenPackagesRealloc.reset(new pkgCacheGenerator::DynamicFunction(
- [this] (void const *oldMap, void const *newMap)
+ [this] (void *oldMap, void *newMap)
{
SeenPackagesType tmp;
for (auto iter: *SeenPackages)
{
- tmp.insert(iter + (static_cast<const char *>(newMap) - static_cast<const char *>(oldMap)));
+ tmp.insert(RebasePointer(iter, oldMap, newMap));
}
SeenPackages->swap(tmp);
diff --git a/apt/apt-pkg/rpm/rpmpackagedata.cc b/apt/apt-pkg/rpm/rpmpackagedata.cc
index 61c15d5..d6c677b 100644
--- a/apt/apt-pkg/rpm/rpmpackagedata.cc
+++ b/apt/apt-pkg/rpm/rpmpackagedata.cc
@@ -23,7 +23,7 @@ RPMPackageData::RPMPackageData()
#endif
{
m_VerMapRealloc.reset(new pkgCacheGenerator::DynamicFunction(
- [this] (void const *oldMap, void const *newMap)
+ [this] (void *oldMap, void *newMap)
{
for (auto iter1 = VerMap.begin(); iter1 != VerMap.end(); ++iter1)
{
--
2.24.0
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [devel] [PATCH for apt 1/2] Add option for debugging DynamicMMap::Allocate
2019-12-09 6:58 ` Aleksei Nikiforov
@ 2019-12-09 10:24 ` Dmitry V. Levin
2019-12-09 11:03 ` [devel] [PATCH for apt 1/2 v3] Add Debug::DynamicMMap::Allocate option Aleksei Nikiforov
0 siblings, 1 reply; 34+ messages in thread
From: Dmitry V. Levin @ 2019-12-09 10:24 UTC (permalink / raw)
To: Aleksei Nikiforov; +Cc: ALT Devel discussion list
[-- Attachment #1: Type: text/plain, Size: 950 bytes --]
On Mon, Dec 09, 2019 at 09:58:55AM +0300, Aleksei Nikiforov wrote:
> 09.12.2019 1:50, Dmitry V. Levin пишет:
> > On Fri, Dec 06, 2019 at 04:16:05PM +0300, Aleksei Nikiforov wrote:
> >> ---
> >> apt/apt-pkg/contrib/mmap.cc | 15 +++++++++++++++
> >> apt/doc/apt.conf.5.sgml | 2 +-
> >> 2 files changed, 16 insertions(+), 1 deletion(-)
> >
> > I suggest adding the name of the new option to the commit message
> > so it would be git-grep'able, for example:
> >
> > Add Debug::DynamicMMap::Allocate option
> >
> > Add a new option for debugging DynamicMMap::Allocate.
>
> But option name is already in commit message. See the email subject
> which is also a part of commit message. Why is it needed to duplicate it?
I don't see a duplication here, but anyway, this is just an example, and
"Add Debug::DynamicMMap::Allocate option" would be better than
"Add option for debugging DynamicMMap::Allocate".
--
ldv
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 34+ messages in thread
* [devel] [PATCH for apt 1/2 v3] Add Debug::DynamicMMap::Allocate option
2019-12-09 10:24 ` Dmitry V. Levin
@ 2019-12-09 11:03 ` Aleksei Nikiforov
2019-12-09 22:59 ` Dmitry V. Levin
0 siblings, 1 reply; 34+ messages in thread
From: Aleksei Nikiforov @ 2019-12-09 11:03 UTC (permalink / raw)
To: devel; +Cc: Aleksei Nikiforov
---
apt/apt-pkg/contrib/mmap.cc | 15 +++++++++++++++
apt/doc/apt.conf.5.sgml | 4 ++--
2 files changed, 17 insertions(+), 2 deletions(-)
diff --git a/apt/apt-pkg/contrib/mmap.cc b/apt/apt-pkg/contrib/mmap.cc
index a3b06cc..cf01be9 100644
--- a/apt/apt-pkg/contrib/mmap.cc
+++ b/apt/apt-pkg/contrib/mmap.cc
@@ -265,6 +265,21 @@ std::experimental::optional<map_ptrloc> DynamicMMap::Allocate(unsigned long Item
// No pool is allocated, use an unallocated one
if (I == Pools + PoolCount)
{
+ static const bool debug_grow = _config->FindB("Debug::DynamicMMap::Allocate", false);
+
+ if (debug_grow)
+ {
+ Pool *pool_iter = Pools;
+ size_t pool_idx = 0;
+
+ _error->Warning(_("DynamicMMap::Allocate: allocating item of size %lu"), ItemSize);
+
+ for (; pool_idx < PoolCount; ++pool_iter, ++pool_idx)
+ {
+ _error->Warning(_("DynamicMMap::Allocate: Pool %zu, item size: %lu, start: %lu, count: %lu"), pool_idx, pool_iter->ItemSize, pool_iter->Start, pool_iter->Count);
+ }
+ }
+
// Woops, we ran out, the calling code should allocate more.
if (Empty == 0)
{
diff --git a/apt/doc/apt.conf.5.sgml b/apt/doc/apt.conf.5.sgml
index 0a72e45..0026359 100644
--- a/apt/doc/apt.conf.5.sgml
+++ b/apt/doc/apt.conf.5.sgml
@@ -525,8 +525,8 @@ DPkg::Pre-Install-Pkgs {"/usr/sbin/dpkg-preconfigure --apt";};
command line for each dpkg invokation. <literal/Debug::IdentCdrom/ will
disable the inclusion of statfs data in CDROM IDs.
</para><para>
- To debug issues related to dynamic memory allocation, an option
- <literal/Debug::DynamicMMap::Grow/ may be used.
+ To debug issues related to dynamic memory allocation, options
+ <literal/Debug::DynamicMMap::Grow/ and <literal/Debug::DynamicMMap::Allocate/ may be used.
</para>
</RefSect1>
--
2.24.0
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [devel] [PATCH for apt 1/2 v3] Add Debug::DynamicMMap::Allocate option
2019-12-09 11:03 ` [devel] [PATCH for apt 1/2 v3] Add Debug::DynamicMMap::Allocate option Aleksei Nikiforov
@ 2019-12-09 22:59 ` Dmitry V. Levin
0 siblings, 0 replies; 34+ messages in thread
From: Dmitry V. Levin @ 2019-12-09 22:59 UTC (permalink / raw)
To: Aleksei Nikiforov; +Cc: ALT Devel discussion list
[-- Attachment #1: Type: text/plain, Size: 4439 bytes --]
On Mon, Dec 09, 2019 at 02:03:00PM +0300, Aleksei Nikiforov wrote:
> ---
> apt/apt-pkg/contrib/mmap.cc | 15 +++++++++++++++
> apt/doc/apt.conf.5.sgml | 4 ++--
> 2 files changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/apt/apt-pkg/contrib/mmap.cc b/apt/apt-pkg/contrib/mmap.cc
> index a3b06cc..cf01be9 100644
> --- a/apt/apt-pkg/contrib/mmap.cc
> +++ b/apt/apt-pkg/contrib/mmap.cc
> @@ -265,6 +265,21 @@ std::experimental::optional<map_ptrloc> DynamicMMap::Allocate(unsigned long Item
> // No pool is allocated, use an unallocated one
> if (I == Pools + PoolCount)
> {
> + static const bool debug_grow = _config->FindB("Debug::DynamicMMap::Allocate", false);
> +
> + if (debug_grow)
> + {
> + Pool *pool_iter = Pools;
> + size_t pool_idx = 0;
> +
> + _error->Warning(_("DynamicMMap::Allocate: allocating item of size %lu"), ItemSize);
> +
> + for (; pool_idx < PoolCount; ++pool_iter, ++pool_idx)
> + {
> + _error->Warning(_("DynamicMMap::Allocate: Pool %zu, item size: %lu, start: %lu, count: %lu"), pool_idx, pool_iter->ItemSize, pool_iter->Start, pool_iter->Count);
> + }
> + }
> +
> // Woops, we ran out, the calling code should allocate more.
> if (Empty == 0)
> {
> diff --git a/apt/doc/apt.conf.5.sgml b/apt/doc/apt.conf.5.sgml
> index 0a72e45..0026359 100644
> --- a/apt/doc/apt.conf.5.sgml
> +++ b/apt/doc/apt.conf.5.sgml
> @@ -525,8 +525,8 @@ DPkg::Pre-Install-Pkgs {"/usr/sbin/dpkg-preconfigure --apt";};
> command line for each dpkg invokation. <literal/Debug::IdentCdrom/ will
> disable the inclusion of statfs data in CDROM IDs.
> </para><para>
> - To debug issues related to dynamic memory allocation, an option
> - <literal/Debug::DynamicMMap::Grow/ may be used.
> + To debug issues related to dynamic memory allocation, options
> + <literal/Debug::DynamicMMap::Grow/ and <literal/Debug::DynamicMMap::Allocate/ may be used.
> </para>
> </RefSect1>
>
Sadly, for some reason most of my comments were ignored.
I'm going to apply the following edition of this patch:
From a60c3cb4fd06c84c61358af597a88717ecc36c05 Mon Sep 17 00:00:00 2001
From: Aleksei Nikiforov <darktemplar@altlinux.org>
Date: Mon, 9 Dec 2019 14:03:00 +0300
Subject: [PATCH] Add Debug::DynamicMMap::Allocate option
Co-developed-by: Dmitry V. Levin <ldv@altlinux.org>
---
apt/apt-pkg/contrib/mmap.cc | 17 +++++++++++++++++
apt/doc/apt.conf.5.sgml | 4 ++--
2 files changed, 19 insertions(+), 2 deletions(-)
diff --git a/apt/apt-pkg/contrib/mmap.cc b/apt/apt-pkg/contrib/mmap.cc
index 779d7a6..56c3cce 100644
--- a/apt/apt-pkg/contrib/mmap.cc
+++ b/apt/apt-pkg/contrib/mmap.cc
@@ -266,6 +266,23 @@ std::experimental::optional<map_ptrloc> DynamicMMap::Allocate(unsigned long Item
// No pool is allocated, use an unallocated one
if (I == Pools + PoolCount)
{
+ static const bool debug_allocate =
+ _config->FindB("Debug::DynamicMMap::Allocate", false);
+
+ if (debug_allocate)
+ {
+ _error->Warning(_("DynamicMMap::Allocate: allocating item of size %lu"),
+ ItemSize);
+
+ for (unsigned int i = 0; i < PoolCount; ++i)
+ {
+ _error->Warning(_("DynamicMMap::Allocate: Pool %u, item size: %lu"
+ ", start: %lu, count: %lu"),
+ i, Pools[i].ItemSize,
+ Pools[i].Start, Pools[i].Count);
+ }
+ }
+
// Woops, we ran out, the calling code should allocate more.
if (Empty == 0)
{
diff --git a/apt/doc/apt.conf.5.sgml b/apt/doc/apt.conf.5.sgml
index 0a72e45..0026359 100644
--- a/apt/doc/apt.conf.5.sgml
+++ b/apt/doc/apt.conf.5.sgml
@@ -525,8 +525,8 @@ DPkg::Pre-Install-Pkgs {"/usr/sbin/dpkg-preconfigure --apt";};
command line for each dpkg invokation. <literal/Debug::IdentCdrom/ will
disable the inclusion of statfs data in CDROM IDs.
</para><para>
- To debug issues related to dynamic memory allocation, an option
- <literal/Debug::DynamicMMap::Grow/ may be used.
+ To debug issues related to dynamic memory allocation, options
+ <literal/Debug::DynamicMMap::Grow/ and <literal/Debug::DynamicMMap::Allocate/ may be used.
</para>
</RefSect1>
--
ldv
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 34+ messages in thread
* [devel] [PATCH apt 0/3] Fix 0.5.15lorg2-alt70~9 fallout
2019-12-06 13:16 ` [devel] [PATCH for apt 2/2] Fix pointer arithmetics Aleksei Nikiforov
2019-12-06 13:36 ` Ivan A. Melnikov
2019-12-08 23:31 ` [devel] [PATCH for apt 2/2] " Dmitry V. Levin
@ 2019-12-09 23:54 ` Dmitry V. Levin
2019-12-09 23:56 ` [devel] [PATCH apt 1/3] apt-pkg/cacheiterators.h: revert #include <set> Dmitry V. Levin
` (2 more replies)
2 siblings, 3 replies; 34+ messages in thread
From: Dmitry V. Levin @ 2019-12-09 23:54 UTC (permalink / raw)
To: ALT Devel discussion list
Commit 6d5e6a689d07de8feef2cbecb24bc42d5994861b aka 0.5.15lorg2-alt70~9
("apt-pkg/pkgcachegen.{cc,h} changes") among many useful changes
introduced a few problematic ones, too.
This series hopefully cleans up the fallout.
Dmitry V. Levin (3):
apt-pkg/cacheiterators.h: revert #include <set>
apt-pkg/contrib/mmap.cc: revert confusing change of Flags to this->Flags
Fix UB in pointer arithmetic
apt/apt-pkg/Makefile.am | 1 +
apt/apt-pkg/cacheiterators.h | 14 +++++++-------
apt/apt-pkg/contrib/mmap.cc | 28 ++++++++++++++--------------
apt/apt-pkg/pkgcachegen.cc | 27 +++++++++++----------------
apt/apt-pkg/rebase_pointer.h | 25 +++++++++++++++++++++++++
apt/apt-pkg/rpm/rpmlistparser.cc | 3 ++-
6 files changed, 60 insertions(+), 38 deletions(-)
create mode 100644 apt/apt-pkg/rebase_pointer.h
--
ldv
^ permalink raw reply [flat|nested] 34+ messages in thread
* [devel] [PATCH apt 1/3] apt-pkg/cacheiterators.h: revert #include <set>
2019-12-09 23:54 ` [devel] [PATCH apt 0/3] Fix 0.5.15lorg2-alt70~9 fallout Dmitry V. Levin
@ 2019-12-09 23:56 ` Dmitry V. Levin
2019-12-09 23:56 ` [devel] [PATCH apt 2/3] apt-pkg/contrib/mmap.cc: revert confusing change of Flags to this->Flags Dmitry V. Levin
2019-12-09 23:56 ` [devel] [PATCH apt 3/3] Fix UB in pointer arithmetic Dmitry V. Levin
2 siblings, 0 replies; 34+ messages in thread
From: Dmitry V. Levin @ 2019-12-09 23:56 UTC (permalink / raw)
To: ALT Devel discussion list
Commit 6d5e6a689d07de8feef2cbecb24bc42d5994861b aka 0.5.15lorg2-alt70~9
among other changes added #include <set> without any reason whatsoever.
Fixes: 6d5e6a68 ("apt-pkg/pkgcachegen.{cc,h} changes")
---
apt/apt-pkg/cacheiterators.h | 2 --
1 file changed, 2 deletions(-)
diff --git a/apt/apt-pkg/cacheiterators.h b/apt/apt-pkg/cacheiterators.h
index bad1e11..9dffeb3 100644
--- a/apt/apt-pkg/cacheiterators.h
+++ b/apt/apt-pkg/cacheiterators.h
@@ -34,8 +34,6 @@
#pragma interface "apt-pkg/cacheiterators.h"
#endif
-#include <set>
-
// Package Iterator
class pkgCache::PkgIterator
{
--
ldv
^ permalink raw reply [flat|nested] 34+ messages in thread
* [devel] [PATCH apt 2/3] apt-pkg/contrib/mmap.cc: revert confusing change of Flags to this->Flags
2019-12-09 23:54 ` [devel] [PATCH apt 0/3] Fix 0.5.15lorg2-alt70~9 fallout Dmitry V. Levin
2019-12-09 23:56 ` [devel] [PATCH apt 1/3] apt-pkg/cacheiterators.h: revert #include <set> Dmitry V. Levin
@ 2019-12-09 23:56 ` Dmitry V. Levin
2019-12-09 23:56 ` [devel] [PATCH apt 3/3] Fix UB in pointer arithmetic Dmitry V. Levin
2 siblings, 0 replies; 34+ messages in thread
From: Dmitry V. Levin @ 2019-12-09 23:56 UTC (permalink / raw)
To: ALT Devel discussion list
Commit 6d5e6a689d07de8feef2cbecb24bc42d5994861b aka 0.5.15lorg2-alt70~9
among other changes replaced Flags with this->Flags in many places where
this is not just unnecessary but also confusing. Only those places
where this->Flags is modified had to be changed this way.
Fixes: 6d5e6a68 ("apt-pkg/pkgcachegen.{cc,h} changes")
---
apt/apt-pkg/contrib/mmap.cc | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/apt/apt-pkg/contrib/mmap.cc b/apt/apt-pkg/contrib/mmap.cc
index a3b06cc..2064fc4 100644
--- a/apt/apt-pkg/contrib/mmap.cc
+++ b/apt/apt-pkg/contrib/mmap.cc
@@ -46,7 +46,7 @@
MMap::MMap(FileFd &F,unsigned long Flags) : Flags(Flags), iSize(0),
Base(nullptr)
{
- if ((this->Flags & NoImmMap) != NoImmMap)
+ if ((Flags & NoImmMap) != NoImmMap)
Map(F);
}
/*}}}*/
@@ -76,9 +76,9 @@ bool MMap::Map(FileFd &Fd)
// Set the permissions.
int Prot = PROT_READ;
int Map = MAP_SHARED;
- if ((this->Flags & ReadOnly) != ReadOnly)
+ if ((Flags & ReadOnly) != ReadOnly)
Prot |= PROT_WRITE;
- if ((this->Flags & Public) != Public)
+ if ((Flags & Public) != Public)
Map = MAP_PRIVATE;
if (iSize == 0)
@@ -97,7 +97,7 @@ bool MMap::Map(FileFd &Fd)
/* */
bool MMap::Close(bool DoSync)
{
- if ((this->Flags & UnMapped) == UnMapped || validData() == false || iSize == 0)
+ if ((Flags & UnMapped) == UnMapped || validData() == false || iSize == 0)
return true;
if (DoSync == true)
@@ -117,11 +117,11 @@ bool MMap::Close(bool DoSync)
not return till all IO is complete */
bool MMap::Sync()
{
- if ((this->Flags & UnMapped) == UnMapped)
+ if ((Flags & UnMapped) == UnMapped)
return true;
#ifdef _POSIX_SYNCHRONIZED_IO
- if ((this->Flags & ReadOnly) != ReadOnly)
+ if ((Flags & ReadOnly) != ReadOnly)
if (msync((char *)Base,iSize,MS_SYNC) != 0)
return _error->Errno("msync","Unable to write mmap");
#endif
@@ -133,12 +133,12 @@ bool MMap::Sync()
/* */
bool MMap::Sync(unsigned long Start,unsigned long Stop)
{
- if ((this->Flags & UnMapped) == UnMapped)
+ if ((Flags & UnMapped) == UnMapped)
return true;
#ifdef _POSIX_SYNCHRONIZED_IO
unsigned long PSize = sysconf(_SC_PAGESIZE);
- if ((this->Flags & ReadOnly) != ReadOnly)
+ if ((Flags & ReadOnly) != ReadOnly)
if (msync((char *)Base+(int)(Start/PSize)*PSize,Stop - Start,MS_SYNC) != 0)
return _error->Errno("msync","Unable to write mmap");
#endif
@@ -362,7 +362,7 @@ bool DynamicMMap::Grow(unsigned long long size)
{
void *tmp_base = MAP_FAILED;
#ifdef MREMAP_MAYMOVE
- if ((this->Flags & Moveable) == Moveable)
+ if ((Flags & Moveable) == Moveable)
tmp_base = mremap(Base, WorkSpace, newSize, MREMAP_MAYMOVE);
else
#endif
@@ -376,7 +376,7 @@ bool DynamicMMap::Grow(unsigned long long size)
Base = tmp_base;
} else {
- if ((this->Flags & Moveable) != Moveable)
+ if ((Flags & Moveable) != Moveable)
return false;
void *tmp_base = realloc(Base, newSize);
--
ldv
^ permalink raw reply [flat|nested] 34+ messages in thread
* [devel] [PATCH apt 3/3] Fix UB in pointer arithmetic
2019-12-09 23:54 ` [devel] [PATCH apt 0/3] Fix 0.5.15lorg2-alt70~9 fallout Dmitry V. Levin
2019-12-09 23:56 ` [devel] [PATCH apt 1/3] apt-pkg/cacheiterators.h: revert #include <set> Dmitry V. Levin
2019-12-09 23:56 ` [devel] [PATCH apt 2/3] apt-pkg/contrib/mmap.cc: revert confusing change of Flags to this->Flags Dmitry V. Levin
@ 2019-12-09 23:56 ` Dmitry V. Levin
2019-12-10 8:18 ` Aleksei Nikiforov
2 siblings, 1 reply; 34+ messages in thread
From: Dmitry V. Levin @ 2019-12-09 23:56 UTC (permalink / raw)
To: ALT Devel discussion list
Commit 6d5e6a689d07de8feef2cbecb24bc42d5994861b aka 0.5.15lorg2-alt70~9
among other changes introduced UB in pointer arithmetic by casting raw
pointers to specific types.
Fix this by introducing two helpers for rebasing pointers in a safe way.
Co-developed-by: Aleksei Nikiforov <darktemplar@altlinux.org>
Fixes: 6d5e6a68 ("apt-pkg/pkgcachegen.{cc,h} changes")
---
apt/apt-pkg/Makefile.am | 1 +
apt/apt-pkg/cacheiterators.h | 14 ++++++++------
apt/apt-pkg/contrib/mmap.cc | 8 ++++----
apt/apt-pkg/pkgcachegen.cc | 27 +++++++++++----------------
apt/apt-pkg/rebase_pointer.h | 25 +++++++++++++++++++++++++
apt/apt-pkg/rpm/rpmlistparser.cc | 3 ++-
6 files changed, 51 insertions(+), 27 deletions(-)
create mode 100644 apt/apt-pkg/rebase_pointer.h
diff --git a/apt/apt-pkg/Makefile.am b/apt/apt-pkg/Makefile.am
index 4c0d234..d038d01 100644
--- a/apt/apt-pkg/Makefile.am
+++ b/apt/apt-pkg/Makefile.am
@@ -94,6 +94,7 @@ libapt_pkg_la_SOURCES = \
pkgsystem.h \
policy.cc \
policy.h \
+ rebase_pointer.h \
repository.cc \
repository.h \
scopeexit.h \
diff --git a/apt/apt-pkg/cacheiterators.h b/apt/apt-pkg/cacheiterators.h
index 9dffeb3..3c60cb8 100644
--- a/apt/apt-pkg/cacheiterators.h
+++ b/apt/apt-pkg/cacheiterators.h
@@ -34,6 +34,8 @@
#pragma interface "apt-pkg/cacheiterators.h"
#endif
+#include <apt-pkg/rebase_pointer.h>
+
// Package Iterator
class pkgCache::PkgIterator
{
@@ -87,7 +89,7 @@ class pkgCache::PkgIterator
{
if (Owner == 0 || Pkg == 0)
return;
- Pkg += static_cast<Package const *>(newMap) - static_cast<Package const *>(oldMap);
+ RebasePointer(Pkg, oldMap, newMap);
}
// Constructors
@@ -149,7 +151,7 @@ class pkgCache::VerIterator
{
if (Owner == 0 || Ver == 0)
return;
- Ver += static_cast<Version const *>(newMap) - static_cast<Version const *>(oldMap);
+ RebasePointer(Ver, oldMap, newMap);
}
inline VerIterator() : Ver(0), Owner(0) {};
@@ -222,7 +224,7 @@ class pkgCache::DepIterator
{
if (Owner == 0 || Dep == 0)
return;
- Dep += static_cast<Dependency const *>(newMap) - static_cast<Dependency const *>(oldMap);
+ RebasePointer(Dep, oldMap, newMap);
}
inline DepIterator(pkgCache &Owner,Dependency *Trg,Version * = 0) :
@@ -281,7 +283,7 @@ class pkgCache::PrvIterator
{
if (Owner == 0 || Prv == 0)
return;
- Prv += static_cast<Provides const *>(newMap) - static_cast<Provides const *>(oldMap);
+ RebasePointer(Prv, oldMap, newMap);
}
inline PrvIterator() : Prv(0), Type(PrvVer), Owner(0) {};
@@ -344,7 +346,7 @@ class pkgCache::PkgFileIterator
{
if (Owner == 0 || File == 0)
return;
- File += static_cast<PackageFile const *>(newMap) - static_cast<PackageFile const *>(oldMap);
+ RebasePointer(File, oldMap, newMap);
}
// Constructors
@@ -385,7 +387,7 @@ class pkgCache::VerFileIterator
{
if (Owner == 0 || FileP == 0)
return;
- FileP += static_cast<VerFile const *>(newMap) - static_cast<VerFile const *>(oldMap);
+ RebasePointer(FileP, oldMap, newMap);
}
inline VerFileIterator() : Owner(0), FileP(0) {};
diff --git a/apt/apt-pkg/contrib/mmap.cc b/apt/apt-pkg/contrib/mmap.cc
index 2064fc4..779d7a6 100644
--- a/apt/apt-pkg/contrib/mmap.cc
+++ b/apt/apt-pkg/contrib/mmap.cc
@@ -30,6 +30,7 @@
#include <apt-pkg/configuration.h>
#include <apt-pkg/mmap.h>
#include <apt-pkg/error.h>
+#include <apt-pkg/rebase_pointer.h>
#include <apti18n.h>
@@ -285,13 +286,12 @@ std::experimental::optional<map_ptrloc> DynamicMMap::Allocate(unsigned long Item
I->Count = size/ItemSize;
Pool* oldPools = Pools;
auto idxResult = RawAllocate(I->Count*ItemSize,ItemSize);
- if (Pools != oldPools)
- I += Pools - oldPools;
// Does the allocation failed ?
if (!idxResult)
return idxResult;
+ RebasePointer(I, oldPools, Pools);
Result = *idxResult;
I->Start = Result;
}
@@ -356,7 +356,7 @@ bool DynamicMMap::Grow(unsigned long long size)
Fd->Write(&C,sizeof(C));
}
- unsigned long const poolOffset = Pools - ((Pool*) Base);
+ const void *old_base = Base;
if (Fd != 0)
{
@@ -393,7 +393,7 @@ bool DynamicMMap::Grow(unsigned long long size)
memset((char*)Base + WorkSpace, 0, newSize - WorkSpace);
}
- Pools = (Pool*) Base + poolOffset;
+ RebasePointer(Pools, old_base, Base);
WorkSpace = newSize;
return true;
diff --git a/apt/apt-pkg/pkgcachegen.cc b/apt/apt-pkg/pkgcachegen.cc
index 56716b5..7a5a20c 100644
--- a/apt/apt-pkg/pkgcachegen.cc
+++ b/apt/apt-pkg/pkgcachegen.cc
@@ -26,6 +26,7 @@
#include <apt-pkg/strutl.h>
#include <apt-pkg/sptr.h>
#include <apt-pkg/pkgsystem.h>
+#include <apt-pkg/rebase_pointer.h>
#include <apti18n.h>
@@ -116,11 +117,11 @@ void pkgCacheGenerator::ReMap(void const * const oldMap, void const * const newM
Cache.ReMap(false);
- CurrentFile += (pkgCache::PackageFile*) newMap - (pkgCache::PackageFile*) oldMap;
+ RebasePointer(CurrentFile, oldMap, newMap);
for (size_t i = 0; i < _count(UniqHash); ++i)
if (UniqHash[i] != 0)
- UniqHash[i] += (pkgCache::StringItem*) newMap - (pkgCache::StringItem*) oldMap;
+ RebasePointer(UniqHash[i], oldMap, newMap);
for (auto i = Dynamic<pkgCache::PkgIterator>::toReMap.begin();
i != Dynamic<pkgCache::PkgIterator>::toReMap.end(); ++i)
@@ -269,11 +270,8 @@ bool pkgCacheGenerator::MergeList(ListParser &List,
continue;
}
- if (oldMap != Map.Data())
- {
- Last += (map_ptrloc*) Map.Data() - (map_ptrloc*) oldMap;
- oldMap = Map.Data();
- }
+ RebasePointer(Last, oldMap, Map.Data());
+ oldMap = Map.Data();
// Skip to the end of the same version set.
if (Res == 0)
@@ -296,8 +294,7 @@ bool pkgCacheGenerator::MergeList(ListParser &List,
return _error->Error(_("Error occurred while processing %s (NewVersion%d)"),
PackageName.c_str(), 1);
- if (oldMap != Map.Data())
- Last += (map_ptrloc*) Map.Data() - (map_ptrloc*) oldMap;
+ RebasePointer(Last, oldMap, Map.Data());
*Last = *verindex;
Ver->ParentPkg = Pkg.Index();
@@ -604,8 +601,9 @@ bool pkgCacheGenerator::ListParser::NewDepends(pkgCache::VerIterator &Ver,
for (pkgCache::DepIterator D = Ver.DependsList(); D.end() == false; D++)
OldDepLast = &D->NextDepends;
OldDepVer = Ver;
- } else if (oldMap != Owner->Map.Data())
- OldDepLast += (map_ptrloc*) Owner->Map.Data() - (map_ptrloc*) oldMap;
+ } else {
+ RebasePointer(OldDepLast, oldMap, Owner->Map.Data());
+ }
// Is it a file dependency?
if (PackageName[0] == '/')
@@ -745,11 +743,8 @@ std::experimental::optional<map_ptrloc> pkgCacheGenerator::WriteUniqString(const
if ((!Item) || (!idxString))
return std::experimental::optional<map_ptrloc>();
- if (oldMap != Map.Data())
- {
- Last += (map_ptrloc*) Map.Data() - (map_ptrloc*) oldMap;
- I += (pkgCache::StringItem*) Map.Data() - (pkgCache::StringItem*) oldMap;
- }
+ RebasePointer(Last, oldMap, Map.Data());
+ RebasePointer(I, oldMap, Map.Data());
*Last = *Item;
diff --git a/apt/apt-pkg/rebase_pointer.h b/apt/apt-pkg/rebase_pointer.h
new file mode 100644
index 0000000..2bbabea
--- /dev/null
+++ b/apt/apt-pkg/rebase_pointer.h
@@ -0,0 +1,25 @@
+#ifndef PKGLIB_REBASE_POINTER_H
+#define PKGLIB_REBASE_POINTER_H
+
+template <class T>
+static inline T*
+GetRebasedPointer(T*, const void *, const void *)
+__attribute__((__warn_unused_result__));
+
+template <class T>
+static inline T*
+GetRebasedPointer(T* ptr, const void *old_base, const void *new_base)
+{
+ // uintptr_t is a type with well-defined integer overflow semantics
+ uintptr_t diff = (uintptr_t) new_base - (uintptr_t) old_base;
+ return (T*) ((uintptr_t) ptr + diff);
+}
+
+template <class T>
+static inline void
+RebasePointer(T* &ptr, const void *old_base, const void *new_base)
+{
+ ptr = GetRebasedPointer(ptr, old_base, new_base);
+}
+
+#endif
diff --git a/apt/apt-pkg/rpm/rpmlistparser.cc b/apt/apt-pkg/rpm/rpmlistparser.cc
index 9b2e9ad..4aeb937 100644
--- a/apt/apt-pkg/rpm/rpmlistparser.cc
+++ b/apt/apt-pkg/rpm/rpmlistparser.cc
@@ -25,6 +25,7 @@
#include <apt-pkg/strutl.h>
#include <apt-pkg/crc-16.h>
#include <apt-pkg/tagfile.h>
+#include <apt-pkg/rebase_pointer.h>
#include <apti18n.h>
@@ -56,7 +57,7 @@ rpmListParser::rpmListParser(RPMHandler *Handler)
for (auto iter: *SeenPackages)
{
- tmp.insert(iter + (static_cast<const char *>(newMap) - static_cast<const char *>(oldMap)));
+ tmp.insert(GetRebasedPointer(iter, oldMap, newMap));
}
SeenPackages->swap(tmp);
--
ldv
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [devel] [PATCH for apt 2/2 v2] Fix pointer arithmetics
2019-12-09 7:08 ` Aleksei Nikiforov
@ 2019-12-10 0:07 ` Dmitry V. Levin
2019-12-10 8:18 ` Aleksei Nikiforov
0 siblings, 1 reply; 34+ messages in thread
From: Dmitry V. Levin @ 2019-12-10 0:07 UTC (permalink / raw)
To: ALT Devel discussion list
[-- Attachment #1: Type: text/plain, Size: 3719 bytes --]
On Mon, Dec 09, 2019 at 10:08:42AM +0300, Aleksei Nikiforov wrote:
> 09.12.2019 2:21, Dmitry V. Levin пишет:
> > On Fri, Dec 06, 2019 at 06:36:55PM +0300, Aleksei Nikiforov wrote:
> > [...]
> >> @@ -85,11 +87,11 @@ class pkgCache::PkgIterator
> >> inline unsigned long long Index() const {return Pkg - Owner->PkgP;};
> >> OkState State() const;
> >>
> >> - void ReMap(void const * const oldMap, void const * const newMap)
> >> + void ReMap(void *oldMap, void *newMap)
> >
> > Is there any particular reason for stripping const here and in other
> > similar places?
>
> Yes, it's needed due to issues emerging from mixing const and non-const
> pointers with new and allegedly more proper way of calculating rebased
> pointers.
Sorry, I don't find this argument convincing.
I have experienced no const issues in my version of this fix.
> > [...]
> >> @@ -301,7 +302,7 @@ std::experimental::optional<map_ptrloc> DynamicMMap::Allocate(unsigned long Item
> >> Pool* oldPools = Pools;
> >> auto idxResult = RawAllocate(I->Count*ItemSize,ItemSize);
> >> if (Pools != oldPools)
> >> - I += Pools - oldPools;
> >> + I = RebasePointer(I, oldPools, Pools);
> >>
> >> // Does the allocation failed ?
> >> if (!idxResult)
> >
> > In my patch RebasePointer invocation was after the idxResult check,
> > not before the check.
>
> Theoretically, order here might be important. In practice, it doesn't
> matter.
We normally try to write code that raises less questions.
> > By the way, in this and other similar cases,
> > is there any reason for "Pools != oldPools" check?
> > Is RebasePointer incapable of handling this, or is it an optimization?
> >
>
> It's just an optimization, it may be removed.
OK
> > [...]
> >> diff --git a/apt/apt-pkg/rebase_pointer.h b/apt/apt-pkg/rebase_pointer.h
> >> new file mode 100644
> >> index 0000000..f6b3c15
> >> --- /dev/null
> >> +++ b/apt/apt-pkg/rebase_pointer.h
> >> @@ -0,0 +1,16 @@
> >> +#ifndef PKGLIB_REBASE_POINTER_H
> >> +#define PKGLIB_REBASE_POINTER_H
> >> +
> >> +template <typename T>
> >> +static inline T* RebasePointer(T *ptr, void *old_base, void *new_base)
> >> +{
> >> + return reinterpret_cast<T*>(reinterpret_cast<char*>(new_base) + (reinterpret_cast<char*>(ptr) - reinterpret_cast<char*>(old_base)));
> >> +}
> >> +
> >> +template <typename T>
> >> +static inline const T* RebasePointer(const T *ptr, void *old_base, void *new_base)
> >> +{
> >> + return reinterpret_cast<const T*>(reinterpret_cast<char*>(new_base) + (reinterpret_cast<const char*>(ptr) - reinterpret_cast<char*>(old_base)));
> >> +}
> >> +
> >> +#endif
> >
> > Do we really need two templates here?
>
> Yes, second template with const ptr is needed for
> rpmListParser::rpmListParser from rpmlistparser.cc.
>
> Variable SeenPackages has type SeenPackagesType, which is a typedef to
> std::set<const char*,cstr_lt_pred>. Thus, elements are 'const char*',
> and either it should be const-casted to 'char*', which is ugly, or
> const-correctness should be achieved some other way, for example by
> getting rid of unimportant const qualifiers like in my changes.
>
> And first template is needed for every other case with non-const ptr.
To be honest, I find my October version of the fix easier to read.
Since all users of RebasePointer except rpmListParser use it in a form of
ptr = RebasePointer(ptr, old_base, new_base);
I find it more natural when RebasePointer updates the pointer,
so one can write
RebasePointer(ptr, old_base, new_base);
instead.
OK, I posted my version of the fix.
--
ldv
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [devel] [PATCH for apt 2/2 v2] Fix pointer arithmetics
2019-12-10 0:07 ` Dmitry V. Levin
@ 2019-12-10 8:18 ` Aleksei Nikiforov
2019-12-10 10:20 ` Dmitry V. Levin
0 siblings, 1 reply; 34+ messages in thread
From: Aleksei Nikiforov @ 2019-12-10 8:18 UTC (permalink / raw)
To: devel
10.12.2019 3:07, Dmitry V. Levin пишет:
> On Mon, Dec 09, 2019 at 10:08:42AM +0300, Aleksei Nikiforov wrote:
>> 09.12.2019 2:21, Dmitry V. Levin пишет:
>>> On Fri, Dec 06, 2019 at 06:36:55PM +0300, Aleksei Nikiforov wrote:
>>> [...]
>>>> @@ -85,11 +87,11 @@ class pkgCache::PkgIterator
>>>> inline unsigned long long Index() const {return Pkg - Owner->PkgP;};
>>>> OkState State() const;
>>>>
>>>> - void ReMap(void const * const oldMap, void const * const newMap)
>>>> + void ReMap(void *oldMap, void *newMap)
>>>
>>> Is there any particular reason for stripping const here and in other
>>> similar places?
>>
>> Yes, it's needed due to issues emerging from mixing const and non-const
>> pointers with new and allegedly more proper way of calculating rebased
>> pointers.
>
> Sorry, I don't find this argument convincing.
> I have experienced no const issues in my version of this fix.
>
Your version is using C-style casts in C++ code. Of course, I could use
C-style casts or const_cast-s too to work around const correctness
issues (i.e. to just hide these issues), and it'd work like your
version. But I'd like to remind you that APT is C++ project, not a C
project. What might be ok for C is sometimes a dirty ugly hack in modern
C++.
>>> [...]
>>>> @@ -301,7 +302,7 @@ std::experimental::optional<map_ptrloc> DynamicMMap::Allocate(unsigned long Item
>>>> Pool* oldPools = Pools;
>>>> auto idxResult = RawAllocate(I->Count*ItemSize,ItemSize);
>>>> if (Pools != oldPools)
>>>> - I += Pools - oldPools;
>>>> + I = RebasePointer(I, oldPools, Pools);
>>>>
>>>> // Does the allocation failed ?
>>>> if (!idxResult)
>>>
>>> In my patch RebasePointer invocation was after the idxResult check,
>>> not before the check.
>>
>> Theoretically, order here might be important. In practice, it doesn't
>> matter.
>
> We normally try to write code that raises less questions.
>
In that case it's better to keep order from my patch, isn't it?
Practically it's fine either way, but theoretically that order is superior.
>>> By the way, in this and other similar cases,
>>> is there any reason for "Pools != oldPools" check?
>>> Is RebasePointer incapable of handling this, or is it an optimization?
>>>
>>
>> It's just an optimization, it may be removed.
>
> OK
>
>>> [...]
>>>> diff --git a/apt/apt-pkg/rebase_pointer.h b/apt/apt-pkg/rebase_pointer.h
>>>> new file mode 100644
>>>> index 0000000..f6b3c15
>>>> --- /dev/null
>>>> +++ b/apt/apt-pkg/rebase_pointer.h
>>>> @@ -0,0 +1,16 @@
>>>> +#ifndef PKGLIB_REBASE_POINTER_H
>>>> +#define PKGLIB_REBASE_POINTER_H
>>>> +
>>>> +template <typename T>
>>>> +static inline T* RebasePointer(T *ptr, void *old_base, void *new_base)
>>>> +{
>>>> + return reinterpret_cast<T*>(reinterpret_cast<char*>(new_base) + (reinterpret_cast<char*>(ptr) - reinterpret_cast<char*>(old_base)));
>>>> +}
>>>> +
>>>> +template <typename T>
>>>> +static inline const T* RebasePointer(const T *ptr, void *old_base, void *new_base)
>>>> +{
>>>> + return reinterpret_cast<const T*>(reinterpret_cast<char*>(new_base) + (reinterpret_cast<const char*>(ptr) - reinterpret_cast<char*>(old_base)));
>>>> +}
>>>> +
>>>> +#endif
>>>
>>> Do we really need two templates here?
>>
>> Yes, second template with const ptr is needed for
>> rpmListParser::rpmListParser from rpmlistparser.cc.
>>
>> Variable SeenPackages has type SeenPackagesType, which is a typedef to
>> std::set<const char*,cstr_lt_pred>. Thus, elements are 'const char*',
>> and either it should be const-casted to 'char*', which is ugly, or
>> const-correctness should be achieved some other way, for example by
>> getting rid of unimportant const qualifiers like in my changes.
>>
>> And first template is needed for every other case with non-const ptr.
>
> To be honest, I find my October version of the fix easier to read.
>
> Since all users of RebasePointer except rpmListParser use it in a form of
> ptr = RebasePointer(ptr, old_base, new_base);
> I find it more natural when RebasePointer updates the pointer,
> so one can write
> RebasePointer(ptr, old_base, new_base);
> instead.
>
> OK, I posted my version of the fix.
>
And it's opposite for me. I prefer to explicitly see when variable is
changed. And for all calls it looks exactly same: no matter how it's
used, new pointer is returned from function as a result of function.
Interface uniformity, obviousness and predictability is important.
>
>
> _______________________________________________
> Devel mailing list
> Devel@lists.altlinux.org
> https://lists.altlinux.org/mailman/listinfo/devel
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [devel] [PATCH apt 3/3] Fix UB in pointer arithmetic
2019-12-09 23:56 ` [devel] [PATCH apt 3/3] Fix UB in pointer arithmetic Dmitry V. Levin
@ 2019-12-10 8:18 ` Aleksei Nikiforov
0 siblings, 0 replies; 34+ messages in thread
From: Aleksei Nikiforov @ 2019-12-10 8:18 UTC (permalink / raw)
To: devel
10.12.2019 2:56, Dmitry V. Levin пишет:
> Commit 6d5e6a689d07de8feef2cbecb24bc42d5994861b aka 0.5.15lorg2-alt70~9
> among other changes introduced UB in pointer arithmetic by casting raw
> pointers to specific types.
>
> Fix this by introducing two helpers for rebasing pointers in a safe way.
>
> Co-developed-by: Aleksei Nikiforov <darktemplar@altlinux.org>
This line is not true. I didn't participate in creation of this version
of patch. Please remove it.
> Fixes: 6d5e6a68 ("apt-pkg/pkgcachegen.{cc,h} changes")
> ---
> apt/apt-pkg/Makefile.am | 1 +
> apt/apt-pkg/cacheiterators.h | 14 ++++++++------
> apt/apt-pkg/contrib/mmap.cc | 8 ++++----
> apt/apt-pkg/pkgcachegen.cc | 27 +++++++++++----------------
> apt/apt-pkg/rebase_pointer.h | 25 +++++++++++++++++++++++++
> apt/apt-pkg/rpm/rpmlistparser.cc | 3 ++-
> 6 files changed, 51 insertions(+), 27 deletions(-)
> create mode 100644 apt/apt-pkg/rebase_pointer.h
>
> diff --git a/apt/apt-pkg/Makefile.am b/apt/apt-pkg/Makefile.am
> index 4c0d234..d038d01 100644
> --- a/apt/apt-pkg/Makefile.am
> +++ b/apt/apt-pkg/Makefile.am
> @@ -94,6 +94,7 @@ libapt_pkg_la_SOURCES = \
> pkgsystem.h \
> policy.cc \
> policy.h \
> + rebase_pointer.h \
> repository.cc \
> repository.h \
> scopeexit.h \
> diff --git a/apt/apt-pkg/cacheiterators.h b/apt/apt-pkg/cacheiterators.h
> index 9dffeb3..3c60cb8 100644
> --- a/apt/apt-pkg/cacheiterators.h
> +++ b/apt/apt-pkg/cacheiterators.h
> @@ -34,6 +34,8 @@
> #pragma interface "apt-pkg/cacheiterators.h"
> #endif
>
> +#include <apt-pkg/rebase_pointer.h>
> +
> // Package Iterator
> class pkgCache::PkgIterator
> {
> @@ -87,7 +89,7 @@ class pkgCache::PkgIterator
> {
> if (Owner == 0 || Pkg == 0)
> return;
> - Pkg += static_cast<Package const *>(newMap) - static_cast<Package const *>(oldMap);
> + RebasePointer(Pkg, oldMap, newMap);
> }
>
> // Constructors
> @@ -149,7 +151,7 @@ class pkgCache::VerIterator
> {
> if (Owner == 0 || Ver == 0)
> return;
> - Ver += static_cast<Version const *>(newMap) - static_cast<Version const *>(oldMap);
> + RebasePointer(Ver, oldMap, newMap);
> }
>
> inline VerIterator() : Ver(0), Owner(0) {};
> @@ -222,7 +224,7 @@ class pkgCache::DepIterator
> {
> if (Owner == 0 || Dep == 0)
> return;
> - Dep += static_cast<Dependency const *>(newMap) - static_cast<Dependency const *>(oldMap);
> + RebasePointer(Dep, oldMap, newMap);
> }
>
> inline DepIterator(pkgCache &Owner,Dependency *Trg,Version * = 0) :
> @@ -281,7 +283,7 @@ class pkgCache::PrvIterator
> {
> if (Owner == 0 || Prv == 0)
> return;
> - Prv += static_cast<Provides const *>(newMap) - static_cast<Provides const *>(oldMap);
> + RebasePointer(Prv, oldMap, newMap);
> }
>
> inline PrvIterator() : Prv(0), Type(PrvVer), Owner(0) {};
> @@ -344,7 +346,7 @@ class pkgCache::PkgFileIterator
> {
> if (Owner == 0 || File == 0)
> return;
> - File += static_cast<PackageFile const *>(newMap) - static_cast<PackageFile const *>(oldMap);
> + RebasePointer(File, oldMap, newMap);
> }
>
> // Constructors
> @@ -385,7 +387,7 @@ class pkgCache::VerFileIterator
> {
> if (Owner == 0 || FileP == 0)
> return;
> - FileP += static_cast<VerFile const *>(newMap) - static_cast<VerFile const *>(oldMap);
> + RebasePointer(FileP, oldMap, newMap);
> }
>
> inline VerFileIterator() : Owner(0), FileP(0) {};
> diff --git a/apt/apt-pkg/contrib/mmap.cc b/apt/apt-pkg/contrib/mmap.cc
> index 2064fc4..779d7a6 100644
> --- a/apt/apt-pkg/contrib/mmap.cc
> +++ b/apt/apt-pkg/contrib/mmap.cc
> @@ -30,6 +30,7 @@
> #include <apt-pkg/configuration.h>
> #include <apt-pkg/mmap.h>
> #include <apt-pkg/error.h>
> +#include <apt-pkg/rebase_pointer.h>
>
> #include <apti18n.h>
>
> @@ -285,13 +286,12 @@ std::experimental::optional<map_ptrloc> DynamicMMap::Allocate(unsigned long Item
> I->Count = size/ItemSize;
> Pool* oldPools = Pools;
> auto idxResult = RawAllocate(I->Count*ItemSize,ItemSize);
> - if (Pools != oldPools)
> - I += Pools - oldPools;
>
> // Does the allocation failed ?
> if (!idxResult)
> return idxResult;
>
> + RebasePointer(I, oldPools, Pools);
> Result = *idxResult;
> I->Start = Result;
> }
> @@ -356,7 +356,7 @@ bool DynamicMMap::Grow(unsigned long long size)
> Fd->Write(&C,sizeof(C));
> }
>
> - unsigned long const poolOffset = Pools - ((Pool*) Base);
> + const void *old_base = Base;
>
> if (Fd != 0)
> {
> @@ -393,7 +393,7 @@ bool DynamicMMap::Grow(unsigned long long size)
> memset((char*)Base + WorkSpace, 0, newSize - WorkSpace);
> }
>
> - Pools = (Pool*) Base + poolOffset;
> + RebasePointer(Pools, old_base, Base);
> WorkSpace = newSize;
>
> return true;
> diff --git a/apt/apt-pkg/pkgcachegen.cc b/apt/apt-pkg/pkgcachegen.cc
> index 56716b5..7a5a20c 100644
> --- a/apt/apt-pkg/pkgcachegen.cc
> +++ b/apt/apt-pkg/pkgcachegen.cc
> @@ -26,6 +26,7 @@
> #include <apt-pkg/strutl.h>
> #include <apt-pkg/sptr.h>
> #include <apt-pkg/pkgsystem.h>
> +#include <apt-pkg/rebase_pointer.h>
>
> #include <apti18n.h>
>
> @@ -116,11 +117,11 @@ void pkgCacheGenerator::ReMap(void const * const oldMap, void const * const newM
>
> Cache.ReMap(false);
>
> - CurrentFile += (pkgCache::PackageFile*) newMap - (pkgCache::PackageFile*) oldMap;
> + RebasePointer(CurrentFile, oldMap, newMap);
>
> for (size_t i = 0; i < _count(UniqHash); ++i)
> if (UniqHash[i] != 0)
> - UniqHash[i] += (pkgCache::StringItem*) newMap - (pkgCache::StringItem*) oldMap;
> + RebasePointer(UniqHash[i], oldMap, newMap);
>
> for (auto i = Dynamic<pkgCache::PkgIterator>::toReMap.begin();
> i != Dynamic<pkgCache::PkgIterator>::toReMap.end(); ++i)
> @@ -269,11 +270,8 @@ bool pkgCacheGenerator::MergeList(ListParser &List,
> continue;
> }
>
> - if (oldMap != Map.Data())
> - {
> - Last += (map_ptrloc*) Map.Data() - (map_ptrloc*) oldMap;
> - oldMap = Map.Data();
> - }
> + RebasePointer(Last, oldMap, Map.Data());
> + oldMap = Map.Data();
>
> // Skip to the end of the same version set.
> if (Res == 0)
> @@ -296,8 +294,7 @@ bool pkgCacheGenerator::MergeList(ListParser &List,
> return _error->Error(_("Error occurred while processing %s (NewVersion%d)"),
> PackageName.c_str(), 1);
>
> - if (oldMap != Map.Data())
> - Last += (map_ptrloc*) Map.Data() - (map_ptrloc*) oldMap;
> + RebasePointer(Last, oldMap, Map.Data());
> *Last = *verindex;
>
> Ver->ParentPkg = Pkg.Index();
> @@ -604,8 +601,9 @@ bool pkgCacheGenerator::ListParser::NewDepends(pkgCache::VerIterator &Ver,
> for (pkgCache::DepIterator D = Ver.DependsList(); D.end() == false; D++)
> OldDepLast = &D->NextDepends;
> OldDepVer = Ver;
> - } else if (oldMap != Owner->Map.Data())
> - OldDepLast += (map_ptrloc*) Owner->Map.Data() - (map_ptrloc*) oldMap;
> + } else {
> + RebasePointer(OldDepLast, oldMap, Owner->Map.Data());
> + }
>
> // Is it a file dependency?
> if (PackageName[0] == '/')
> @@ -745,11 +743,8 @@ std::experimental::optional<map_ptrloc> pkgCacheGenerator::WriteUniqString(const
> if ((!Item) || (!idxString))
> return std::experimental::optional<map_ptrloc>();
>
> - if (oldMap != Map.Data())
> - {
> - Last += (map_ptrloc*) Map.Data() - (map_ptrloc*) oldMap;
> - I += (pkgCache::StringItem*) Map.Data() - (pkgCache::StringItem*) oldMap;
> - }
> + RebasePointer(Last, oldMap, Map.Data());
> + RebasePointer(I, oldMap, Map.Data());
>
> *Last = *Item;
>
> diff --git a/apt/apt-pkg/rebase_pointer.h b/apt/apt-pkg/rebase_pointer.h
> new file mode 100644
> index 0000000..2bbabea
> --- /dev/null
> +++ b/apt/apt-pkg/rebase_pointer.h
> @@ -0,0 +1,25 @@
> +#ifndef PKGLIB_REBASE_POINTER_H
> +#define PKGLIB_REBASE_POINTER_H
> +
> +template <class T>
> +static inline T*
> +GetRebasedPointer(T*, const void *, const void *)
> +__attribute__((__warn_unused_result__));
> +
> +template <class T>
> +static inline T*
> +GetRebasedPointer(T* ptr, const void *old_base, const void *new_base)
> +{
> + // uintptr_t is a type with well-defined integer overflow semantics
> + uintptr_t diff = (uintptr_t) new_base - (uintptr_t) old_base;
> + return (T*) ((uintptr_t) ptr + diff);
> +}
> +
> +template <class T>
> +static inline void
> +RebasePointer(T* &ptr, const void *old_base, const void *new_base)
> +{
> + ptr = GetRebasedPointer(ptr, old_base, new_base);
> +}
> +
> +#endif
> diff --git a/apt/apt-pkg/rpm/rpmlistparser.cc b/apt/apt-pkg/rpm/rpmlistparser.cc
> index 9b2e9ad..4aeb937 100644
> --- a/apt/apt-pkg/rpm/rpmlistparser.cc
> +++ b/apt/apt-pkg/rpm/rpmlistparser.cc
> @@ -25,6 +25,7 @@
> #include <apt-pkg/strutl.h>
> #include <apt-pkg/crc-16.h>
> #include <apt-pkg/tagfile.h>
> +#include <apt-pkg/rebase_pointer.h>
>
> #include <apti18n.h>
>
> @@ -56,7 +57,7 @@ rpmListParser::rpmListParser(RPMHandler *Handler)
>
> for (auto iter: *SeenPackages)
> {
> - tmp.insert(iter + (static_cast<const char *>(newMap) - static_cast<const char *>(oldMap)));
> + tmp.insert(GetRebasedPointer(iter, oldMap, newMap));
> }
>
> SeenPackages->swap(tmp);
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [devel] [PATCH for apt 2/2 v2] Fix pointer arithmetics
2019-12-10 8:18 ` Aleksei Nikiforov
@ 2019-12-10 10:20 ` Dmitry V. Levin
2019-12-10 10:58 ` Aleksei Nikiforov
0 siblings, 1 reply; 34+ messages in thread
From: Dmitry V. Levin @ 2019-12-10 10:20 UTC (permalink / raw)
To: ALT Devel discussion list
[-- Attachment #1: Type: text/plain, Size: 5454 bytes --]
On Tue, Dec 10, 2019 at 11:18:06AM +0300, Aleksei Nikiforov wrote:
> 10.12.2019 3:07, Dmitry V. Levin пишет:
> > On Mon, Dec 09, 2019 at 10:08:42AM +0300, Aleksei Nikiforov wrote:
> >> 09.12.2019 2:21, Dmitry V. Levin пишет:
> >>> On Fri, Dec 06, 2019 at 06:36:55PM +0300, Aleksei Nikiforov wrote:
> >>> [...]
> >>>> @@ -85,11 +87,11 @@ class pkgCache::PkgIterator
> >>>> inline unsigned long long Index() const {return Pkg - Owner->PkgP;};
> >>>> OkState State() const;
> >>>>
> >>>> - void ReMap(void const * const oldMap, void const * const newMap)
> >>>> + void ReMap(void *oldMap, void *newMap)
> >>>
> >>> Is there any particular reason for stripping const here and in other
> >>> similar places?
> >>
> >> Yes, it's needed due to issues emerging from mixing const and non-const
> >> pointers with new and allegedly more proper way of calculating rebased
> >> pointers.
> >
> > Sorry, I don't find this argument convincing.
> > I have experienced no const issues in my version of this fix.
>
> Your version is using C-style casts in C++ code. Of course, I could use
> C-style casts or const_cast-s too to work around const correctness
> issues (i.e. to just hide these issues), and it'd work like your
> version. But I'd like to remind you that APT is C++ project, not a C
> project. What might be ok for C is sometimes a dirty ugly hack in modern
> C++.
Sorry, I don't share you point of view on this matter.
Being a C++ project allows you to use C++ constructs, that's true,
but why do you think it prevents you from using C constructs when
appropriate?
> >>>> @@ -301,7 +302,7 @@ std::experimental::optional<map_ptrloc> DynamicMMap::Allocate(unsigned long Item
> >>>> Pool* oldPools = Pools;
> >>>> auto idxResult = RawAllocate(I->Count*ItemSize,ItemSize);
> >>>> if (Pools != oldPools)
> >>>> - I += Pools - oldPools;
> >>>> + I = RebasePointer(I, oldPools, Pools);
> >>>>
> >>>> // Does the allocation failed ?
> >>>> if (!idxResult)
> >>>
> >>> In my patch RebasePointer invocation was after the idxResult check,
> >>> not before the check.
> >>
> >> Theoretically, order here might be important. In practice, it doesn't
> >> matter.
> >
> > We normally try to write code that raises less questions.
>
> In that case it's better to keep order from my patch, isn't it?
> Practically it's fine either way, but theoretically that order is superior.
The order in question was introduced by your commit
6d5e6a68 ("apt-pkg/pkgcachegen.{cc,h} changes").
If I was reviewing that commit, this would have been fixed already.
> >>> [...]
> >>>> diff --git a/apt/apt-pkg/rebase_pointer.h b/apt/apt-pkg/rebase_pointer.h
> >>>> new file mode 100644
> >>>> index 0000000..f6b3c15
> >>>> --- /dev/null
> >>>> +++ b/apt/apt-pkg/rebase_pointer.h
> >>>> @@ -0,0 +1,16 @@
> >>>> +#ifndef PKGLIB_REBASE_POINTER_H
> >>>> +#define PKGLIB_REBASE_POINTER_H
> >>>> +
> >>>> +template <typename T>
> >>>> +static inline T* RebasePointer(T *ptr, void *old_base, void *new_base)
> >>>> +{
> >>>> + return reinterpret_cast<T*>(reinterpret_cast<char*>(new_base) + (reinterpret_cast<char*>(ptr) - reinterpret_cast<char*>(old_base)));
> >>>> +}
> >>>> +
> >>>> +template <typename T>
> >>>> +static inline const T* RebasePointer(const T *ptr, void *old_base, void *new_base)
> >>>> +{
> >>>> + return reinterpret_cast<const T*>(reinterpret_cast<char*>(new_base) + (reinterpret_cast<const char*>(ptr) - reinterpret_cast<char*>(old_base)));
> >>>> +}
> >>>> +
> >>>> +#endif
> >>>
> >>> Do we really need two templates here?
> >>
> >> Yes, second template with const ptr is needed for
> >> rpmListParser::rpmListParser from rpmlistparser.cc.
> >>
> >> Variable SeenPackages has type SeenPackagesType, which is a typedef to
> >> std::set<const char*,cstr_lt_pred>. Thus, elements are 'const char*',
> >> and either it should be const-casted to 'char*', which is ugly, or
> >> const-correctness should be achieved some other way, for example by
> >> getting rid of unimportant const qualifiers like in my changes.
> >>
> >> And first template is needed for every other case with non-const ptr.
> >
> > To be honest, I find my October version of the fix easier to read.
> >
> > Since all users of RebasePointer except rpmListParser use it in a form of
> > ptr = RebasePointer(ptr, old_base, new_base);
> > I find it more natural when RebasePointer updates the pointer,
> > so one can write
> > RebasePointer(ptr, old_base, new_base);
> > instead.
> >
> > OK, I posted my version of the fix.
>
> And it's opposite for me. I prefer to explicitly see when variable is
> changed. And for all calls it looks exactly same: no matter how it's
> used, new pointer is returned from function as a result of function.
> Interface uniformity, obviousness and predictability is important.
What I don't like in your approach is that it's error-prone:
it's very easy to forget the assignment or to assign the result to a wrong
variable. In fact, I had to use the following regular expression just
to check whether all uses of RebasePointer are correct in that respect:
$ git grep -Fw RebasePointer |grep -v '\<\([[:alpha:]][][[:alnum:]_]*\) = RebasePointer(\1,'
This is surely not the way how things should be done,
neither in C nor in C++.
--
ldv
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [devel] [PATCH for apt 2/2 v2] Fix pointer arithmetics
2019-12-10 10:20 ` Dmitry V. Levin
@ 2019-12-10 10:58 ` Aleksei Nikiforov
2019-12-10 22:20 ` Dmitry V. Levin
0 siblings, 1 reply; 34+ messages in thread
From: Aleksei Nikiforov @ 2019-12-10 10:58 UTC (permalink / raw)
To: devel
10.12.2019 13:20, Dmitry V. Levin пишет:
> On Tue, Dec 10, 2019 at 11:18:06AM +0300, Aleksei Nikiforov wrote:
>> 10.12.2019 3:07, Dmitry V. Levin пишет:
>>> On Mon, Dec 09, 2019 at 10:08:42AM +0300, Aleksei Nikiforov wrote:
>>>> 09.12.2019 2:21, Dmitry V. Levin пишет:
>>>>> On Fri, Dec 06, 2019 at 06:36:55PM +0300, Aleksei Nikiforov wrote:
>>>>> [...]
>>>>>> @@ -85,11 +87,11 @@ class pkgCache::PkgIterator
>>>>>> inline unsigned long long Index() const {return Pkg - Owner->PkgP;};
>>>>>> OkState State() const;
>>>>>>
>>>>>> - void ReMap(void const * const oldMap, void const * const newMap)
>>>>>> + void ReMap(void *oldMap, void *newMap)
>>>>>
>>>>> Is there any particular reason for stripping const here and in other
>>>>> similar places?
>>>>
>>>> Yes, it's needed due to issues emerging from mixing const and non-const
>>>> pointers with new and allegedly more proper way of calculating rebased
>>>> pointers.
>>>
>>> Sorry, I don't find this argument convincing.
>>> I have experienced no const issues in my version of this fix.
>>
>> Your version is using C-style casts in C++ code. Of course, I could use
>> C-style casts or const_cast-s too to work around const correctness
>> issues (i.e. to just hide these issues), and it'd work like your
>> version. But I'd like to remind you that APT is C++ project, not a C
>> project. What might be ok for C is sometimes a dirty ugly hack in modern
>> C++.
>
> Sorry, I don't share you point of view on this matter.
> Being a C++ project allows you to use C++ constructs, that's true,
> but why do you think it prevents you from using C constructs when
> appropriate?
>
I didn't say that something prevents from using C constructs when
appropriate. I'm saying that these constructs are not appropriate here.
>>>>>> @@ -301,7 +302,7 @@ std::experimental::optional<map_ptrloc> DynamicMMap::Allocate(unsigned long Item
>>>>>> Pool* oldPools = Pools;
>>>>>> auto idxResult = RawAllocate(I->Count*ItemSize,ItemSize);
>>>>>> if (Pools != oldPools)
>>>>>> - I += Pools - oldPools;
>>>>>> + I = RebasePointer(I, oldPools, Pools);
>>>>>>
>>>>>> // Does the allocation failed ?
>>>>>> if (!idxResult)
>>>>>
>>>>> In my patch RebasePointer invocation was after the idxResult check,
>>>>> not before the check.
>>>>
>>>> Theoretically, order here might be important. In practice, it doesn't
>>>> matter.
>>>
>>> We normally try to write code that raises less questions.
>>
>> In that case it's better to keep order from my patch, isn't it?
>> Practically it's fine either way, but theoretically that order is superior.
>
> The order in question was introduced by your commit
> 6d5e6a68 ("apt-pkg/pkgcachegen.{cc,h} changes").
>
> If I was reviewing that commit, this would have been fixed already.
>
So, do you have any reason why it should be changed?
>>>>> [...]
>>>>>> diff --git a/apt/apt-pkg/rebase_pointer.h b/apt/apt-pkg/rebase_pointer.h
>>>>>> new file mode 100644
>>>>>> index 0000000..f6b3c15
>>>>>> --- /dev/null
>>>>>> +++ b/apt/apt-pkg/rebase_pointer.h
>>>>>> @@ -0,0 +1,16 @@
>>>>>> +#ifndef PKGLIB_REBASE_POINTER_H
>>>>>> +#define PKGLIB_REBASE_POINTER_H
>>>>>> +
>>>>>> +template <typename T>
>>>>>> +static inline T* RebasePointer(T *ptr, void *old_base, void *new_base)
>>>>>> +{
>>>>>> + return reinterpret_cast<T*>(reinterpret_cast<char*>(new_base) + (reinterpret_cast<char*>(ptr) - reinterpret_cast<char*>(old_base)));
>>>>>> +}
>>>>>> +
>>>>>> +template <typename T>
>>>>>> +static inline const T* RebasePointer(const T *ptr, void *old_base, void *new_base)
>>>>>> +{
>>>>>> + return reinterpret_cast<const T*>(reinterpret_cast<char*>(new_base) + (reinterpret_cast<const char*>(ptr) - reinterpret_cast<char*>(old_base)));
>>>>>> +}
>>>>>> +
>>>>>> +#endif
>>>>>
>>>>> Do we really need two templates here?
>>>>
>>>> Yes, second template with const ptr is needed for
>>>> rpmListParser::rpmListParser from rpmlistparser.cc.
>>>>
>>>> Variable SeenPackages has type SeenPackagesType, which is a typedef to
>>>> std::set<const char*,cstr_lt_pred>. Thus, elements are 'const char*',
>>>> and either it should be const-casted to 'char*', which is ugly, or
>>>> const-correctness should be achieved some other way, for example by
>>>> getting rid of unimportant const qualifiers like in my changes.
>>>>
>>>> And first template is needed for every other case with non-const ptr.
>>>
>>> To be honest, I find my October version of the fix easier to read.
>>>
>>> Since all users of RebasePointer except rpmListParser use it in a form of
>>> ptr = RebasePointer(ptr, old_base, new_base);
>>> I find it more natural when RebasePointer updates the pointer,
>>> so one can write
>>> RebasePointer(ptr, old_base, new_base);
>>> instead.
>>>
>>> OK, I posted my version of the fix.
>>
>> And it's opposite for me. I prefer to explicitly see when variable is
>> changed. And for all calls it looks exactly same: no matter how it's
>> used, new pointer is returned from function as a result of function.
>> Interface uniformity, obviousness and predictability is important.
>
> What I don't like in your approach is that it's error-prone:
> it's very easy to forget the assignment or to assign the result to a wrong
> variable. In fact, I had to use the following regular expression just
> to check whether all uses of RebasePointer are correct in that respect:
>
> $ git grep -Fw RebasePointer |grep -v '\<\([[:alpha:]][][[:alnum:]_]*\) = RebasePointer(\1,'
>
> This is surely not the way how things should be done,
> neither in C nor in C++.
>
>
It's also very easy to miss one of places where such pointer
recalculation is required, but you still want this solution instead of
generic and centralized memory alignment one. So much for uniformity of
approaches and solutions.
As for forgetting assignment, your addition of attribute 'warn unused
result' in your version of patch fixes this potential issue. As for
other potential issues, they are very far-fetched and synthetic.
>
> _______________________________________________
> Devel mailing list
> Devel@lists.altlinux.org
> https://lists.altlinux.org/mailman/listinfo/devel
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [devel] [PATCH for apt 2/2 v2] Fix pointer arithmetics
2019-12-10 10:58 ` Aleksei Nikiforov
@ 2019-12-10 22:20 ` Dmitry V. Levin
2019-12-11 7:50 ` Aleksei Nikiforov
0 siblings, 1 reply; 34+ messages in thread
From: Dmitry V. Levin @ 2019-12-10 22:20 UTC (permalink / raw)
To: ALT Devel discussion list
[-- Attachment #1: Type: text/plain, Size: 7161 bytes --]
On Tue, Dec 10, 2019 at 01:58:17PM +0300, Aleksei Nikiforov wrote:
> 10.12.2019 13:20, Dmitry V. Levin пишет:
> > On Tue, Dec 10, 2019 at 11:18:06AM +0300, Aleksei Nikiforov wrote:
> >> 10.12.2019 3:07, Dmitry V. Levin пишет:
> >>> On Mon, Dec 09, 2019 at 10:08:42AM +0300, Aleksei Nikiforov wrote:
> >>>> 09.12.2019 2:21, Dmitry V. Levin пишет:
> >>>>> On Fri, Dec 06, 2019 at 06:36:55PM +0300, Aleksei Nikiforov wrote:
> >>>>> [...]
> >>>>>> @@ -85,11 +87,11 @@ class pkgCache::PkgIterator
> >>>>>> inline unsigned long long Index() const {return Pkg - Owner->PkgP;};
> >>>>>> OkState State() const;
> >>>>>>
> >>>>>> - void ReMap(void const * const oldMap, void const * const newMap)
> >>>>>> + void ReMap(void *oldMap, void *newMap)
> >>>>>
> >>>>> Is there any particular reason for stripping const here and in other
> >>>>> similar places?
> >>>>
> >>>> Yes, it's needed due to issues emerging from mixing const and non-const
> >>>> pointers with new and allegedly more proper way of calculating rebased
> >>>> pointers.
> >>>
> >>> Sorry, I don't find this argument convincing.
> >>> I have experienced no const issues in my version of this fix.
> >>
> >> Your version is using C-style casts in C++ code. Of course, I could use
> >> C-style casts or const_cast-s too to work around const correctness
> >> issues (i.e. to just hide these issues), and it'd work like your
> >> version. But I'd like to remind you that APT is C++ project, not a C
> >> project. What might be ok for C is sometimes a dirty ugly hack in modern
> >> C++.
> >
> > Sorry, I don't share you point of view on this matter.
> > Being a C++ project allows you to use C++ constructs, that's true,
> > but why do you think it prevents you from using C constructs when
> > appropriate?
>
> I didn't say that something prevents from using C constructs when
> appropriate. I'm saying that these constructs are not appropriate here.
Why do you think they are not appropriate here?
> >>>>>> @@ -301,7 +302,7 @@ std::experimental::optional<map_ptrloc> DynamicMMap::Allocate(unsigned long Item
> >>>>>> Pool* oldPools = Pools;
> >>>>>> auto idxResult = RawAllocate(I->Count*ItemSize,ItemSize);
> >>>>>> if (Pools != oldPools)
> >>>>>> - I += Pools - oldPools;
> >>>>>> + I = RebasePointer(I, oldPools, Pools);
> >>>>>>
> >>>>>> // Does the allocation failed ?
> >>>>>> if (!idxResult)
> >>>>>
> >>>>> In my patch RebasePointer invocation was after the idxResult check,
> >>>>> not before the check.
> >>>>
> >>>> Theoretically, order here might be important. In practice, it doesn't
> >>>> matter.
> >>>
> >>> We normally try to write code that raises less questions.
> >>
> >> In that case it's better to keep order from my patch, isn't it?
> >> Practically it's fine either way, but theoretically that order is superior.
> >
> > The order in question was introduced by your commit
> > 6d5e6a68 ("apt-pkg/pkgcachegen.{cc,h} changes").
> >
> > If I was reviewing that commit, this would have been fixed already.
>
> So, do you have any reason why it should be changed?
One of the most basic coding rules says: the return value that needs
checking has to be checked prior to any meaningful use.
> >>>>> [...]
> >>>>>> diff --git a/apt/apt-pkg/rebase_pointer.h b/apt/apt-pkg/rebase_pointer.h
> >>>>>> new file mode 100644
> >>>>>> index 0000000..f6b3c15
> >>>>>> --- /dev/null
> >>>>>> +++ b/apt/apt-pkg/rebase_pointer.h
> >>>>>> @@ -0,0 +1,16 @@
> >>>>>> +#ifndef PKGLIB_REBASE_POINTER_H
> >>>>>> +#define PKGLIB_REBASE_POINTER_H
> >>>>>> +
> >>>>>> +template <typename T>
> >>>>>> +static inline T* RebasePointer(T *ptr, void *old_base, void *new_base)
> >>>>>> +{
> >>>>>> + return reinterpret_cast<T*>(reinterpret_cast<char*>(new_base) + (reinterpret_cast<char*>(ptr) - reinterpret_cast<char*>(old_base)));
> >>>>>> +}
> >>>>>> +
> >>>>>> +template <typename T>
> >>>>>> +static inline const T* RebasePointer(const T *ptr, void *old_base, void *new_base)
> >>>>>> +{
> >>>>>> + return reinterpret_cast<const T*>(reinterpret_cast<char*>(new_base) + (reinterpret_cast<const char*>(ptr) - reinterpret_cast<char*>(old_base)));
> >>>>>> +}
> >>>>>> +
> >>>>>> +#endif
> >>>>>
> >>>>> Do we really need two templates here?
> >>>>
> >>>> Yes, second template with const ptr is needed for
> >>>> rpmListParser::rpmListParser from rpmlistparser.cc.
> >>>>
> >>>> Variable SeenPackages has type SeenPackagesType, which is a typedef to
> >>>> std::set<const char*,cstr_lt_pred>. Thus, elements are 'const char*',
> >>>> and either it should be const-casted to 'char*', which is ugly, or
> >>>> const-correctness should be achieved some other way, for example by
> >>>> getting rid of unimportant const qualifiers like in my changes.
> >>>>
> >>>> And first template is needed for every other case with non-const ptr.
> >>>
> >>> To be honest, I find my October version of the fix easier to read.
> >>>
> >>> Since all users of RebasePointer except rpmListParser use it in a form of
> >>> ptr = RebasePointer(ptr, old_base, new_base);
> >>> I find it more natural when RebasePointer updates the pointer,
> >>> so one can write
> >>> RebasePointer(ptr, old_base, new_base);
> >>> instead.
> >>>
> >>> OK, I posted my version of the fix.
> >>
> >> And it's opposite for me. I prefer to explicitly see when variable is
> >> changed. And for all calls it looks exactly same: no matter how it's
> >> used, new pointer is returned from function as a result of function.
> >> Interface uniformity, obviousness and predictability is important.
> >
> > What I don't like in your approach is that it's error-prone:
> > it's very easy to forget the assignment or to assign the result to a wrong
> > variable. In fact, I had to use the following regular expression just
> > to check whether all uses of RebasePointer are correct in that respect:
> >
> > $ git grep -Fw RebasePointer |grep -v '\<\([[:alpha:]][][[:alnum:]_]*\) = RebasePointer(\1,'
> >
> > This is surely not the way how things should be done,
> > neither in C nor in C++.
>
> It's also very easy to miss one of places where such pointer
> recalculation is required,
There must be a way to exclude this possibility.
> but you still want this solution instead of
> generic and centralized memory alignment one.
The approach you mentioned is definitely wasteful,
but it's by no means generic or centralized.
> So much for uniformity of approaches and solutions.
>
> As for forgetting assignment, your addition of attribute 'warn unused
> result' in your version of patch fixes this potential issue.
Unfortunately, warn_unused_result attribute does not fix anything yet
because it's too easy to miss a new warning among several hundreds of
already existing warnings. This might help someday in the future when
the whole codebase is ready for -Werror.
> As for other potential issues, they are very far-fetched and synthetic.
Well, I don't think so. :)
--
ldv
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [devel] [PATCH for apt 2/2 v2] Fix pointer arithmetics
2019-12-10 22:20 ` Dmitry V. Levin
@ 2019-12-11 7:50 ` Aleksei Nikiforov
2019-12-12 19:43 ` Andrey Savchenko
0 siblings, 1 reply; 34+ messages in thread
From: Aleksei Nikiforov @ 2019-12-11 7:50 UTC (permalink / raw)
To: devel
11.12.2019 1:20, Dmitry V. Levin пишет:
> On Tue, Dec 10, 2019 at 01:58:17PM +0300, Aleksei Nikiforov wrote:
>> 10.12.2019 13:20, Dmitry V. Levin пишет:
>>> On Tue, Dec 10, 2019 at 11:18:06AM +0300, Aleksei Nikiforov wrote:
>>>> 10.12.2019 3:07, Dmitry V. Levin пишет:
>>>>> On Mon, Dec 09, 2019 at 10:08:42AM +0300, Aleksei Nikiforov wrote:
>>>>>> 09.12.2019 2:21, Dmitry V. Levin пишет:
>>>>>>> On Fri, Dec 06, 2019 at 06:36:55PM +0300, Aleksei Nikiforov wrote:
>>>>>>> [...]
>>>>>>>> @@ -85,11 +87,11 @@ class pkgCache::PkgIterator
>>>>>>>> inline unsigned long long Index() const {return Pkg - Owner->PkgP;};
>>>>>>>> OkState State() const;
>>>>>>>>
>>>>>>>> - void ReMap(void const * const oldMap, void const * const newMap)
>>>>>>>> + void ReMap(void *oldMap, void *newMap)
>>>>>>>
>>>>>>> Is there any particular reason for stripping const here and in other
>>>>>>> similar places?
>>>>>>
>>>>>> Yes, it's needed due to issues emerging from mixing const and non-const
>>>>>> pointers with new and allegedly more proper way of calculating rebased
>>>>>> pointers.
>>>>>
>>>>> Sorry, I don't find this argument convincing.
>>>>> I have experienced no const issues in my version of this fix.
>>>>
>>>> Your version is using C-style casts in C++ code. Of course, I could use
>>>> C-style casts or const_cast-s too to work around const correctness
>>>> issues (i.e. to just hide these issues), and it'd work like your
>>>> version. But I'd like to remind you that APT is C++ project, not a C
>>>> project. What might be ok for C is sometimes a dirty ugly hack in modern
>>>> C++.
>>>
>>> Sorry, I don't share you point of view on this matter.
>>> Being a C++ project allows you to use C++ constructs, that's true,
>>> but why do you think it prevents you from using C constructs when
>>> appropriate?
>>
>> I didn't say that something prevents from using C constructs when
>> appropriate. I'm saying that these constructs are not appropriate here.
>
> Why do you think they are not appropriate here?
>
In good C++ code there is no place for const_cast. Maybe there are some
exceptions to it, but they have to be justified. It doesn't matter that
you are hiding it behind C-style cast.
>>>>>>>> @@ -301,7 +302,7 @@ std::experimental::optional<map_ptrloc> DynamicMMap::Allocate(unsigned long Item
>>>>>>>> Pool* oldPools = Pools;
>>>>>>>> auto idxResult = RawAllocate(I->Count*ItemSize,ItemSize);
>>>>>>>> if (Pools != oldPools)
>>>>>>>> - I += Pools - oldPools;
>>>>>>>> + I = RebasePointer(I, oldPools, Pools);
>>>>>>>>
>>>>>>>> // Does the allocation failed ?
>>>>>>>> if (!idxResult)
>>>>>>>
>>>>>>> In my patch RebasePointer invocation was after the idxResult check,
>>>>>>> not before the check.
>>>>>>
>>>>>> Theoretically, order here might be important. In practice, it doesn't
>>>>>> matter.
>>>>>
>>>>> We normally try to write code that raises less questions.
>>>>
>>>> In that case it's better to keep order from my patch, isn't it?
>>>> Practically it's fine either way, but theoretically that order is superior.
>>>
>>> The order in question was introduced by your commit
>>> 6d5e6a68 ("apt-pkg/pkgcachegen.{cc,h} changes").
>>>
>>> If I was reviewing that commit, this would have been fixed already.
>>
>> So, do you have any reason why it should be changed?
>
> One of the most basic coding rules says: the return value that needs
> checking has to be checked prior to any meaningful use.
>
Ok, considering this rule, looks good.
>>>>>>> [...]
>>>>>>>> diff --git a/apt/apt-pkg/rebase_pointer.h b/apt/apt-pkg/rebase_pointer.h
>>>>>>>> new file mode 100644
>>>>>>>> index 0000000..f6b3c15
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/apt/apt-pkg/rebase_pointer.h
>>>>>>>> @@ -0,0 +1,16 @@
>>>>>>>> +#ifndef PKGLIB_REBASE_POINTER_H
>>>>>>>> +#define PKGLIB_REBASE_POINTER_H
>>>>>>>> +
>>>>>>>> +template <typename T>
>>>>>>>> +static inline T* RebasePointer(T *ptr, void *old_base, void *new_base)
>>>>>>>> +{
>>>>>>>> + return reinterpret_cast<T*>(reinterpret_cast<char*>(new_base) + (reinterpret_cast<char*>(ptr) - reinterpret_cast<char*>(old_base)));
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +template <typename T>
>>>>>>>> +static inline const T* RebasePointer(const T *ptr, void *old_base, void *new_base)
>>>>>>>> +{
>>>>>>>> + return reinterpret_cast<const T*>(reinterpret_cast<char*>(new_base) + (reinterpret_cast<const char*>(ptr) - reinterpret_cast<char*>(old_base)));
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +#endif
>>>>>>>
>>>>>>> Do we really need two templates here?
>>>>>>
>>>>>> Yes, second template with const ptr is needed for
>>>>>> rpmListParser::rpmListParser from rpmlistparser.cc.
>>>>>>
>>>>>> Variable SeenPackages has type SeenPackagesType, which is a typedef to
>>>>>> std::set<const char*,cstr_lt_pred>. Thus, elements are 'const char*',
>>>>>> and either it should be const-casted to 'char*', which is ugly, or
>>>>>> const-correctness should be achieved some other way, for example by
>>>>>> getting rid of unimportant const qualifiers like in my changes.
>>>>>>
>>>>>> And first template is needed for every other case with non-const ptr.
>>>>>
>>>>> To be honest, I find my October version of the fix easier to read.
>>>>>
>>>>> Since all users of RebasePointer except rpmListParser use it in a form of
>>>>> ptr = RebasePointer(ptr, old_base, new_base);
>>>>> I find it more natural when RebasePointer updates the pointer,
>>>>> so one can write
>>>>> RebasePointer(ptr, old_base, new_base);
>>>>> instead.
>>>>>
>>>>> OK, I posted my version of the fix.
>>>>
>>>> And it's opposite for me. I prefer to explicitly see when variable is
>>>> changed. And for all calls it looks exactly same: no matter how it's
>>>> used, new pointer is returned from function as a result of function.
>>>> Interface uniformity, obviousness and predictability is important.
>>>
>>> What I don't like in your approach is that it's error-prone:
>>> it's very easy to forget the assignment or to assign the result to a wrong
>>> variable. In fact, I had to use the following regular expression just
>>> to check whether all uses of RebasePointer are correct in that respect:
>>>
>>> $ git grep -Fw RebasePointer |grep -v '\<\([[:alpha:]][][[:alnum:]_]*\) = RebasePointer(\1,'
>>>
>>> This is surely not the way how things should be done,
>>> neither in C nor in C++.
>>
>> It's also very easy to miss one of places where such pointer
>> recalculation is required,
>
> There must be a way to exclude this possibility.
>
>> but you still want this solution instead of
>> generic and centralized memory alignment one.
>
> The approach you mentioned is definitely wasteful,
> but it's by no means generic or centralized.
>
Not as wasteful as you speculated it is. And no, it is centralized and
generic. It fixes all issues caused by remainders of division being
non-zero in pointer math.
>> So much for uniformity of approaches and solutions.
>>
>> As for forgetting assignment, your addition of attribute 'warn unused
>> result' in your version of patch fixes this potential issue.
>
> Unfortunately, warn_unused_result attribute does not fix anything yet
> because it's too easy to miss a new warning among several hundreds of
> already existing warnings. This might help someday in the future when
> the whole codebase is ready for -Werror.
>
It's possible to add -Werror=warning-type compiler option to convert
specific warning into error. You wouldn't miss an error generated by
compiler, would you? :) Btw, while we're discussing compiler options,
you can turn errors back into warnings with -Wno-error=warning-type
compiler option when using -Werror.
>> As for other potential issues, they are very far-fetched and synthetic.
>
> Well, I don't think so. :)
>
You can fix that :)
>
>
> _______________________________________________
> Devel mailing list
> Devel@lists.altlinux.org
> https://lists.altlinux.org/mailman/listinfo/devel
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [devel] [PATCH for apt 2/2 v2] Fix pointer arithmetics
2019-12-09 6:54 ` Aleksei Nikiforov
@ 2019-12-12 19:20 ` Andrey Savchenko
2019-12-13 7:58 ` Aleksei Nikiforov
0 siblings, 1 reply; 34+ messages in thread
From: Andrey Savchenko @ 2019-12-12 19:20 UTC (permalink / raw)
To: ALT Linux Team development discussions
[-- Attachment #1: Type: text/plain, Size: 18080 bytes --]
On Mon, 9 Dec 2019 09:54:27 +0300 Aleksei Nikiforov wrote:
> 07.12.2019 17:52, Andrey Savchenko пишет:
> > Hi all!
> >
> > On Fri, 6 Dec 2019 18:36:55 +0300 Aleksei Nikiforov wrote:
> >> This change should fix pointer arithmetic issues for e2k.
> >
> > This commit message is misleading. Pointer arithmetic is broken on
> > all architectures, but by pure chance in happens to work correctly
> > when gcc is used. On e2k the lcc compiler is used, so problem
> > manifests itself. But this doesn't change the fact that pointer
> > arithmetic was flawed on all architectures.
> >
>
> Your first sentence contradicts later ones.
No.
> You just repeated my sentence about issue manifesting on e2k,
No.
> but you did it more verbosely.
Yes.
> Yes, there is a flaw for all architectures, but it manifests only for
> e2k in practice, thus change fixes it for e2k. So, what's misleading there?
When a commit message is of concern it does matter what is written,
and what was intended but kept behind the scenes is of no
importance.
When one writes "issue A is fixed for B" this implies that A is
B-specific issue, which is false in the discussed case.
> >> ---
> >> apt/apt-pkg/Makefile.am | 1 +
> >> apt/apt-pkg/cacheiterators.h | 26 ++++++++++++++------------
> >> apt/apt-pkg/contrib/mmap.cc | 8 +++++---
> >> apt/apt-pkg/pkgcachegen.cc | 29 +++++++++++++++--------------
> >> apt/apt-pkg/pkgcachegen.h | 6 +++---
> >> apt/apt-pkg/rebase_pointer.h | 16 ++++++++++++++++
> >> apt/apt-pkg/rpm/rpmlistparser.cc | 5 +++--
> >> apt/apt-pkg/rpm/rpmpackagedata.cc | 2 +-
> >> 8 files changed, 58 insertions(+), 35 deletions(-)
> >> create mode 100644 apt/apt-pkg/rebase_pointer.h
> >>
> >> diff --git a/apt/apt-pkg/Makefile.am b/apt/apt-pkg/Makefile.am
> >> index 4c0d234..d038d01 100644
> >> --- a/apt/apt-pkg/Makefile.am
> >> +++ b/apt/apt-pkg/Makefile.am
> >> @@ -94,6 +94,7 @@ libapt_pkg_la_SOURCES = \
> >> pkgsystem.h \
> >> policy.cc \
> >> policy.h \
> >> + rebase_pointer.h \
> >> repository.cc \
> >> repository.h \
> >> scopeexit.h \
> >> diff --git a/apt/apt-pkg/cacheiterators.h b/apt/apt-pkg/cacheiterators.h
> >> index a4bf670..118838e 100644
> >> --- a/apt/apt-pkg/cacheiterators.h
> >> +++ b/apt/apt-pkg/cacheiterators.h
> >> @@ -34,6 +34,8 @@
> >> #pragma interface "apt-pkg/cacheiterators.h"
> >> #endif
> >>
> >> +#include <apt-pkg/rebase_pointer.h>
> >> +
> >> #include <set>
> >>
> >> // Package Iterator
> >> @@ -85,11 +87,11 @@ class pkgCache::PkgIterator
> >> inline unsigned long long Index() const {return Pkg - Owner->PkgP;};
> >> OkState State() const;
> >>
> >> - void ReMap(void const * const oldMap, void const * const newMap)
> >> + void ReMap(void *oldMap, void *newMap)
> >> {
> >> if (Owner == 0 || Pkg == 0)
> >> return;
> >> - Pkg += static_cast<Package const *>(newMap) - static_cast<Package const *>(oldMap);
> >> + Pkg = RebasePointer(Pkg, oldMap, newMap);
> >> }
> >>
> >> // Constructors
> >> @@ -147,11 +149,11 @@ class pkgCache::VerIterator
> >> bool Automatic() const;
> >> VerFileIterator NewestFile() const;
> >>
> >> - void ReMap(void const * const oldMap, void const * const newMap)
> >> + void ReMap(void *oldMap, void *newMap)
> >> {
> >> if (Owner == 0 || Ver == 0)
> >> return;
> >> - Ver += static_cast<Version const *>(newMap) - static_cast<Version const *>(oldMap);
> >> + Ver = RebasePointer(Ver, oldMap, newMap);
> >> }
> >>
> >> inline VerIterator() : Ver(0), Owner(0) {};
> >> @@ -220,11 +222,11 @@ class pkgCache::DepIterator
> >> inline const char *CompType() {return Owner->CompType(Dep->CompareOp);};
> >> inline const char *DepType() {return Owner->DepType(Dep->Type);};
> >>
> >> - void ReMap(void const * const oldMap, void const * const newMap)
> >> + void ReMap(void *oldMap, void *newMap)
> >> {
> >> if (Owner == 0 || Dep == 0)
> >> return;
> >> - Dep += static_cast<Dependency const *>(newMap) - static_cast<Dependency const *>(oldMap);
> >> + Dep = RebasePointer(Dep, oldMap, newMap);
> >> }
> >>
> >> inline DepIterator(pkgCache &Owner,Dependency *Trg,Version * = 0) :
> >> @@ -279,11 +281,11 @@ class pkgCache::PrvIterator
> >> inline PkgIterator OwnerPkg() const {return PkgIterator(*Owner,Owner->PkgP + Owner->VerP[Prv->Version].ParentPkg);};
> >> inline unsigned long long Index() const {return Prv - Owner->ProvideP;};
> >>
> >> - void ReMap(void const * const oldMap, void const * const newMap)
> >> + void ReMap(void *oldMap, void *newMap)
> >> {
> >> if (Owner == 0 || Prv == 0)
> >> return;
> >> - Prv += static_cast<Provides const *>(newMap) - static_cast<Provides const *>(oldMap);
> >> + Prv = RebasePointer(Prv, oldMap, newMap);
> >> }
> >>
> >> inline PrvIterator() : Prv(0), Type(PrvVer), Owner(0) {};
> >> @@ -342,11 +344,11 @@ class pkgCache::PkgFileIterator
> >> bool IsOk();
> >> string RelStr();
> >>
> >> - void ReMap(void const * const oldMap, void const * const newMap)
> >> + void ReMap(void *oldMap, void *newMap)
> >> {
> >> if (Owner == 0 || File == 0)
> >> return;
> >> - File += static_cast<PackageFile const *>(newMap) - static_cast<PackageFile const *>(oldMap);
> >> + File = RebasePointer(File, oldMap, newMap);
> >> }
> >>
> >> // Constructors
> >> @@ -383,11 +385,11 @@ class pkgCache::VerFileIterator
> >> inline PkgFileIterator File() const {return PkgFileIterator(*Owner,FileP->File + Owner->PkgFileP);};
> >> inline unsigned long long Index() const {return FileP - Owner->VerFileP;};
> >>
> >> - void ReMap(void const * const oldMap, void const * const newMap)
> >> + void ReMap(void *oldMap, void *newMap)
> >> {
> >> if (Owner == 0 || FileP == 0)
> >> return;
> >> - FileP += static_cast<VerFile const *>(newMap) - static_cast<VerFile const *>(oldMap);
> >> + FileP = RebasePointer(FileP, oldMap, newMap);
> >> }
> >>
> >> inline VerFileIterator() : Owner(0), FileP(0) {};
> >> diff --git a/apt/apt-pkg/contrib/mmap.cc b/apt/apt-pkg/contrib/mmap.cc
> >> index cf01be9..35f1a25 100644
> >> --- a/apt/apt-pkg/contrib/mmap.cc
> >> +++ b/apt/apt-pkg/contrib/mmap.cc
> >> @@ -30,6 +30,7 @@
> >> #include <apt-pkg/configuration.h>
> >> #include <apt-pkg/mmap.h>
> >> #include <apt-pkg/error.h>
> >> +#include <apt-pkg/rebase_pointer.h>
> >>
> >> #include <apti18n.h>
> >>
> >> @@ -301,7 +302,7 @@ std::experimental::optional<map_ptrloc> DynamicMMap::Allocate(unsigned long Item
> >> Pool* oldPools = Pools;
> >> auto idxResult = RawAllocate(I->Count*ItemSize,ItemSize);
> >> if (Pools != oldPools)
> >> - I += Pools - oldPools;
> >> + I = RebasePointer(I, oldPools, Pools);
> >>
> >> // Does the allocation failed ?
> >> if (!idxResult)
> >> @@ -371,7 +372,7 @@ bool DynamicMMap::Grow(unsigned long long size)
> >> Fd->Write(&C,sizeof(C));
> >> }
> >>
> >> - unsigned long const poolOffset = Pools - ((Pool*) Base);
> >> + void *old_base = Base;
> >>
> >> if (Fd != 0)
> >> {
> >> @@ -408,7 +409,8 @@ bool DynamicMMap::Grow(unsigned long long size)
> >> memset((char*)Base + WorkSpace, 0, newSize - WorkSpace);
> >> }
> >>
> >> - Pools = (Pool*) Base + poolOffset;
> >> + if (Base != old_base)
> >> + Pools = RebasePointer(Pools, old_base, Base);
> >> WorkSpace = newSize;
> >>
> >> return true;
> >> diff --git a/apt/apt-pkg/pkgcachegen.cc b/apt/apt-pkg/pkgcachegen.cc
> >> index 654c81c..37364e9 100644
> >> --- a/apt/apt-pkg/pkgcachegen.cc
> >> +++ b/apt/apt-pkg/pkgcachegen.cc
> >> @@ -26,6 +26,7 @@
> >> #include <apt-pkg/strutl.h>
> >> #include <apt-pkg/sptr.h>
> >> #include <apt-pkg/pkgsystem.h>
> >> +#include <apt-pkg/rebase_pointer.h>
> >>
> >> #include <apti18n.h>
> >>
> >> @@ -109,18 +110,18 @@ pkgCacheGenerator::~pkgCacheGenerator()
> >> Map.Sync(0,sizeof(pkgCache::Header));
> >> }
> >> /*}}}*/
> >> -void pkgCacheGenerator::ReMap(void const * const oldMap, void const * const newMap)
> >> +void pkgCacheGenerator::ReMap(void *oldMap, void *newMap)
> >> {
> >> if (oldMap == newMap)
> >> return;
> >>
> >> Cache.ReMap(false);
> >>
> >> - CurrentFile += (pkgCache::PackageFile*) newMap - (pkgCache::PackageFile*) oldMap;
> >> + CurrentFile = RebasePointer(CurrentFile, oldMap, newMap);
> >>
> >> for (size_t i = 0; i < _count(UniqHash); ++i)
> >> if (UniqHash[i] != 0)
> >> - UniqHash[i] += (pkgCache::StringItem*) newMap - (pkgCache::StringItem*) oldMap;
> >> + UniqHash[i] = RebasePointer(UniqHash[i], oldMap, newMap);
> >>
> >> for (auto i = Dynamic<pkgCache::PkgIterator>::toReMap.begin();
> >> i != Dynamic<pkgCache::PkgIterator>::toReMap.end(); ++i)
> >> @@ -147,7 +148,7 @@ void pkgCacheGenerator::ReMap(void const * const oldMap, void const * const newM
> >> // CacheGenerator::WriteStringInMap /*{{{*/
> >> std::experimental::optional<map_ptrloc> pkgCacheGenerator::WriteStringInMap(const char *String,
> >> unsigned long Len) {
> >> - void const * const oldMap = Map.Data();
> >> + void *oldMap = Map.Data();
> >> const auto index = Map.WriteString(String, Len);
> >> if (index)
> >> ReMap(oldMap, Map.Data());
> >> @@ -156,7 +157,7 @@ std::experimental::optional<map_ptrloc> pkgCacheGenerator::WriteStringInMap(cons
> >> /*}}}*/
> >> // CacheGenerator::WriteStringInMap /*{{{*/
> >> std::experimental::optional<map_ptrloc> pkgCacheGenerator::WriteStringInMap(const char *String) {
> >> - void const * const oldMap = Map.Data();
> >> + void *oldMap = Map.Data();
> >> const auto index = Map.WriteString(String);
> >> if (index)
> >> ReMap(oldMap, Map.Data());
> >> @@ -164,7 +165,7 @@ std::experimental::optional<map_ptrloc> pkgCacheGenerator::WriteStringInMap(cons
> >> }
> >> /*}}}*/
> >> std::experimental::optional<map_ptrloc> pkgCacheGenerator::AllocateInMap(unsigned long size) {/*{{{*/
> >> - void const * const oldMap = Map.Data();
> >> + void *oldMap = Map.Data();
> >> const auto index = Map.Allocate(size);
> >> if (index)
> >> ReMap(oldMap, Map.Data());
> >> @@ -229,7 +230,7 @@ bool pkgCacheGenerator::MergeList(ListParser &List,
> >> pkgCache::VerIterator Ver = Pkg.VersionList();
> >> Dynamic<pkgCache::VerIterator> DynVer(Ver);
> >> map_ptrloc *Last = &Pkg->VersionList;
> >> - const void * oldMap = Map.Data();
> >> + void *oldMap = Map.Data();
> >> int Res = 1;
> >> for (; Ver.end() == false; Last = &Ver->NextVer, Ver++)
> >> {
> >> @@ -271,7 +272,7 @@ bool pkgCacheGenerator::MergeList(ListParser &List,
> >>
> >> if (oldMap != Map.Data())
> >> {
> >> - Last += (map_ptrloc*) Map.Data() - (map_ptrloc*) oldMap;
> >> + Last = RebasePointer(Last, oldMap, Map.Data());
> >> oldMap = Map.Data();
> >> }
> >>
> >> @@ -297,7 +298,7 @@ bool pkgCacheGenerator::MergeList(ListParser &List,
> >> PackageName.c_str(), 1);
> >>
> >> if (oldMap != Map.Data())
> >> - Last += (map_ptrloc*) Map.Data() - (map_ptrloc*) oldMap;
> >> + Last = RebasePointer(Last, oldMap, Map.Data());
> >> *Last = *verindex;
> >>
> >> Ver->ParentPkg = Pkg.Index();
> >> @@ -553,7 +554,7 @@ bool pkgCacheGenerator::ListParser::NewDepends(pkgCache::VerIterator &Ver,
> >> unsigned int Op,
> >> unsigned int Type)
> >> {
> >> - void const * const oldMap = Owner->Map.Data();
> >> + void *oldMap = Owner->Map.Data();
> >> pkgCache &Cache = Owner->Cache;
> >>
> >> // Get a structure
> >> @@ -605,7 +606,7 @@ bool pkgCacheGenerator::ListParser::NewDepends(pkgCache::VerIterator &Ver,
> >> OldDepLast = &D->NextDepends;
> >> OldDepVer = Ver;
> >> } else if (oldMap != Owner->Map.Data())
> >> - OldDepLast += (map_ptrloc*) Owner->Map.Data() - (map_ptrloc*) oldMap;
> >> + OldDepLast = RebasePointer(OldDepLast, oldMap, Owner->Map.Data());
> >>
> >> // Is it a file dependency?
> >> if (PackageName[0] == '/')
> >> @@ -739,7 +740,7 @@ std::experimental::optional<map_ptrloc> pkgCacheGenerator::WriteUniqString(const
> >> }
> >>
> >> // Get a structure
> >> - void const * const oldMap = Map.Data();
> >> + void *oldMap = Map.Data();
> >> const auto Item = AllocateInMap(sizeof(pkgCache::StringItem));
> >> const auto idxString = WriteStringInMap(S, Size);
> >> if ((!Item) || (!idxString))
> >> @@ -747,8 +748,8 @@ std::experimental::optional<map_ptrloc> pkgCacheGenerator::WriteUniqString(const
> >>
> >> if (oldMap != Map.Data())
> >> {
> >> - Last += (map_ptrloc*) Map.Data() - (map_ptrloc*) oldMap;
> >> - I += (pkgCache::StringItem*) Map.Data() - (pkgCache::StringItem*) oldMap;
> >> + Last = RebasePointer(Last, oldMap, Map.Data());
> >> + I = RebasePointer(I, oldMap, Map.Data());
> >> }
> >>
> >> *Last = *Item;
> >> diff --git a/apt/apt-pkg/pkgcachegen.h b/apt/apt-pkg/pkgcachegen.h
> >> index 77075a6..0faf6e1 100644
> >> --- a/apt/apt-pkg/pkgcachegen.h
> >> +++ b/apt/apt-pkg/pkgcachegen.h
> >> @@ -72,7 +72,7 @@ class pkgCacheGenerator
> >> class DynamicFunction
> >> {
> >> public:
> >> - typedef std::function<void(const void *, const void *)> function;
> >> + typedef std::function<void(void *, void *)> function;
> >>
> >> static std::set<DynamicFunction*> toReMap;
> >>
> >> @@ -93,7 +93,7 @@ class pkgCacheGenerator
> >> toReMap.erase(this);
> >> }
> >>
> >> - void call(const void *oldMap, const void *newMap)
> >> + void call(void *oldMap, void *newMap)
> >> {
> >> if (m_function)
> >> m_function(oldMap, newMap);
> >> @@ -140,7 +140,7 @@ class pkgCacheGenerator
> >> // CNC:2003-03-18
> >> inline void ResetFileDeps() {FoundFileDeps = false;};
> >>
> >> - void ReMap(void const * const oldMap, void const * const newMap);
> >> + void ReMap(void *oldMap, void *newMap);
> >>
> >> pkgCacheGenerator(DynamicMMap *Map,OpProgress *Progress);
> >> ~pkgCacheGenerator();
> >> diff --git a/apt/apt-pkg/rebase_pointer.h b/apt/apt-pkg/rebase_pointer.h
> >> new file mode 100644
> >> index 0000000..f6b3c15
> >> --- /dev/null
> >> +++ b/apt/apt-pkg/rebase_pointer.h
> >> @@ -0,0 +1,16 @@
> >> +#ifndef PKGLIB_REBASE_POINTER_H
> >> +#define PKGLIB_REBASE_POINTER_H
> >> +
> >> +template <typename T>
> >> +static inline T* RebasePointer(T *ptr, void *old_base, void *new_base)
> >> +{
> >> + return reinterpret_cast<T*>(reinterpret_cast<char*>(new_base) + (reinterpret_cast<char*>(ptr) - reinterpret_cast<char*>(old_base)));
> >> +}
> >> +
> >> +template <typename T>
> >> +static inline const T* RebasePointer(const T *ptr, void *old_base, void *new_base)
> >> +{
> >> + return reinterpret_cast<const T*>(reinterpret_cast<char*>(new_base) + (reinterpret_cast<const char*>(ptr) - reinterpret_cast<char*>(old_base)));
> >> +}
> >> +
> >> +#endif
> >> diff --git a/apt/apt-pkg/rpm/rpmlistparser.cc b/apt/apt-pkg/rpm/rpmlistparser.cc
> >> index 9b2e9ad..bd13b99 100644
> >> --- a/apt/apt-pkg/rpm/rpmlistparser.cc
> >> +++ b/apt/apt-pkg/rpm/rpmlistparser.cc
> >> @@ -25,6 +25,7 @@
> >> #include <apt-pkg/strutl.h>
> >> #include <apt-pkg/crc-16.h>
> >> #include <apt-pkg/tagfile.h>
> >> +#include <apt-pkg/rebase_pointer.h>
> >>
> >> #include <apti18n.h>
> >>
> >> @@ -50,13 +51,13 @@ rpmListParser::rpmListParser(RPMHandler *Handler)
> >> {
> >> SeenPackages.reset(new SeenPackagesType);
> >> m_SeenPackagesRealloc.reset(new pkgCacheGenerator::DynamicFunction(
> >> - [this] (void const *oldMap, void const *newMap)
> >> + [this] (void *oldMap, void *newMap)
> >> {
> >> SeenPackagesType tmp;
> >>
> >> for (auto iter: *SeenPackages)
> >> {
> >> - tmp.insert(iter + (static_cast<const char *>(newMap) - static_cast<const char *>(oldMap)));
> >> + tmp.insert(RebasePointer(iter, oldMap, newMap));
> >> }
> >>
> >> SeenPackages->swap(tmp);
> >> diff --git a/apt/apt-pkg/rpm/rpmpackagedata.cc b/apt/apt-pkg/rpm/rpmpackagedata.cc
> >> index 61c15d5..d6c677b 100644
> >> --- a/apt/apt-pkg/rpm/rpmpackagedata.cc
> >> +++ b/apt/apt-pkg/rpm/rpmpackagedata.cc
> >> @@ -23,7 +23,7 @@ RPMPackageData::RPMPackageData()
> >> #endif
> >> {
> >> m_VerMapRealloc.reset(new pkgCacheGenerator::DynamicFunction(
> >> - [this] (void const *oldMap, void const *newMap)
> >> + [this] (void *oldMap, void *newMap)
> >> {
> >> for (auto iter1 = VerMap.begin(); iter1 != VerMap.end(); ++iter1)
> >> {
> >
> >
> > Best regards,
> > Andrew Savchenko
> >
> >
> > _______________________________________________
> > Devel mailing list
> > Devel@lists.altlinux.org
> > https://lists.altlinux.org/mailman/listinfo/devel
> >
> _______________________________________________
> Devel mailing list
> Devel@lists.altlinux.org
> https://lists.altlinux.org/mailman/listinfo/devel
Best regards,
Andrew Savchenko
[-- Attachment #2: Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [devel] [PATCH for apt 2/2 v2] Fix pointer arithmetics
2019-12-11 7:50 ` Aleksei Nikiforov
@ 2019-12-12 19:43 ` Andrey Savchenko
2019-12-13 8:01 ` Aleksei Nikiforov
0 siblings, 1 reply; 34+ messages in thread
From: Andrey Savchenko @ 2019-12-12 19:43 UTC (permalink / raw)
To: ALT Linux Team development discussions
[-- Attachment #1: Type: text/plain, Size: 3256 bytes --]
On Wed, 11 Dec 2019 10:50:22 +0300 Aleksei Nikiforov wrote:
> 11.12.2019 1:20, Dmitry V. Levin пишет:
> > On Tue, Dec 10, 2019 at 01:58:17PM +0300, Aleksei Nikiforov wrote:
> >> 10.12.2019 13:20, Dmitry V. Levin пишет:
> >>> On Tue, Dec 10, 2019 at 11:18:06AM +0300, Aleksei Nikiforov wrote:
> >>>> 10.12.2019 3:07, Dmitry V. Levin пишет:
> >>>>> On Mon, Dec 09, 2019 at 10:08:42AM +0300, Aleksei Nikiforov wrote:
> >>>>>> 09.12.2019 2:21, Dmitry V. Levin пишет:
> >>>>>>> On Fri, Dec 06, 2019 at 06:36:55PM +0300, Aleksei Nikiforov wrote:
> >>>>>>> [...]
> >>>>>>>> @@ -85,11 +87,11 @@ class pkgCache::PkgIterator
> >>>>>>>> inline unsigned long long Index() const {return Pkg - Owner->PkgP;};
> >>>>>>>> OkState State() const;
> >>>>>>>>
> >>>>>>>> - void ReMap(void const * const oldMap, void const * const newMap)
> >>>>>>>> + void ReMap(void *oldMap, void *newMap)
> >>>>>>>
> >>>>>>> Is there any particular reason for stripping const here and in other
> >>>>>>> similar places?
> >>>>>>
> >>>>>> Yes, it's needed due to issues emerging from mixing const and non-const
> >>>>>> pointers with new and allegedly more proper way of calculating rebased
> >>>>>> pointers.
> >>>>>
> >>>>> Sorry, I don't find this argument convincing.
> >>>>> I have experienced no const issues in my version of this fix.
> >>>>
> >>>> Your version is using C-style casts in C++ code. Of course, I could use
> >>>> C-style casts or const_cast-s too to work around const correctness
> >>>> issues (i.e. to just hide these issues), and it'd work like your
> >>>> version. But I'd like to remind you that APT is C++ project, not a C
> >>>> project. What might be ok for C is sometimes a dirty ugly hack in modern
> >>>> C++.
> >>>
> >>> Sorry, I don't share you point of view on this matter.
> >>> Being a C++ project allows you to use C++ constructs, that's true,
> >>> but why do you think it prevents you from using C constructs when
> >>> appropriate?
> >>
> >> I didn't say that something prevents from using C constructs when
> >> appropriate. I'm saying that these constructs are not appropriate here.
> >
> > Why do you think they are not appropriate here?
> >
>
> In good C++ code there is no place for const_cast.
This statement is ungrounded.
> Maybe there are some
> exceptions to it, but they have to be justified. It doesn't matter that
> you are hiding it behind C-style cast.
Please read some good book on C++ like [1] or at least the official
reference manual [2].
Aside from C-style cast C++ supports four casts (in their safety
order, the safest first):
const_cast
static_cast
dynamic_cast
reinterpret_cast
One can see their preference order base on how explicit C-style
cast is being intrepreted by the C++ compiler [3].
So actually the reinterpret_cast should be avoided when it is
possible to use more strict casts, because reinterpret_cast
disables all safety checks aside from constness and volatileness
one.
[1] Stanley B. Lippman, Josee Lajoie, C++ Primer. Chapter 4.14
Types Conversion.
[2] https://en.cppreference.com
[3] https://en.cppreference.com/w/cpp/language/explicit_cast
Best regards,
Andrew Savchenko
[-- Attachment #2: Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [devel] [PATCH for apt 2/2 v2] Fix pointer arithmetics
2019-12-12 19:20 ` Andrey Savchenko
@ 2019-12-13 7:58 ` Aleksei Nikiforov
0 siblings, 0 replies; 34+ messages in thread
From: Aleksei Nikiforov @ 2019-12-13 7:58 UTC (permalink / raw)
To: devel
12.12.2019 22:20, Andrey Savchenko пишет:
> On Mon, 9 Dec 2019 09:54:27 +0300 Aleksei Nikiforov wrote:
>> 07.12.2019 17:52, Andrey Savchenko пишет:
>>> Hi all!
>>>
>>> On Fri, 6 Dec 2019 18:36:55 +0300 Aleksei Nikiforov wrote:
>>>> This change should fix pointer arithmetic issues for e2k.
>>>
>>> This commit message is misleading. Pointer arithmetic is broken on
>>> all architectures, but by pure chance in happens to work correctly
>>> when gcc is used. On e2k the lcc compiler is used, so problem
>>> manifests itself. But this doesn't change the fact that pointer
>>> arithmetic was flawed on all architectures.
>>>
>>
>> Your first sentence contradicts later ones.
>
> No.
>
>> You just repeated my sentence about issue manifesting on e2k,
>
> No.
>
>> but you did it more verbosely.
>
> Yes.
>
>> Yes, there is a flaw for all architectures, but it manifests only for
>> e2k in practice, thus change fixes it for e2k. So, what's misleading there?
>
> When a commit message is of concern it does matter what is written,
> and what was intended but kept behind the scenes is of no
> importance.
>
> When one writes "issue A is fixed for B" this implies that A is
> B-specific issue, which is false in the discussed case.
>
Is it a complain to latest patch version or old one? If it's to old one,
I'll just skip it.
>>>> ---
>>>> apt/apt-pkg/Makefile.am | 1 +
>>>> apt/apt-pkg/cacheiterators.h | 26 ++++++++++++++------------
>>>> apt/apt-pkg/contrib/mmap.cc | 8 +++++---
>>>> apt/apt-pkg/pkgcachegen.cc | 29 +++++++++++++++--------------
>>>> apt/apt-pkg/pkgcachegen.h | 6 +++---
>>>> apt/apt-pkg/rebase_pointer.h | 16 ++++++++++++++++
>>>> apt/apt-pkg/rpm/rpmlistparser.cc | 5 +++--
>>>> apt/apt-pkg/rpm/rpmpackagedata.cc | 2 +-
>>>> 8 files changed, 58 insertions(+), 35 deletions(-)
>>>> create mode 100644 apt/apt-pkg/rebase_pointer.h
>>>>
>>>> diff --git a/apt/apt-pkg/Makefile.am b/apt/apt-pkg/Makefile.am
>>>> index 4c0d234..d038d01 100644
>>>> --- a/apt/apt-pkg/Makefile.am
>>>> +++ b/apt/apt-pkg/Makefile.am
>>>> @@ -94,6 +94,7 @@ libapt_pkg_la_SOURCES = \
>>>> pkgsystem.h \
>>>> policy.cc \
>>>> policy.h \
>>>> + rebase_pointer.h \
>>>> repository.cc \
>>>> repository.h \
>>>> scopeexit.h \
>>>> diff --git a/apt/apt-pkg/cacheiterators.h b/apt/apt-pkg/cacheiterators.h
>>>> index a4bf670..118838e 100644
>>>> --- a/apt/apt-pkg/cacheiterators.h
>>>> +++ b/apt/apt-pkg/cacheiterators.h
>>>> @@ -34,6 +34,8 @@
>>>> #pragma interface "apt-pkg/cacheiterators.h"
>>>> #endif
>>>>
>>>> +#include <apt-pkg/rebase_pointer.h>
>>>> +
>>>> #include <set>
>>>>
>>>> // Package Iterator
>>>> @@ -85,11 +87,11 @@ class pkgCache::PkgIterator
>>>> inline unsigned long long Index() const {return Pkg - Owner->PkgP;};
>>>> OkState State() const;
>>>>
>>>> - void ReMap(void const * const oldMap, void const * const newMap)
>>>> + void ReMap(void *oldMap, void *newMap)
>>>> {
>>>> if (Owner == 0 || Pkg == 0)
>>>> return;
>>>> - Pkg += static_cast<Package const *>(newMap) - static_cast<Package const *>(oldMap);
>>>> + Pkg = RebasePointer(Pkg, oldMap, newMap);
>>>> }
>>>>
>>>> // Constructors
>>>> @@ -147,11 +149,11 @@ class pkgCache::VerIterator
>>>> bool Automatic() const;
>>>> VerFileIterator NewestFile() const;
>>>>
>>>> - void ReMap(void const * const oldMap, void const * const newMap)
>>>> + void ReMap(void *oldMap, void *newMap)
>>>> {
>>>> if (Owner == 0 || Ver == 0)
>>>> return;
>>>> - Ver += static_cast<Version const *>(newMap) - static_cast<Version const *>(oldMap);
>>>> + Ver = RebasePointer(Ver, oldMap, newMap);
>>>> }
>>>>
>>>> inline VerIterator() : Ver(0), Owner(0) {};
>>>> @@ -220,11 +222,11 @@ class pkgCache::DepIterator
>>>> inline const char *CompType() {return Owner->CompType(Dep->CompareOp);};
>>>> inline const char *DepType() {return Owner->DepType(Dep->Type);};
>>>>
>>>> - void ReMap(void const * const oldMap, void const * const newMap)
>>>> + void ReMap(void *oldMap, void *newMap)
>>>> {
>>>> if (Owner == 0 || Dep == 0)
>>>> return;
>>>> - Dep += static_cast<Dependency const *>(newMap) - static_cast<Dependency const *>(oldMap);
>>>> + Dep = RebasePointer(Dep, oldMap, newMap);
>>>> }
>>>>
>>>> inline DepIterator(pkgCache &Owner,Dependency *Trg,Version * = 0) :
>>>> @@ -279,11 +281,11 @@ class pkgCache::PrvIterator
>>>> inline PkgIterator OwnerPkg() const {return PkgIterator(*Owner,Owner->PkgP + Owner->VerP[Prv->Version].ParentPkg);};
>>>> inline unsigned long long Index() const {return Prv - Owner->ProvideP;};
>>>>
>>>> - void ReMap(void const * const oldMap, void const * const newMap)
>>>> + void ReMap(void *oldMap, void *newMap)
>>>> {
>>>> if (Owner == 0 || Prv == 0)
>>>> return;
>>>> - Prv += static_cast<Provides const *>(newMap) - static_cast<Provides const *>(oldMap);
>>>> + Prv = RebasePointer(Prv, oldMap, newMap);
>>>> }
>>>>
>>>> inline PrvIterator() : Prv(0), Type(PrvVer), Owner(0) {};
>>>> @@ -342,11 +344,11 @@ class pkgCache::PkgFileIterator
>>>> bool IsOk();
>>>> string RelStr();
>>>>
>>>> - void ReMap(void const * const oldMap, void const * const newMap)
>>>> + void ReMap(void *oldMap, void *newMap)
>>>> {
>>>> if (Owner == 0 || File == 0)
>>>> return;
>>>> - File += static_cast<PackageFile const *>(newMap) - static_cast<PackageFile const *>(oldMap);
>>>> + File = RebasePointer(File, oldMap, newMap);
>>>> }
>>>>
>>>> // Constructors
>>>> @@ -383,11 +385,11 @@ class pkgCache::VerFileIterator
>>>> inline PkgFileIterator File() const {return PkgFileIterator(*Owner,FileP->File + Owner->PkgFileP);};
>>>> inline unsigned long long Index() const {return FileP - Owner->VerFileP;};
>>>>
>>>> - void ReMap(void const * const oldMap, void const * const newMap)
>>>> + void ReMap(void *oldMap, void *newMap)
>>>> {
>>>> if (Owner == 0 || FileP == 0)
>>>> return;
>>>> - FileP += static_cast<VerFile const *>(newMap) - static_cast<VerFile const *>(oldMap);
>>>> + FileP = RebasePointer(FileP, oldMap, newMap);
>>>> }
>>>>
>>>> inline VerFileIterator() : Owner(0), FileP(0) {};
>>>> diff --git a/apt/apt-pkg/contrib/mmap.cc b/apt/apt-pkg/contrib/mmap.cc
>>>> index cf01be9..35f1a25 100644
>>>> --- a/apt/apt-pkg/contrib/mmap.cc
>>>> +++ b/apt/apt-pkg/contrib/mmap.cc
>>>> @@ -30,6 +30,7 @@
>>>> #include <apt-pkg/configuration.h>
>>>> #include <apt-pkg/mmap.h>
>>>> #include <apt-pkg/error.h>
>>>> +#include <apt-pkg/rebase_pointer.h>
>>>>
>>>> #include <apti18n.h>
>>>>
>>>> @@ -301,7 +302,7 @@ std::experimental::optional<map_ptrloc> DynamicMMap::Allocate(unsigned long Item
>>>> Pool* oldPools = Pools;
>>>> auto idxResult = RawAllocate(I->Count*ItemSize,ItemSize);
>>>> if (Pools != oldPools)
>>>> - I += Pools - oldPools;
>>>> + I = RebasePointer(I, oldPools, Pools);
>>>>
>>>> // Does the allocation failed ?
>>>> if (!idxResult)
>>>> @@ -371,7 +372,7 @@ bool DynamicMMap::Grow(unsigned long long size)
>>>> Fd->Write(&C,sizeof(C));
>>>> }
>>>>
>>>> - unsigned long const poolOffset = Pools - ((Pool*) Base);
>>>> + void *old_base = Base;
>>>>
>>>> if (Fd != 0)
>>>> {
>>>> @@ -408,7 +409,8 @@ bool DynamicMMap::Grow(unsigned long long size)
>>>> memset((char*)Base + WorkSpace, 0, newSize - WorkSpace);
>>>> }
>>>>
>>>> - Pools = (Pool*) Base + poolOffset;
>>>> + if (Base != old_base)
>>>> + Pools = RebasePointer(Pools, old_base, Base);
>>>> WorkSpace = newSize;
>>>>
>>>> return true;
>>>> diff --git a/apt/apt-pkg/pkgcachegen.cc b/apt/apt-pkg/pkgcachegen.cc
>>>> index 654c81c..37364e9 100644
>>>> --- a/apt/apt-pkg/pkgcachegen.cc
>>>> +++ b/apt/apt-pkg/pkgcachegen.cc
>>>> @@ -26,6 +26,7 @@
>>>> #include <apt-pkg/strutl.h>
>>>> #include <apt-pkg/sptr.h>
>>>> #include <apt-pkg/pkgsystem.h>
>>>> +#include <apt-pkg/rebase_pointer.h>
>>>>
>>>> #include <apti18n.h>
>>>>
>>>> @@ -109,18 +110,18 @@ pkgCacheGenerator::~pkgCacheGenerator()
>>>> Map.Sync(0,sizeof(pkgCache::Header));
>>>> }
>>>> /*}}}*/
>>>> -void pkgCacheGenerator::ReMap(void const * const oldMap, void const * const newMap)
>>>> +void pkgCacheGenerator::ReMap(void *oldMap, void *newMap)
>>>> {
>>>> if (oldMap == newMap)
>>>> return;
>>>>
>>>> Cache.ReMap(false);
>>>>
>>>> - CurrentFile += (pkgCache::PackageFile*) newMap - (pkgCache::PackageFile*) oldMap;
>>>> + CurrentFile = RebasePointer(CurrentFile, oldMap, newMap);
>>>>
>>>> for (size_t i = 0; i < _count(UniqHash); ++i)
>>>> if (UniqHash[i] != 0)
>>>> - UniqHash[i] += (pkgCache::StringItem*) newMap - (pkgCache::StringItem*) oldMap;
>>>> + UniqHash[i] = RebasePointer(UniqHash[i], oldMap, newMap);
>>>>
>>>> for (auto i = Dynamic<pkgCache::PkgIterator>::toReMap.begin();
>>>> i != Dynamic<pkgCache::PkgIterator>::toReMap.end(); ++i)
>>>> @@ -147,7 +148,7 @@ void pkgCacheGenerator::ReMap(void const * const oldMap, void const * const newM
>>>> // CacheGenerator::WriteStringInMap /*{{{*/
>>>> std::experimental::optional<map_ptrloc> pkgCacheGenerator::WriteStringInMap(const char *String,
>>>> unsigned long Len) {
>>>> - void const * const oldMap = Map.Data();
>>>> + void *oldMap = Map.Data();
>>>> const auto index = Map.WriteString(String, Len);
>>>> if (index)
>>>> ReMap(oldMap, Map.Data());
>>>> @@ -156,7 +157,7 @@ std::experimental::optional<map_ptrloc> pkgCacheGenerator::WriteStringInMap(cons
>>>> /*}}}*/
>>>> // CacheGenerator::WriteStringInMap /*{{{*/
>>>> std::experimental::optional<map_ptrloc> pkgCacheGenerator::WriteStringInMap(const char *String) {
>>>> - void const * const oldMap = Map.Data();
>>>> + void *oldMap = Map.Data();
>>>> const auto index = Map.WriteString(String);
>>>> if (index)
>>>> ReMap(oldMap, Map.Data());
>>>> @@ -164,7 +165,7 @@ std::experimental::optional<map_ptrloc> pkgCacheGenerator::WriteStringInMap(cons
>>>> }
>>>> /*}}}*/
>>>> std::experimental::optional<map_ptrloc> pkgCacheGenerator::AllocateInMap(unsigned long size) {/*{{{*/
>>>> - void const * const oldMap = Map.Data();
>>>> + void *oldMap = Map.Data();
>>>> const auto index = Map.Allocate(size);
>>>> if (index)
>>>> ReMap(oldMap, Map.Data());
>>>> @@ -229,7 +230,7 @@ bool pkgCacheGenerator::MergeList(ListParser &List,
>>>> pkgCache::VerIterator Ver = Pkg.VersionList();
>>>> Dynamic<pkgCache::VerIterator> DynVer(Ver);
>>>> map_ptrloc *Last = &Pkg->VersionList;
>>>> - const void * oldMap = Map.Data();
>>>> + void *oldMap = Map.Data();
>>>> int Res = 1;
>>>> for (; Ver.end() == false; Last = &Ver->NextVer, Ver++)
>>>> {
>>>> @@ -271,7 +272,7 @@ bool pkgCacheGenerator::MergeList(ListParser &List,
>>>>
>>>> if (oldMap != Map.Data())
>>>> {
>>>> - Last += (map_ptrloc*) Map.Data() - (map_ptrloc*) oldMap;
>>>> + Last = RebasePointer(Last, oldMap, Map.Data());
>>>> oldMap = Map.Data();
>>>> }
>>>>
>>>> @@ -297,7 +298,7 @@ bool pkgCacheGenerator::MergeList(ListParser &List,
>>>> PackageName.c_str(), 1);
>>>>
>>>> if (oldMap != Map.Data())
>>>> - Last += (map_ptrloc*) Map.Data() - (map_ptrloc*) oldMap;
>>>> + Last = RebasePointer(Last, oldMap, Map.Data());
>>>> *Last = *verindex;
>>>>
>>>> Ver->ParentPkg = Pkg.Index();
>>>> @@ -553,7 +554,7 @@ bool pkgCacheGenerator::ListParser::NewDepends(pkgCache::VerIterator &Ver,
>>>> unsigned int Op,
>>>> unsigned int Type)
>>>> {
>>>> - void const * const oldMap = Owner->Map.Data();
>>>> + void *oldMap = Owner->Map.Data();
>>>> pkgCache &Cache = Owner->Cache;
>>>>
>>>> // Get a structure
>>>> @@ -605,7 +606,7 @@ bool pkgCacheGenerator::ListParser::NewDepends(pkgCache::VerIterator &Ver,
>>>> OldDepLast = &D->NextDepends;
>>>> OldDepVer = Ver;
>>>> } else if (oldMap != Owner->Map.Data())
>>>> - OldDepLast += (map_ptrloc*) Owner->Map.Data() - (map_ptrloc*) oldMap;
>>>> + OldDepLast = RebasePointer(OldDepLast, oldMap, Owner->Map.Data());
>>>>
>>>> // Is it a file dependency?
>>>> if (PackageName[0] == '/')
>>>> @@ -739,7 +740,7 @@ std::experimental::optional<map_ptrloc> pkgCacheGenerator::WriteUniqString(const
>>>> }
>>>>
>>>> // Get a structure
>>>> - void const * const oldMap = Map.Data();
>>>> + void *oldMap = Map.Data();
>>>> const auto Item = AllocateInMap(sizeof(pkgCache::StringItem));
>>>> const auto idxString = WriteStringInMap(S, Size);
>>>> if ((!Item) || (!idxString))
>>>> @@ -747,8 +748,8 @@ std::experimental::optional<map_ptrloc> pkgCacheGenerator::WriteUniqString(const
>>>>
>>>> if (oldMap != Map.Data())
>>>> {
>>>> - Last += (map_ptrloc*) Map.Data() - (map_ptrloc*) oldMap;
>>>> - I += (pkgCache::StringItem*) Map.Data() - (pkgCache::StringItem*) oldMap;
>>>> + Last = RebasePointer(Last, oldMap, Map.Data());
>>>> + I = RebasePointer(I, oldMap, Map.Data());
>>>> }
>>>>
>>>> *Last = *Item;
>>>> diff --git a/apt/apt-pkg/pkgcachegen.h b/apt/apt-pkg/pkgcachegen.h
>>>> index 77075a6..0faf6e1 100644
>>>> --- a/apt/apt-pkg/pkgcachegen.h
>>>> +++ b/apt/apt-pkg/pkgcachegen.h
>>>> @@ -72,7 +72,7 @@ class pkgCacheGenerator
>>>> class DynamicFunction
>>>> {
>>>> public:
>>>> - typedef std::function<void(const void *, const void *)> function;
>>>> + typedef std::function<void(void *, void *)> function;
>>>>
>>>> static std::set<DynamicFunction*> toReMap;
>>>>
>>>> @@ -93,7 +93,7 @@ class pkgCacheGenerator
>>>> toReMap.erase(this);
>>>> }
>>>>
>>>> - void call(const void *oldMap, const void *newMap)
>>>> + void call(void *oldMap, void *newMap)
>>>> {
>>>> if (m_function)
>>>> m_function(oldMap, newMap);
>>>> @@ -140,7 +140,7 @@ class pkgCacheGenerator
>>>> // CNC:2003-03-18
>>>> inline void ResetFileDeps() {FoundFileDeps = false;};
>>>>
>>>> - void ReMap(void const * const oldMap, void const * const newMap);
>>>> + void ReMap(void *oldMap, void *newMap);
>>>>
>>>> pkgCacheGenerator(DynamicMMap *Map,OpProgress *Progress);
>>>> ~pkgCacheGenerator();
>>>> diff --git a/apt/apt-pkg/rebase_pointer.h b/apt/apt-pkg/rebase_pointer.h
>>>> new file mode 100644
>>>> index 0000000..f6b3c15
>>>> --- /dev/null
>>>> +++ b/apt/apt-pkg/rebase_pointer.h
>>>> @@ -0,0 +1,16 @@
>>>> +#ifndef PKGLIB_REBASE_POINTER_H
>>>> +#define PKGLIB_REBASE_POINTER_H
>>>> +
>>>> +template <typename T>
>>>> +static inline T* RebasePointer(T *ptr, void *old_base, void *new_base)
>>>> +{
>>>> + return reinterpret_cast<T*>(reinterpret_cast<char*>(new_base) + (reinterpret_cast<char*>(ptr) - reinterpret_cast<char*>(old_base)));
>>>> +}
>>>> +
>>>> +template <typename T>
>>>> +static inline const T* RebasePointer(const T *ptr, void *old_base, void *new_base)
>>>> +{
>>>> + return reinterpret_cast<const T*>(reinterpret_cast<char*>(new_base) + (reinterpret_cast<const char*>(ptr) - reinterpret_cast<char*>(old_base)));
>>>> +}
>>>> +
>>>> +#endif
>>>> diff --git a/apt/apt-pkg/rpm/rpmlistparser.cc b/apt/apt-pkg/rpm/rpmlistparser.cc
>>>> index 9b2e9ad..bd13b99 100644
>>>> --- a/apt/apt-pkg/rpm/rpmlistparser.cc
>>>> +++ b/apt/apt-pkg/rpm/rpmlistparser.cc
>>>> @@ -25,6 +25,7 @@
>>>> #include <apt-pkg/strutl.h>
>>>> #include <apt-pkg/crc-16.h>
>>>> #include <apt-pkg/tagfile.h>
>>>> +#include <apt-pkg/rebase_pointer.h>
>>>>
>>>> #include <apti18n.h>
>>>>
>>>> @@ -50,13 +51,13 @@ rpmListParser::rpmListParser(RPMHandler *Handler)
>>>> {
>>>> SeenPackages.reset(new SeenPackagesType);
>>>> m_SeenPackagesRealloc.reset(new pkgCacheGenerator::DynamicFunction(
>>>> - [this] (void const *oldMap, void const *newMap)
>>>> + [this] (void *oldMap, void *newMap)
>>>> {
>>>> SeenPackagesType tmp;
>>>>
>>>> for (auto iter: *SeenPackages)
>>>> {
>>>> - tmp.insert(iter + (static_cast<const char *>(newMap) - static_cast<const char *>(oldMap)));
>>>> + tmp.insert(RebasePointer(iter, oldMap, newMap));
>>>> }
>>>>
>>>> SeenPackages->swap(tmp);
>>>> diff --git a/apt/apt-pkg/rpm/rpmpackagedata.cc b/apt/apt-pkg/rpm/rpmpackagedata.cc
>>>> index 61c15d5..d6c677b 100644
>>>> --- a/apt/apt-pkg/rpm/rpmpackagedata.cc
>>>> +++ b/apt/apt-pkg/rpm/rpmpackagedata.cc
>>>> @@ -23,7 +23,7 @@ RPMPackageData::RPMPackageData()
>>>> #endif
>>>> {
>>>> m_VerMapRealloc.reset(new pkgCacheGenerator::DynamicFunction(
>>>> - [this] (void const *oldMap, void const *newMap)
>>>> + [this] (void *oldMap, void *newMap)
>>>> {
>>>> for (auto iter1 = VerMap.begin(); iter1 != VerMap.end(); ++iter1)
>>>> {
>>>
>>>
>>> Best regards,
>>> Andrew Savchenko
>>>
>>>
>>> _______________________________________________
>>> Devel mailing list
>>> Devel@lists.altlinux.org
>>> https://lists.altlinux.org/mailman/listinfo/devel
>>>
>> _______________________________________________
>> Devel mailing list
>> Devel@lists.altlinux.org
>> https://lists.altlinux.org/mailman/listinfo/devel
>
> Best regards,
> Andrew Savchenko
>
>
> _______________________________________________
> Devel mailing list
> Devel@lists.altlinux.org
> https://lists.altlinux.org/mailman/listinfo/devel
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [devel] [PATCH for apt 2/2 v2] Fix pointer arithmetics
2019-12-12 19:43 ` Andrey Savchenko
@ 2019-12-13 8:01 ` Aleksei Nikiforov
0 siblings, 0 replies; 34+ messages in thread
From: Aleksei Nikiforov @ 2019-12-13 8:01 UTC (permalink / raw)
To: devel
12.12.2019 22:43, Andrey Savchenko пишет:
> On Wed, 11 Dec 2019 10:50:22 +0300 Aleksei Nikiforov wrote:
>> 11.12.2019 1:20, Dmitry V. Levin пишет:
>>> On Tue, Dec 10, 2019 at 01:58:17PM +0300, Aleksei Nikiforov wrote:
>>>> 10.12.2019 13:20, Dmitry V. Levin пишет:
>>>>> On Tue, Dec 10, 2019 at 11:18:06AM +0300, Aleksei Nikiforov wrote:
>>>>>> 10.12.2019 3:07, Dmitry V. Levin пишет:
>>>>>>> On Mon, Dec 09, 2019 at 10:08:42AM +0300, Aleksei Nikiforov wrote:
>>>>>>>> 09.12.2019 2:21, Dmitry V. Levin пишет:
>>>>>>>>> On Fri, Dec 06, 2019 at 06:36:55PM +0300, Aleksei Nikiforov wrote:
>>>>>>>>> [...]
>>>>>>>>>> @@ -85,11 +87,11 @@ class pkgCache::PkgIterator
>>>>>>>>>> inline unsigned long long Index() const {return Pkg - Owner->PkgP;};
>>>>>>>>>> OkState State() const;
>>>>>>>>>>
>>>>>>>>>> - void ReMap(void const * const oldMap, void const * const newMap)
>>>>>>>>>> + void ReMap(void *oldMap, void *newMap)
>>>>>>>>>
>>>>>>>>> Is there any particular reason for stripping const here and in other
>>>>>>>>> similar places?
>>>>>>>>
>>>>>>>> Yes, it's needed due to issues emerging from mixing const and non-const
>>>>>>>> pointers with new and allegedly more proper way of calculating rebased
>>>>>>>> pointers.
>>>>>>>
>>>>>>> Sorry, I don't find this argument convincing.
>>>>>>> I have experienced no const issues in my version of this fix.
>>>>>>
>>>>>> Your version is using C-style casts in C++ code. Of course, I could use
>>>>>> C-style casts or const_cast-s too to work around const correctness
>>>>>> issues (i.e. to just hide these issues), and it'd work like your
>>>>>> version. But I'd like to remind you that APT is C++ project, not a C
>>>>>> project. What might be ok for C is sometimes a dirty ugly hack in modern
>>>>>> C++.
>>>>>
>>>>> Sorry, I don't share you point of view on this matter.
>>>>> Being a C++ project allows you to use C++ constructs, that's true,
>>>>> but why do you think it prevents you from using C constructs when
>>>>> appropriate?
>>>>
>>>> I didn't say that something prevents from using C constructs when
>>>> appropriate. I'm saying that these constructs are not appropriate here.
>>>
>>> Why do you think they are not appropriate here?
>>>
>>
>> In good C++ code there is no place for const_cast.
>
> This statement is ungrounded.
>
It is grounded. Read my answer below (and links [2] and [3] to
documentation you provided).
>> Maybe there are some
>> exceptions to it, but they have to be justified. It doesn't matter that
>> you are hiding it behind C-style cast.
>
> Please read some good book on C++ like [1] or at least the official
> reference manual [2].
Same to you.
>
> Aside from C-style cast C++ supports four casts (in their safety
> order, the safest first):
> const_cast
> static_cast
> dynamic_cast
> reinterpret_cast
>
Now, this statement is ungrounded. Where did you get this list from? Do
you serously think that dynamic_cast is less safe than static_cast? And
how did you judge safety of const_cast vs safety of reinterpret_cast?
How did you get const_cast more safe than static_cast and dynamic_cast?
For your education: const_cast is indeed safe when 'T' is casted to
'const T', but when 'const T' is casted to 'T' it can lead to UB,
assuming 'T' is a non-const type.
Read your own documentation:
https://en.cppreference.com/w/cpp/language/const_cast
> One can see their preference order base on how explicit C-style
> cast is being intrepreted by the C++ compiler [3].
>
> So actually the reinterpret_cast should be avoided when it is
> possible to use more strict casts, because reinterpret_cast
> disables all safety checks aside from constness and volatileness
> one.
>
Please show me how you'd write this code without reinterpret_cast and
without C-style cast since it will just implicitly use reinterpret_cast.
C-style cast is the least strict cast in C++ since it tries to apply
every cast except for dynamic_cast. I agree with you that stricter casts
should be applied, and thus there's no place for C-style casts.
> [1] Stanley B. Lippman, Josee Lajoie, C++ Primer. Chapter 4.14
> Types Conversion.
> [2] https://en.cppreference.com
> [3] https://en.cppreference.com/w/cpp/language/explicit_cast
>
> Best regards,
> Andrew Savchenko
>
>
> _______________________________________________
> Devel mailing list
> Devel@lists.altlinux.org
> https://lists.altlinux.org/mailman/listinfo/devel
>
^ permalink raw reply [flat|nested] 34+ messages in thread
end of thread, other threads:[~2019-12-13 8:01 UTC | newest]
Thread overview: 34+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-06 13:16 [devel] [PATCH for apt 1/2] Add option for debugging DynamicMMap::Allocate Aleksei Nikiforov
2019-12-06 13:16 ` [devel] [PATCH for apt 2/2] Fix pointer arithmetics Aleksei Nikiforov
2019-12-06 13:36 ` Ivan A. Melnikov
2019-12-06 15:32 ` Aleksei Nikiforov
2019-12-06 15:36 ` [devel] [PATCH for apt 2/2 v2] " Aleksei Nikiforov
2019-12-07 14:52 ` Andrey Savchenko
2019-12-08 22:56 ` Dmitry V. Levin
2019-12-09 6:54 ` Aleksei Nikiforov
2019-12-12 19:20 ` Andrey Savchenko
2019-12-13 7:58 ` Aleksei Nikiforov
2019-12-08 23:21 ` Dmitry V. Levin
2019-12-09 7:08 ` Aleksei Nikiforov
2019-12-10 0:07 ` Dmitry V. Levin
2019-12-10 8:18 ` Aleksei Nikiforov
2019-12-10 10:20 ` Dmitry V. Levin
2019-12-10 10:58 ` Aleksei Nikiforov
2019-12-10 22:20 ` Dmitry V. Levin
2019-12-11 7:50 ` Aleksei Nikiforov
2019-12-12 19:43 ` Andrey Savchenko
2019-12-13 8:01 ` Aleksei Nikiforov
2019-12-08 23:31 ` [devel] [PATCH for apt 2/2] " Dmitry V. Levin
2019-12-09 7:09 ` Aleksei Nikiforov
2019-12-09 7:16 ` [devel] [PATCH for apt 2/2 v3] " Aleksei Nikiforov
2019-12-09 23:54 ` [devel] [PATCH apt 0/3] Fix 0.5.15lorg2-alt70~9 fallout Dmitry V. Levin
2019-12-09 23:56 ` [devel] [PATCH apt 1/3] apt-pkg/cacheiterators.h: revert #include <set> Dmitry V. Levin
2019-12-09 23:56 ` [devel] [PATCH apt 2/3] apt-pkg/contrib/mmap.cc: revert confusing change of Flags to this->Flags Dmitry V. Levin
2019-12-09 23:56 ` [devel] [PATCH apt 3/3] Fix UB in pointer arithmetic Dmitry V. Levin
2019-12-10 8:18 ` Aleksei Nikiforov
2019-12-08 22:50 ` [devel] [PATCH for apt 1/2] Add option for debugging DynamicMMap::Allocate Dmitry V. Levin
2019-12-09 6:58 ` Aleksei Nikiforov
2019-12-09 10:24 ` Dmitry V. Levin
2019-12-09 11:03 ` [devel] [PATCH for apt 1/2 v3] Add Debug::DynamicMMap::Allocate option Aleksei Nikiforov
2019-12-09 22:59 ` Dmitry V. Levin
2019-12-09 7:00 ` [devel] [PATCH for apt 1/2 v2] Add option for debugging Debug::DynamicMMap::Allocate Aleksei Nikiforov
ALT Linux Team development discussions
This inbox may be cloned and mirrored by anyone:
git clone --mirror http://lore.altlinux.org/devel/0 devel/git/0.git
# If you have public-inbox 1.1+ installed, you may
# initialize and index your mirror using the following commands:
public-inbox-init -V2 devel devel/ http://lore.altlinux.org/devel \
devel@altlinux.org devel@altlinux.ru devel@lists.altlinux.org devel@lists.altlinux.ru devel@linux.iplabs.ru mandrake-russian@linuxteam.iplabs.ru sisyphus@linuxteam.iplabs.ru
public-inbox-index devel
Example config snippet for mirrors.
Newsgroup available over NNTP:
nntp://lore.altlinux.org/org.altlinux.lists.devel
AGPL code for this site: git clone https://public-inbox.org/public-inbox.git