Skip to content

Change jerry_port interface to allow for a correct implementation of timezones. #2540

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 17, 2018
Merged

Change jerry_port interface to allow for a correct implementation of timezones. #2540

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 36 additions & 31 deletions docs/05.PORT-API.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,27 +76,37 @@ void jerry_port_log (jerry_log_level_t level, const char *fmt, ...);

```c
/**
* Jerry time zone structure
*/
typedef struct
{
int offset; /**< minutes from west */
int daylight_saving_time; /**< daylight saving time (1 - DST applies, 0 - not on DST) */
} jerry_time_zone_t;

/**
* Get timezone and daylight saving data
* Get local time zone adjustment, in milliseconds, for the given timestamp.
* The timestamp can be specified in either UTC or local time, depending on
* the value of is_utc. Adding the value returned from this function to
* a timestamp in UTC time should result in local time for the current time
* zone, and subtracting it from a timestamp in local time should result in
* UTC time.
*
* Ideally, this function should satisfy the stipulations applied to LocalTZA
* in section 20.3.1.7 of the ECMAScript version 9.0 spec.
*
* See Also:
* ECMA-262 v9, 20.3.1.7
*
* Note:
* This port function is called by jerry-core when
* CONFIG_DISABLE_DATE_BUILTIN is _not_ defined. Otherwise this function is
* not used.
*
* @param[out] tz_p time zone structure to fill.
* @return true - if success
* false - otherwise
* @param unix_ms The unix timestamp we want an offset for, given in
* millisecond precision (could be now, in the future,
* or in the past). As with all unix timestamps, 0 refers to
* 1970-01-01, a day is exactly 86 400 000 milliseconds, and
* leap seconds cause the same second to occur twice.
* @param is_utc Is the given timestamp in UTC time? If false, it is in local
* time.
*
* @return milliseconds between local time and UTC for the given timestamp,
* if available
*. 0 if not available / we are in UTC.
*/
bool jerry_port_get_time_zone (jerry_time_zone_t *tz_p);
double jerry_port_get_local_time_zone_adjustment (double unix_ms, bool is_utc);

/**
* Get system time
Expand Down Expand Up @@ -194,31 +204,26 @@ jerry_port_log (jerry_log_level_t level, /**< log level */
## Date

```c
#include <time.h>
#include <sys/time.h>
#include "jerryscript-port.h"

/**
* Default implementation of jerry_port_get_time_zone.
* Default implementation of jerry_port_get_local_time_zone_adjustment.
*/
bool jerry_port_get_time_zone (jerry_time_zone_t *tz_p)
double jerry_port_get_local_time_zone_adjustment (double unix_ms, /**< ms since unix epoch */
bool is_utc) /**< is the time above in UTC? */
{
struct timeval tv;
struct timezone tz;

/* gettimeofday may not fill tz, so zero-initializing */
tz.tz_minuteswest = 0;
tz.tz_dsttime = 0;

if (gettimeofday (&tv, &tz) != 0)
struct tm tm;
time_t now = (time_t) (unix_ms / 1000);
localtime_r (&now, &tm);
if (!is_utc)
{
return false;
now -= tm.tm_gmtoff;
localtime_r (&now, &tm);
}

tz_p->offset = tz.tz_minuteswest;
tz_p->daylight_saving_time = tz.tz_dsttime > 0 ? 1 : 0;

return true;
} /* jerry_port_get_time_zone */
return ((double) tm.tm_gmtoff) * 1000;
} /* jerry_port_get_local_time_zone_adjustment */

/**
* Default implementation of jerry_port_get_current_time.
Expand Down
2 changes: 1 addition & 1 deletion jerry-core/ecma/builtin-objects/ecma-builtin-date-prototype.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -600,7 +600,7 @@ ecma_builtin_date_prototype_dispatch_routine (uint16_t builtin_routine_id, /**<

if (!BUILTIN_DATE_FUNCTION_IS_UTC (builtin_routine_id))
{
this_num += ecma_date_local_time_zone (this_num);
this_num += ecma_date_local_time_zone_adjustment (this_num);
}

if (builtin_routine_id <= ECMA_DATE_PROTOTYPE_GET_UTC_TIMEZONE_OFFSET)
Expand Down
77 changes: 13 additions & 64 deletions jerry-core/ecma/builtin-objects/ecma-builtin-helpers-date.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,83 +302,32 @@ ecma_date_week_day (ecma_number_t time) /**< time value */
} /* ecma_date_week_day */

