CuraEngine icon indicating copy to clipboard operation
CuraEngine copied to clipboard

Fix unexpected type casting issues

Open casperlamboo opened this issue 1 year ago • 3 comments

Description

We encountered some issues when compiling for a different operating system where size_t is 32bit rather then 64bit. this causes the following simple example

void main() {
  size_t x = 1;
  int64_t y = -x;
  std::cout << y << std::endl;
}

to unexpectedly output 4294967295 rather then -1.

Note: this PR is based on https://github.com/Ultimaker/CuraEngine/tree/NP-327_emscripten_communication; merge PR https://github.com/Ultimaker/CuraEngine/pull/2131 before this one.

Type of change

Prevent unexpected integer overflows by more explicitly casting.

  • [x] Bug fix (non-breaking change which fixes an issue)
  • [ ] New feature (non-breaking change which adds functionality)

How Has This Been Tested?

yes

Test Configuration:

  • Operating System: MacOS 13.3

Checklist:

  • [x] My code follows the style guidelines of this project as described in UltiMaker Meta
  • [x] I have read the Contribution guide
  • [x] I have commented my code, particularly in hard-to-understand areas
  • [x] I have uploaded any files required to test this change

NP-343

casperlamboo avatar Aug 28 '24 07:08 casperlamboo

@jellespijker in the example I shared above the unexpected behaviour occurs when casting a smaller to a larger type due to the sign flipping. This is a different issue then you're trying solve with the stated example.

casperlamboo avatar Aug 28 '24 19:08 casperlamboo

  size_t x = 1;
  int64_t y = -x;

The example was intended as inspiration and a quick showcase. Maybe the example below is more suited and recognizable. https://godbolt.org/z/ocEs5aW71

#include <type_traits>
#include <concepts>
#include <cstdint>

template<typename T, typename U>
concept SafeConversion = requires {
    // Check for arithmetic types
    requires std::is_arithmetic_v<T> && std::is_arithmetic_v<U>;

    // Ensure the size of T is at least the same as U to avoid truncation
    requires sizeof(T) >= sizeof(U);

    // Handle signed/unsigned conversions to prevent unintended behavior
    requires (!std::is_integral_v<T> || !std::is_integral_v<U>) || 
             (std::is_signed_v<T> == std::is_signed_v<U>) ||
             (std::is_unsigned_v<T> && std::is_signed_v<U> && sizeof(T) > sizeof(U)); // Allow if unsigned T is larger

    // Prevent direct comparisons between integral and floating-point types
    requires (!std::is_integral_v<T> || !std::is_floating_point_v<U>) &&
             (!std::is_floating_point_v<T> || !std::is_integral_v<U>);
};

int main() {
    std::size_t x = 1;
    std::int64_t y = -x;
    static_assert(SafeConversion<decltype(x), decltype(y)>);
    return y;
}

Resulting in:

<source>:26:19: error: static assertion failed
   26 |     static_assert(SafeConversion<decltype(x), decltype(y)>);
      |                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
<source>:26:19: note: constraints not satisfied
<source>:6:9:   required by the constraints of 'template<class T, class U> concept SafeConversion'
<source>:6:26:   in requirements  [with T = long unsigned int; U = long int]
<source>:14:14: note: nested requirement '((((! is_integral_v<T>) || (! is_integral_v<U>)) || (is_signed_v<T> == is_signed_v<U>)) || ((is_unsigned_v<T> && is_signed_v<U>) && (sizeof (T) > sizeof (U))))' is not satisfied
   14 |     requires (!std::is_integral_v<T> || !std::is_integral_v<U>) ||
      |     ~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   15 |              (std::is_signed_v<T> == std::is_signed_v<U>) ||
      |              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   16 |              (std::is_unsigned_v<T> && std::is_signed_v<U> && sizeof(T) > sizeof(U)); // Allow if unsigned T is larger

jellespijker avatar Aug 28 '24 22:08 jellespijker

@jellespijker You really have a concepts-based solution for every situation :stuck_out_tongue:

In the present situation, this would indeed generate a proper error instead of a warning, but it doesn't actually fix the issue. What we discussed briefly with @casperlamboo would be to have our own types for everything (which we already do in the engine, partially), and proper conversion operators between them, so that you can have a compact, readable and error/warning-free code.

However we agreed for now to just fix this issue locally, and possibly add a C&C ticket to discuss this.

wawanbreton avatar Aug 29 '24 06:08 wawanbreton

I've been trying to compile this in WASM, and I believe I found one more place that needs to be cast:

Raft::LayerType Raft::getLayerType(LayerIndex layer_index)
{
    const auto airgap = Raft::getFillerLayerCount();
    const auto interface_layers = Raft::getInterfaceLayers();
    const auto surface_layers = Raft::getSurfaceLayers();

    if (layer_index < -airgap - surface_layers - interface_layers)

On my architecture this ends up saying layer_index < Really big number.

ToyboxZach avatar Sep 24 '24 18:09 ToyboxZach