RFR 8164632: Node indices should be treated as unsigned integers

[Vladimir Kozlov]

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