Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Truffle] Add SystemCallError Layout for errno field #4005

Merged
merged 4 commits into from
Jul 11, 2016

Conversation

bjfish
Copy link
Contributor

@bjfish bjfish commented Jul 9, 2016

Please review the new layout usage here.

@bjfish
Copy link
Contributor Author

bjfish commented Jul 9, 2016

It looks like MRI uses hidden instance vars for these kinds of internal fields. Is using layouts our best equivalent for this?

cc: @nirvdrum

@bjfish
Copy link
Contributor Author

bjfish commented Jul 9, 2016

@jruby/truffle Would it be nice if we supported the same thing as MRI for hidden instance vars? I think (please confirm) in MRI if you omit the @ symbol rb_ivar_set("errno", 1) the instance var becomes hidden from things like instance_variables and instance_variable_get/set. Or is there a better approach and I should use layouts like this pull request?

I noticed our NoMethodError currently shows @name instance var in instance_variables when it should be hidden. It is defined as noMethodError.define("@name", context.getSymbolTable().getSymbol(name), 0);

@chrisseaton
Copy link
Contributor

You should use a layout if you need to be able to access the fields from Java without too much hassle.

You should use a HiddenKey field name, rather than a j.l.String, to produce the kind of hidden instance variables you're after.

@@ -1827,6 +1828,12 @@ public RubyNode visitInstAsgnNode(org.jruby.ast.InstAsgnNode node) {
ret = new WriteInstanceVariableNode(context, sourceSection, name, self, IntegerCastNodeGen.create(context, sourceSection, rhs));
return addNewlineIfNeeded(node, ret);
}
} else if (path.equals(corePath + "exception.rb")) {
if (name.equals("@errno")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we just modify exception.rb in place to use a primitive to read the error, rather than having to rewrite here?

Copy link
Contributor Author

@bjfish bjfish Jul 9, 2016

Choose a reason for hiding this comment

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

@chrisseaton I've updated this to use a primitive now

Copy link
Contributor

Choose a reason for hiding this comment

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

This all looks good then.

@@ -148,8 +149,14 @@ public DynamicObject allocate(DynamicObject rubyClass) {
public static abstract class ExceptionErrnoErrorPrimitiveNode extends PrimitiveArrayArgumentsNode {

@Specialization
public DynamicObject exceptionErrnoError(DynamicObject message, int errno) {
return coreExceptions().errnoError(errno, message.toString(), this);
public DynamicObject exceptionErrnoError(DynamicObject message, int errno, DynamicObject location) {
Copy link
Member

Choose a reason for hiding this comment

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

This now needs a @TruffleBoundary since it uses String concatenation.

@eregon
Copy link
Member

eregon commented Jul 11, 2016

It looks like MRI uses hidden instance vars for these kinds of internal fields. Is using layouts our best equivalent for this?

Currently I believe yes. There are many potential other ways to hide them, but that's the only currently implemented. noMethodError.define("@name", ... is pretty slow so the layout is better perf-wise.

@eregon
Copy link
Member

eregon commented Jul 11, 2016

Looks good, let's just wait for CI and then let's merge it.

@eregon eregon merged commit b865a52 into master Jul 11, 2016
@eregon eregon deleted the truffle-system-call-error branch July 11, 2016 19:33
@enebo enebo modified the milestone: truffle-dev Aug 26, 2016
@enebo enebo added this to the Invalid or Duplicate milestone Dec 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants