Couple of POSSIBLE Cog fixes [WAS] Re: [Vm-dev] [COG] primitiveClosureValue assert is failing with StackVM (possible BUG in bytecodes!!)

[David T. Lewis]

On Thu, Dec 23, 2010 at 01:52:15PM +0100, Mariano Martinez Peck wrote:
>  
> On Wed, Dec 22, 2010 at 10:34 PM, Eliot Miranda <eliot.miranda at gmail.com>wrote:
> 
> >
> > Hi Mariano,
> >
> >     indeed it's a bug.  Your fix (objectMemory is: rcvr instanceOf: (self
> > splObj: ClassBlockClosure) compactClassIndex:
> > ClassBlockClosureCompactIndex) is the correct one.  There are a few other
> > places where this needs to be done also.  I'll roll out new code soon.
> >
> >
> Hi Eliot. Before you roll out new code, I would like to share with you some
> "fixes" I have to do to Cog, mostly to improve the support of SmallInteger
> as methods. I attach a .cs but still, some need some explanations. Don't
> expect them to be the better solution/fix, but hopefully their are useful to
> show you a bug/problem and you may come with a better solution.
> 
> So, some notes about the changes:
> 
> - #primitiveIndexOf:   now answers 0 if it is not a ooCompiledMethod.
> Otherwise, it crash. This method is called by, for example,
> #primitiveFlushCacheByMethod. If I want to use SmallInteger as methods, I
> need to implement #flushCache and call the primitive. But this crashes if we
> don't apply this change.
> 
> - Igor have changed CoInterpreter >> readImageFromFile: f HeapSize:
> desiredHeapSize StartingAt: imageOffset
> and added a <var: #desiredHeapSize type: 'usqInt'>. With this we can compile
> the MacOS platform code.
> However, the same change need to be applied to StackInterpreter.

The #readImageFromFile:HeapSize:StartingAt: patch is correct. John
originally provided the fix, and it should go into oscog also. But
I would expect this to cause problems only when compiling for 64 bit
pointers, so it should not really affect Cog right now (?). From
the MC change history:

  Name: VMMaker-dtl.165
  Author: dtl
  Time: 28 March 2010, 6:54:54 pm
  UUID: e7d7d22f-035f-46f2-ad5b-cda5b704a5aa
  Ancestors: VMMaker-dtl.164
  
  Fix declaration of readImageFromFile:HeapSize:StartingAt: as per John's note
  at http://lists.squeakfoundation.org/pipermail/vm-dev/2010-January/003614.html

Dave

> 
> - In #activateInterpreterMethodFromMachineCode  the assert "self assert:
> (self primitiveIndexOf: newMethod) ~= 0"  fails when using Objects as
> methods, since "primitiveFunctionPointer" is different than zero, but
> #primitiveIndexOf:  is zero. Check #addNewMethodToCache:  to see where the
> #primitiveInvokeObjectAsMethod  is set
> FOr this problem, there is probably a better way to solve my ugly hack. But
> at least, the assert doesn't fail ;)
> 
> - #methodHasCogMethod:   I am not sure about this one. What do you think?
> Because I have some crashes but I cannot garuantee 100% it was because of
> that.
> 
> - #primitiveFlushCacheByMethod  needs to check if the method is really a
> method, because it can be a SmallINteger or any other object
> 
> That's all.
> 
> Cheers
> 
> Mariano
> 
> 
> 
> > best
> > Eliot
> >
> > On Wed, Dec 22, 2010 at 12:03 PM, Mariano Martinez Peck <
> > marianopeck at gmail.com> wrote:
> >
> >>
> >> Hi Eliot. If I compile a StackVM, and I just press the left arrow in a
> >> code pane of a OB windows, in a pharo image, I get an asser that fails:
> >> (oop & 1) 19202
> >>
> >> Sorry I bother with these asserts, but I am trying to solve the crashes I
> >> have and I think starting for the asserts is a good idea.
> >>
> >> The assert that fails is in primitiveClosureValue()  the part:
> >>
> >> blockClosure = longAt(GIV(stackPointer) + (GIV(argumentCount) *
> >> BytesPerWord));
> >>     /* begin quickFetchInteger:ofObject: */
> >>     oop = longAt((blockClosure + BaseHeaderSize) + (ClosureNumArgsIndex <<
> >> ShiftForWord));
> >>     assert((oop & 1));
> >>     numArgs = (oop >> 1);
> >>     if (!(GIV(argumentCount) == numArgs)) {
> >>         /* begin primitiveFail */
> >>         if (GIV(primFailCode) == 0) {
> >>             GIV(primFailCode) = 1;
> >>         }
> >>         return;
> >>     }
> >>
> >>
> >> numArgs results to be a large number (using #printNum:   was 718613205),
> >> and if I try to do a #printOop:  I got a " 0x55aa55aa0x55aa55aa is not on
> >> the heap"
> >>
> >> Now, of course, the "if (!(GIV(argumentCount) == numArgs)) " fails.
> >>
> >> What it is funny is that I did a #printOop: of blockClosure instVar, and
> >> this is what I get:  0x1f59b578: a(n) Association
> >>
> >> Association? WTF ?   so...I guess something weird is happening. I continue
> >> debugging it..and I noticed that:
> >>
> >> - Several classes were called in addition to Association,like Character.
> >> These seems to be compact classes and implement #value and #value:   ;)
> >> - Both, #bytecodePrimValue and #bytecodePrimValueWithArg  were not calling
> >> #primitiveClosureValue. So, "isBlock" was false...
> >>
> >> I tried modifying #bytecodePrimValueWithArg and #bytecodePrimValue  this
> >> line:
> >>
> >> isBlock := objectMemory is: rcvr instanceOf: ClassBlockClosurecompactClassIndex: ClassBlockClosureCompactIndex.
> >>
> >> to this one:
> >>
> >> isBlock := objectMemory is: rcvr instanceOf: (self splObj:
> >> ClassBlockClosure) compactClassIndex: ClassBlockClosureCompactIndex.
> >>
> >> WIth this change, at least, the assert doesn't fail anymore. I made
> >> progress (maybe). But still, for normal closures, "isBlock" seems to be
> >> false. So.....all block values are being managed as a normal send ????
> >>
> >> Now, I don't understand why ClassBlockClosureCompactIndex  is 0. I think
> >> this may be the problem. Why it is not 12? (in Pharo  Smalltalk
> >> compactClassesArray at: 12 ->>> BlockClosure.)
> >> I notice that ObjectMemory >> initializeCompactClassIndices  does:
> >>
> >>     ClassBlockClosureCompactIndex := 0 "12". "Prospective.  Currently
> >> TranslatedMethod class"
> >>
> >>
> >> Notice that this ZERO changes the sematic of #is: oop instanceOf: classOop
> >> compactClassIndex: compactClassIndex
> >> called from the #bytecodePrimValue and bytecodePrimValueWithArg
> >>
> >> Finally, I changed the mentioned line to this:
> >>
> >> isBlock := objectMemory is: rcvr instanceOf: (self splObj:
> >> ClassBlockClosure) compactClassIndex: 12.
> >>
> >> and finally, with block closures, it is calling the primitive, and for
> >> Association it doesn't fail the assert.  ahh and the image doesn't crash ;)
> >>
> >> I don't know if this is a bug, if my solution was ok, nor it impact.
> >> Please let me know.
> >>
> >> Cheers
> >>
> >> Mariano
> >>
> >>
> >>
> >>
> >>
> >>
> >
> >