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

[Yang, Rong R]

Pushed, thanks.

> -----Original Message-----
> From: Beignet [mailto:beignet-bounces at lists.freedesktop.org] On Behalf Of
> Junyan He
> Sent: Friday, August 14, 2015 14:28
> To: Luo, Xionghu; beignet at 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 at intel.com
> > Sent: Mon, 10 Aug 2015 15:40:13 +0800
> > To: beignet at lists.freedesktop.org
> > Subject: [Beignet] [Patch v2] GBE/PRINTF: store variable instead of
> > pointer in "slots".
> >
> > From: Luo Xionghu <xionghu.luo at 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 at 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 at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/beignet
> 
> 
> _______________________________________________
> Beignet mailing list
> Beignet at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/beignet