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

Fix issues discovered using Error Prone #4860

Merged
merged 1 commit into from Dec 5, 2017
Merged

Conversation

@nick-someone
Copy link
Contributor

@nick-someone nick-someone commented Nov 21, 2017

Hello!

As part of the investigation that led to #4844, I integrated Error Prone (http://errorprone.info) into the JRuby compiler. (See configuration in nick-someone@ae3a5cb).

This found a number of bugs (ranging in severity/nagginess). I'm happy to revert or change any of these, but all of them represent real defects:

RubyEnumerable - each_with_index19 is trivially infinitely-recursive. I assumed that each_with_index is appropriate
StructLayout/SymbolProc - hashCode() ends up RSHIFT'ing an int value by 32 bits, which always ends up resulting in 0. I changed to 16 bits, but if you needed a stable hash code, I can change to XOR 0
RipperContext/FullInterpreterContext - Array.toString() is unhelpful for error messages. Manually unrolled them with Arrays.toString.
RealClassGenerator - an exception was created and thrown on the floor in an exceptional condition. I threw it instead.
Pack - ulMask was an int constant left shifted by 56, but since it was an int, it shifts out to 0 instead of 0xfe000000.
SelectExecutor - Here, I don't have enough context to make the right changes, but the errorKeyList.contains() calls will never return true, since the file descriptor objects can't be contained in errorKeyList

@kares
Copy link
Member

@kares kares commented Nov 21, 2017

these are looking really good ... @headius let's have these for 9.1.15 shall we?
have tried find bugs + IDEA's analysis previously but the amount was just too much to find such real gems.

@enebo
Copy link
Member

@enebo enebo commented Nov 21, 2017

The ripper suggestion is wrong even though it did point out there was a bug there. These all look good except that one. That one should be something like:

StringBuilder buf = new StringBuilder();
buf.append((char) end);

@nglorioso change to that or omit that change otherwise I think it all looks good to me. Although I surprising bad at bit math stuff so the StructLayout change would be nice if someone else said that is ok.

Copy link
Member

@headius headius left a comment

About half reasonable changes, half unnecessary or incorrect.

