RFR 8164632: Node indices should be treated as unsigned integers

[Hohensee, Paul]

Hi, Vladimir,

What do you think of the following?

1. Fix 8164632, i.e., replace int with uint, and add guarantees where idxs are passed to a different type (as in e.g., Eric's webrev).
2. New issue: Define an enum type for _instance_id, (typedef uint instance_idx_t) and change the guarantees to check < InstanceTop and > InstanceBot (InstanceTop = ~(uint)0, InstanceBot = 0). And change from instance ids from int to instance_idx_t.
3. New issue: Change from uint to node_idx_t.

Thanks,
Paul

On 8/13/20, 4:00 PM, "hotspot-compiler-dev on behalf of Vladimir Kozlov" <hotspot-compiler-dev-retn at openjdk.java.net on behalf of vladimir.kozlov at oracle.com> wrote:

    Yes, it is sloppy :(

    Mostly it bases on value of MaxNodeLimit = 80000 by default and as result node's idx will never reach MAX_INT.

    For EA we need 2 special types TOP and BOTTOM as Paul correctly pointed in RFE.
    We can make InstanceTop == max_juint and node_idx_t type for _instance_id . We don't do arithmetic on it, see
    TypeOopPtr::meet_instance_id(). But we can't use assert in this case to check incoming idx because max_juint will be
    valid value - InstanceTop.

    And I agree that we should use node_idx_t everywhere.

    For example, Node::Init(), init_node_notes(), node_notes_at() and set_node_notes_at() should use it.

    Same goes for req and other Node's methods arguments. All Node fields defined as node_idx_t but we have mix of int and
    uint when referencing them.

    Warning: it is not small change.

    Regards,
    Vladimir

    On 8/13/20 2:51 PM, Hohensee, Paul wrote:
    > Shouldn't all the uint type uses that represent node indices actually be node_idx_t?
    >
    > Thanks,
    > Paul
    >
    > On 8/13/20, 12:34 AM, "hotspot-compiler-dev on behalf of Tobias Hartmann" <hotspot-compiler-dev-retn at openjdk.java.net on behalf of tobias.hartmann at oracle.com> wrote:
    >
    >      Hi Eric,
    >
    >      there are other places where Node::_idx is casted to int (and a potential overflow might happen).
    >      For example, calls to Compile::node_notes_at.
    >
    >      The purpose of this RFE was to replace all Node::_idx uint -> int casts and consistently use uint
    >      for the node index. If that's not feasible, we should at least add a guarantee (not only an assert)
    >      checking that _idx is always <= MAX_INT.
    >
    >      Best regards,
    >      Tobias
    >
    >      On 12.08.20 00:41, Eric, Chan wrote:
    >      > Hi,
    >      >
    >      > Requesting review for
    >      >
    >      > Webrev : http://cr.openjdk.java.net/~xliu/eric/8164632/00/webrev/
    >      > JBS : https://bugs.openjdk.java.net/browse/JDK-8164632
    >      >
    >      > The change cast uint ni to integer so that the parameter that pass to method TypeOopPtr::cast_to_instance_id is a integer.
    >      >
    >      > I have tested this builds successfully .
    >      >
    >      > Ensured that there are no regressions in hotspot : tier1 tests.
    >      >
    >      > Regards,
    >      > Eric Chen
    >      >
    >