Reimplement getMethodFromName around other queries

[jdmpapin]

This series reimplements getMethodFromName in terms of getSystemClassFromClassName and getMethodFromClass in order to eliminate the unnecessary MethodByName validation kind.

[dsouzai]

One question, if the callingClass is optional, why do we need to specify it in the validation record at all?

[jdmpapin]

If provided, it's used for testing visibility

[dsouzai]

As I'm not an expert on the Java semantics, @andrewcraik could you give this a once over?

[dsouzai]

If provided, it's used for testing visibility

Is it possible during the validation run where previously we would fail the AOT load when getClassFromID returned NULL, now because it's optional we might incorrectly get back NULL and not fail the load?

[andrewcraik]

I'll look tomorrow morning when I have time to go through it properly - I'm going to launch sanity now in the hope the review doesn't require major changes.

[andrewcraik]

Jenkins test sanity xlinux,win,plinux jdk8,jdk11

[dsouzai]

TOTAL: 211   EXECUTED: 134   PASSED: 134   FAILED: 0   SKIPPED: 77
ALL TESTS PASSED

No idea why the xlinux jdk11 build shows as failed :S. I asked the question here https://openj9.slack.com/archives/C8312LCV9/p1546610068034700

[jdmpapin]

If provided, it's used for testing visibility

Is it possible during the validation run where previously we would fail the AOT load when getClassFromID returned NULL, now because it's optional we might incorrectly get back NULL and not fail the load?

We would only fail the load as a quiet failure of the assertion that prevents unexpected null return values, but e.g. debug builds would crash. It's not a legitimate situation that requires load failure.

We can get null only if beholderID is the null ID, i.e. if at compile time the callingClass was null. No other ID can become null, since that would collide with the null ID. Any (earlier) validation that tried to set the value of another ID to null would have already failed the load beforehand.

[dsouzai]

We can get null only if beholderID is the null ID, i.e. if at compile time the callingClass was null.

I see, that makes sense; I got confused because of the SymOptional which evoked the idea that the SVM could return NULL if an ID wasn't found; but taking a look at SymbolValidationManager::getSymbolFromID, an ID that wasn't found will result in the appropriate failure via the SVM_ASSERT.

[dsouzai]

I'm ok to merge this once @andrewcraik takes a look at the refactoring done for TR_J9VM::getMethodFromName

[andrewcraik]

So I'm fine with this - there is a capability reduction in that the current code can find a non-system loader from the constant pool and use that to look up a class while the new implementation only uses the system loader. That being said the types we lookup by name are loaded by the system loader since they are only core JCL types. Further, the lookup using the non-system loader falls back to the system loader so you don't get an accurate result for a lookup anyway (you will miss some failures) so the non-system case is not correct anyway.

I'm fine with this change - if we need the non-system loader case it deserves a different API anyway.

[andrewcraik]

LGTM