[PATCH] Introduce needsCleanup() for APFloat and APInt

[Manuel Klimek]

+richard smith, who proposed to implement it this way

On Mon, May 13, 2013 at 6:08 PM, 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?
>

APFloat/APInt is placement new'ed in the code in question, and thus we need
to call the destructors of any objects that do memory allocation
themselves. We can save those destructor calls otherwise.


> 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   };
>

Richard wrote that code, and thus probably can explain the decision.

Cheers,
/Manuel


>
>
>
>
> 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
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130513/07eab01d/attachment.html>