Opened 3 years ago

Closed 3 years ago

#19079 closed defect (fixed)

clang -m32 -ftrapv seems buggy with 64-bit signed integer multiply

Reported by: nickm Owned by: nickm
Priority: High Milestone: Tor: 0.2.9.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: TorCoreTeam201605
Cc: Actual Points: .1
Parent ID: Points: .1
Reviewer: Sponsor:

Description

If you try building with clang -m32 -ftrapv, you get a bunch of these warnings:

12:39:59 /home/jenkins/workspace/tor-ci-linux-master-clang/ARCHITECTURE/i386/SUITE/sid/src/or/dirvote.c:828: undefined reference to `__mulodi4'
12:39:59 /home/jenkins/workspace/tor-ci-linux-master-clang/ARCHITECTURE/i386/SUITE/sid/src/or/dirvote.c:828: undefined reference to `__mulodi4'
12:39:59 /home/jenkins/workspace/tor-ci-linux-master-clang/ARCHITECTURE/i386/SUITE/sid/src/or/dirvote.c:855: undefined reference to `__mulodi4'
12:39:59 /home/jenkins/workspace/tor-ci-linux-master-clang/ARCHITECTURE/i386/SUITE/sid/src/or/dirvote.c:930: undefined reference to `__mulodi4'
12:39:59 /home/jenkins/workspace/tor-ci-linux-master-clang/ARCHITECTURE/i386/SUITE/sid/src/or/dirvote.c:930: undefined reference to `__mulodi4'
12:39:59 src/or/libtor-testing.a(src_or_libtor_testing_a-dirvote.o):/home/jenkins/workspace/tor-ci-linux-master-clang/ARCHITECTURE/i386/SUITE/sid/src/or/dirvote.c:834: more undefined references to `__mulodi4' follow

Since -ftrapv is now on by default, that's a problem!

Others have seen errors like this before; for example see:

https://bugs.chromium.org/p/chromium/issues/detail?id=503229

and

https://llvm.org/bugs/show_bug.cgi?id=14469

I'm calling this must-fix-for-029, since otherwise we just broke 32-bit clang.

Child Tickets

Change History (5)

comment:1 Changed 3 years ago by nickm

Owner: set to nickm
Status: newaccepted

comment:2 Changed 3 years ago by nickm

Actual Points: .1
Points: .1
Status: acceptedneeds_review

I have a fix in link_ftrapv_clang32.

comment:3 Changed 3 years ago by nickm

Keywords: TorCoreTeam201605 added

comment:4 Changed 3 years ago by teor

Status: needs_reviewneeds_revision

code review:

6d6c828 Include mulodi4 in libor_ctime when it fixes clang -m32 -ftrapv

You forgot an atoi on argv[3], and you said on IRC it won't matter.

3303460 Add mulodi4 source to src/ext

Do we need unit tests for this?
Or should we just assume the llvm source is ok?
(I wonder about bugs like http://kqueue.org/blog/2013/09/17/cltq/ , the specific instance isn't an issue here because the types are all the same, but in general, these sort of subtle compiler bugs concern me.)

Other than that, looks good to merge.

comment:5 Changed 3 years ago by nickm

Resolution: fixed
Status: needs_revisionclosed

I'm assuming that the llvm source is okay. :) Merging to master.

Note: See TracTickets for help on using tickets.