On Wed, 3 Sep 2025 20:19:38 GMT, Vladimir Ivanov <vlivanov at openjdk.org> wrote:
>> src/hotspot/share/opto/c2_globals.hpp line 83:
>>
>>> 81: \
>>> 82: product(bool, StressReachabilityFences, false, DIAGNOSTIC, \
>>> 83: "Randomly insert ReachabilityFence nodes") \
>>
>> Drive-by sniping: what about a hello-world test where you test out these flags?
>
> Good idea. Added one.
Also: you promise that it happens randomly. But it seems to be added deterministically everywhere. Did I miss something?
>> src/hotspot/share/opto/callnode.hpp line 497:
>>
>>> 495: // Are we guaranteed that this node is a safepoint? Not true for leaf calls and
>>> 496: // for some macro nodes whose expansion does not have a safepoint on the fast path.
>>> 497: virtual bool guaranteed_safepoint() { return true; }
>>
>> I see you only copied it. It makes me a little nervous when we call the "default" case safe. Because when you add more cases, you just assume it is safe... and if it is not we first have to discover that through a bug. What do you think?
>
> Well, it's a SafePointNode class after all. I lifted it from `CallNode` subclass to avoid elaborate check on SafePoint nodes (!is_Call() || as_Call() && guaranteed_safepoint()`)).
>
> If some node extends SafePointNode, but doesn't keep JVM state, it has to communicate it to users one way or another. And changing the default doesn't improve the situation IMO: reporting a safepoint node as a non-safepoint is still a bug.
Hmm. The way it is formulated it sounds more like:
- `true` -> we are guaranteed that it is a safepoint.
- `false` -> it may or may not be a safepoint - no guarantees.
Am I understanding this right?
If yes, then it would make more sense to have a default that is `no guarantee`. But maybe that makes things more complicated in other ways. All I'm saying it makes me nervous ;)
>> src/hotspot/share/opto/parse.hpp line 361:
>>
>>> 359: bool _wrote_fields; // Did we write any field?
>>> 360: Node* _alloc_with_final_or_stable; // An allocation node with final or @Stable field
>>> 361: Node* _stress_rf_hook; // StressReachabilityFences support
>>
>> You could write out the `rf`
>
> I'd like to avoid that. `_stress_reachability_fence_hook` is way too verbose IMO. The declaration and all the accesses are accompanied by `StressReachabilityFences` which should make it clear what `rf` refers to.
Fair enough. It's always a trade-off. Works here because of `StressReachabilityFences` :)
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2330253854
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2330166192
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2330245481