[RQt-devel] Latest update breaks qtbase

[Justin Talbot]

That sounds about right. Thanks for figuring this all out.

Justin

On Thu, Nov 19, 2009 at 3:15 PM, Michael Lawrence <lawremi at gmail.com> wrote:
>
>
> On Thu, Nov 19, 2009 at 12:17 PM, Justin Talbot <justintalbot at gmail.com>
> wrote:
>>
>> I think this is the right fix. I was just playing with this.
>>
>> marshal_QString and marshal_QByteArray both have checks that look like:
>>
>> if(m->cleanup() || m->type().isStack())
>>
>> so the logic in cleanup() is ignored if the variable is on the stack.
>>
>> The check for isStack() should be folded into the cleanup() function
>> where we should:
>> * trigger clean up if it's a temporary object on the stack
>> * but not cleanup if the object is in an R slot invocation, since
>> that's never on the stack despite what the method signature says
>>
>
> I'm not sure if I completely understand those two bullets. Let's just lay
> out the various scenarios and how memory is managed. In the below, we can
> think of "callbacks" as invocation of R slots or invocation of R virtual
> overrides.
>
> Foreign method invocations:
>
>   Parameters:
>
> Can just allocate on stack, but often allocate on heap, because this shares
> code with marshalling return values from callbacks. Thus, cleanup() should
> return TRUE here.
>
>   Return values:
>
> For smoke, always allocated on the heap, even if the type is not a pointer
> (and so is assumed to be "on the stack"). For Moc, there's only about 5
> cases out of 450 where a slot returns something that will not fit in a
> Smoke::StackItem (mostly from the dbus module), but in those cases we will
> need to allocate the memory ourselves, somehow. Anyway, "stack" types
> returned from a foreign method should be cleaned up.
>
> R callbacks:
>
>   Parameters:
>
> These are never owned by us, never free them.
>
>   Return values:
>
> We need to allocate on the heap anything that does not fit into a
> Smoke::StackItem. Smoke is smart enough to free these. Again, need to
> special-case for Moc stuff (e.g. return from slot, but very rare).
>
> So in summary, we need to cleanup whenever the type does not fit into a
> Smoke::StackItem AND (either we are passing parameters from R to Smoke (the
> current behavior) OR when we receive a return value from Smoke). The "does
> not fit into a StackItem" includes QString, QList, etc, basically any
> non-primitive type.
>
> Make sense?
>
> Michael
>
>> Justin
>>
>>
>> On Thu, Nov 19, 2009 at 11:50 AM, Michael Lawrence <lawremi at gmail.com>
>> wrote:
>> > I was just thinking that this problem is not specific to R slot
>> > invocations.
>> > It also applies to calling R virtual method overrides (from Smoke).
>> >
>> > Perhaps an easy way to solve this is to modify the logic of
>> > MethodCall::cleanup(). Right now, this only returns true when we are
>> > passing
>> > arguments to a foreign method, i.e. when we need to clean up any
>> > temporarily
>> > allocated objects. But we also have to clean up for Smoke whenever
>> > something
>> > is *returned* on the stack (but not when passed via an argument to a
>> > slot/virtual). Every time Deepayan executes QGraphicsScene::items,
>> > memory is
>> > leaked, because Smoke dynamically allocates the QList, as Justin
>> > mentioned.
>> >
>> > Would this work Justin or am I missing something big here?
>> >
>> > Michael
>> >
>> > On Thu, Nov 12, 2009 at 7:39 AM, Justin Talbot <justintalbot at gmail.com>
>> > wrote:
>> >>
>> >> The problem I fixed is actually 2 closely related problems:
>> >>
>> >> 1. Smoke memory leak
>> >> The smoke marshaling code expects to hold pointers to object
>> >> parameters and return types (StackItem in smoke.h)
>> >> But, a number of QT functions return objects on the stack.
>> >> In such cases, the smoke wrapper code (in all the x_* files) creates a
>> >> copy of the return object on the heap and sticks it in a StackItem.
>> >> This creates a memory leak since no one owns this new pointer.
>> >>
>> >> The memory leak was hackily fixed for QString and QByteArray return
>> >> types (see this discussion
>> >> http://markmail.org/message/rvilkkjb5zqvrzxh). In marshal_QString and
>> >> marshal_QByteArray in type-handlers.cpp the code checks if the passed
>> >> in parameter was allocated on the stack and deletes it. The memory
>> >> leak will occur on any other return type.
>> >>
>> >> 2. Dynamic slots
>> >> When we call a dynamic slot, we assume that we can marshal the
>> >> parameters using the QMetaMethod type signature.
>> >> QMetaMethod does not distinguish between value and reference value
>> >> parameters; it says all the parameters are on the stack.
>> >> When the parameters are marshaled to call the dynamic slot,
>> >> marshal_QString sees that the parameter is supposedly on the stack and
>> >> tries to delete it (due to the hack fix for the previous problem).
>> >> However, if the parameter was really a reference parameter it fails and
>> >> crashes.
>> >>
>> >> My fix to both problems adds a flag to StackItem indicating if
>> >> StackItem owns the contained pointer or not. If so, the marshal_*
>> >> functions will free the pointer when they are done with the StackItem.
>> >> When calling a dynamic slot through MocMethod I set the flag to false
>> >> since we don't know if the parameter is value or reference, so can't
>> >> safely delete it.
>> >>
>> >> Justin
>> >>
>> >> On Thu, Nov 12, 2009 at 7:04 AM, Michael Lawrence <lawremi at gmail.com>
>> >> wrote:
>> >> >
>> >> >
>> >> > On Thu, Nov 12, 2009 at 6:49 AM, Justin Talbot
>> >> > <justintalbot at gmail.com>
>> >> > wrote:
>> >> >>
>> >> >> I agree that the changes should be pushed upstream. If you prefer
>> >> >> not
>> >> >> to have them in our repository, I'll just hand you changelists to
>> >> >> look
>> >> >> at.
>> >> >>
>> >> >
>> >> > I guess it's fine to make quick-fixes, as long as we keep pushing
>> >> > them
>> >> > up.
>> >> >
>> >> >>
>> >> >> I can't build the smokegenerator tool right now, is that expected? I
>> >> >> browsed through the smokegenerator code and it appears to have the
>> >> >> same problem that kalyptus had, so I expect that my change will
>> >> >> still
>> >> >> be needed in some form.
>> >> >>
>> >> >
>> >> > The smokegenerator stuff builds for me, so please let me know what's
>> >> > up.
>> >> > If
>> >> > it's an easy fix, go ahead and make the necessary change in
>> >> > smokegenerator.
>> >> > Could you give a quick synopsis of what's wrong?
>> >> >
>> >> >>
>> >> >> Justin
>> >> >>
>> >> >> On Thu, Nov 12, 2009 at 6:41 AM, Michael Lawrence
>> >> >> <lawremi at gmail.com>
>> >> >> wrote:
>> >> >> > Thanks Justin. I didn't realize you were contributing so much;
>> >> >> > much
>> >> >> > appreciated.
>> >> >> >
>> >> >> > Btw, you should probably avoid modifying the stuff in the
>> >> >> > kdebindings
>> >> >> > directory. I'd like to keep that in sync with upstream. If there's
>> >> >> > a
>> >> >> > bug, we
>> >> >> > should probably file it with them.
>> >> >> >
>> >> >> > Thanks again,
>> >> >> > Michael
>> >> >> >
>> >> >> > On Thu, Nov 12, 2009 at 6:03 AM, Justin Talbot
>> >> >> > <justintalbot at gmail.com>
>> >> >> > wrote:
>> >> >> >>
>> >> >> >> Sorry about that. It's likely because you needed to clean and
>> >> >> >> then
>> >> >> >> reinstall qtbase to rebuild the smoke stuff. The build system
>> >> >> >> caches
>> >> >> >> a
>> >> >> >> number of intermediate files and they don't get rebuilt correctly
>> >> >> >> when
>> >> >> >> things change. I don't get a crash on my machine.
>> >> >> >>
>> >> >> >> I checked in a test case (qtbase/tests/slots.R) that crashed R
>> >> >> >> prior
>> >> >> >> to my fix. If it works with the new smokegenerator tool that's
>> >> >> >> great.
>> >> >> >>
>> >> >> >> Justin
>> >> >> >>
>> >> >> >> On Thu, Nov 12, 2009 at 5:39 AM, Michael Lawrence
>> >> >> >> <lawremi at gmail.com>
>> >> >> >> wrote:
>> >> >> >> > Not sure what Justin was doing there. I just made a major
>> >> >> >> > update
>> >> >> >> > though,
>> >> >> >> > moving from kalyptus to the new smokegenerator tool. This is
>> >> >> >> > likely
>> >> >> >> > to
>> >> >> >> > cause
>> >> >> >> > some breakage, but kalyptus is no longer maintained.
>> >> >> >> >
>> >> >> >> > On Thu, Nov 12, 2009 at 1:11 AM, Deepayan Sarkar
>> >> >> >> > <deepayan.sarkar at gmail.com>
>> >> >> >> > wrote:
>> >> >> >> >>
>> >> >> >> >> With the latest update (-r172), I reliably get
>> >> >> >> >>
>> >> >> >> >> > library(qtbase)
>> >> >> >> >> > Qt$QTextStream
>> >> >> >> >>
>> >> >> >> >>  *** caught segfault ***
>> >> >> >> >> address 0x8, cause 'memory not mapped'
>> >> >> >> >>
>> >> >> >> >> Traceback:
>> >> >> >> >>  1: .Call(qt_qenums, x)
>> >> >> >> >>  2: qenums(cl)
>> >> >> >> >>  3: qsmokeClass(lib, className)
>> >> >> >> >>  4: function () {    class <- qsmokeClass(lib, className)
>> >> >> >> >>  rm(list
>> >> >> >> >> =
>> >> >> >> >> className, envir = lib)    assign(className, class, lib)
>> >> >> >> >> lockBinding(className, lib)    class}()
>> >> >> >> >>
>> >> >> >> >>
>> >> >> >> >> From a cursory investigation, the offending change seems to be
>> >> >> >> >>
>> >> >> >> >> $ svn diff -r 171:172 src/MocStack.cpp
>> >> >> >> >> Index: src/MocStack.cpp
>> >> >> >> >>
>> >> >> >> >>
>> >> >> >> >> ===================================================================
>> >> >> >> >> --- src/MocStack.cpp    (revision 171)
>> >> >> >> >> +++ src/MocStack.cpp    (revision 172)
>> >> >> >> >> @@ -218,6 +218,8 @@
>> >> >> >> >>     }
>> >> >> >> >>     break;
>> >> >> >> >>   }
>> >> >> >> >> +  // We never own a pointer parameter passed through a
>> >> >> >> >> dynamic
>> >> >> >> >> slot
>> >> >> >> >> and thus should never try to delete it.
>> >> >> >> >> +  item[0].s_ownptr = false;
>> >> >> >> >>  }
>> >> >> >> >>  void MocStack::setSmoke(Smoke::Stack stack,
>> >> >> >> >> QVector<SmokeType>
>> >> >> >> >> types)
>> >> >> >> >>  {
>> >> >> >> >>
>> >> >> >> >> -Deepayan
>> >> >> >> >
>> >> >> >> >
>> >> >> >
>> >> >> >
>> >> >
>> >> >
>> >
>> >
>
>