Review Board

1.0

LanguageKit Interpreter

Updated 10 months, 3 weeks ago

Eric Wasylishen Reviewers
EtoileCore
TheRaven
None Etoile trunk (etoile/trunk/Etoile) 
Adds an interpreter to LanguageKit. -i option for edlc forces the use of the interpreter, otherwise it is only used if no code generator is available.
On OS X, passes entire test suite except for TestRetainOnlyOnce (expected), TestPrototypes (prototypes not ported to OS X), TestMutRecursiveClassDefs (not an interpreter problem).
A few tests fail due to formatting but should actually pass; I'm planning to fix these tests at some point.
Review request changed
Updated 10 months, 3 weeks ago (October 18th, 2009, 12:24 a.m.)
Tweaks to make it build on GNUstep. objc_registerClassPair() still doesn't seem to work, but the interpreter builds and doesn't break LK on GNUstep.
David Chisnall
Posted 10 months, 3 weeks ago (October 18th, 2009, 4:05 a.m.)
Looks okay overall.  A few things I'm a bit concerned about; for example I don't really like how it's been jammed into LKCompiler and you seem to have changed the ABI for BlockContext without making the (very large number of) changes required to support this in all of the rest of the code that depends on it.  

Global comment about formatting:

The rationale between the space between type names and *s in declarations is that the * applies to the object being declared, not the type.  For example, this is ambiguous:

type* pointer, pointee;

While this is easy to read:

type *pointer, pointee;

This does not apply in cast expressions, because the * is part of the type, so:

pointer = (type*)pointee;

is more readable.

Also, make sure you have space after colons, but no spaces after the close bracket in cast expressions (including method declarations), for example:

- (id)method;
not
- (id) method;
/Languages/Compiler/GNUmakefile (Diff revision 3) 
 
 
Make LanguageKit link against the interpreter.  It should be small enough not to cause too much bloat.  It's not like LLVM, which pulls in a 300MB library...
/Languages/Compiler/main.m (Diff revision 3) 
 
 
This will attempt to create the default JIT even when interpret is set, which will load the huge LLVM bundle.
/Languages/Compiler/main.m (Diff revision 3) 
 
 
Why have you removed this?
You've put this in a few places, and it's not quite right because we also want to be using objc/runtime.h when we are using the GNUstep runtime (but not the GCC runtime) so can you factor this out into a local header so I only need to fix it in one place later?
Make this and the next function static if they are only used in this file.  If they are not, give them a sensible prefix (LK, or LKI)
Have you tested what happens if:

LK code passes block to ObjC code.
ObjC code calls Block in an @try{} block.
Block does non-local return.

This works in the compiler, but it looks like the interpreter will have problems here.
[self subclassResponsibility: _cmd];
This doesn't seem to be actually constructing the NSArray instance ever...
Does this handle the case where the ivar is not an object, or it is an object but the rvalue is a SmallInt?  I think you need some auto boxing...
Please remove spaces between type name and * in cast expressions, also remove space between (id) and executeWithArguments (here and elsewhere).  
Should this not be autoreleased?
Why not NSSelectorFromString?
Why?
No SmallInts?
Missing break statement.  Statements after return statement should not be reached.
You also need to call the +load method in the new class here. 
I'd prefer LKInterpretedBlockClosure for this class name.
This looks like you copied it from CodeGen.  Is it possible for you to factor it out so that it can be used by both?  Maybe move it into an LKTypeHelpers file in LK?
Yes it is, please fix it.
Not correct.  What happens if you subclass an interpreted class and then override a method which calls super?  Message sends to super need the receiving class to be encoded at compile time, not looked up at run time.
Spaces after colons.
Please don't overload these like this.  Either produce a CodeGenerator for the interpreter, or provide a second method for the interpreter.
Why is this needed?  appendString: nil should append (nil) automatically.
/Languages/LanguageKit/LKModule.m (Diff revision 3) 
 
 
I think this is actually wrong on 64-bit platforms.  We should fix it by looking up the type of NSArray's -count.
Why the change?  OS X can still parse NeXT-style property lists, and they're easier to read.
Are you sure about this?  won't that iterate once if you loop from n to n, when it should iterate zero times?
No, this code should be in BlockClosure.  It is required for non-local returns and retaining blocks to work, and has nothing to do with debugging.
Why is this removed?
objects[0] should always be self.  If not, the block is invalid.  
Should be i=1.
No.  The symbol table array has one fewer element than the count.  Object 0 is always the self pointer and so does not have an entry in the symbol table.
Wait, what?

If you have changed count to mean something different then this is going to badly (really badly) break the compiler in some horrible ways.  
Where did this code go?
Review request changed
Updated 10 months, 3 weeks ago (October 22nd, 2009, 1:08 a.m.)
Try to fix all of problems theraven found.
Ship it!
David Chisnall
Posted 10 months, 3 weeks ago (October 22nd, 2009, 4:44 a.m.)
Looks mostly okay.  A few bits in BlockClosure I'm not convinced by, but commit when the remaining things are fixed.
I can't see where you are accessing this outside of this file.  If it's nowhere, please don't make it public.  If it's somewhere, then can you expose it as a class / singleton method somewhere so that Smalltalk code can find it?
Space between switch and (
Somewhere define:

NSString *LKInterpreterException = @"LKInterpreterException";

And add in the header:

extern NSString *LKInterpreterException;

There are some macros somewhere in LK for doing this.  Feel free to use them.
Not a priority, but at some point it would be nice to add a category on NSString that provided these functions, so you can use them in Smalltalk easily.
Ah, there is is.  Okay, keep this LK-prefixed.  Ignore my other comment.
/Languages/LanguageKit/Runtime/BigInt.m (Diff revisions 3 - 4) 
 
 
This is a bit of a hack.  I'd rather the interpreter used SmallInts for things that will fit in a pointer and boxed / unboxed them as required.  Sending BigInt messages for everything will be very slow.  You can compile MsgSendSmallInt.m into LK to make these functions accessible to the interpreter.
Why is this here and in the other place?
If you are retaining the values, then this should be a category on RetainedStackContext, not on BlockContext.
Please revert these.  They shouldn't actually be retaining here, they should be retaining only in a retained stack context.  There are a few little memory management bugs like this, in fact, that need fixing in the compiler.
What is this?