Project

General

Profile

Bug #16159

rubyspec about time fails in Asia/Kuala_Lumpur timezone

Added by jimmynguyc (Jimmy Ngu) 11 months ago. Updated 5 months ago.

Status:
Closed
Priority:
Normal
Target version:
-
ruby -v:
ruby 2.7.0dev (2019-09-09T12:27:40Z master 89c5d5a64e) [x86_64-darwin18]
[ruby-core:94866]

Description

Steps to reproduce:

  1. Check out current master from Github (89c5d5a64e12cea23b230913b79c3d499bf30b12)
  2. Run autoconf
  3. Run ./configure
  4. Run make check -- --with-openssl-dir=/usr/local/opt/openssl/lib

Result:

 $ make check -- --with-openssl-dir=/usr/local/opt/openssl/lib
        BASERUBY = /Users/jimmy/.rbenv/shims/ruby --disable=gems
        CC = clang
        LD = ld
        LDSHARED = clang -dynamiclib
        CFLAGS = -O3 -ggdb3 -Wall -Wextra -Werror=deprecated-declarations -Werror=division-by-zero -Werror=implicit-function-declaration -Werror=implicit-int -Werror=pointer-arith -Werror=shorten-64-to-32 -Werror=write-strings -Wmissing-noreturn -Wno-constant-logical-operand -Wno-long-long -Wno-missing-field-initializers -Wno-overlength-strings -Wno-parentheses-equality -Wno-self-assign -Wno-tautological-compare -Wno-unused-parameter -Wno-unused-value -Wunused-variable -Werror=extra-tokens -std=gnu99  -pipe 
        XCFLAGS = -D_FORTIFY_SOURCE=2 -fstack-protector-strong -fno-strict-overflow -DRUBY_DEVEL=1 -fvisibility=hidden -DRUBY_EXPORT -fPIE -DCANONICALIZATION_FOR_MATHN -I. -I.ext/include/x86_64-darwin18 -I./include -I. -I./enc/unicode/12.1.0
        CPPFLAGS = -I/usr/local/opt/openssl/include -D_XOPEN_SOURCE -D_DARWIN_C_SOURCE -D_DARWIN_UNLIMITED_SELECT -D_REENTRANT   
        DLDFLAGS = -L/usr/local/opt/openssl/lib -Wl,-undefined,dynamic_lookup -Wl,-multiply_defined,suppress -fstack-protector-strong -Wl,-pie -framework Security -framework Foundation  
        SOLIBS = -lpthread -lgmp -ldl -lobjc
        LANG = en_US.UTF-8
        LC_ALL = 
        LC_CTYPE = en_US.UTF-8
        MFLAGS = 
Apple LLVM version 10.0.1 (clang-1001.0.46.4)
Target: x86_64-apple-darwin18.7.0
Thread model: posix
InstalledDir: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin
./revision.h unchanged
generating encdb.h
encdb.h unchanged
generating enc.mk
making srcs under enc
make[1]: Nothing to be done for `srcs'.
generating transdb.h
transdb.h unchanged
generating makefiles ext/configure-ext.mk
ext/configure-ext.mk unchanged
generating makefile exts.mk
exts.mk unchanged
./revision.h unchanged
make[1]: Nothing to be done for `note'.
making enc
make[1]: Nothing to be done for `enc'.
making trans
make[1]: Nothing to be done for `./enc/trans'.
making encs
make[1]: Nothing to be done for `encs'.
Generating RDoc documentation

No newer files.

  Files:      0

  Classes:    0 (0 undocumented)
  Modules:    0 (0 undocumented)
  Constants:  0 (0 undocumented)
  Attributes: 0 (0 undocumented)
  Methods:    0 (0 undocumented)

  Total:      0 (0 undocumented)
    0.00% documented

  Elapsed: 0.0s

Fiber count: 10000 (skipping)
Thread count: 4094 (can't create Thread: Resource temporarily unavailable)
PASS all 1408 tests
exec ./miniruby -I./lib -I. -I.ext/common  ./tool/runruby.rb --extout=.ext  -- --disable-gems "./bootstraptest/runner.rb" --ruby="ruby --disable-gems"   ./KNOWNBUGS.rb
2019-09-10 00:54:00 +0800
Driver is ruby 2.7.0dev (2019-09-09T12:27:40Z master 89c5d5a64e) [x86_64-darwin18]
Target is ruby 2.7.0dev (2019-09-09T12:27:40Z master 89c5d5a64e) [x86_64-darwin18]

KNOWNBUGS.rb  PASS 0
No tests, no problem

test succeeded
Run options: "--ruby=./miniruby -I./lib -I. -I.ext/common  ./tool/runruby.rb --extout=.ext  -- --disable-gems"

# Running tests:

Finished tests in 2.620115s, 87.4007 tests/s, 190.0680 assertions/s.                                            
229 tests, 498 assertions, 0 failures, 0 errors, 0 skips

ruby -v: ruby 2.7.0dev (2019-09-09T12:27:40Z master 89c5d5a64e) [x86_64-darwin18]
Run options: "--ruby=./miniruby -I./lib -I. -I.ext/common  ./tool/runruby.rb --extout=.ext  -- --disable-gems" --excludes-dir=./test/excludes --name=!/memory_leak/

# Running tests:

Finished tests in 504.198097s, 41.7792 tests/s, 5397.4460 assertions/s.                                                                                                         
21065 tests, 2721382 assertions, 0 failures, 0 errors, 37 skips

ruby -v: ruby 2.7.0dev (2019-09-09T12:27:40Z master 89c5d5a64e) [x86_64-darwin18]
$ /Users/jimmy/Projects/ruby/miniruby -I/Users/jimmy/Projects/ruby/lib /Users/jimmy/Projects/ruby/tool/runruby.rb --archdir=/Users/jimmy/Projects/ruby --extout=.ext -- /Users/jimmy/Projects/ruby/spec/mspec/bin/mspec-run -B ./spec/default.mspec
ruby 2.7.0dev (2019-09-09T12:27:40Z master 89c5d5a64e) [x86_64-darwin18]

1)
Time.local handles years from 0 as such FAILED
Expected 1932
 to equal 1933

/Users/jimmy/Projects/ruby/spec/ruby/core/time/shared/time_params.rb:138:in `block (3 levels) in <top (required)>'
/Users/jimmy/Projects/ruby/spec/ruby/core/time/shared/time_params.rb:136:in `upto'
/Users/jimmy/Projects/ruby/spec/ruby/core/time/shared/time_params.rb:136:in `block (2 levels) in <top (required)>'
/Users/jimmy/Projects/ruby/spec/ruby/core/time/local_spec.rb:5:in `<top (required)>'

2)
Time.mktime handles years from 0 as such FAILED
Expected 1932
 to equal 1933

/Users/jimmy/Projects/ruby/spec/ruby/core/time/shared/time_params.rb:138:in `block (3 levels) in <top (required)>'
/Users/jimmy/Projects/ruby/spec/ruby/core/time/shared/time_params.rb:136:in `upto'
/Users/jimmy/Projects/ruby/spec/ruby/core/time/shared/time_params.rb:136:in `block (2 levels) in <top (required)>'
/Users/jimmy/Projects/ruby/spec/ruby/core/time/mktime_spec.rb:5:in `<top (required)>'

3)
Time.new handles years from 0 as such FAILED
Expected 1932
 to equal 1933

/Users/jimmy/Projects/ruby/spec/ruby/core/time/shared/time_params.rb:138:in `block (3 levels) in <top (required)>'
/Users/jimmy/Projects/ruby/spec/ruby/core/time/shared/time_params.rb:136:in `upto'
/Users/jimmy/Projects/ruby/spec/ruby/core/time/shared/time_params.rb:136:in `block (2 levels) in <top (required)>'
/Users/jimmy/Projects/ruby/spec/ruby/core/time/new_spec.rb:11:in `<top (required)>'
[\ | ==================100%================== | 00:00:00]      3F      0E 

Finished in 59.678478 seconds

3771 files, 30927 examples, 158359 expectations, 3 failures, 0 errors, 0 tagged
make: *** [yes-test-spec] Error 1

I'm running a 15" MacBook Pro 2.9 GHz Intel Core i7

Updated by jimmynguyc (Jimmy Ngu) 11 months ago

Ran it 3 times and oddly enough they all fail at Expected 1932 to equal 1933

Updated by nobu (Nobuyoshi Nakada) 11 months ago

What timezone are you using?

Updated by jimmynguyc (Jimmy Ngu) 11 months ago

GMT+8

$ date +%z
+0800

Updated by jimmynguyc (Jimmy Ngu) 11 months ago

Also, I ran it multiple times between 11pm - 1.30am

Updated by mame (Yusuke Endoh) 8 months ago

  • Assignee set to Eregon (Benoit Daloze)
  • Status changed from Open to Assigned
  • Subject changed from Off by 1 errors from time_params tests to rubyspec about time fails in Asia/Kuala_Lumpur timezone

There is no 00:00:00 1 Jan. 1933 in the timezone Asia/Kuala_Lumpur

$ TZ=Asia/Kuala_Lumpur irb
irb(main):001:0> t = Time.new(1932, 12, 31, 23, 59, 59)
=> 1932-12-31 23:59:59 +0700
irb(main):002:0> t + 1
=> 1933-01-01 00:20:00 +0720

https://en.wikipedia.org/wiki/Time_in_Malaysia

rubyspec expects Time.new(n).year == n for all n in 0..2100, but the expectation is not satisfied in the timezone.

Eregon (Benoit Daloze) How can we fix the spec?

Updated by Eregon (Benoit Daloze) 8 months ago

Isn't it weird that

$ TZ=Asia/Kuala_Lumpur ruby -e 'p Time.new(1933)'
1932-12-31 00:00:00 +0700

i.e., Time.new(1933) returns a time in 1932?

If that's actually what native calls return and not a bug then let's use with_timezone in that spec.

Updated by Eregon (Benoit Daloze) 8 months ago

Using Time.new(year,1,2) + a comment indicating might be a workaround, but it's not exactly nice.

Updated by mame (Yusuke Endoh) 8 months ago

  • Assignee changed from Eregon (Benoit Daloze) to akr (Akira Tanaka)

By further investigation, now I think that it is a bug of Time.new.

find_time_t in time.c uses integer division like ((tptr->tm_year-69)/4). Because 1933 is older than 1970, tptr->tm_year is negative. The result of division is not guaranteed to be floor'ed in C, which leads to the bug.

akr (Akira Tanaka) Could you review this?

diff --git a/time.c b/time.c
index 25b843e405..e7f9202cf4 100644
--- a/time.c
+++ b/time.c
@@ -3121,6 +3121,10 @@ static VALUE find_time_numguess_getter(void)
 #define DEBUG_FIND_TIME_NUMGUESS_INC
 #endif

+static int FLOOR_DIV(int n, int m) {
+    return FIX2INT(rb_fix_div_fix(INT2FIX(n), INT2FIX(m)));
+}
+
 static const char *
 find_time_t(struct tm *tptr, int utc_p, time_t *tp)
 {
@@ -3365,12 +3369,12 @@ find_time_t(struct tm *tptr, int utc_p, time_t *tp)

     *tp = guess_lo +
           ((tptr->tm_year - tm_lo.tm_year) * 365 +
-           ((tptr->tm_year-69)/4) -
-           ((tptr->tm_year-1)/100) +
-           ((tptr->tm_year+299)/400) -
-           ((tm_lo.tm_year-69)/4) +
-           ((tm_lo.tm_year-1)/100) -
-           ((tm_lo.tm_year+299)/400) +
+           FLOOR_DIV((tptr->tm_year-69), 4) -
+           FLOOR_DIV((tptr->tm_year-1), 100) +
+           FLOOR_DIV((tptr->tm_year+299), 400) -
+           FLOOR_DIV((tm_lo.tm_year-69), 4) +
+           FLOOR_DIV((tm_lo.tm_year-1), 100) -
+           FLOOR_DIV((tm_lo.tm_year+299), 400) +
            tptr_tm_yday -
            tm_lo.tm_yday) * 86400 +
           (tptr->tm_hour - tm_lo.tm_hour) * 3600 +

Updated by mame (Yusuke Endoh) 8 months ago

With the patch applied, Time.new(1933) works as expected in Asia/Kuala_Lumpur

$ TZ=Asia/Kuala_Lumpur ./miniruby -e 'p Time.new(1933)'
1933-01-01 00:20:00 +0720

Updated by Eregon (Benoit Daloze) 8 months ago

Seems a bug indeed. Using java.time (by using TruffleRuby) gives the same result as the patch.

Updated by akr (Akira Tanaka) 8 months ago

mame (Yusuke Endoh) wrote:

By further investigation, now I think that it is a bug of Time.new.

find_time_t in time.c uses integer division like ((tptr->tm_year-69)/4). Because 1933 is older than 1970, tptr->tm_year is negative. The result of division is not guaranteed to be floor'ed in C, which leads to the bug.

akr (Akira Tanaka) Could you review this?

diff --git a/time.c b/time.c
index 25b843e405..e7f9202cf4 100644
--- a/time.c
+++ b/time.c
@@ -3121,6 +3121,10 @@ static VALUE find_time_numguess_getter(void)
 #define DEBUG_FIND_TIME_NUMGUESS_INC
 #endif

+static int FLOOR_DIV(int n, int m) {
+    return FIX2INT(rb_fix_div_fix(INT2FIX(n), INT2FIX(m)));
+}
+
 static const char *
 find_time_t(struct tm *tptr, int utc_p, time_t *tp)
 {
@@ -3365,12 +3369,12 @@ find_time_t(struct tm *tptr, int utc_p, time_t *tp)

     *tp = guess_lo +
           ((tptr->tm_year - tm_lo.tm_year) * 365 +
-           ((tptr->tm_year-69)/4) -
-           ((tptr->tm_year-1)/100) +
-           ((tptr->tm_year+299)/400) -
-           ((tm_lo.tm_year-69)/4) +
-           ((tm_lo.tm_year-1)/100) -
-           ((tm_lo.tm_year+299)/400) +
+           FLOOR_DIV((tptr->tm_year-69), 4) -
+           FLOOR_DIV((tptr->tm_year-1), 100) +
+           FLOOR_DIV((tptr->tm_year+299), 400) -
+           FLOOR_DIV((tm_lo.tm_year-69), 4) +
+           FLOOR_DIV((tm_lo.tm_year-1), 100) -
+           FLOOR_DIV((tm_lo.tm_year+299), 400) +
            tptr_tm_yday -
            tm_lo.tm_yday) * 86400 +
           (tptr->tm_hour - tm_lo.tm_hour) * 3600 +

Use DIV already defined in time.c.
The patch seems fine except that.

Updated by mame (Yusuke Endoh) 8 months ago

Thanks, I will commit this soon.

diff --git a/time.c b/time.c
index 25b843e405..ef8a995da4 100644
--- a/time.c
+++ b/time.c
@@ -3365,12 +3365,12 @@ find_time_t(struct tm *tptr, int utc_p, time_t *tp)

     *tp = guess_lo +
           ((tptr->tm_year - tm_lo.tm_year) * 365 +
-           ((tptr->tm_year-69)/4) -
-           ((tptr->tm_year-1)/100) +
-           ((tptr->tm_year+299)/400) -
-           ((tm_lo.tm_year-69)/4) +
-           ((tm_lo.tm_year-1)/100) -
-           ((tm_lo.tm_year+299)/400) +
+           DIV((tptr->tm_year-69), 4) -
+           DIV((tptr->tm_year-1), 100) +
+           DIV((tptr->tm_year+299), 400) -
+           DIV((tm_lo.tm_year-69), 4) +
+           DIV((tm_lo.tm_year-1), 100) -
+           DIV((tm_lo.tm_year+299), 400) +
            tptr_tm_yday -
            tm_lo.tm_yday) * 86400 +
           (tptr->tm_hour - tm_lo.tm_hour) * 3600 +
#13

Updated by mame (Yusuke Endoh) 8 months ago

  • Status changed from Assigned to Closed

Applied in changeset git|d6a2bce64a7fa1099e507e1d36b5f1533f42f60f.


time.c (find_time_t): fix round-to-zero bug

find_time_t did not work correctly for year older than the Epoch
because it used C's integer division (which rounds negative to zero).

For example, TIme.new(1933) returned a wrong time whose year is 1922
in Asia/Kuala_Lumpur because there is no 00:00:00 1st Jan. 1933 in the
time zone.

$ TZ=Asia/Kuala_Lumpur ruby -e 'p Time.new(1933)'
1932-12-31 00:00:00 +0700

This change fixes the issue by using DIV macro instead of /.
Now Time.new(1933) returns a time in 1933.

$ TZ=Asia/Kuala_Lumpur ruby -e 'p Time.new(1933)'
1933-01-01 00:20:00 +0720

[Bug #16159]

#14

Updated by nobu (Nobuyoshi Nakada) 8 months ago

  • Backport changed from 2.5: UNKNOWN, 2.6: UNKNOWN to 2.5: REQUIRED, 2.6: REQUIRED

Updated by nagachika (Tomoyuki Chikanaga) 8 months ago

  • Backport changed from 2.5: REQUIRED, 2.6: REQUIRED to 2.5: REQUIRED, 2.6: DONE

ruby_2_6 r67836 merged revision(s) d6a2bce64a7fa1099e507e1d36b5f1533f42f60f,c687be4bc01c9ce52ea990945d9304d6fe59fe9b.

Updated by usa (Usaku NAKAMURA) 5 months ago

  • Backport changed from 2.5: REQUIRED, 2.6: DONE to 2.5: DONE, 2.6: DONE

ruby_2_5 r67864 merged revision(s) d6a2bce64a7fa1099e507e1d36b5f1533f42f60f,c687be4bc01c9ce52ea990945d9304d6fe59fe9b.

Also available in: Atom PDF