Skip to content

add explicitly loaded classes to reachability analysis#27

Merged
pettermahlen merged 4 commits intospotify:masterfrom
pettermahlen:verify-loaded-classes
Sep 16, 2015
Merged

add explicitly loaded classes to reachability analysis#27
pettermahlen merged 4 commits intospotify:masterfrom
pettermahlen:verify-loaded-classes

Conversation

@pettermahlen
Copy link
Member

Fixes #25

@pettermahlen
Copy link
Member Author

@mattnworb @spkrka what do you think?

@mattnworb
Copy link
Member

I'm confused - is the ldc instruction similar to a class implementing an interface or extending a superclass?

@pettermahlen
Copy link
Member Author

The relevant part of the specification is here: http://docs.oracle.com/javase/specs/jvms/se8/html/jvms-6.html#jvms-6.5.ldc. Basically, if ldc is invoked with a symbolic reference to a class, that class is resolved. This PR ensures that missinglink can detect failures to resolve classes that way. An example in code of how that happens is shown in #25, and in the system test that is added by the second commit. missinglink 0.1.1 fails to find the error in that test, but with this PR, it does find it.

@mattnworb
Copy link
Member

Thanks for the explanation. (I noticed for some reason Github doesn't send emails to notify me of new issues or PRs until you mention me , that is annoying). This is a good catch of something we missed.

I'm not sure though if I agree or understand the change in ConflictChecker that is a result of treating "parent classes" and "loaded classes" as the same thing - that bit of code is trying to find which class defines a method that is called on SomeSubClass and walks the inheritance chain of that class. But a class loaded by ldc is not necessarily a superclass of the class, is it? This seems like it might introduce false negatives where previously we were detecting a conflict - for example:

  • Foo invokes a method x() on an instance of SomeSubClass which extends TheSuperClass
  • SomeSubClass and TheSuperClass are defined in different jars.
  • x() used to be defined in TheSuperClass but due to version changes no longer is, and the version of the jar containing SomeSubClass was built against the old version
  • At some point, SomeSubClass has a ldc instruction that references AnotherClass
  • AnotherClass coincidentally has a method named x()

It seems like ConflictChecker would no longer detect this. Or am I misunderstanding something?

@pettermahlen
Copy link
Member Author

I think you're on to something, I didn't notice that the parents were traversed looking for method declarations, so I thought the concept was more general. Good point, I'll fix!

@pettermahlen
Copy link
Member Author

I've 'fixed it a bit', but would like to add some more tests around this feature before merging. Not sure when I will be able to find the time for that, though.

@pettermahlen
Copy link
Member Author

@mattnworb @spkrka how does this look? I'm personally unhappy with how hard it is to test in particular the ConflictChecker and ClassLoader. I'd like to do some refactoring or add some testing harness to simplify that at some point.

@mattnworb
Copy link
Member

Separating out parents and loadedClasses looks good to me.

Regarding testing, I wonder if at a certain point it would be easier to generate classfiles by hand or some other process other than javac to get the sort of "invalid" instructions we want to exit in the test.

pettermahlen added a commit that referenced this pull request Sep 16, 2015
add explicitly loaded classes to reachability analysis
@pettermahlen pettermahlen merged commit 4e7aaa1 into spotify:master Sep 16, 2015
@pettermahlen pettermahlen deleted the verify-loaded-classes branch September 16, 2015 19:13
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.

2 participants