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> <20191210222017.GA31774@altlinux.org> From: Aleksei Nikiforov Message-ID: Date: Wed, 11 Dec 2019 10:50:22 +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: <20191210222017.GA31774@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: Wed, 11 Dec 2019 07:50:27 -0000 Archived-At: List-Archive: List-Post: 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 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 >>>>>>>> +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, > > 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 >