[PATCH] Introduce needsCleanup() for APFloat and APInt

[Manuel Klimek]

Ping.


On Wed, May 15, 2013 at 9:58 PM, Richard Smith <richard at metafoo.co.uk>wrote:

> On Mon, May 13, 2013 at 10:08 AM, Shuxin Yang <shuxin.llvm at gmail.com>wrote:
>
>>  I'm afraid this change is going to open a big can of worms. You are
>> essentially promote
>> private member function to be public. Such practice should be generally
>> avoided even for
>> the local/private class which is used in small scope, let alone these
>> fundamental structures
>> which are extensively used in the compiler.
>>
>> > object needs the destructor called after its memory was freed
>>
>> If the block of memory containing APFloat/APInt is already freed, how do
>> you know
>> the state of these objects are still valid? Depending on the
>> implementation, free-operation
>> might clobber the memory with some special value for debugging purpose.
>> If it were safe
>> to call needsCleanup(), why not directly call the destructor? Is
>> needsCleanup() sufficient
>> and/or necessary condition for calling the destructor? How do we keep
>> them in sync in the
>> future?
>>
>
> I think Manuel's phrasing here may be throwing you. What we want is a
> mechanism to determine whether the destructor would be trivial (and thus we
> don't need to bother cleaning the object up). We are *not* calling a
> destructor after the memory was freed.
>
> I think you have to restructure the code that using APFloat/APInt to avoid
>> memory leak.
>> I certainly understand that restructuring sometimes is not easy
>> undertaking, however,
>> we have no other way around.
>>
>> BTW, by following the link, I take a glimpse of APValue, and spot a
>> potential problem there
>> (I think it is irrelevant to the problem you are trying to tackle).
>>
>> The union member "Data" will be cost to APFloat/APInt etc, and the
>> "Aligner" is the dummy
>> field used to make sure the union sitting at right place. However, how do
>> we know
>> the alignof(APFloat|APInt|etc) is no more stringent than alignof(void*)?
>> Shouldn't we use AlignedCharArrayUnion for this purpose?
>>
>> cat -n tools/clang/include/clang/AST/APValue.h
>> 38 class APValue {
>> ...
>>
>> 116   union {
>> 117     void *Aligner;
>> 118     char Data[MaxSize];
>> 119   };
>>
>>
>>
>> On 5/12/13 10:59 PM, Manuel Klimek wrote:
>>
>> This is needed so one can check if the object needs the destructor called after
>> its memory was freed. See http://llvm-reviews.chandlerc.com/D736 for where this
>> is useful.
>> http://llvm-reviews.chandlerc.com/D783
>>
>> Files:
>>   include/llvm/ADT/APFloat.h
>>   include/llvm/ADT/APInt.h
>>   lib/Support/APFloat.cpp
>>
>> Index: include/llvm/ADT/APFloat.h
>> ===================================================================
>> --- include/llvm/ADT/APFloat.h
>> +++ include/llvm/ADT/APFloat.h
>> @@ -190,6 +190,10 @@
>>      APFloat(const APFloat &);
>>      ~APFloat();
>>
>> +    bool needsCleanup() const {
>> +      return partCount() > 1;
>> +    }
>> +
>>      // Convenience "constructors"
>>      static APFloat getZero(const fltSemantics &Sem, bool Negative = false) {
>>        return APFloat(Sem, fcZero, Negative);
>> Index: include/llvm/ADT/APInt.h
>> ===================================================================
>> --- include/llvm/ADT/APInt.h
>> +++ include/llvm/ADT/APInt.h
>> @@ -283,14 +283,18 @@
>>
>>    /// @brief Destructor.
>>    ~APInt() {
>> -    if (!isSingleWord())
>> +    if (needsCleanup())
>>        delete [] pVal;
>>    }
>>
>>    /// Default constructor that creates an uninitialized APInt.  This is useful
>>    ///  for object deserialization (pair this with the static method Read).
>>    explicit APInt() : BitWidth(1) {}
>>
>> +  bool needsCleanup() const {
>> +    return !isSingleWord();
>> +  }
>> +
>>    /// Profile - Used to insert APInt objects, or objects that contain APInt
>>    ///  objects, into FoldingSets.
>>    void Profile(FoldingSetNodeID& id) const;
>> Index: lib/Support/APFloat.cpp
>> ===================================================================
>> --- lib/Support/APFloat.cpp
>> +++ lib/Support/APFloat.cpp
>> @@ -580,7 +580,7 @@
>>  void
>>  APFloat::freeSignificand()
>>  {
>> -  if (partCount() > 1)
>> +  if (needsCleanup())
>>      delete [] significand.parts;
>>  }
>>
>>
>>
>> _______________________________________________
>> llvm-commits mailing listllvm-commits at cs.uiuc.eduhttp://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130521/ff8605f2/attachment.html>