Integrated: 8302779: HelidonAppTest.java fails with "assert(_cb == CodeCache::find_blob(pc())) failed: Must be the same" or SIGSEGV

[Patricio Chilano Mateo]

On Tue, 7 Mar 2023 22:14:39 GMT, Patricio Chilano Mateo <pchilanomate at openjdk.org> wrote:

> Please review the following fix. The Method instance representing Continuation.enterSpecial() is replaced by a new Method during redefinition of the Continuation class. The already existing nmethod for it is not used, but a new one will be generated the first time enterSpecial() is resolved after redefinition. This means we could have more than one nmethod representing enterSpecial(), in particular, one generated before redefinition took place, and one after it. Now, when walking the stack, if we found a return barrier pc (as in Continuation::is_return_barrier_entry()) and we want to keep walking the physical stack then we know the sender will be the enterSpecial frame so we create it by calling ContinuationEntry::to_frame(). This method assumes there can only be one nmethod associated with enterSpecial() so we hit an assert later on. See the bug for more details of the crash.
> 
> As I mentioned in the bug we don't need to rely on this assumption since we can re-read the updated value from _enter_special. But reading both _enter_special and _return_pc means we would need some kind of synchronization since to_frame() could be called concurrently with set_enter_code(). To avoid that we could just read _return_pc and calculate the blob from it each time, but I'm also assuming that overhead is undesired and that's why the static variable was introduced. Alternatively _enter_special could be read and _return_pc could be derived from it (by adding an extra field in the nmethod class). But if we go this route I think we would need to do a small fix on thaw too. After redefinition and before a new call to resolve enterSpecial(), the last thaw call for some virtual thread would create an entry frame with an old _return_pc (see ThawBase::new_entry_frame() and ThawBase::patch_return()). I'm not sure about the lifecycle of the old CodeBlob but it seems it could have bee
 n already removed if enterSpecial was not found while traversing everybody's stack. Maybe there are more issues.
> 
> The simple solution implemented here is just to disallow redefinition of the Continuation class altogether. Another less restrictive option would be to keep the already generated enterSpecial nmethod, if there is one. I can also investigate one of the routes mentioned previously if desired.
> 
> I tested the fix with the simple reproducer I added to the bug and also with the previously crashing HelidonAppTest.java test.
> 
> Thanks,
> Patricio

This pull request has now been integrated.

Changeset: 8b740b46
Author:    Patricio Chilano Mateo <pchilanomate at openjdk.org>
URL:       https://git.openjdk.org/jdk/commit/8b740b46091c853c7cb66c361deda6dfbb2cedc8
Stats:     4 lines in 1 file changed: 4 ins; 0 del; 0 mod

8302779: HelidonAppTest.java fails with "assert(_cb == CodeCache::find_blob(pc())) failed: Must be the same" or SIGSEGV

Reviewed-by: coleenp, sspitsyn

-------------

PR: https://git.openjdk.org/jdk/pull/12911