git log 048e30c1f8231e6cd144a9251061f6fa127b353e..4434b0029009d6d0327eaca1ff22a0c39711291d -- basic points out 3 changes, and indeed reverting fce604c8ae11b462113305aba080d77f8193cfea locally fixes the problem.

Adding Cc: to Noel Grandin; Could you possibly take a look at this one? Thanks

Oh, and 'xray ThisComponent' is executed when a single empty Writer document is open (again, in case it matters).

Noel's commit changes the return value of GetSystemTicks to SbxSALUINT64. This  is the first function with a SbxSALUINT64 return value. It seems not all code is ready for this. 
One spot is fixed in https://gerrit.libreoffice.org/#/c/33906/

but I have no idea how to test if this is cause of the problem with 'xray ThisComponent'

Aha, the exact error is:

"Inadmissible value or data type.
Overflow."

on this line:

doubleClickTicks = (getSystemTicks() -lastTime) * 3 ' around 300 ms

This will break lots of macros in documents you can't change, I'm afraid.

Hmmm, vmikloos any idea what the maximum data type is we could return here?

We're going to have to right shift the time value to fit inside whatever that is.

Well, what was the problem with SbxLONG in the first place? I don't know much about the basic interpreter, but here is why the old code looks correct:

You changed from SbxLONG, which has a comment: // * Long integer (sal_Int32)

You changed to SbxSALUINT64. I was not sure why changing from signed to unsigned, so I changed to SbxSALINT64, but that didn't help. Next idea was to use a 32bit type, which is exactly SbxLONG, there is no explicit SbxSALINT32.

So I'm not sure what would be a better fix, other than just reverting this change.

See also at some other place:

case SbxLONG:       aRetType = cppu::UnoType<sal_Int32>::get(); break;

I.e. it doesn't look like SbxLONG is a wrapper around the problematic sal_Long that needs cleanup. But perhaps I missed something. :-)

I changed to SbxSALUINT64 to match the return type of tools::GetSystemTicks()

The problem was that tools::GetSystemTicks() returns a sal_uInt64, and on some systems, when converting that down to sal_Int32 in basic, we triggered wraparound, i.e. the value became negative.

Now, tools::GetSystemTicks() attempts to return a time in millis. On Linux it uses UTC as a time base, and on Windows it uses an arbitrary base.

So we could probably fix the original problem by changing Linux to use application startup time as a time-base in tools::GetSystemTicks()

Aha. An option would be to revert back to the old return type for the original function, and add a new GetSystemTicks64() function that has the new return type.

This way new users could opt in for the 64bit behavior, and we would not break existing users, like xray.

Does that make sense? :-)

pasting the IRC conversation here we had back then:


Jan 09 11:58:58 <moggi>	sberg: <noelgrandin> I have a fix coming for the tinderbox failures, in case anyone else is looking at it <noelgrandin> very entertaining, looks like we were always returning a 64-bit time value in a 32-bit field, and now time has marched on, and we exceed the boundaries
Jan 09 11:59:30 <tml_>	is the tinderbox in question a Windows one?
Jan 09 11:59:41 <noelgrandin>	sberg, https://gerrit.libreoffice.org/#/c/32879/
Jan 09 11:59:43 <IZBot>	gerrit: »[API CHANGE] return unsigned 64-bit value from GetSystemTicks in basic code« by Noel Grandin for master [NEW]
Jan 09 11:59:45 <tml_>	on Windows that GetSystemTicks() uses an epoch that probably is the boot time
Jan 09 11:59:46 <sberg>	moggi, ah, see my temporary removal of that test code a few lines above
Jan 09 12:00:45 <sberg>	tml_, and on non-Windows it uses some world-global value?
Jan 09 12:02:00 <tml_>	sberg: the Unix epoch
Jan 09 12:03:19 <tml_>	(and the time since the Unix epoch, in milli-seconds, surely has passed the limits of a 32-bit int long ago?)
Jan 09 12:04:04 <tml_>	so probably that GetSystemTicks() has never worked "correctly" on Unix?
Jan 09 12:06:05 <tml_>	while on Windows it has worked for a bit over a month after a boot?
Jan 09 12:06:09 <tml_>	just guesstimating
Jan 09 12:07:19 <sberg>	tml_, so it looks like lower 32-bit part of Unix epoch happened to turn "negative" yesterday and was "positive" on Friday---just in time for that commit :)
Jan 09 12:07:41 <tml_>	how often does it switch sign?
Jan 09 12:08:26 <tml_>	lower 32 bit of Unix epoch times 1000, you mean; as seconds it still fits into 31 bits
Jan 09 12:09:09 <sberg>	would switch every 2^31 seconds


So the fact that this basic function "works" is just that it's mostly lucky - it's almost always either positive or negative for the duration of the basic program.
If the basic program was unlucky enough to run during a transition, it would generate an incorrect result when performing calculation on those values.

I think our best bet here to fix the basic program and this regression is

(a) revert that API CHANGE on master and on the branch

(b) on master change tools::GetSystemTicks to 
  (i)  use application startup as the epoch
  (ii) return a signed 32-bit value to make it obvious what our requirement is (and document why)

(In reply to Noel Grandin from comment #9)
> (b) on master change tools::GetSystemTicks to 
>   (i)  use application startup as the epoch
>   (ii) return a signed 32-bit value to make it obvious what our requirement
> is (and document why)

From 'git grep -Fw Time::GetSystemTicks' it looks like tools' Time::GetSystemTicks is used in more places than just the implementation of BASIC's GetSystemTicks.  Why cripple it for everybody by changing its return type from sal_uInt64 to sal_Int32?

You didn't give a rationale (beyond "looks broken") for fce604c8ae11b462113305aba080d77f8193cfea "[API CHANGE] return unsigned 64-bit value from GetSystemTicks in basic code"; are you aware of any actual issue that it fixed?

I'd argue to leave BASIC's GetSystemTicks alone in it's broken pre-fce604c8ae11b462113305aba080d77f8193cfea state, for backwards compatibility, and optionally add a new BASIC function as discussed in comment 8.

(In reply to Stephan Bergmann from comment #10)
> 
> You didn't give a rationale (beyond "looks broken") for
> fce604c8ae11b462113305aba080d77f8193cfea "[API CHANGE] return unsigned
> 64-bit value from GetSystemTicks in basic code"; are you aware of any actual
> issue that it fixed?


I spotted the same build-break you did (see the IRC log) and that prompted me to look at the code.

(In reply to Noel Grandin from comment #11)
> (In reply to Stephan Bergmann from comment #10)
> > 
> > You didn't give a rationale (beyond "looks broken") for
> > fce604c8ae11b462113305aba080d77f8193cfea "[API CHANGE] return unsigned
> > 64-bit value from GetSystemTicks in basic code"; are you aware of any actual
> > issue that it fixed?
> 
> 
> I spotted the same build-break you did (see the IRC log) and that prompted
> me to look at the code.

Ah, right, the then-newly-introduced (060c2b9b1b0d3c9cf27f1b289a620cfa82b5b060 "QA Basic: split tests of methods in different files"; adding new tests despite the commit's subject line) test checking for a non-negative GetSystemTicks value.  Whatever made Laurent assume that that should always hold.  I'd argue that that test can be considered broken.

 93bde3156284df4419c49447cbf455de9d74f00a fixes this bug, thanks! :-)