Re: [PATCH Rust front-end v3 38/46] gccrs: Add HIR to GCC GENERIC lowering entry point

[Richard Biener <richard.guenther@gmail.com>]

On Tue, Nov 29, 2022 at 7:10 PM Arthur Cohen <arthur.cohen@embecosm.com> wrote:
>
> Hi Richard,
>
> (...)
>
> >>>> +
> >>>> +  unsigned HOST_WIDE_INT ltype_length
> >>>> +    = wi::ext (wi::to_offset (TYPE_MAX_VALUE (ltype_domain))
> >>>> +                - wi::to_offset (TYPE_MIN_VALUE (ltype_domain)) + 1,
> >>>
> >>> TYPE_MIN_VALUE is not checked to be constant, also the correct
> >>> check would be to use TREE_CODE (..) == INTEGER_CST, in
> >>> the GCC middle-end an expression '1 + 2' (a PLUS_EXPR) would
> >>> be TREE_CONSTANT but wi::to_offset would ICE.
> >>>
> >>>> +              TYPE_PRECISION (TREE_TYPE (ltype_domain)),
> >>>> +              TYPE_SIGN (TREE_TYPE (ltype_domain)))
> >>>> +       .to_uhwi ();
> >>>
> >>> .to_uhwi will just truncate if the value doesn't fit, the same result as
> >>> above is achieved with
> >>>
> >>>    unsigned HOST_WIDE_INT ltype_length
> >>>       = TREE_INT_CST_LOW (TYPE_MAX_VALUE (..))
> >>>         - TREE_INT_CST_LOW (TYPE_MIN_VALUE (...)) + 1;
> >>>
> >>> so it appears you wanted to be "more correct" here (but if I see
> >>> correctly you fail on that attempt)?
> >>>
>
> I've made the changes you proposed and noticed failure on our 32-bit CI.
>
> I've had a look at the values in detail, and it seems that truncating
> was the expected behavior.
>
> On our 64 bit CI, with a testcase containing an array of zero elements,
> we get the following values:
>
> TREE_INT_CST_LOW(TYPE_MAX_VALUE(...)) = 18446744073709551615;
> TREE_INT_CST_LOW(TYPE_MIN_VALUE(...)) = 0;
>
> Adding 1 to the result of the substraction results in an overflow,
> wrapping back to zero.
>
> With the -m32 flag, we get the following values:
>
> TREE_INT_CST_LOW(TYPE_MAX_VALUE(...)) = 4294967295;
> TREE_INT_CST_LOW(TYPE_MIN_VALUE(...)) = 0;
>
> The addition of 1 does not overflow the unsigned HOST_WIDE_INT type and
> we end up with 4294967296 as the length of our array.
>
> I am not sure on how to fix this behavior, and whether or not it is the
> expected one, nor am I familiar enough with the tree API to reproduce
> the original behavior. Any input is welcome.
>
> In the meantime, I'll revert those changes and probably keep the
> existing code in the patches if that's okay with you.

Sure - take my comments as that the code needs comments explaining
what it tries to do.  Apparently I misunderstood the intent (and still don't
get it, but I don't remember the part in detail either).

> >>> Overall this part of the rust frontend looks OK.  Take the comments as
> >>> suggestions (for future
> >>> enhancements).
>
> Which seems to be the case :)
>
> The v4 of patches, which contains a lot of fixes for the issues you
> mentioned, will be sent soon.
>
> All the best,
>
> Arthur