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

[Arthur Cohen]

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.





TREE_INT_CST_LOW(TYPE_MAX_VALUE(...)) = 18446744073709551615;
TREE_INT_CST_LOW(TYPE_MIN_VALUE(...)) = 0;



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;

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: Adding 1 to the result of the substraction results in an overflow, wrapping back to zero. 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 :)



All the best,

Arthur

The v4 of patches, which contains a lot of fixes for the issues you mentioned, will be sent soon.

OpenPGP_0x1B3465B044AD9C65.asc
Description: OpenPGP public key

OpenPGP_signature
Description: OpenPGP digital signature