-
-
Notifications
You must be signed in to change notification settings - Fork 925
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
Conversation
It looks like MRI uses hidden instance vars for these kinds of internal fields. Is using layouts our best equivalent for this? cc: @nirvdrum |
@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 I noticed our NoMethodError currently shows |
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 |
@@ -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")) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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.
Currently I believe yes. There are many potential other ways to hide them, but that's the only currently implemented. |
Looks good, let's just wait for CI and then let's merge it. |
Please review the new layout usage here.