From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Wed, 11 Dec 2019 01:20:18 +0300 From: "Dmitry V. Levin" To: ALT Devel discussion list Message-ID: <20191210222017.GA31774@altlinux.org> References: <20191206133647.dculnmwkd3yf2wjp@titan.localdomain> <20191206153655.86334-1-darktemplar@altlinux.org> <20191208232108.GC30742@altlinux.org> <20191210000737.GD15867@altlinux.org> <1ee850d3-9ebd-ae95-2665-f5ba7fb86ad4@altlinux.org> <20191210102009.GB22650@altlinux.org> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="ReaqsoxgOBHFXBhH" Content-Disposition: inline In-Reply-To: Subject: Re: [devel] [PATCH for apt 2/2 v2] Fix pointer arithmetics X-BeenThere: devel@lists.altlinux.org X-Mailman-Version: 2.1.12 Precedence: list Reply-To: ALT Linux Team development discussions List-Id: ALT Linux Team development discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 10 Dec 2019 22:20:18 -0000 Archived-At: List-Archive: List-Post: --ReaqsoxgOBHFXBhH Content-Type: text/plain; charset=koi8-r Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Dec 10, 2019 at 01:58:17PM +0300, Aleksei Nikiforov wrote: > 10.12.2019 13:20, Dmitry V. Levin =D0=C9=DB=C5=D4: > > On Tue, Dec 10, 2019 at 11:18:06AM +0300, Aleksei Nikiforov wrote: > >> 10.12.2019 3:07, Dmitry V. Levin =D0=C9=DB=C5=D4: > >>> On Mon, Dec 09, 2019 at 10:08:42AM +0300, Aleksei Nikiforov wrote: > >>>> 09.12.2019 2:21, Dmitry V. Levin =D0=C9=DB=C5=D4: > >>>>> 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; > >>>>>> =20 > >>>>>> - void ReMap(void const * const oldMap, void const * const newMa= p) > >>>>>> + 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-co= nst > >>>> pointers with new and allegedly more proper way of calculating rebas= ed > >>>> 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 mode= rn > >> C++. > >=20 > > 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? >=20 > I didn't say that something prevents from using C constructs when=20 > 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 Dynami= cMMap::Allocate(unsigned long Item > >>>>>> Pool* oldPools =3D Pools; > >>>>>> auto idxResult =3D RawAllocate(I->Count*ItemSize,ItemSiz= e); > >>>>>> if (Pools !=3D oldPools) > >>>>>> - I +=3D Pools - oldPools; > >>>>>> + I =3D RebasePointer(I, oldPools, Pools); > >>>>>> =20 > >>>>>> // 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 supe= rior. > >=20 > > The order in question was introduced by your commit > > 6d5e6a68 ("apt-pkg/pkgcachegen.{cc,h} changes"). > >=20 > > If I was reviewing that commit, this would have been fixed already. >=20 > 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_poi= nter.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 > >>>>>> +static inline T* RebasePointer(T *ptr, void *old_base, void *new_= base) > >>>>>> +{ > >>>>>> + return reinterpret_cast(reinterpret_cast(new_base) = + (reinterpret_cast(ptr) - reinterpret_cast(old_base))); > >>>>>> +} > >>>>>> + > >>>>>> +template > >>>>>> +static inline const T* RebasePointer(const T *ptr, void *old_base= , void *new_base) > >>>>>> +{ > >>>>>> + return reinterpret_cast(reinterpret_cast(new_= base) + (reinterpret_cast(ptr) - reinterpret_cast(old_b= ase))); > >>>>>> +} > >>>>>> + > >>>>>> +#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. 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 for= m of > >>> ptr =3D 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. > >=20 > > 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 wr= ong > > variable. In fact, I had to use the following regular expression just > > to check whether all uses of RebasePointer are correct in that respect: > >=20 > > $ git grep -Fw RebasePointer |grep -v '\<\([[:alpha:]][][[:alnum:]_]*\)= =3D RebasePointer(\1,' > >=20 > > This is surely not the way how things should be done, > > neither in C nor in C++. >=20 > It's also very easy to miss one of places where such pointer=20 > recalculation is required, There must be a way to exclude this possibility. > but you still want this solution instead of=20 > 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. >=20 > As for forgetting assignment, your addition of attribute 'warn unused=20 > 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. :) --=20 ldv --ReaqsoxgOBHFXBhH Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIcBAEBCAAGBQJd8BohAAoJEAVFT+BVnCUI0PoQAPXRIFZ//AgM87leui1fDvvH ZQNpPpQfdz4Sz47sEJyXdtoNX6u4Prth3NwkM4cZI1SrSPMPHX4jOTR4+JMYIMoM Y7PXbkvZlzwRSNrlpgHKYAgZptCEF/W8T/gtbF0j05B74vm3XELnQBtdYfbcoXf3 sI2sIAV30XQQtPtbopY6edWiUGWUnU6pf4vcrrzxx11ig6BV/RgElU1zXw7Fujds dhXkRW0CBhuvKTAWz6LtAFO6C2JLkWoj7EC/oNgGiY9UwCpJEqE3osWrYyMlB6xT 5sokV8x4w1du6Tj5La28F578jV2Fif0T8Gm5pgjt8LO9G3ckguVUpqnhJBNJVxOL alFnkjVE6HR1F2mZkAHtEdEIHavxxDfR/qacBXQTJ3dm0wWrZKIzymZvRzXZR62b DJYA7D1uHm+SWAeyc69TXD9jO7Q/Is9EX6rslzzPOWzdh0jpcRIowW3v2526AMHa ealIqErvKTgQT8lymo9/FOA70YsYJlQeFEG577yby3hfOpTX+SU8/U66Ch3IrQB/ 5OX4ADa4DVuvg2rWNrfzUGPXLzBEtjzPaq8ztQ9wIm21d/rKXbfvEklhwmTI7Q9n hxLhMl4pSuHCTRZKzloBbGBDqz4eptPDnVzAqDat/8i63on0efzLPbsKGFwNPDuz QPSmhBdLFopeYLDse0hj =GctM -----END PGP SIGNATURE----- --ReaqsoxgOBHFXBhH--