RFR 8164632: Node indices should be treated as unsigned integers

[Vladimir Kozlov]

Missed this.

On 8/14/20 1:54 PM, Hohensee, Paul wrote:
> 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.

Yes, I am okay with fixing them first.

Thanks,
Vladimir K

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