Integrated: 8302670: use-after-free related to PhaseIterGVN interaction with Unique_Node_List and Node_Stack

[Emanuel Peter]

On Fri, 5 May 2023 14:08:42 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

> **Motivation**
> 
> - Generally: we copy containers by value: the consequence is that we copy all internals by value, including size, capacity and pointers. From there on, the containers can diverge, and make each other inconsistent. One may be destructed and free the memory of the other one. In theory this could cause a bug on the main-branch. In practice, we probably (maybe?) use the correct one of the many copies that is currently supposed to be alive. If one pushes to the wrong copy, then one will most likely eventually hit a SIGSEGV - which has happened to me and @TobiHartmann a few times - it is very annoying. Plus: copy by value of containers is very bad design, and makes it difficult to understand which one is the "live" copy.
> - We also overwrite igvn phases. One case is particularly hairy: `igvn = ccp` (truncate ccp, and store it into igvn variable. Aka `object slicing in c++`)
> 
> @jcking 's first version https://github.com/openjdk/jdk/pull/12703. He dropped it as a "discussion or starting point for somebody else". I took a lot of ideas from him, but went a bit more aggressive with refactoring instead of the `replace_with` move-like approach.
> 
> **Changes**
> 
> - Make many containers `NONCOPYABLE`:
>   - `Dict`
>   - `VectorSet`
>   - `Node_Array`, `Node_List`, `Unique_Node_List`
>   - `Node_Stack`
>   - `NodeHash`
>   - `Type_Array`
>   - `Phase`
>   - Note: for many classes I still allow the `A(A&&) = default;` constructor. This allows implicit moves (rvalues) so that we can return containers from functions and capture them.
> - Create "global" containers for `Compile`:
>   - `C->igvn_worklist()` (renamed from `for_igvn`, referenced to by `PhaseIterGVN._worklist`)
>   - `C->type_array()` (referenced to by `PhaseValues._types`)
>   - `C->node_hash_table()` (referenced to by `PhaseValues._table`)
>   - They are created in the `Compile` constructor. The phases can then hold a reference (`&`) to them.
>   - Note: before, these were located in the phases, and passed back and forth by value. They were passed downward via the phase constructor, where the corresponding fields were taken over from the previous phase. Then they were passed upward by `PhaseGVN.replace_with` (for `_table` and `_types`), or by simply overwriting the old `igvn` variable with a newly constructed igvn that has the containers passed into its constructor from the previous phase. I imagine it as "weaving" the containers from phase to phase, where the ownership travels. The messy part was that oth...

This pull request has now been integrated.

Changeset: 1f1f6040
Author:    Emanuel Peter <epeter at openjdk.org>
URL:       https://git.openjdk.org/jdk/commit/1f1f604071dc2fca4849eb6ce251d5f18b443e16
Stats:     717 lines in 37 files changed: 204 ins; 296 del; 217 mod

8302670: use-after-free related to PhaseIterGVN interaction with Unique_Node_List and Node_Stack

Co-authored-by: Justin King <jcking at openjdk.org>
Reviewed-by: thartmann, chagedorn, jcking

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

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