[PATCH] Introduce needsCleanup() for APFloat and APInt

[Richard Smith]

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