[PATCH] Introduce needsCleanup() for APFloat and APInt

[Manuel Klimek]

Ping.


On Tue, May 21, 2013 at 11:19 AM, Manuel Klimek <klimek at google.com> wrote:

> 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/20130527/9e08d8c0/attachment.html>