From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on sa.local.altlinux.org X-Spam-Level: X-Spam-Status: No, score=-2.9 required=5.0 tests=ALL_TRUSTED,BAYES_00 autolearn=unavailable autolearn_force=no version=3.4.1 To: devel@lists.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> From: Aleksei Nikiforov Message-ID: Date: Tue, 10 Dec 2019 13:58:17 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.8.0 MIME-Version: 1.0 In-Reply-To: <20191210102009.GB22650@altlinux.org> Content-Type: text/plain; charset=koi8-r; format=flowed Content-Language: ru Content-Transfer-Encoding: 8bit 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 10:58:41 -0000 Archived-At: List-Archive: List-Post: 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 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 >>>>>> +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_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. 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 >