Fix SVM Assert #4635
Fix SVM Assert #4635
Conversation
|
fyi @aviansie-ben |
|
At a quick glance the PR title does not match what the commit is doing. |
|
Heh yeah it's not immediately obvious, but i still have to write up proper commit messages, as well as a proper PR description (hence the WIP). |
dsouzai
force-pushed
the
fixAssertIssue
branch
from
a57c595 to
22cede6
Compare
Signed-off-by: Irwin D'Souza <dsouzai@ca.ibm.com>
When enabling SVM asserts for AOT w/ SVM, an assert will trigger during the relocation of an inlined site if its caller is a profiled inlined method who's site was disabled (as part of _its_ relocation). This is because, under the SVM, relocation of the inlined table entries are guaranteed (meaning, though the inlined site may be disabled, the J9Method pointer in the inlined table in the metadata will be updated to *not* be -1). However, when a profiled inlined method is relocated, it does not go through the same code path as other inlined method relocations, and so did not fix up the metadata when the site was disabled. This commit refactors the profiled inlined method relocation to use the class ID from the SVM. It also relocates the inlined table entry regardless of whether the inlined site can be activated. Signed-off-by: Irwin D'Souza <dsouzai@ca.ibm.com>
dsouzai
force-pushed
the
fixAssertIssue
branch
from
22cede6 to
892fe81
Compare
|
@jdmpapin could you please review? |
| @@ -2277,27 +2293,22 @@ TR_RelocationRecordInlinedMethod::inlinedSiteValid(TR_RelocationRuntime *reloRun | |||
| } | |||
| else | |||
| { | |||
| inlinedSiteIsValid = false; | |||
There was a problem hiding this comment.
Is there any particular significance in moving this line below the assertion? Just curious; it looks harmless
There was a problem hiding this comment.
Initially, I had moved the code that checks compileRomClass == currentRomClass into inlinedSiteCanBeActivated at whcih point the inlinedSiteIsValid = false; got changed to a return false, and so had to be moved after the check for SVM. It turns out commoning that code to also be used for profiled inlined methods didn't work (because the rom class stored in the relorecord was not the rom class of the method that was inlined), so I had to un-refactor the code. I guess that resulted in the inlinedSiteIsValid = false ended up after the if :).
| TR_OpaqueMethodBlock *inlinedMethod = * (TR_OpaqueMethodBlock **) (((uint8_t *)reloPrivateData->_inlinedCodeClass) + vTableSlot(reloTarget)); | ||
| inlinedMethod = * (TR_OpaqueMethodBlock **) (((uint8_t *)reloPrivateData->_inlinedCodeClass) + vTableSlot(reloTarget)); | ||
| if (!inlinedMethod) | ||
| inlinedSiteIsValid = false; |
There was a problem hiding this comment.
I don't think it's possible for the vtable slot to be null
There was a problem hiding this comment.
I see, do you think it's fine as is, or would you prefer I rebase with this check removed?
There was a problem hiding this comment.
I'd ask for it to be changed if there were something else to necessitate a round of updates, but I think it can be left alone. It doesn't really hurt
| if (inlinedMethod) | ||
| fixInlinedSiteInfo(reloRuntime, reloTarget, inlinedMethod); | ||
| else if (reloRuntime->comp()->getOption(TR_UseSymbolValidationManager)) | ||
| SVM_ASSERT(inlinedMethod != NULL, "inlinedMethod should not be NULL when using the SVM!"); |
| @@ -294,6 +294,7 @@ uint8_t *J9::X86::AheadOfTimeCompile::initializeAOTRelocationHeader(TR::Iterated | |||
| case TR_ProfiledMethodGuardRelocation : | |||
| { | |||
| guard = (TR_VirtualGuard *)relocation->getTargetAddress2(); | |||
| TR_OpaqueClassBlock *inlinedCodeClass = guard->getThisClass(); | |||
There was a problem hiding this comment.
This is the profiled class, yes? Not the defining class of the method, which may have been inherited
There was a problem hiding this comment.
Yeah that's right, we don't store the romclass of the defining class of the method.
|
@fjeremic do you mind shepherding this PR (since you're already here :P) |
|
|
|
jenkins test sanity all jdk8,jdk11 |
When enabling SVM asserts for AOT w/ SVM, an assert will
trigger during the relocation of an inlined site if its caller is a
profiled inlined method who's site was disabled (as part of its
relocation). This is because, under the SVM, relocation of the inlined
table entries are guaranteed (meaning, though the inlined site may be
disabled, the J9Method pointer in the inlined table in the metadata will
be updated to not be -1). However, when a profiled inlined method is
relocated, it does not go through the same code path as other inlined
method relocations, and so did not fix up the metadata when the site was
disabled.
This PR refactors the profiled inlined method relocation to use the
class ID from the SVM. It also relocates the inlined table entry
regardless of whether the inlined site can be activated.