Bug #16159: rubyspec about time fails in Asia/Kuala_Lumpur timezone

• Status: Closed
• Assignee: akr (Akira Tanaka)
• Target version: -
• ruby -v: ruby 2.7.0dev (2019-09-09T12:27:40Z master 89c5d5a64e) [x86_64-darwin18]
• Backport: 2.5: DONE, 2.6: DONE

[jimmynguyc (Jimmy Ngu)]

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

[jimmynguyc (Jimmy Ngu)]

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

[nobu (Nobuyoshi Nakada)]

What timezone are you using?

[jimmynguyc (Jimmy Ngu)]

GMT+8

$ date +%z
+0800

[jimmynguyc (Jimmy Ngu)]

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

[mame (Yusuke Endoh)]

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

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?

[Eregon (Benoit Daloze)]

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.

[Eregon (Benoit Daloze)]

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

[mame (Yusuke Endoh)]

• 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 +

[mame (Yusuke Endoh)]

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

[Eregon (Benoit Daloze)]

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

[akr (Akira Tanaka)]

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.

[mame (Yusuke Endoh)]

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 +

[mame (Yusuke Endoh)]

• 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]

[nobu (Nobuyoshi Nakada)]

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

[nagachika (Tomoyuki Chikanaga)]

• 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.

[usa (Usaku NAKAMURA)]

• 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.