Ticket #48841

packets_gen.c equality test on floating point numbers

Open Date: 2023-10-13 07:51 Last Update: 2024-04-13 19:07

Reporter:
Owner:
Type:
Status:
Closed
Component:
MileStone:
Priority:
5 - Medium
Severity:
5 - Medium
Resolution:
Fixed
File:
1

Details

CodeQL about main branch:

Equality test on floating-point values
common/packets_gen.c:37792

Ticket History (3/7 Histories)

2023-10-13 07:51 Updated by: cazfi
  • New Ticket "packets_gen.c equality test on floating point numbers" created
2023-10-13 20:56 Updated by: alienvalkyrie
  • Component Update from General to Bootstrap
Comment

Since this is part of the delta protocol, there is a case for this being a place where exact floating-point comparison is reasonable ~> need to tell the static analysis tools to ignore it here. Alternatively, since in the binary format, we're transmitting these as fixed-point ~> could add appropriate comparison functions to dataio and call into those (though the JSON format's comparison would still need to be exact).

We could also not diff float fields at all and save those bits in the header (though we don't currently have a mechanism to exempt only individual fields from delta; would have to add that).

2023-10-15 03:47 Updated by: cazfi
Comment

Reply To alienvalkyrie

We could also not diff float fields at all and save those bits in the header (though we don't currently have a mechanism to exempt only individual fields from delta; would have to add that).

I guess there would be also an option to just set the bit always, with no check involved at all.

2024-03-18 07:16 Updated by: cazfi
Comment

Reply To alienvalkyrie

Since this is part of the delta protocol, there is a case for this being a place where exact floating-point comparison is reasonable ~> need to tell the static analysis tools to ignore it here.

Actually, there's a bug there. The delta protocol is interested about whether the value to be carried over the network differs from the one sent earlier. So we should not compare exact floating point values, but the rounded values that are actually sent.

2024-04-12 05:17 Updated by: alienvalkyrie
  • Owner Update from (None) to alienvalkyrie
  • Resolution Update from None to Accepted
  • Milestone Update from (None) to 3.3.0
Comment

Reply To cazfi

So we should not compare exact floating point values, but the rounded values that are actually sent.

Probably most reasonable to go with this. Although this does change the behavior of the JSON protocol, which still transmits exact floating point values; in cases where the exact value is different, but the rounded value is not (so the binary protocol would've unnecessarily sent the same rounded value again).

The alternative to avoid this would've been to add aforementioned context-aware comparison methods to the dataio layer, which know whether they're dealing with a JSON connection and can do an exact comparison in that case; but that feels unnecessarily clunky, especially since there's still debate to be had about whether we want to keep the fixed-point handling we have.

2024-04-13 19:07 Updated by: alienvalkyrie
  • Status Update from Open to Closed
  • Resolution Update from Accepted to Fixed

Edit

Please login to add comment to this ticket » Login