/**
* Helper function to get local time zone adjustment.
*
* See also:
* ECMA-262 v5, 15.9.1.7
*
* @return local time zone adjustment
*/
static inline ecma_number_t JERRY_ATTR_ALWAYS_INLINE
ecma_date_local_tza (jerry_time_zone_t *tz) /**< time zone information */
{
return tz->offset * -ECMA_DATE_MS_PER_MINUTE;
} /* ecma_date_local_tza */

/**
* Helper function to get the daylight saving time adjustment.
*
* See also:
* ECMA-262 v5, 15.9.1.8
*
* @return daylight saving time adjustment
*/
static inline ecma_number_t JERRY_ATTR_ALWAYS_INLINE
ecma_date_daylight_saving_ta (jerry_time_zone_t *tz, /**< time zone information */
ecma_number_t time) /**< time value */
{
JERRY_ASSERT (!ecma_number_is_nan (time));

/*
* TODO: Fix daylight saving calculation.
* https://github.com/jerryscript-project/jerryscript/issues/1661
*/
return tz->daylight_saving_time * ECMA_DATE_MS_PER_HOUR;
} /* ecma_date_daylight_saving_ta */

/**
* Helper function to get local time from UTC.
* Helper function to get the local time zone offset at a given UTC timestamp.
* You can add this number to the given UTC timestamp to get local time.
*
* See also:
* ECMA-262 v5, 15.9.1.9
*
* @return local time
* @return local time zone adjustment
*/
inline ecma_number_t JERRY_ATTR_ALWAYS_INLINE
ecma_date_local_time_zone (ecma_number_t time) /**< time value */
ecma_date_local_time_zone_adjustment (ecma_number_t time) /**< time value */
{
jerry_time_zone_t tz;

if (ecma_number_is_nan (time)
|| !jerry_port_get_time_zone (&tz))
{
return ecma_number_make_nan ();
}

return ecma_date_local_tza (&tz) + ecma_date_daylight_saving_ta (&tz, time);
} /* ecma_date_local_time_zone */
return jerry_port_get_local_time_zone_adjustment (time, true);
} /* ecma_date_local_time_zone_adjustment */

/**
* Helper function to get utc from local time.
* Helper function to get UTC time from local time.
*
* See also:
* ECMA-262 v5, 15.9.1.9
*
* @return utc time
* @return UTC time
*/
ecma_number_t
ecma_date_utc (ecma_number_t time) /**< time value */
{
jerry_time_zone_t tz;

if (ecma_number_is_nan (time)
|| !jerry_port_get_time_zone (&tz))
{
return ecma_number_make_nan ();
}

ecma_number_t simple_utc_time = time - ecma_date_local_tza (&tz);
return simple_utc_time - ecma_date_daylight_saving_ta (&tz, simple_utc_time);
return time - jerry_port_get_local_time_zone_adjustment (time, false);
} /* ecma_date_utc */

/**
Expand Down Expand Up @@ -615,7 +564,7 @@ ecma_date_timezone_offset (ecma_number_t time) /**< time value */
{
JERRY_ASSERT (!ecma_number_is_nan (time));

return (-ecma_date_local_time_zone (time)) / ECMA_DATE_MS_PER_MINUTE;
return (-ecma_date_local_time_zone_adjustment (time)) / ECMA_DATE_MS_PER_MINUTE;
} /* ecma_date_timezone_offset */

