search

nmslib / hnswlib

Header-only C++/python library for fast approximate nearest neighbors
https://github.com/nmslib/hnswlib
Apache License 2.0
4.12k stars 609 forks source link

Fixed linking error with Visual Studio #466

Closed jrade closed 1 year ago

jrade commented 1 year ago

If you include the header hnswlib/hnswlib.h in multiple C++ files and compile with Visual Studio then you get the following error

error LNK2005: "void __cdecl cpuid(int * const,int,int)" (?cpuid@@YAXQEAHHH@Z) already defined in Pch.obj fatal error LNK1169: one or more multiply defined symbols found

This is easy to fix by declaring the function cpuid() static

jrade commented 1 year ago

I used the keyword static since several other functions in the library are declared static for the same reason. But I think it would be better to declare all these functions inline instead. Why?

It is all about the ODR, the one-definition-rule. It says that a function can only be defined once. If you define a function in a header and include that header in 5 source files, then you have defined it 5 times which violates the ODR.

This is usually handled by declaring the function inline. Then you tell the compiler that all 5 definitions are identical definitions of one and the the same function, which is what you want here. Note that the keyword inline does not have anything to do with inlining in modern C++. It is all about the ODR. (See https://en.cppreference.com/w/cpp/language/inline)

If you handle this by declaring the function static, then you tell the compiler that these are 5 different functions that just happen to have the same name, which is not what you want here. The compiler will generate 5 different function bodies. The linker will probably notice that they are identical and throw away all but one of them. This probably works but is inefficient and a bit obscure.

yurymalkov commented 1 year ago

Hi @jrade, Thank you for the PR and the comment. The PR in its current form seems to be a duplicate of https://github.com/nmslib/hnswlib/pull/463 which we planned to merge.

I wonder, in case we switch it to inline instead of static, is there a (probably very small) chance of cpuid signature collision with other libraries?

jrade commented 1 year ago

There is such a risk regardless of whether the function is declared inline or static. Is it possible to move the functions at the beginning of hnswlib.h inside the namespace hnswlib?

jrade commented 1 year ago

I made another commit where I changed all static functions to inline and moved all functions inside namespace hnswlib

yurymalkov commented 1 year ago

Hey @jrade,

It looks like tests are not passing with

D:\a\hnswlib\hnswlib\hnswlib\space_l2.h(147): error C7525: inline variables require at least '/std:c++17'

Is it a part of C++17?

jrade commented 1 year ago

That's right. I have reverted those lines, so now it should be C++11 only.

yurymalkov commented 1 year ago

Hi @jrade. It still does not pass the tests. Can you please look into it?

jrade commented 1 year ago

Missed one line. Fixed now.

jrade commented 1 year ago

Looking some more at the code I get rather concerned. It is not safe to use static variables in inline functions. I will cancel this pull request and submit another one when I have looked more carefully at the matter. In the meantime, just accept PR #463.

yurymalkov commented 1 year ago

Got it, thank you!