Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix typos in #if and nonstandard implicit cast. #310

Closed
wants to merge 1 commit into from

Conversation

lyuma
Copy link
Contributor

@lyuma lyuma commented Nov 9, 2020

Regarding the changes in math.h, it appears that _MSC_VER was typoed in preprocessor statements in two different ways: once as __MSV_VER and again as __MSC_VER (note the double underscore) which means they will be treated as 0. Combined with < or <=, it means that the check will be true on all compilers building for windows.

The typos in math.h (and missing defined(_MSC_VER)) caused the following errors when cross-compiling with llvm-mingw on a Linux system:

In file included from kernels/common/accelset.cpp:5:
In file included from kernels/common/scene.h:16:
In file included from kernels/common/scene_subdiv_mesh.h:11:
In file included from kernels/common/../subdiv/patch.h:6:
In file included from kernels/common/../subdiv/catmullclark_patch.h:6:
kernels/common/../subdiv/catmullclark_ring.h:365:20: error: call to 'isinf' is ambiguous
      if (unlikely(std::isinf(vertex_crease_weight)))
                   ^~~~~~~~~~
kernels/common/../../common/tasking/../sys/platform.h:152:48: note: expanded from macro 'unlikely'
#define unlikely(expr) __builtin_expect((bool)(expr),false)
                                               ^~~~
/opt/llvm-mingw/x86_64-w64-mingw32/include/c++/v1/math.h:491:1: note: candidate function
isinf(float __lcpp_x) _NOEXCEPT { return __libcpp_isinf(__lcpp_x); }
^
kernels/common/../../common/algorithms/../math/math.h:19:22: note: candidate function
  __forceinline bool isinf ( const float x ) { return _finite(x) == 0; }

The second instance guards what appear to be fallback implementations of roundf and nextafter.

One concern I have is these Visual Studio 2013(?) implementations of nextafter(float, float) and roundf(float) are actually wildly incorrect, and specifically roundf was likely being used on all windows builds:

  • nextafter(x, -1.0) was unused except in tutorial code, but if it were used, as in tutorials/common/texture/texture2d.cpp, it would cause the texture dimensions to be multiplied by 0.9
  • roundf(float x) seems mostly correct for float values which are representable as a 32-bit integer, but for those which are not, will cause some form of overflow. This seems like potential for bugs.

This PR will inadvertently fix these issues, by fixing the condition for the "fallback" implementations to be used. Hopefully nothing depended on the overflow mechanic of the old roundf implementation...


Regarding the other change, the missing (void*) cast in common/sys/library.cpp is apparently a non-standard Microsoft extension, allowed only in the Microsoft-compatible setting of clang, and not by default. Using the same command line as on Windows, this implicit cast from FARPROC to void* causes the following error.

common/sys/library.cpp:30:12: error: cannot initialize return object of type 'void *' with an rvalue of type 'FARPROC' (aka 'long long (*)()')
    return GetProcAddress(HMODULE(lib),sym.c_str());
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.

By adding -fms-compatibility to the command line on a clang build, it is possible to reduce this to a warning instead of an error:

warning: implicit conversion between pointer-to-function and pointer-to-object is a Microsoft extension [-Wmicrosoft-cast]

However, it would be preferable to solve this by performing an explicit cast to void*.

Collectively, these two fixes allow embree to be cross compiled using llvm-mingw on Linux without any non-standard options.

@lyuma lyuma force-pushed the cross_compile_fixes branch from 1ac976c to 922dfc0 Compare November 9, 2020 13:21
Collectively, these two fixes allow embree to be cross compiled using llvm-mingw on Linux:
Note that in addition to fixing the typo with _MSC_VER, the defined check must be added to avoid undefined symbols acting as 0.
@lyuma lyuma force-pushed the cross_compile_fixes branch from 922dfc0 to 5dd51e1 Compare November 9, 2020 13:22
lyuma added a commit to V-Sekai/godot that referenced this pull request Nov 9, 2020
Fix typos in #if and nonstandard implicit cast.
Collectively, these two fixes allow embree to be cross compiled using llvm-mingw on Linux:
Note that in addition to fixing the typo with _MSC_VER, the defined check must be added to avoid undefined symbols acting as 0.
See RenderKit/embree#310
@svenwoop
Copy link
Collaborator

Just merged this into our internal devel branch. Fix will be in the next release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants