[jdk8u-dev] RFR: 8303466: C2: failed: malformed control flow. Limit type made precise with MaxL/MinL

[Francisco Ferrari Bihurriet]

On Tue, 2 Jul 2024 09:07:26 GMT, Roman Marchenko <rmarchenko at openjdk.org> wrote:

>> Yes, as [JDK-8146629](https://bugs.openjdk.org/browse/JDK-8146629 "Make phase->is_IterGVN() accessible from Node::Identity and Node::Value") (openjdk/jdk11u at 69b52aa28be9054ea53bf9587ef7b563a658e510) doesn't have a 8u backport, it would have been neater to have `PhaseTransform* phase`.
>> 
>> What happened here is that neither the patch application nor the compilation failed, since `PhaseGVN` inherits from `PhaseValues`, which inherits from `PhaseTransform`.
>> 
>> However, there shouldn't be any issue, as the C++ type system is ensuring that all the callers are passing a `PhaseGVN` pointer.
>
> Compilation didn't fail because not all the warnings are enabled.
> 
> Enabling `-Woverloaded-virtual` causes a number of warnings like the following:
> 
> hotspot/src/share/vm/opto/addnode.hpp:50:17: error: ‘virtual Node* AddNode::Identity(PhaseTransform*)’ was hidden [-Werror=overloaded-virtual]
>    50 |   virtual Node *Identity( PhaseTransform *phase );
> hotspot/src/share/vm/opto/addnode.hpp:312:17: error:   by ‘virtual Node* MinLNode::Identity(PhaseGVN*)’ [-Werror=overloaded-virtual]
>   312 |   virtual Node* Identity(PhaseGVN* phase);
> 
> that may point to a serious bug.
> 
> In few words, `MinLNode::Identity` may be never called in case:
> 
> AddNode *obj = new MinLNode;
> obj->Identity(...); // AddNode::Identity() is called.

You are right, I was missing the `MaxLNode`←`MaxNode`←`AddNode` inheritance and the fact that `MaxLNode::Identity(PhaseGVN* phase)` doesn't override but hides `AddNode::Identity(PhaseTransform* phase)`, sorry for the confusion.

After my comment, we continued analyzing it with @martinuy and arrived to similar conclusions. We'll be proposing a fix for this later today.

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

PR Review Comment: https://git.openjdk.org/jdk8u-dev/pull/529#discussion_r1662399194