RFR: 8302670: use-after-free related to PhaseIterGVN interaction with Unique_Node_List and Node_Stack [v10]

[Kim Barrett]

On Thu, 11 May 2023 07:38:45 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. Th...
>
> Emanuel Peter has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Second batch of suggestions from @chhagedorn

Not really a review, just some drive-by comments.

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

PR Review: https://git.openjdk.org/jdk/pull/13833#pullrequestreview-1432418554