RFR 8164632: Node indices should be treated as unsigned integers

[Hohensee, Paul]

By "e.g.", I meant "ones like the one in the webrev". Tobais is correct that there are more. I grep'ed for "(int idx", ", int idx", "(int idx)", and so on, and found a bunch (not all of them are node_idx_t, but many of those that aren't should probably be uint too). So those would be fixed first.

Thanks,
Paul

On 8/14/20, 11:04 AM, "Vladimir Kozlov" <vladimir.kozlov at oracle.com> wrote:

    On 8/14/20 9:05 AM, Hohensee, Paul wrote:
    > 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).

    I see only this change:

    -      const TypeOopPtr* tinst = t->cast_to_instance_id(ni);
    +      assert(ni<=INT_MAX,"node index cannot be negative");
    +      const TypeOopPtr* tinst = t->cast_to_instance_id((int)ni);

    I would like to see first what you are suggesting.

    > 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.

    Yes, it is fine to split these 2.

    Regards,
    Vladimir

    >
    > 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
    >      >      >
    >      >
    >