Skip to content

Commit

Permalink
Fix a subtle bug. Since block methods can be defined in different cla…
Browse files Browse the repository at this point in the history
…sses depending on the count of their arguments we must take this into consideration for guarding dispatch chains depending on blocks
  • Loading branch information
charig committed Jul 27, 2016
1 parent f91a370 commit da10f5f
Showing 1 changed file with 19 additions and 0 deletions.
19 changes: 19 additions & 0 deletions src/som/interpreter/nodes/dispatch/DispatchGuard.java
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,10 @@ public static DispatchGuard create(final Object obj) {
if (obj instanceof DynamicObject) {
return new CheckSObject(((DynamicObject) obj).getShape());
}

if (obj instanceof SBlock) {
return new CheckSBlock(((SBlock) obj).getSOMClass());
}

return new CheckClass(obj.getClass());
}
Expand Down Expand Up @@ -88,4 +92,19 @@ public boolean entryMatches(final Object obj) throws InvalidAssumptionException
((DynamicObject) obj).getShape() == expected;
}
}

private static final class CheckSBlock extends DispatchGuard {
private final DynamicObject expected;

public CheckSBlock(final DynamicObject blockClass) {
this.expected = blockClass;
}

@Override
public boolean entryMatches(final Object obj) throws InvalidAssumptionException {

return obj instanceof SBlock &&
((SBlock) obj).getSOMClass() == expected;
}
}
}

5 comments on commit da10f5f

@charig
Copy link
Owner Author

@charig charig commented on da10f5f Jul 27, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@smarr I needed to redefine in each block the numArgs method. I could have done it in the Block superclass and reflectively look for the name of the actual class. But then i saw there are other methods like value: which is redefined in every subclass. If the dispatch guard do not consider the different blocks the system misbehaves on sites where two different blocks call that method. So i guess that although subtle and probably not experienced until now, this fix is also valid for TruffleSOM. In the mid-term, I think blocks need a refactoring to avoid this issues. Probably they should also become DynamicObjects, but meanwhile i think this is needed.

@smarr
Copy link

@smarr smarr commented on da10f5f Jul 27, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand a word.
What can go wrong?

Having blocks with different arity at a single site of a #value (etc) send?

@charig
Copy link
Owner Author

@charig charig commented on da10f5f Jul 27, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. At a single site for any message. The guards are not considering the block arity.

@smarr
Copy link

@smarr smarr commented on da10f5f Jul 27, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it when I see a failing test ;)

@charig
Copy link
Owner Author

@charig charig commented on da10f5f Jul 27, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here the test.
You find the numArgs method already in Block1 and Block2. Actually this was hard to catch because it is just misbehaving (calling the wrong method) but not failing.
The last line of the test is commented because it is capturing a similar issue but for the value selector which is specialized with a primitive. In this case the code just fails because of the wrong number of arguments and i haven't deal with it yet.

Please sign in to comment.