Asserts fired due to PROD_WITH_ASSUMES commented out #15509
Conversation
Asserts from Issues: eclipse-openj9#15440, eclipse-openj9#15447, eclipse-openj9#15472, eclipse-openj9#15482 Signed-off-by: Manasha Vetrivelu <[email protected]>
There was a problem hiding this comment.
I would prefer the asserts that fail while running the jitdump code to stay. These are secondary asserts in the sense that before they fire something bad already has happened.
Also, it may useful to write a comment explaining why the asserts have been commented out. Otherwise, other people may simply delete them (we tend to delete code rather than comment it out)
|
Regarding the copyrights, every time you modify a file you have to make sure that the years that appear in the copyright are correct. For instance, J9MonitorTable.cpp shows |
Secondary Asserts have not been commented out. A reason for why the assert is commented has been provided. Copyright fixed for J9MonitorTable.cpp Signed-off-by: Manasha Vetrivelu <[email protected]>
| @@ -1,5 +1,5 @@ | |||
| /******************************************************************************* | |||
| * Copyright (c) 2000, 2021 IBM Corp. and others | |||
| * Copyright (c) 2000, 2022 IBM Corp. and others | |||
There was a problem hiding this comment.
This file only has the date in the copyright changed. We can skip it.
| @@ -10776,7 +10776,8 @@ void J9::X86::TreeEvaluator::VMwrtbarWithoutStoreEvaluator( | |||
| { | |||
| TR::Compilation *comp = cg->comp(); | |||
| TR_J9VMBase *fej9 = (TR_J9VMBase *)(cg->fe()); | |||
| TR_ASSERT(!(comp->getOptions()->realTimeGC()),"Call the real-time barrier"); | |||
| //assert below is commented because it is fired when compiled with PROD_WITH_ASSUMES | |||
There was a problem hiding this comment.
Suggest something like:
// Temporarily disable the asset below until we figure out the root cause
| @@ -11599,7 +11599,8 @@ TR::CompilationInfo::replenishInvocationCount(J9Method *method, TR::Compilation | |||
| } | |||
| else | |||
| { | |||
| TR_ASSERT(false, "Unexpected value for method->extra = %p (method=%p)\n", TR::CompilationInfo::getJ9MethodExtra(method), method); | |||
| //assert below is commented because it is fired when compiled with PROD_WITH_ASSUMES | |||
| //TR_ASSERT(false, "Unexpected value for method->extra = %p (method=%p)\n", TR::CompilationInfo::getJ9MethodExtra(method), method); | |||
There was a problem hiding this comment.
I would like to further investigate this case
Deleted assert mentioned in Issue:eclipse-openj9#15481. Commented the other primary asserts. Signed-off-by: Manasha Vetrivelu <[email protected]>
| @@ -2109,7 +2109,6 @@ bool TR::CompilationInfo::shouldAbortCompilation(TR_MethodToBeCompiled *entry, T | |||
|
|
|||
| if (entry->_unloadedMethod) // method was unloaded while we were trying to compile it | |||
| { | |||
| TR_ASSERT(entry->_compErrCode == compilationInterrupted, "Received error code %u, expect compilationInterrupted when the method was unloaded", entry->_compErrCode); | |||
There was a problem hiding this comment.
For the record, this assert was eliminated because we found it to be bogus. A compilation can fail with some error code and the immediately a redefinition even can take place which marks the compilation entry as unloaded. So it's totally possible to have the entry marked as unloaded and the error code be something else than compilationInterrupted.
| @@ -10776,7 +10776,7 @@ void J9::X86::TreeEvaluator::VMwrtbarWithoutStoreEvaluator( | |||
| { | |||
| TR::Compilation *comp = cg->comp(); | |||
| TR_J9VMBase *fej9 = (TR_J9VMBase *)(cg->fe()); | |||
| //assert below is commented because it is fired when compiled with PROD_WITH_ASSUMES | |||
| // Temporarily disable the asset below until we figure out the root cause | |||
|
The description of the PR mentions 4 different issues, but in the PR there are only 3 asserts that are dealt with. |
|
When the PR is merged, the issues may automatically close. We need to reopen the issues for the asserts that are temporarily commented out because we want to fix the problems not just cover them up. |
|
Initially 4 asserts were changed but I decided to cancel the changes that were made to the secondary asserts. |
|
I will reopen issues for all the asserts if they are closed once it is merged. |
Asserts from Issues: #15440, #15447, #15472, #15482
Signed-off-by: Manasha Vetrivelu [email protected]