/**
Expand Down Expand Up @@ -724,7 +673,7 @@ ecma_date_to_string_format (ecma_number_t datetime_number, /**< datetime */
}
case LIT_CHAR_LOWERCASE_Z: /* Time zone minutes part. */
{
int32_t time_zone = (int32_t) ecma_date_local_time_zone (datetime_number);
int32_t time_zone = (int32_t) ecma_date_local_time_zone_adjustment (datetime_number);

if (time_zone >= 0)
{
Expand All @@ -744,7 +693,7 @@ ecma_date_to_string_format (ecma_number_t datetime_number, /**< datetime */
{
JERRY_ASSERT (*format_p == LIT_CHAR_UPPERCASE_Z); /* Time zone seconds part. */

int32_t time_zone = (int32_t) ecma_date_local_time_zone (datetime_number);
int32_t time_zone = (int32_t) ecma_date_local_time_zone_adjustment (datetime_number);

if (time_zone < 0)
{
Expand Down Expand Up @@ -805,7 +754,7 @@ ecma_date_to_string_format (ecma_number_t datetime_number, /**< datetime */
ecma_value_t
ecma_date_value_to_string (ecma_number_t datetime_number) /**< datetime */
{
datetime_number += ecma_date_local_time_zone (datetime_number);
datetime_number += ecma_date_local_time_zone_adjustment (datetime_number);
return ecma_date_to_string_format (datetime_number, "$W $M $D $Y $h:$m:$s GMT$z:$Z");
} /* ecma_date_value_to_string */

Expand Down
2 changes: 1 addition & 1 deletion jerry-core/ecma/builtin-objects/ecma-builtin-helpers.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ ecma_number_t ecma_date_year_from_time (ecma_number_t time);
ecma_number_t ecma_date_month_from_time (ecma_number_t time);
ecma_number_t ecma_date_date_from_time (ecma_number_t time);
ecma_number_t ecma_date_week_day (ecma_number_t time);
ecma_number_t ecma_date_local_time_zone (ecma_number_t time);
ecma_number_t ecma_date_local_time_zone_adjustment (ecma_number_t time);
ecma_number_t ecma_date_utc (ecma_number_t time);
ecma_number_t ecma_date_hour_from_time (ecma_number_t time);
ecma_number_t ecma_date_min_from_time (ecma_number_t time);
Expand Down
38 changes: 24 additions & 14 deletions jerry-core/include/jerryscript-port.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,27 +111,37 @@ void JERRY_ATTR_FORMAT (printf, 2, 3) jerry_port_log (jerry_log_level_t level, c
*/

/**
* Jerry time zone structure
*/
typedef struct
{
int offset; /**< minutes from west */
int daylight_saving_time; /**< daylight saving time (1 - DST applies, 0 - not on DST) */
} jerry_time_zone_t;

/**
* Get timezone and daylight saving data
* Get local time zone adjustment, in milliseconds, for the given timestamp.
* The timestamp can be specified in either UTC or local time, depending on
* the value of is_utc. Adding the value returned from this function to
* a timestamp in UTC time should result in local time for the current time
* zone, and subtracting it from a timestamp in local time should result in
* UTC time.
*
* Ideally, this function should satisfy the stipulations applied to LocalTZA
* in section 20.3.1.7 of the ECMAScript version 9.0 spec.
*
* See Also:
* ECMA-262 v9, 20.3.1.7
*
* Note:
* This port function is called by jerry-core when
* CONFIG_DISABLE_DATE_BUILTIN is _not_ defined. Otherwise this function is
* not used.
*
* @param[out] tz_p time zone structure to fill.
* @return true - if success
* false - otherwise
* @param unix_ms The unix timestamp we want an offset for, given in
* millisecond precision (could be now, in the future,
* or in the past). As with all unix timestamps, 0 refers to
* 1970-01-01, a day is exactly 86 400 000 milliseconds, and
* leap seconds cause the same second to occur twice.
* @param is_utc Is the given timestamp in UTC time? If false, it is in local
* time.
*
* @return milliseconds between local time and UTC for the given timestamp,
* if available
*. 0 if not available / we are in UTC.
*/
bool jerry_port_get_time_zone (jerry_time_zone_t *tz_p);
double jerry_port_get_local_time_zone_adjustment (double unix_ms, bool is_utc);
Copy link
Member

@akosthekiss akosthekiss Oct 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question to other reviewers (again, ping @zherczeg @LaszloLango ): do we want this long identifiers / port API identifiers? jerry_port_get_local_time_zone_adjustment or jerry_port_get_local_tza or ...?

Copy link
Member

@zherczeg zherczeg Oct 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer full words, so I like the current one.


/**
* Get system time
Expand Down
13 changes: 13 additions & 0 deletions jerry-port/default/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,19 @@ file(GLOB SOURCE_PORT_DEFAULT *.c)
# (should only be necessary if we used compiler default libc but not checking that)
set(DEFINES_PORT_DEFAULT _BSD_SOURCE _DEFAULT_SOURCE)

INCLUDE (CheckStructHasMember)
# CHECK_STRUCT_HAS_MEMBER works by trying to compile some C code that accesses the
# given field of the given struct. However, our default compiler options break this
# C code, so turn a couple of them off for this.
set(CMAKE_REQUIRED_FLAGS "-Wno-error=strict-prototypes -Wno-error=old-style-definition")
# tm.tm_gmtoff is non-standard, so glibc doesn't expose it in c99 mode
# (our default). Define some macros to expose it anyway.
set(CMAKE_REQUIRED_DEFINITIONS "-D_BSD_SOURCE -D_DEFAULT_SOURCE")
CHECK_STRUCT_HAS_MEMBER ("struct tm" tm_gmtoff time.h HAVE_TM_GMTOFF)
if(HAVE_TM_GMTOFF)
set(DEFINES_PORT_DEFAULT ${DEFINES_PORT_DEFAULT} HAVE_TM_GMTOFF)
endif()
Copy link
Member

@zherczeg zherczeg Oct 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this only affects the default implementation?

Copy link
Contributor Author

@crazy2be crazy2be Oct 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. It also doesn't seem to work, and I haven't been able to figure out why yet. My cmake-fu is not great.

Copy link
Member

@akosthekiss akosthekiss Oct 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't investigated all the details of the problem but my guess is that the root of the problem is that tm_gmtoff is non-standard. See, for example, in glibc:

So, if CHECK_STRUCT_HAS_MEMBER calls the compiler with default settings, i.e., with no extra defines, then the compilation during the check will not see tm_gmtoff. Perhaps setting CMAKE_REQUIRED_DEFINITIONS to _DEFAULT_SOURCE before CHECK_STRUCT_HAS_MEMBER may solve the problem (see: https://cmake.org/cmake/help/v3.0/module/CheckStructHasMember.html). But even if this works, it may be glibc-specific.

Copy link
Contributor Author

@crazy2be crazy2be Oct 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is something to do with my defines not getting passed. If I put a message() inside the if, it gets printed (at least, if I run (cd jerry-port/default && cmake .)). And compiling outside of cmake or any build system compiles fine without errors:

#include <stdio.h>
#include <time.h>
#include <stdbool.h>

double jerry_port_get_local_time_zone_adjustment (double unix_ms,  /**< ms since unix epoch */
                                                  bool is_utc)  /**< is the time above in UTC? */
{
  struct tm tm;
  time_t now = (time_t) (unix_ms / 1000);
  localtime_r (&now, &tm);
  if (!is_utc)
  {
    now -= tm.tm_gmtoff;
    localtime_r (&now, &tm);
  }
  return ((double) tm.tm_gmtoff) * 1000;
} /* jerry_port_get_local_time_zone_adjustment */

int main() {
	double adj = jerry_port_get_local_time_zone_adjustment(0, 0);
	printf("%lf\n", adj);
}

(the above compiles without any warnings or errors with just gcc test_gmtoff.c)

I verified that it isn't trying to use tm.tm_gmtoff (and getting, say, bogus values) by putting a printf in default-date.c before each return statement. The one we hit is the one immediately before return 0;, so it's not even attempting to use the tm.tm_gmtoff path.

Copy link
Contributor Author

@crazy2be crazy2be Oct 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... looking at CMakeCache.txt after a clean build, maybe I'm wrong, and for some reason, when run as part of the larger project, tm.tm_gmtoff isn't found.
image

Copy link
Contributor Author

@crazy2be crazy2be Oct 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed that when running cmake . in jerry-port/default, my CMakeCache.txt is generated with HAVE_TM_GMTOFF:INTERNAL=1, but when run as part of tools/build.py, the generated CMakeCache.txt (in build/tests/test262_tests/CMakeCache.txt) has HAVE_TM_GMTOFF:INTERNAL=, as above.

Looking at the two different CMakeCache.txt files, no glaring differences in build configuration stick out at me.

Copy link
Contributor Author

@crazy2be crazy2be Oct 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see now what you were saying. I didn't realize jerryscript compiled in strict c99 mode (which, as you said, makes tm.tm_gmtoff invisible). gcc -std=c99 test_gmtoff.c does indeed fail as you predicted.

Copy link
Contributor Author

@crazy2be crazy2be Oct 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, so the default source code that check_struct_has_member uses to check if the given struct has the given member actually fails to compile with the default jerryscript compiler options... Ugh.

Performing C SOURCE FILE Test HAVE_TM_GMTOFF failed with the following output:
Change Dir: /home/fbuser/src/jerryscript/build/tests/test262_tests/CMakeFiles/CMakeTmp

Run Build Command:"/usr/bin/make" "cmTC_a80df/fast"
/usr/bin/make -f CMakeFiles/cmTC_a80df.dir/build.make CMakeFiles/cmTC_a80df.dir/build
make[1]: Entering directory '/home/fbuser/src/jerryscript/build/tests/test262_tests/CMakeFiles/CMakeTmp'
Building C object CMakeFiles/cmTC_a80df.dir/src.c.o
/usr/bin/cc  -D_DEFAULT_SOURCE  -flto -fno-fat-lto-objects -std=c99 -pedantic -fno-builtin -fno-stack-protector -Wall -Werror=all -Wextra -Werror=extra -Wformat-nonliteral -Werror=format-nonliteral -Winit-self -Werror=init-self -Wconversion -Werror=conversion -Wsign-conversion -Werror=sign-conversion -Wformat-security -Werror=format-security -Wmissing-declarations -Werror=missing-declarations -Wshadow -Werror=shadow -Wstrict-prototypes -Werror=strict-prototypes -Wundef -Werror=undef -Wold-style-definition -Werror=old-style-definition -Wno-stack-protector -Wno-attributes -Werror -Wlogical-op -Werror=logical-op -DHAVE_TM_GMTOFF   -o CMakeFiles/cmTC_a80df.dir/src.c.o   -c /home/fbuser/src/jerryscript/build/tests/test262_tests/CMakeFiles/CMakeTmp/src.c
/home/fbuser/src/jerryscript/build/tests/test262_tests/CMakeFiles/CMakeTmp/src.c:4:5: error: function declaration isn’t a prototype [-Werror=strict-prototypes]
 int main()
     ^
/home/fbuser/src/jerryscript/build/tests/test262_tests/CMakeFiles/CMakeTmp/src.c: In function ‘main’:
/home/fbuser/src/jerryscript/build/tests/test262_tests/CMakeFiles/CMakeTmp/src.c:4:5: error: old-style function definition [-Werror=old-style-definition]
cc1: all warnings being treated as errors
CMakeFiles/cmTC_a80df.dir/build.make:65: recipe for target 'CMakeFiles/cmTC_a80df.dir/src.c.o' failed
make[1]: *** [CMakeFiles/cmTC_a80df.dir/src.c.o] Error 1
make[1]: Leaving directory '/home/fbuser/src/jerryscript/build/tests/test262_tests/CMakeFiles/CMakeTmp'
Makefile:126: recipe for target 'cmTC_a80df/fast' failed
make: *** [cmTC_a80df/fast] Error 2

Source file was:

#include <time.h>

int main()
{
   (void)sizeof(((struct tm *)0)->tm_gmtoff);
   return 0;
}

Copy link
Contributor Author

@crazy2be crazy2be Oct 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finally got it all to build. On the plus side, I'm a lot more familiar with cmake now :).


# Sleep function availability check
INCLUDE (CheckIncludeFiles)
CHECK_INCLUDE_FILES (time.h HAVE_TIME_H)
Expand Down
48 changes: 25 additions & 23 deletions jerry-port/default/default-date.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,41 +13,43 @@
* limitations under the License.
*/

#ifdef HAVE_TM_GMTOFF
#include <time.h>
#endif /* HAVE_TM_GMTOFF */
#ifdef __GNUC__
#include <sys/time.h>
#endif
#endif /* __GNUC__ */

#include "jerryscript-port.h"
#include "jerryscript-port-default.h"

/**
* Default implementation of jerry_port_get_time_zone. Uses 'gettimeofday' if
* available on the system, does nothing otherwise.
* Default implementation of jerry_port_get_local_time_zone_adjustment. Uses the 'tm_gmtoff' field
* of 'struct tm' (a GNU extension) filled by 'localtime_r' if available on the
* system, does nothing otherwise.
*
* @return true - if 'gettimeofday' is available and executed successfully,
* false - otherwise.
* @return offset between UTC and local time at the given unix timestamp, if
* available. Otherwise, returns 0, assuming UTC time.
*/
bool jerry_port_get_time_zone (jerry_time_zone_t *tz_p) /**< [out] time zone structure to fill */
double jerry_port_get_local_time_zone_adjustment (double unix_ms, /**< ms since unix epoch */
bool is_utc) /**< is the time above in UTC? */
{
#ifdef __GNUC__
struct timeval tv;
struct timezone tz;

/* gettimeofday may not fill tz, so zero-initializing */
tz.tz_minuteswest = 0;
tz.tz_dsttime = 0;

if (gettimeofday (&tv, &tz) == 0)
#ifdef HAVE_TM_GMTOFF
struct tm tm;
time_t now = (time_t) (unix_ms / 1000);
localtime_r (&now, &tm);
Copy link
Member

@akosthekiss akosthekiss Oct 4, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A little bit of thinking aloud: localtime_r is not C99 but an extension defined by the POSIX standard. Which means that this code will not necessarily compile on all targets. Unless the existence of the tm.tm_gmtoff GNU extension implies the POSIX extension as well.

Another approach could be to use timegm(&tm) - t instead of tm.tm_gmtoff but timegm is also a GNU/BSD extension. (Which could be emulated if not present on the system, but I'm not sure it would be worth the effort.)

So, it seems to me that we need to rely on at least two extensions to get this work. I'm not sure yet which way to go. (That's why I was thinking aloud. I'm open to inputs.)

Copy link
Contributor Author

@crazy2be crazy2be Oct 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

localtime_r seems ubiquitous, even on embedded systems. I find it hard to imagine a system without it but with tm.tm_gmtoff. tm.tm_gmtoff is not specified in any standard, but appears to exist as a GNU extension on linux (as you said) [0], and also in FreeBSD [1], OpenBSD [2], and OSX [3]. Windows has localtime_s [4], which seems to be the same as localtime_r, but with a different name. However, it doesn't have tm_gmtoff, so we couldn't support it this way anyway. Windows also doesn't support timegm, and the mkgmtime it supports doesn't support dates prior to 1970 [5], so you are likely going to need some non-standard solution there anyway (v8 resorts to calling WIN32API functions on windows).

[0] https://linux.die.net/man/3/localtime_r
[1] https://www.freebsd.org/cgi/man.cgi?query=localtime_r
[2] https://man.openbsd.org/ctime.3
[3] https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man3/localtime_r.3.html
[4] https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/localtime-s-localtime32-s-localtime64-s?view=vs-2017
[5] https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/mkgmtime-mkgmtime32-mkgmtime64?view=vs-2017

Copy link
Member

@akosthekiss akosthekiss Oct 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please note that we are targeting embedded systems way below the embedded-Linux world as well. And there, it is easy to find missing bits and pieces. E.g., it's a bit hard to dig for info, but it's probable that neither TI's MSP430 C compiler nor the IAR Embedded Workbench have support for localtime_r or tm.tm_gmtoff. And the ARM Compiler (version 6) does have a _localtime_r but no info on struct tm.

That being said, the platforms these compilers compile for will most probably have their own implementation for all/most jerry port functions instead of these default implementations.

Still, it's good to keep those ideals (i.e., C99 conformance) in mind that drive the project. For the core engine library, we must be strict, I think. For libraries around it, we might be less strict, provided that the extensions are properly guarded. Here that seems to be the case.

Anyway, I'd like to hear the opinion of other reviewers. I summon @zherczeg @LaszloLango

Copy link
Contributor Author

@crazy2be crazy2be Oct 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My point was mainly that if a given platform has tm.tm_gmtoff then it most probably has localtime_r as well. But maybe there is a better way that has support on more platforms, I'm open to suggestions. We can always change / improve this later, however.

Copy link
Member

@zherczeg zherczeg Oct 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly this is the default implementation. Low-end targets must have their own implementation (or return 0 if not supported).

Copy link
Member

@zherczeg zherczeg Oct 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw what about windows? Does gcc on windows support this?

Copy link
Contributor Author

@crazy2be crazy2be Oct 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, this is the default implementation. Most embedded targets won't even use this, or they will just use the return 0; default implementation, which should have the same "assume UTC" behaviour that they always had.

Windows does not support tm.tm_gmtoff. It's also not supported by mingw. However, it's fairly straightforward to shim it by using GetTimeZoneInformationForYear if someone wants to do this at some point. I'd rather not do it in this PR, as it's already gotten quite big (and it's a PITA for me to test on windows...)

if (!is_utc)
{
tz_p->offset = tz.tz_minuteswest;
tz_p->daylight_saving_time = tz.tz_dsttime > 0 ? 1 : 0;

return true;
now -= tm.tm_gmtoff;
localtime_r (&now, &tm);
}
#endif /* __GNUC__ */

return false;
} /* jerry_port_get_time_zone */
return ((double) tm.tm_gmtoff) * 1000;
#else /* !HAVE_TM_GMTOFF */
(void) unix_ms;
(void) is_utc;
return 0.0;
#endif /* HAVE_TM_GMTOFF */
} /* jerry_port_get_local_time_zone_adjustment */

/**
* Default implementation of jerry_port_get_current_time. Uses 'gettimeofday' if
Expand Down
Loading