G17 format specifier doesn't always round-trip double values

[Metalnem]

The documentation for numeric format strings says that the G17 format specifier is the recommended way for round-tripping double values, but it doesn't always work. Here is the full example of one such case:

using System;

namespace Roundtripping
{
  public class Program
  {
    public static void Main(string[] args)
    {
      var s = "23723333333333333433333337";

      double d1 = Double.Parse(s);
      double d2 = Double.Parse(d1.ToString("G17"));

      Console.WriteLine(d1 == d2);
    }
  }
}

This program prints false on .NET Core 2.2.104, and true on .NET Core 3.0.100-preview-010184. I see that there were a lot of issues and commits fixing the R specifier, but I was under the impression that G17 was always supposed to work.

Found via SharpFuzz.

[stephentoub]

cc: @tannergooding

[tannergooding]

Looks like a regression was checked in for netcoreapp2.1 and I fixed it with the recent compliance work/testing.

I'm still digging in to see if I can find what introduced the bug/etc.

For reference, in .netcoreapp2.0 and prior, as well as in netcoreapp3.0, you would get:

    23723333333333333433333337

G15 2.37233333333333E+25
G17 2.3723333333333335E+25
HEX 45339F9C7A1813C5

G15 2.37233333333333E+25
G17 2.3723333333333335E+25
HEX 45339F9C7A1813C5

    2.3723333333333335496196096E25

True

In .netcoreapp2.1 and .netcoreapp2.2 you would get:

    23723333333333333433333337

G15 2.37233333333333E+25
G17 2.3723333333333338E+25
HEX 45339F9C7A1813C5

G15 2.37233333333333E+25
G17 2.372333333333334E+25
HEX 45339F9C7A1813C6

    2.3723333333333335496196096E25

False

[tannergooding]

The bug was in the original native implementation of Grisu3/Dragon4. It was resolved when I ported the code to a managed implementation here: dotnet/coreclr#19999

Still digging down to determine what the fix was, but it was likely this: dotnet/coreclr#19999 (comment)

[tannergooding]

I was able to confirm that the the issue in the netcoreapp2.1/netcoreapp2.2 native code is dotnet/coreclr#19999 (comment)

Fixing the issue (changing the relevant line from memset(m_blocks, 0, shiftBlocks); to memset(m_blocks, 0, shiftBlocks * sizeof(UINT32));) resolves the issue as it ensures the lower bits are in-fact zero.

@stephentoub, what are the chances of this meeting the bar for backporting the fix?

[stephentoub]

what are the chances of this meeting the bar for backporting the fix?

It's worth proposing the fix and running it through shiproom. Since the fix is so localized and simple, that gives it a higher chance of success, but it'll be helpful to understand what the real-world impact is, how many apps are likely to be affected and to what extent, etc.

[tannergooding]

but it'll be helpful to understand what the real-world impact is

The real world impact is likely for serialization code, which often uses G17 to workaround the fact that R doesn't actually roundtrip (prior to netcoreapp3.0).

how many apps are likely to be affected and to what extent, etc.

Any app that has a number that hits this bug will get a string that parses back to a result that is off by at least 1ULP. (unit in last place). I'll try to do some analysis to determine if there is a certain category of inputs that will always hit this corner case (might possibly just run the 100m ES6 roundtripping tests and determine how many fail).

[danmoseley]

Also relevant (especially for port to 2.1) is how many reports we get from customers of this actually causing impact.

[tannergooding]

OK, I think I've finished investigating this.

Taking the fix described in https://github.com/dotnet/corefx/issues/35369#issuecomment-464535412, resolves the exact case listed in the issue and a few other similar cases. However, it does not solve the underlying issue described here, which is that value.ToString("G17") does not always roundtrip back to value.

Even after fixing the formatting issue, there are a couple of remaining issues in the parsing logic. These paring issues are non-trivial to fix and would likely not be allowed through shiproom given it is the same behavior we have going back to netcoreapp1.0.

[tannergooding]

I think what should happen is:

1. We take the fix for the formatting regression through shiproom. It is a trivial, one-line, fix and will remove at least one source of issues for people moving from netcoreapp2.1/netcoreapp2.2 to netcoreapp3.0.
2. We update the documentation to explain that prior to netcoreapp3.0, there was no formatting specifier which would guarantee the string produced would roundtrip.

[danmoseley]

Are you still pursuing this @tannergooding

[tannergooding]

I can, although I'm unfamiliar with the current process for taking something through shiproom and may need further pointers 😄

[karelz]

Just curious if we still consider this for servicing ...

[tannergooding]

I believe @danmosemsft indicated the chance of this was low.

[karelz]

Should we remove it from 2.2.x milestone, or is someone supposed to make a decision?

[danmoseley]

I suggest to wait on customer feedback. Right now we just have a fuzzing hit - I don't see evidence here this is impacting real apps. Every back-port has a cost and a risk of breaking others. We are particularly conservative with LTS releases. If we think we might gather that feedback here, I'm fine with leaving this open sitting in this milestone for a while.

[karelz]

I don't think we will get the feedback here. Or it does not matter if it is opened, or closed.
Closing and moving to 3.0. Anyway who needs it in 2.2.x can speak up here. Thansk!