480636 - Create Porting APIs for date and time functionality

Status: VERIFIED FIXED

(Tamarin Graveyard :: Virtual Machine, defect)

Description

[Rishit Shah]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.6) Gecko/2009011912 Firefox/3.0.6
Build Identifier: 

Add VMPI APIs for date and time functions used by Tamarin.  The existing platform specific implementations in Date[platform].cpp and OSDep[platform].cpp files should be abstracted and their implementations moved to the platform porting layer

Reproducible: Always

Comment 1

[Rishit Shah]

Attachment: Patch [v1] - date and time APIs

Summary -

New APIs - VMPI_getTime, VMPI_getDate, VMPI_getLocalTimeOffset, VMPI_isTimeDST

Platform implementations of these APIs added to [Win/Posix]PortUtils.cpp

OSDep[Win/Mac/Unix].cpp goes away.

Comment 2

[Lars T Hansen]

VMPI_getTimeMs would possibly be more expressive (certainly closer to current code) than VMPI_getTime, but it's only a mild preference; your chosen name reads better.

Assuming that the DST offset is one hour is presumably a bug that's carried over from existing code, but it is a bug.  According to the Wikipedia's article on Daylight Savings Time, "A one-hour shift is customary, but Australia's Lord Howe Island uses a half-hour shift.[86] Twenty-minute and two-hour shifts have been used in the past."  Consequently, I think that there should be another porting interface here (probably VMPI_DaylightSavingsTA), the comments should note that always assuming one hour is not correct and suggest looking for an OS API to provide the correct adjustment.  (Am I tilting at windmills?  Steven?)

Comment 3

[Rick Reitmaier]

Newfoundland uses 1/2hr shift!  IIRC some of the islands in the south pacific also rely on < 1hr offset.

Comment 4

[Rishit Shah]

(In reply to comment #2)
> VMPI_getTimeMs would possibly be more expressive (certainly closer to current
> code) than VMPI_getTime, but it's only a mild preference; your chosen name
> reads better.
Original name I gave was getTimeMilliseconds but that felt too long.  Also, wanted to avoid abbreviations in API names (though I already violated in isTimeDST) and then decided upon keeping in simple and readable.

> Assuming that the DST offset is one hour is presumably a bug that's carried
> over from existing code, but it is a bug.  According to the Wikipedia's article
> on Daylight Savings Time, "A one-hour shift is customary, but Australia's Lord
> Howe Island uses a half-hour shift.[86] Twenty-minute and two-hour shifts have
> been used in the past."  Consequently, I think that there should be another
> porting interface here (probably VMPI_DaylightSavingsTA), the comments should
> note that always assuming one hour is not correct and suggest looking for an OS
> API to provide the correct adjustment.  (Am I tilting at windmills?  Steven?)
I'll change the API and add comments to that respect.

Comment 5

[Steven Johnson]

(In reply to comment #4)
> (In reply to comment #2)
> Original name I gave was getTimeMilliseconds but that felt too long.  Also,
> wanted to avoid abbreviations in API names (though I already violated in
> isTimeDST) and then decided upon keeping in simple and readable.

Avoiding abbreviations is a good goal, but only where meaning is clear without it. isTimeDST is a necessary evil IMHO.

Comment 6

[Steven Johnson]

Comment on attachment 365073 [details] [diff] [review]
Patch [v1] - date and time APIs

Approved assuming we fix the DST=1-hour assumption.

nit: there are a few uint64 that should probably be uint64_t.

question: there is a naked call to VMPI_getDate() in Assembler::nInit that ignores the result... what's up with that? (The call predates this change)

Comment 7

[Rishit Shah]

(In reply to comment #6)
> (From update of attachment 365073 [details] [diff] [review])
> Approved assuming we fix the DST=1-hour assumption.
Will fix this.

> nit: there are a few uint64 that should probably be uint64_t.
the uint64 types were already in existing code.  I didn't want to change them to uint64_t because that could introduce warnings/errors for RVCT compilers for assignment code at other locations.  Addressing those warnings was out-of-scope of this bug.

> question: there is a naked call to VMPI_getDate() in Assembler::nInit that
> ignores the result... what's up with that? (The call predates this change)
I noticed that too and that call looks useless.

Comment 8

[Rick Reitmaier]

>> question: there is a naked call to VMPI_getDate() in Assembler::nInit that
>> ignores the result... what's up with that? (The call predates this change)
> I noticed that too and that call looks useless.

Remove it;  IIRC its a vestige from when we were doing performance timings.

Comment 9

[Rishit Shah]

Attachment: Final patch [v2] - incorporating feedback from patch [v1] review

Comment 10

[Lars T Hansen]

Comment on attachment 365472 [details] [diff] [review]
Final patch [v2] - incorporating feedback from patch [v1] review

Fine with me.  Seeding the RNG from the clock may not be smart given that getTime is allowed to return the time "since the system started", where "system" is not defined - could it be the VM?  If so, the seeding could be too deterministic in practice.  But, this is a separate bug (if it is a bug at all).

Comment 11

[Rishit Shah]

Changeset: 6183b3f1b6d8