Opened 4 months ago

Closed 3 months ago

#27139 closed defect (fixed)

macOS i386 fails time unit tests

Reported by: teor Owned by: nickm
Priority: Medium Milestone: Tor: 0.3.5.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: regression, macos, i386, 034-backport, arm32, 32-bit
Cc: rl1987 Actual Points:
Parent ID: Points:
Reviewer: catalyst Sponsor:

Description

circuitmux/compute_ticks: [forking] 
  FAIL src/test/test_circuitmux.c:109: assert(rem OP_LT .150000001): 0.1505 vs 0.15
  [compute_ticks FAILED]
mainloop/update_time_jumps: [forking] 
  FAIL src/test/test_mainloop.c:115: expected log to not contain entries  Captured logs:
    1. notice: "Your system clock just jumped 1800 seconds forward; assuming established circuits no longer work.\n"

  [update_time_jumps FAILED]

Should we set up macOS i386 CI to detect issues like this?
Or should we remove macOS i386 from our supported platforms?
(Apple won't support i386 in the next macOS, so in ~3 years time, we won't need to support it.)

Child Tickets

Change History (21)

comment:1 Changed 4 months ago by nickm

Are these intermittent or consistent?

I'd be okay with or without OSX i386 CI.

comment:2 Changed 4 months ago by rl1987

Summary: macOS i386 fails time unit testamacOS i386 fails time unit tests

comment:3 Changed 4 months ago by rl1987

Cc: rl1987 added

comment:4 Changed 4 months ago by teor

Keywords: 035-must 034-must 035-roadmap-proposed added; i386 removed

We must check if this issue affects Linux and BSD on 0.3.4 before releasing 0.3.4.

comment:5 Changed 4 months ago by teor

Keywords: i386 added; 035-must 034-must 035-roadmap-proposed removed

Oops, wrong ticket for that comment.

comment:6 in reply to:  1 Changed 4 months ago by teor

Parent ID: #26987

Replying to nickm:

Are these intermittent or consistent?

Consistent, with exactly those numbers in the tests every time.

I don't see the same failures on macOS x86_64, but I do occasionally see odd time jumps after sleep on x86_64, see #26987. Maybe they are the same bug.

I'd be okay with or without OSX i386 CI.

Same here. Tor Browser doesn't distribute i386 builds on macOS, and Homebrew only supports 64-bit. (MacPorts still builds 320bit tor, though.)

Version 0, edited 4 months ago by teor (next)

comment:7 Changed 4 months ago by nickm

I'd like to fix this, but it's not a much-used platform. We should investigate a little more, and see if it's easy to fix.

comment:8 Changed 4 months ago by nickm

Milestone: Tor: 0.3.5.x-finalTor: 0.3.4.x-final

comment:9 Changed 4 months ago by teor

Milestone: Tor: 0.3.4.x-finalTor: 0.3.5.x-final
Parent ID: #26987

Not related to #26987

comment:10 Changed 4 months ago by teor

Parent ID: #27296

comment:11 Changed 3 months ago by nickm

I've started investigating these more closely.

One thing that would be helpful here would be to know which values mach_timebase_info() puts into its output on this platforms.

comment:12 Changed 3 months ago by nickm

Owner: set to nickm
Status: newaccepted

The first one is probably due to monotime_coarse_diff_msec32_() having some rounding error. I think that should be easy to resolve.

comment:13 Changed 3 months ago by nickm

I think the second one may also be due to rounding error in that function.

comment:14 Changed 3 months ago by nickm

Keywords: 034-backport arm32 32-bit added

comment:15 Changed 3 months ago by nickm

Status: acceptedneeds_review

Okay, I think I have it. The problem was in our 32-bit monotime_coarse_diff_msec32_ implementation: it wasn't precise enough, and it didn't try hard enough to avoid integer overflows. One of our unit tests was assuming an amount of precision it just couldn't provide.

Bugfix in branch bug27139_034; PR at https://github.com/torproject/tor/pull/334 .

I tested this by editing the compat_time.h header so that the monotime_coarse_diff_msec32() inline function would always use the 32-bit implementation.

comment:16 Changed 3 months ago by nickm

Parent ID: #27296

(This turns out not to have anything to do with its ostensible parent ticket)

comment:17 Changed 3 months ago by asn

Reviewer: catalyst

comment:18 Changed 3 months ago by teor

I ran this branch with CC="clang -arch i386", and the necessary dependencies from MacPorts.

make check and make test-network-all passed.

comment:19 Changed 3 months ago by catalyst

Status: needs_reviewmerge_ready

Looks good to me! I only tested using nickm's strategy of forcing the use of the 32-bit computation on 64-bit, and trust teor's evidence.

I'm a little nervous about some of the fixed-point hackery here, but given that it's increasingly unlikely that 32-bit macOS will continue to be relevant, I guess we can fix it later if it turns out to be a problem.

comment:20 in reply to:  19 Changed 3 months ago by teor

Replying to catalyst:

I'm a little nervous about some of the fixed-point hackery here, but given that it's increasingly unlikely that 32-bit macOS will continue to be relevant, I guess we can fix it later if it turns out to be a problem.

I downgraded macOS i386 to Maintained in our supported platforms policy:
https://trac.torproject.org/projects/tor/wiki/org/teams/NetworkTeam/SupportedPlatforms?action=diff&version=14

comment:21 Changed 3 months ago by nickm

Resolution: fixed
Status: merge_readyclosed

Okay, merged to 0.3.4 and forward!

Note: See TracTickets for help on using tickets.