[v2] GBE/PRINTF: store variable instead of pointer in "slots".

Submitted by Luo, Xionghu on Aug. 10, 2015, 7:40 a.m.

Details

Message ID 1439192413-795-1-git-send-email-xionghu.luo@intel.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Luo, Xionghu Aug. 10, 2015, 7:40 a.m.
From: Luo Xionghu <xionghu.luo@intel.com>

this could fix the bug: https://bugs.freedesktop.org/show_bug.cgi?id=90472
v2: the vector "slots" stores the pointer of PrintfSlot from vector "fmts",
but the push_back operation of "fmts" will cause resize if capacity is not
enough and call the copy constructor and destructor of that PrintfSlot,
leading to a illegal pointer in "slots", so this patch change to store the
variable instead of pointer.
  update the destructor of PrintfSlot according to the SLOT_TYPE.

Signed-off-by: Luo Xionghu <xionghu.luo@intel.com>
---
 backend/src/ir/printf.cpp               | 21 +++++++++++++++++++--
 backend/src/ir/printf.hpp               | 17 +++++++----------
 backend/src/llvm/llvm_printf_parser.cpp |  2 +-
 3 files changed, 27 insertions(+), 13 deletions(-)

Patch hide | download patch | download mbox

diff --git a/backend/src/ir/printf.cpp b/backend/src/ir/printf.cpp
index 3d9b2fd..eb1c199 100644
--- a/backend/src/ir/printf.cpp
+++ b/backend/src/ir/printf.cpp
@@ -31,6 +31,23 @@  namespace gbe
 
     pthread_mutex_t PrintfSet::lock = PTHREAD_MUTEX_INITIALIZER;
 
+    PrintfSlot::~PrintfSlot(void)
+    {
+        if (ptr)
+        {
+          if (type == PRINTF_SLOT_TYPE_STRING) {
+            free(ptr);
+            ptr = NULL;
+          } else if (type == PRINTF_SLOT_TYPE_STATE) {
+            delete state;
+            state = NULL;
+          } else {
+            type = PRINTF_SLOT_TYPE_NONE;
+            ptr = NULL;
+          }
+        }
+    }
+
     uint32_t PrintfSet::append(PrintfFmt* fmt, Unit& unit)
     {
       fmts.push_back(*fmt);
@@ -40,12 +57,12 @@  namespace gbe
         if (f->type == PRINTF_SLOT_TYPE_STRING)
           continue;
 
-        slots.push_back(&(*f));
+        slots.push_back(*f);
       }
 
       /* Update the total size of size. */
       if (slots.size() > 0)
-        sizeOfSize = slots.back()->state->out_buf_sizeof_offset
+        sizeOfSize = slots.back().state->out_buf_sizeof_offset
                      + getPrintfBufferElementSize(slots.size() - 1);
 
       return (uint32_t)fmts.size();
diff --git a/backend/src/ir/printf.hpp b/backend/src/ir/printf.hpp
index cbab759..df58437 100644
--- a/backend/src/ir/printf.hpp
+++ b/backend/src/ir/printf.hpp
@@ -159,10 +159,7 @@  namespace gbe
         ptr = p;
       }
 
-      ~PrintfSlot(void) {
-        if (ptr)
-          free(ptr);
-      }
+      ~PrintfSlot(void);
     };
 
     class Context;
@@ -177,7 +174,7 @@  namespace gbe
         }
 
         for (size_t i = 0; i < other.slots.size(); ++i) {
-          PrintfSlot* s = other.slots[i];
+          PrintfSlot s = other.slots[i];
           slots.push_back(s);
         }
 
@@ -215,15 +212,15 @@  namespace gbe
       uint8_t getIndexBufBTI() const { return btiIndexBuf; }
 
       uint32_t getPrintfBufferElementSize(uint32_t i) {
-        PrintfSlot* slot = slots[i];
+        PrintfSlot& slot = slots[i];
         int vec_num = 1;
-        if (slot->state->vector_n > 0) {
-          vec_num = slot->state->vector_n;
+        if (slot.state->vector_n > 0) {
+          vec_num = slot.state->vector_n;
         }
 
         assert(vec_num > 0 && vec_num <= 16);
 
-        switch (slot->state->conversion_specifier) {
+        switch (slot.state->conversion_specifier) {
           case PRINTF_CONVERSION_I:
           case PRINTF_CONVERSION_D:
           case PRINTF_CONVERSION_O:
@@ -257,7 +254,7 @@  namespace gbe
 
     private:
       vector<PrintfFmt> fmts;
-      vector<PrintfSlot*> slots;
+      vector<PrintfSlot> slots;
       uint32_t sizeOfSize; // Total sizeof size.
       friend struct LockOutput;
       uint8_t btiBuf;
diff --git a/backend/src/llvm/llvm_printf_parser.cpp b/backend/src/llvm/llvm_printf_parser.cpp
index 2f85443..3d84457 100644
--- a/backend/src/llvm/llvm_printf_parser.cpp
+++ b/backend/src/llvm/llvm_printf_parser.cpp
@@ -291,7 +291,7 @@  again:
 #if 0
     {
       int j = 0;
-      for (auto &s : *printf_fmt) {
+      for (auto &s : printf_fmt->first) {
         j++;
         if (s.type == PRINTF_SLOT_TYPE_STATE) {
           fprintf(stderr, "---- %d ---: state : \n", j);

Comments

LGTM, very thanks for fixing this bug

> -----Original Message-----
> From: xionghu.luo@intel.com
> Sent: Mon, 10 Aug 2015 15:40:13 +0800
> To: beignet@lists.freedesktop.org
> Subject: [Beignet] [Patch v2] GBE/PRINTF: store variable instead of
> pointer in "slots".
> 
> From: Luo Xionghu <xionghu.luo@intel.com>
> 
> this could fix the bug:
> https://bugs.freedesktop.org/show_bug.cgi?id=90472
> v2: the vector "slots" stores the pointer of PrintfSlot from vector
> "fmts",
> but the push_back operation of "fmts" will cause resize if capacity is
> not
> enough and call the copy constructor and destructor of that PrintfSlot,
> leading to a illegal pointer in "slots", so this patch change to store
> the
> variable instead of pointer.
>   update the destructor of PrintfSlot according to the SLOT_TYPE.
> 
> Signed-off-by: Luo Xionghu <xionghu.luo@intel.com>
> ---
>  backend/src/ir/printf.cpp               | 21 +++++++++++++++++++--
>  backend/src/ir/printf.hpp               | 17 +++++++----------
>  backend/src/llvm/llvm_printf_parser.cpp |  2 +-
>  3 files changed, 27 insertions(+), 13 deletions(-)
> 
> diff --git a/backend/src/ir/printf.cpp b/backend/src/ir/printf.cpp
> index 3d9b2fd..eb1c199 100644
> --- a/backend/src/ir/printf.cpp
> +++ b/backend/src/ir/printf.cpp
> @@ -31,6 +31,23 @@ namespace gbe
> 
>      pthread_mutex_t PrintfSet::lock = PTHREAD_MUTEX_INITIALIZER;
> 
> +    PrintfSlot::~PrintfSlot(void)
> +    {
> +        if (ptr)
> +        {
> +          if (type == PRINTF_SLOT_TYPE_STRING) {
> +            free(ptr);
> +            ptr = NULL;
> +          } else if (type == PRINTF_SLOT_TYPE_STATE) {
> +            delete state;
> +            state = NULL;
> +          } else {
> +            type = PRINTF_SLOT_TYPE_NONE;
> +            ptr = NULL;
> +          }
> +        }
> +    }
> +
>      uint32_t PrintfSet::append(PrintfFmt* fmt, Unit& unit)
>      {
>        fmts.push_back(*fmt);
> @@ -40,12 +57,12 @@ namespace gbe
>          if (f->type == PRINTF_SLOT_TYPE_STRING)
>            continue;
> 
> -        slots.push_back(&(*f));
> +        slots.push_back(*f);
>        }
> 
>        /* Update the total size of size. */
>        if (slots.size() > 0)
> -        sizeOfSize = slots.back()->state->out_buf_sizeof_offset
> +        sizeOfSize = slots.back().state->out_buf_sizeof_offset
>                       + getPrintfBufferElementSize(slots.size() - 1);
> 
>        return (uint32_t)fmts.size();
> diff --git a/backend/src/ir/printf.hpp b/backend/src/ir/printf.hpp
> index cbab759..df58437 100644
> --- a/backend/src/ir/printf.hpp
> +++ b/backend/src/ir/printf.hpp
> @@ -159,10 +159,7 @@ namespace gbe
>          ptr = p;
>        }
> 
> -      ~PrintfSlot(void) {
> -        if (ptr)
> -          free(ptr);
> -      }
> +      ~PrintfSlot(void);
>      };
> 
>      class Context;
> @@ -177,7 +174,7 @@ namespace gbe
>          }
> 
>          for (size_t i = 0; i < other.slots.size(); ++i) {
> -          PrintfSlot* s = other.slots[i];
> +          PrintfSlot s = other.slots[i];
>            slots.push_back(s);
>          }
> 
> @@ -215,15 +212,15 @@ namespace gbe
>        uint8_t getIndexBufBTI() const { return btiIndexBuf; }
> 
>        uint32_t getPrintfBufferElementSize(uint32_t i) {
> -        PrintfSlot* slot = slots[i];
> +        PrintfSlot& slot = slots[i];
>          int vec_num = 1;
> -        if (slot->state->vector_n > 0) {
> -          vec_num = slot->state->vector_n;
> +        if (slot.state->vector_n > 0) {
> +          vec_num = slot.state->vector_n;
>          }
> 
>          assert(vec_num > 0 && vec_num <= 16);
> 
> -        switch (slot->state->conversion_specifier) {
> +        switch (slot.state->conversion_specifier) {
>            case PRINTF_CONVERSION_I:
>            case PRINTF_CONVERSION_D:
>            case PRINTF_CONVERSION_O:
> @@ -257,7 +254,7 @@ namespace gbe
> 
>      private:
>        vector<PrintfFmt> fmts;
> -      vector<PrintfSlot*> slots;
> +      vector<PrintfSlot> slots;
>        uint32_t sizeOfSize; // Total sizeof size.
>        friend struct LockOutput;
>        uint8_t btiBuf;
> diff --git a/backend/src/llvm/llvm_printf_parser.cpp
> b/backend/src/llvm/llvm_printf_parser.cpp
> index 2f85443..3d84457 100644
> --- a/backend/src/llvm/llvm_printf_parser.cpp
> +++ b/backend/src/llvm/llvm_printf_parser.cpp
> @@ -291,7 +291,7 @@ again:
>  #if 0
>      {
>        int j = 0;
> -      for (auto &s : *printf_fmt) {
> +      for (auto &s : printf_fmt->first) {
>          j++;
>          if (s.type == PRINTF_SLOT_TYPE_STATE) {
>            fprintf(stderr, "---- %d ---: state : \n", j);
> --
> 1.9.1
> 
> _______________________________________________
> Beignet mailing list
> Beignet@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/beignet
Pushed, thanks.

> -----Original Message-----

> From: Beignet [mailto:beignet-bounces@lists.freedesktop.org] On Behalf Of

> Junyan He

> Sent: Friday, August 14, 2015 14:28

> To: Luo, Xionghu; beignet@lists.freedesktop.org

> Cc: Luo, Xionghu

> Subject: Re: [Beignet] [Patch v2] GBE/PRINTF: store variable instead of

> pointer in "slots".

> 

> LGTM, very thanks for fixing this bug

> 

> > -----Original Message-----

> > From: xionghu.luo@intel.com

> > Sent: Mon, 10 Aug 2015 15:40:13 +0800

> > To: beignet@lists.freedesktop.org

> > Subject: [Beignet] [Patch v2] GBE/PRINTF: store variable instead of

> > pointer in "slots".

> >

> > From: Luo Xionghu <xionghu.luo@intel.com>

> >

> > this could fix the bug:

> > https://bugs.freedesktop.org/show_bug.cgi?id=90472

> > v2: the vector "slots" stores the pointer of PrintfSlot from vector

> > "fmts", but the push_back operation of "fmts" will cause resize if

> > capacity is not enough and call the copy constructor and destructor of

> > that PrintfSlot, leading to a illegal pointer in "slots", so this

> > patch change to store the variable instead of pointer.

> >   update the destructor of PrintfSlot according to the SLOT_TYPE.

> >

> > Signed-off-by: Luo Xionghu <xionghu.luo@intel.com>

> > ---

> >  backend/src/ir/printf.cpp               | 21 +++++++++++++++++++--

> >  backend/src/ir/printf.hpp               | 17 +++++++----------

> >  backend/src/llvm/llvm_printf_parser.cpp |  2 +-

> >  3 files changed, 27 insertions(+), 13 deletions(-)

> >

> > diff --git a/backend/src/ir/printf.cpp b/backend/src/ir/printf.cpp

> > index 3d9b2fd..eb1c199 100644

> > --- a/backend/src/ir/printf.cpp

> > +++ b/backend/src/ir/printf.cpp

> > @@ -31,6 +31,23 @@ namespace gbe

> >

> >      pthread_mutex_t PrintfSet::lock = PTHREAD_MUTEX_INITIALIZER;

> >

> > +    PrintfSlot::~PrintfSlot(void)

> > +    {

> > +        if (ptr)

> > +        {

> > +          if (type == PRINTF_SLOT_TYPE_STRING) {

> > +            free(ptr);

> > +            ptr = NULL;

> > +          } else if (type == PRINTF_SLOT_TYPE_STATE) {

> > +            delete state;

> > +            state = NULL;

> > +          } else {

> > +            type = PRINTF_SLOT_TYPE_NONE;

> > +            ptr = NULL;

> > +          }

> > +        }

> > +    }

> > +

> >      uint32_t PrintfSet::append(PrintfFmt* fmt, Unit& unit)

> >      {

> >        fmts.push_back(*fmt);

> > @@ -40,12 +57,12 @@ namespace gbe

> >          if (f->type == PRINTF_SLOT_TYPE_STRING)

> >            continue;

> >

> > -        slots.push_back(&(*f));

> > +        slots.push_back(*f);

> >        }

> >

> >        /* Update the total size of size. */

> >        if (slots.size() > 0)

> > -        sizeOfSize = slots.back()->state->out_buf_sizeof_offset

> > +        sizeOfSize = slots.back().state->out_buf_sizeof_offset

> >                       + getPrintfBufferElementSize(slots.size() - 1);

> >

> >        return (uint32_t)fmts.size();

> > diff --git a/backend/src/ir/printf.hpp b/backend/src/ir/printf.hpp

> > index cbab759..df58437 100644

> > --- a/backend/src/ir/printf.hpp

> > +++ b/backend/src/ir/printf.hpp

> > @@ -159,10 +159,7 @@ namespace gbe

> >          ptr = p;

> >        }

> >

> > -      ~PrintfSlot(void) {

> > -        if (ptr)

> > -          free(ptr);

> > -      }

> > +      ~PrintfSlot(void);

> >      };

> >

> >      class Context;

> > @@ -177,7 +174,7 @@ namespace gbe

> >          }

> >

> >          for (size_t i = 0; i < other.slots.size(); ++i) {

> > -          PrintfSlot* s = other.slots[i];

> > +          PrintfSlot s = other.slots[i];

> >            slots.push_back(s);

> >          }

> >

> > @@ -215,15 +212,15 @@ namespace gbe

> >        uint8_t getIndexBufBTI() const { return btiIndexBuf; }

> >

> >        uint32_t getPrintfBufferElementSize(uint32_t i) {

> > -        PrintfSlot* slot = slots[i];

> > +        PrintfSlot& slot = slots[i];

> >          int vec_num = 1;

> > -        if (slot->state->vector_n > 0) {

> > -          vec_num = slot->state->vector_n;

> > +        if (slot.state->vector_n > 0) {

> > +          vec_num = slot.state->vector_n;

> >          }

> >

> >          assert(vec_num > 0 && vec_num <= 16);

> >

> > -        switch (slot->state->conversion_specifier) {

> > +        switch (slot.state->conversion_specifier) {

> >            case PRINTF_CONVERSION_I:

> >            case PRINTF_CONVERSION_D:

> >            case PRINTF_CONVERSION_O:

> > @@ -257,7 +254,7 @@ namespace gbe

> >

> >      private:

> >        vector<PrintfFmt> fmts;

> > -      vector<PrintfSlot*> slots;

> > +      vector<PrintfSlot> slots;

> >        uint32_t sizeOfSize; // Total sizeof size.

> >        friend struct LockOutput;

> >        uint8_t btiBuf;

> > diff --git a/backend/src/llvm/llvm_printf_parser.cpp

> > b/backend/src/llvm/llvm_printf_parser.cpp

> > index 2f85443..3d84457 100644

> > --- a/backend/src/llvm/llvm_printf_parser.cpp

> > +++ b/backend/src/llvm/llvm_printf_parser.cpp

> > @@ -291,7 +291,7 @@ again:

> >  #if 0

> >      {

> >        int j = 0;

> > -      for (auto &s : *printf_fmt) {

> > +      for (auto &s : printf_fmt->first) {

> >          j++;

> >          if (s.type == PRINTF_SLOT_TYPE_STATE) {

> >            fprintf(stderr, "---- %d ---: state : \n", j);

> > --

> > 1.9.1

> >

> > _______________________________________________

> > Beignet mailing list

> > Beignet@lists.freedesktop.org

> > http://lists.freedesktop.org/mailman/listinfo/beignet

> 

> 

> _______________________________________________

> Beignet mailing list

> Beignet@lists.freedesktop.org

> http://lists.freedesktop.org/mailman/listinfo/beignet