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

[Arthur Cohen <arthur.cohen@embecosm.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 2399 bytes --]

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.

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

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3195 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]