Works for me on x86_64. Is this on i386 or some other architecture?

Looking at the code, we’re probably not benefiting from parsing the usec string to a gdouble in parse_iso8601_time(), then converting it back to a gint64 in g_date_time_new().

Compounding the *=0.1 multiplication factors in get_iso8601_seconds() might be causing problems as well. I haven’t done the maths to work out the point at which the floating point error sums up to more than a usec there.

Robert, can you take a look at this please?

Created attachment 366647 [details]
reproducer (run on i386)

Yes, just i386 (I only tested x86_64 and i386 though) - sorry, I said that in the description but should have said in the subject too.

I briefly played more. This is still broken if I eliminate new_from_iso8601:

(bionic-i386)root@bionic:~# LD_LIBRARY_PATH=/build/glib2.0-zSL511/glib2.0-2.55.1/debian/build/deb/glib/.libs/ ./a.out # 2.55.1
microsecs: 123455
(bionic-i386)root@bionic:~# ./a.out  # 2.54.1
microsecs: 123456

Trivial reproducer attached.

Here’s a more trivial (hopefully) reproducer. Could you run it on i386 please (I don’t have an i386 machine).

/* gcc -o foo foo.c `pkg-config --cflags --libs glib-2.0` -Wall */
#include <glib.h>
#include <glib/gprintf.h>

int
main (void)
{
#define USEC_PER_SECOND      (G_GINT64_CONSTANT (1000000))
  gdouble seconds = 42.123456;
  gint64 usec;

  usec = seconds * USEC_PER_SECOND;
  g_printf ("usec 1: %" G_GINT64_FORMAT "\n", usec);
  if ((usec + 1) * 1e-6 <= seconds) {
    usec++;
    g_printf ("Incrementing usec\n");
  }

  usec = usec % USEC_PER_SECOND;
  g_printf ("usec 2: %" G_GINT64_FORMAT "\n", usec);

  return 0;
}

---

I get the following output on x86_64:
  usec 1: 42123456
  usec 2: 123456

I can’t upload it as an attachment because Bugzilla is giving me an error 500 when I try, for some reason. :-(

Yeah, that's broken. (FWIW I don't have an i386 system either - this is a chroot on my x86_64 system.)

(bionic-i386)root@bionic:/build# ./foo
usec 1: 42123455
usec 2: 123455

Something interesting going on with types / promotion / truncating / something here I guess

---

/* gcc -o foo foo.c `pkg-config --cflags --libs glib-2.0` -Wall */
#include <glib.h>
#include <glib/gprintf.h>

int
main (void)
{
#define USEC_PER_SECOND      (G_GINT64_CONSTANT (1000000))
  gdouble seconds = 42.123456;
  gdouble usecd;
  gint64 usec;
  gint useci;
  gint usect;

  usec = seconds * USEC_PER_SECOND;
  usecd = seconds * USEC_PER_SECOND;
  useci = seconds * USEC_PER_SECOND;
  usect = usecd;

  g_printf ("%" G_GINT64_FORMAT " / %f / %d / %d / %d\n",
	    usec,
	    usecd,
	    useci,
	    usect,
	    (int) (seconds * USEC_PER_SECOND));
}

---

Gives

(bionic-i386)root@bionic:/build# ./foo
42123455 / 42123456.000000 / 42123455 / 42123456 / 42123455

To be honest I don't understand where the 42123455 comes from, or why the last value is different from the penultimate...

Uff, I guess I need to dig into the C spec and work this all out.

Before I do that, would you be able to compile with a different compiler on i386 (clang, for example), just to try and rule out compiler bugs in gcc?

(In reply to Philip Withnall from comment #5)
> Uff, I guess I need to dig into the C spec and work this all out.
> 
> Before I do that, would you be able to compile with a different compiler on
> i386 (clang, for example), just to try and rule out compiler bugs in gcc?

Oh for the love of...

(bionic-i386)root@bionic:/build# clang -o foo b.c `pkg-config --cflags --libs glib-2.0` -Wall
(bionic-i386)root@bionic:/build# ./foo
42123456 / 42123456.000000 / 42123456 / 42123456 / 42123456

/o\

OK, so I tried

---
#include <stdio.h>

void main()
{
	double a = 42.123456;

	unsigned long long b = a * 1000000LL;

	printf ("%llu\n", b);
}
---

and it's broken on 18.04 pre-release (gcc version 7.2.0 (Ubuntu 7.2.0-18ubuntu2)) and Rawhide (gcc version 7.2.1 20180104 (Red Hat 7.2.1-6) (GCC)), and working on clang.

Not sure if this is a compiler bug or if we're spec non-compliant.

Could you see if 123456 / 1000000LL is the same on both gcc and clang? I just decoded them as a double to match the existing API, but can easily change to doing that.

Created attachment 366684 [details] [review]
Avoid repeated floating point multiplies with ISO 8601 parsing

Speculative patch that avoids any rounding errors with the repeated multiplication.

Someone on #ubuntu-devel hinted me towards `-ffloat-store', and that "fixes" GDateTime for me - and all of the reproducers given.

Additionally the ones which avoid GLib are "fixed" by compiling with -O1 or above, but for GLib itself that isn't enough.

Created attachment 366727 [details] [review]
gdatetime: Mark the usecs as volatile

Maybe this? I'm a bit out of my depth, but this fixes it for me. We need it for the intermediate variable too, so I split it out.

I read

https://stackoverflow.com/questions/3206101/extended-80-bit-double-floating-point-in-x87-not-sse2-we-dont-miss-it

and this being the problem probably explains why -ffloat-store fixes it too.

Review of attachment 366727 [details] [review]:

That looks like a reasonable approach to me, thanks a lot. Could you add a comment in the code about the use of `volatile`, and then we can push it?

Review of attachment 366684 [details] [review]:

Looks good to me, and the tests continue to pass on x86_64 with it. Iain, would you be able to run this patch through the tests on i386 too please, if you are doing another round of i386 testing, before we push it?

Cheers, added a comment and pushed both after successfully testing on i386.

(I slightly messed up git-bz and didn't get the bug URL added to the latter commit, so for the record...

https://git.gnome.org/browse/glib/commit/?id=e2054240c2fc14f0a5bba432475697ab2639b176)

Thanks! \o/