@@ -658,7 +658,7 @@ public boolean equals(Object obj) {

@Override
public int hashCode() {
return 53 * 5 + (int) (this.offset ^ (this.offset >>> 32)) ^ type.hashCode();
return 53 * 5 + (int) (this.offset ^ (this.offset >>> 16)) ^ type.hashCode();

This comment has been minimized.

@headius

headius Nov 21, 2017
Member

This change fixes the offset int getting shifted to oblivion. However, I'd prefer to replace this with a hashCode generated by IntelliJ as follows:

diff --git a/core/src/main/java/org/jruby/ext/ffi/StructLayout.java b/core/src/main/java/org/jruby/ext/ffi/StructLayout.java
index f7e269d777..4d5bfdf0a8 100644
--- a/core/src/main/java/org/jruby/ext/ffi/StructLayout.java
+++ b/core/src/main/java/org/jruby/ext/ffi/StructLayout.java
@@ -658,7 +658,10 @@ public final class StructLayout extends Type {
 
         @Override
         public int hashCode() {
-            return 53 * 5 + (int) (this.offset ^ (this.offset >>> 32)) ^ type.hashCode();
+            int result = super.hashCode();
+            result = 31 * result + type.hashCode();
+            result = 31 * result + offset;
+            return result;
         }
 
         /**
@@ -146,7 +146,7 @@ public int parseString(RipperLexer lexer, LexerSource src) throws IOException {
}

private String parseRegexpFlags(RipperLexer lexer, LexerSource src) throws IOException {
StringBuilder buf = new StringBuilder(end);
StringBuilder buf = new StringBuilder();

This comment has been minimized.

@headius

headius Nov 21, 2017
Member

I don't understand this change.

This comment has been minimized.

@nick-someone

nick-someone Nov 21, 2017
Author Contributor

http://errorprone.info/bugpattern/StringBuilderInitWithChar

The prior version of the code pre-allocated the StringBuilder to the length of the int represented by the char. (Not starting pre-initialized to a single-character).

From the rest of the code, it doesn't appear that 'end' was intended to show up in the actual output, so I removed the eager preallocation here.

This comment has been minimized.

@enebo

enebo Nov 21, 2017
Member

I made a general comment in the PR about this. The result of that string ends up appended to another StringBuilder (which indicates perhaps this could get cleaned up) but 'end' should definitely be in the string. It was likely me assuming character was an acceptable constructor and because this was ported C code we saved the char as an int and confusion ensued.

This comment has been minimized.

@enebo

enebo Nov 21, 2017
Member

@nglorioso Actually just remove this. The existing code should be passing in the StringBuilder from the previous builder in. I will refactor this a tiny bit and that StringBuilder will go away.

This comment has been minimized.

@enebo

enebo Nov 21, 2017
Member

@nglorioso and tip of the hat for saying it is not in the output. We append the original StringBuilder to an outer StringBuilder but in the case of RegexpEnd end we throw out all that constructed output! Since it never used it did not matter what was in either of these strings.

This comment has been minimized.

@enebo

enebo Nov 21, 2017
Member

Addressed in 58cd133 on jruby-9.1 and cpd over to master.

@@ -188,6 +189,6 @@ public int determineRPC(int ipc) {
if (ipc <= rescueIPCs[i]) return rescueIPCs[i + 1];
}

throw new RuntimeException("BUG: no RPC found for " + getFileName() + ":" + getName() + ":" + ipc + getInstructions());
throw new RuntimeException("BUG: no RPC found for " + getFileName() + ":" + getName() + ":" + ipc + Arrays.toString(getInstructions()));

This comment has been minimized.

@headius

headius Nov 21, 2017
Member

This is wrong both ways. The array will just be a mangled Instr]@0x1234 sort of toString, and the Arrays version will include ALL of the instructions in the error message.

This comment has been minimized.

@nick-someone

nick-someone Nov 21, 2017
Author Contributor

Hrm. Perhaps do you want me to replace getInstructions() with rescueIPCs and Arrays.toString that?

This comment has been minimized.

@enebo

enebo Nov 21, 2017
Member

@nglorioso just remove getInstructions() altogether. Originally we only saw this while developing our new runtime so we probably never cared about anything other than ipc since we could just run the program again and look at the instructions on a second run. If we corrected this to print out all instrs I think users would complain about a gazillion lines of output.

This comment has been minimized.

@enebo

enebo Nov 21, 2017
Member

As an added note we should consider possibly changing this to be an hserr-like output in addition to notifying the user we hit an internal bug. We could then dump CFG, instrs, ipc, etc into a file so they could paste that into an issue (sorry for adding this to an incidental PR review but sometimes inspiration strikes!). I am not asking that here though :)

@@ -34,7 +34,7 @@ public Object createCacheObject(ThreadContext context) {

@Override
public int hashCode() {
return 47 * 7 + (int) (this.name.hashCode() ^ (this.encoding.hashCode() >>> 32));
return 47 * 7 + (int) (this.name.hashCode() ^ (this.encoding.hashCode() >>> 16));

This comment has been minimized.

@headius

headius Nov 21, 2017
Member

Same reason for fix and argument for generated hashCode as for StructLayout.

@@ -144,7 +144,7 @@ public static Class createRealImplClass(Class superClass, Class[] interfaces, Ru

Class newClass = defineRealImplClass(ruby, name, superClass, superTypeNames, simpleToAll);
if (!newClass.isAssignableFrom(interfaces[0])) {
new RuntimeException(newClass.getInterfaces()[0].getClassLoader() + " " + interfaces[0].getClassLoader());
throw new RuntimeException(newClass.getInterfaces()[0].getClassLoader() + " " + interfaces[0].getClassLoader());

This comment has been minimized.

@headius

headius Nov 21, 2017
Member

This should never happen anyway, but it's a good change.

This comment has been minimized.

@nick-someone

nick-someone Nov 21, 2017
Author Contributor

Looks like there a number of failures on the Travis build that are triggered by this change: https://travis-ci.org/jruby/jruby/builds/305072247

Haven't dug into this, but I can revert this change if there are too many failing tests to deal with.

This comment has been minimized.

@nick-someone

nick-someone Nov 26, 2017
Author Contributor

Still waiting on this. Can revert the change to not shake up the CI, or leave it in :)

This comment has been minimized.

@headius

headius Nov 27, 2017
Member

Revert this change and file a bug about this useless line.

This comment has been minimized.

@nick-someone

nick-someone Dec 1, 2017
Author Contributor

#4880 filed

@@ -209,6 +209,7 @@ IRubyObject selectInternal(ThreadContext context) throws IOException {
RubyIO io = TypeConverter.ioGetIO(runtime, obj);
RubyIO write_io = io.GetWriteIO();
fptr = io.getOpenFileChecked();
/* BUG: errorKeyList (SelectionKey) can't contain fptr.fd() (ChannelFD) */

This comment has been minimized.

@headius

headius Nov 21, 2017
Member

This should be excluded. I suppose it objects to a comment starting with "BUG".

This comment has been minimized.

@nick-someone

nick-someone Nov 21, 2017
Author Contributor

Yeah, sorry, should have cleared up during the PR comment.

There exists a bug here, and I wanted to mark it. I'm happy to remove the comment from this PR. Should I file a distinct issue to look into this?

This comment has been minimized.

@nick-someone

nick-someone Nov 26, 2017
Author Contributor

FIled #4867 for this.

This comment has been minimized.

@headius

headius Nov 27, 2017
Member

Thank you. Remove this from the PR and we'll discuss it separately.

Copy link
Member

@headius headius left a comment

Might as well squash all these into one commit, but it looks good to me.

Copy link
Member

@headius headius left a comment

Few more tweaks and we can merge.

@@ -144,7 +144,7 @@ public static Class createRealImplClass(Class superClass, Class[] interfaces, Ru

Class newClass = defineRealImplClass(ruby, name, superClass, superTypeNames, simpleToAll);
if (!newClass.isAssignableFrom(interfaces[0])) {
new RuntimeException(newClass.getInterfaces()[0].getClassLoader() + " " + interfaces[0].getClassLoader());
throw new RuntimeException(newClass.getInterfaces()[0].getClassLoader() + " " + interfaces[0].getClassLoader());

This comment has been minimized.

@headius

headius Nov 27, 2017
Member

Revert this change and file a bug about this useless line.

@@ -2349,7 +2350,7 @@ public boolean tokenAddMBC(int first_byte, ByteList buffer) {
int length = precise_mbclen();

if (length <= 0) {
compile_error("invalid multibyte char (" + getEncoding().getName() + ")");
compile_error("invalid multibyte char (" + Arrays.toString(getEncoding().getName()) + ")");

This comment has been minimized.

@headius

headius Nov 27, 2017
Member

I didn't notice this before, but this should just be getEncoding(). The default toString for Encoding is to return the name of the encoding.

@@ -209,6 +209,7 @@ IRubyObject selectInternal(ThreadContext context) throws IOException {
RubyIO io = TypeConverter.ioGetIO(runtime, obj);
RubyIO write_io = io.GetWriteIO();
fptr = io.getOpenFileChecked();
/* BUG: errorKeyList (SelectionKey) can't contain fptr.fd() (ChannelFD) */

This comment has been minimized.

@headius

headius Nov 27, 2017
Member

Thank you. Remove this from the PR and we'll discuss it separately.

@nick-someone nick-someone force-pushed the nick-someone:bugfixes branch from d36dae7 to 0e959fe Dec 1, 2017
@nick-someone
Copy link
Contributor Author

@nick-someone nick-someone commented Dec 1, 2017

Thanks! Extraneous things removed, should be good to merge (assuming no other CI failures)

@headius headius merged commit 0e959fe into jruby:master Dec 5, 2017
1 check failed
1 check failed
continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
@headius
Copy link
Member

@headius headius commented Dec 5, 2017

Thanks for your patience and help!

@nick-someone nick-someone deleted the nick-someone:bugfixes branch Dec 5, 2017
@nick-someone
Copy link
Contributor Author

@nick-someone nick-someone commented Dec 5, 2017

Thank you! Glad to help improve things :)

@enebo enebo added this to the JRuby 9.1.15.0 milestone Dec 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.