Opened 6 weeks ago

Closed 5 weeks ago

#27185 closed defect (fixed)

test/test.c and lround() prototype

Reported by: gvanem Owned by: teor
Priority: Medium Milestone: Tor: 0.3.5.x-final
Component: Core Tor/Tor Version: Tor: 0.2.2.2-alpha
Severity: Minor Keywords: clang-cl warning regression? windows fast-fix 029-backport 032-backport 033-backport 034-backport
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

First off, I have to express my frustration with this Trac-system. Clumsy as hell; the Markdown support is a joke. I'd wish you'd moved to Github or Gitlab.

Okay. To the issue. Compiling test/test.c using MSVC-2017 or clang-cl, spits out this warning:

test.c(42,10):  warning: 'lround' redeclared without 'dllimport' attribute: previous 'dllimport' ignored [-Winconsistent-dllimport]
long int lround(double x);

From MSVC:

test.c(42): warning C4273: 'lround': inconsistent dll linkage
f:\ProgramFiler-x86\WindowsKits\Include\10.0.16299.0\ucrt\corecrt_math.h(520): 
note: see previous definition of 'lround'

For Windows, it's already prototypes as (after C-preprocessing):

__declspec (dllimport)
  long long __cdecl llround (double _X);

Why not use tor_lround() instead?
Same goes for fabs() I guess, but this function is not marked with dllimport!? Seems to be an intrinsic for clang-cl + MSVC.

Child Tickets

Change History (11)

comment:1 Changed 6 weeks ago by gvanem

The C-preprocessed prototype should be:

_declspec (dllimport)
  long __cdecl lround (double _X);

comment:2 in reply to:  description Changed 6 weeks ago by teor

Thanks for reporting this issue.

Replying to gvanem:

First off, I have to express my frustration with this Trac-system. Clumsy as hell; the Markdown support is a joke. I'd wish you'd moved to Github or Gitlab.

Trac uses WikiFormatting, not Markdown.

We tried to move to our own Gitlab instance, but it didn't work out.

We accept pull requests at https://github.com/torproject/tor , but please also open a minimal trac ticket with a title and link to the pull request.

Okay. To the issue. Compiling test/test.c using MSVC-2017 or clang-cl, spits out this warning:

test.c(42,10):  warning: 'lround' redeclared without 'dllimport' attribute: previous 'dllimport' ignored [-Winconsistent-dllimport]
long int lround(double x);

From MSVC:

test.c(42): warning C4273: 'lround': inconsistent dll linkage
f:\ProgramFiler-x86\WindowsKits\Include\10.0.16299.0\ucrt\corecrt_math.h(520): 
note: see previous definition of 'lround'

Our CI uses gcc, otherwise we might have picked up this issue earlier.
Do you think it would be easy to extend our Appveyor CI config to use clang-cl?
https://github.com/torproject/tor/blob/master/.appveyor.yml

For Windows, it's already prototypes as (after C-preprocessing):

__declspec (dllimport)
  long long __cdecl llround (double _X);

Why not use tor_lround() instead?
Same goes for fabs() I guess, but this function is not marked with dllimport!? Seems to be an intrinsic for clang-cl + MSVC.

Does the warning go away if we use tor_lround() instead?

comment:3 Changed 6 weeks ago by teor

Status: newneeds_information

comment:4 Changed 6 weeks ago by teor

Component: Core TorCore Tor/Tor
Keywords: regression? windows added
Milestone: Tor: 0.3.5.x-final

comment:5 Changed 5 weeks ago by gvanem

Does the warning go away if we use tor_lround() instead?

Yes.

comment:6 Changed 5 weeks ago by teor

Keywords: fast-fix 029-backport 032-backport 033-backport 034-backport added
Owner: set to teor
Status: needs_informationassigned

Ok, so the suggested patch is to use tor_lround() and tor_fabs() in the unit tests (and anywhere outside the module that defines them.)

I think we have a script that does these kinds of checks. We should update it too.

We should also backport this change to 0.2.9 and later.

comment:7 Changed 5 weeks ago by gvanem

Okay. Thanks, I'll wait.

BTW. All emails from Trac (for this ticket) got trapped in the Yahoo spam-filter. Hence I saw these replies very late. Not sure why.

comment:8 Changed 5 weeks ago by teor

Status: assignedneeds_review
Version: Tor: 0.3.4.6-rcTor: 0.2.2.2-alpha

See my branch bug27185 on https://github.com/teor2345/tor.git , which removes the lround() declaration, and uses math.h for fabs.

The CI is here:
https://travis-ci.org/teor2345/tor/builds/419553907
https://ci.appveyor.com/project/teor2345/tor/build/1.0.81

comment:9 Changed 5 weeks ago by nickm

Status: needs_reviewmerge_ready

LGTM; should we backport?

comment:10 Changed 5 weeks ago by teor

Sure! It's just a warning, but they're worth getting rid of.

See my branch bug27185-029, the CI is here:
https://travis-ci.org/teor2345/tor/builds/419891982

The merge into master has conflicts due to header renames. But it's relatively simple, and should match my bug27185 branch.

comment:11 Changed 5 weeks ago by nickm

Resolution: fixed
Status: merge_readyclosed

okay, merged to 0.2.9 and forward!

Note: See TracTickets for help on using tickets.