Fix T55054: Resolve ODR violation in cycles/msvc
ClosedPublic
Actions

Authored by Ray Molenkamp (LazyDodo) on Jul 23 2019, 7:06 PM.

Details

Reviewers

Brecht Van Lommel (brecht)
Sergey Sharybin (sergey)

Commits

rC9933b10a15c2: Fix T55054: possible use of unsupported instructions in Cycles texture code
rBea8e0df67271: Fix T55054: possible use of unsupported instructions in Cycles texture code

Summary

See T55054 for details, I can no longer reproduce the crash however using
dumpbin on the object files one can easily validate if the symbols are
public or private.

before:

2CA 00000000 SECTDC notype ()    External     | ?interp_3d_tricubic@?$TextureInterpolator@Vhalf@ccl@@@ccl@@SA?AUfloat4@2@AEBUTextureInfo@2@MMM@Z (public: static struct ccl::float4 __cdecl ccl::TextureInterpolator<class ccl::half>::interp_3d_tricubic(struct ccl::TextureInfo const &,float,float,float))
2CF 00000000 SECTDD notype ()    External     | ?interp_3d_tricubic@?$TextureInterpolator@E@ccl@@SA?AUfloat4@2@AEBUTextureInfo@2@MMM@Z (public: static struct ccl::float4 __cdecl ccl::TextureInterpolator<unsigned char>::interp_3d_tricubic(struct ccl::TextureInfo const &,float,float,float))
2D4 00000000 SECTDE notype ()    External     | ?interp_3d_tricubic@?$TextureInterpolator@G@ccl@@SA?AUfloat4@2@AEBUTextureInfo@2@MMM@Z (public: static struct ccl::float4 __cdecl ccl::TextureInterpolator<unsigned short>::interp_3d_tricubic(struct ccl::TextureInfo const &,float,float,float))
2D9 00000000 SECTDF notype ()    External     | ?interp_3d_tricubic@?$TextureInterpolator@M@ccl@@SA?AUfloat4@2@AEBUTextureInfo@2@MMM@Z (public: static struct ccl::float4 __cdecl ccl::TextureInterpolator<float>::interp_3d_tricubic(struct ccl::TextureInfo const &,float,float,float))
2DE 00000000 SECTE0 notype ()    External     | ?interp_3d_tricubic@?$TextureInterpolator@Uhalf4@ccl@@@ccl@@SA?AUfloat4@2@AEBUTextureInfo@2@MMM@Z (public: static struct ccl::float4 __cdecl ccl::TextureInterpolator<struct ccl::half4>::interp_3d_tricubic(struct ccl::TextureInfo const &,float,float,float))
2E3 00000000 SECTE1 notype ()    External     | ?interp_3d_tricubic@?$TextureInterpolator@Uuchar4@ccl@@@ccl@@SA?AUfloat4@2@AEBUTextureInfo@2@MMM@Z (public: static struct ccl::float4 __cdecl ccl::TextureInterpolator<struct ccl::uchar4>::interp_3d_tricubic(struct ccl::TextureInfo const &,float,float,float))
2E8 00000000 SECTE2 notype ()    External     | ?interp_3d_tricubic@?$TextureInterpolator@Uushort4@ccl@@@ccl@@SA?AUfloat4@2@AEBUTextureInfo@2@MMM@Z (public: static struct ccl::float4 __cdecl ccl::TextureInterpolator<struct ccl::ushort4>::interp_3d_tricubic(struct ccl::TextureInfo const &,float,float,float))
2ED 00000000 SECTE3 notype ()    External     | ?interp_3d_tricubic@?$TextureInterpolator@Ufloat4@ccl@@@ccl@@SA?AUfloat4@2@AEBUTextureInfo@2@MMM@Z (public: static struct ccl::float4 __cdecl ccl::TextureInterpolator<struct ccl::float4>::interp_3d_tricubic(struct ccl::TextureInfo const &,float,float,float))

after:

2CA 00000000 SECTDC notype ()    Static       | ?interp_3d_tricubic@?$TextureInterpolator@Vhalf@ccl@@@?A0x9A03A44B@ccl@@SA?AUfloat4@2@AEBUTextureInfo@2@MMM@Z (public: static struct A0x9A03A44B::float4 __cdecl ccl::`anonymous namespace'::TextureInterpolator<class ccl::half>::interp_3d_tricubic(struct A0x9A03A44B::TextureInfo const &,float,float,float))
2CF 00000000 SECTDD notype ()    Static       | ?interp_3d_tricubic@?$TextureInterpolator@E@?A0x9A03A44B@ccl@@SA?AUfloat4@2@AEBUTextureInfo@2@MMM@Z (public: static struct A0x9A03A44B::float4 __cdecl ccl::`anonymous namespace'::TextureInterpolator<unsigned char>::interp_3d_tricubic(struct A0x9A03A44B::TextureInfo const &,float,float,float))
2D4 00000000 SECTDE notype ()    Static       | ?interp_3d_tricubic@?$TextureInterpolator@G@?A0x9A03A44B@ccl@@SA?AUfloat4@2@AEBUTextureInfo@2@MMM@Z (public: static struct A0x9A03A44B::float4 __cdecl ccl::`anonymous namespace'::TextureInterpolator<unsigned short>::interp_3d_tricubic(struct A0x9A03A44B::TextureInfo const &,float,float,float))
2D9 00000000 SECTDF notype ()    Static       | ?interp_3d_tricubic@?$TextureInterpolator@M@?A0x9A03A44B@ccl@@SA?AUfloat4@2@AEBUTextureInfo@2@MMM@Z (public: static struct A0x9A03A44B::float4 __cdecl ccl::`anonymous namespace'::TextureInterpolator<float>::interp_3d_tricubic(struct A0x9A03A44B::TextureInfo const &,float,float,float))
2DE 00000000 SECTE0 notype ()    Static       | ?interp_3d_tricubic@?$TextureInterpolator@Uhalf4@ccl@@@?A0x9A03A44B@ccl@@SA?AUfloat4@2@AEBUTextureInfo@2@MMM@Z (public: static struct A0x9A03A44B::float4 __cdecl ccl::`anonymous namespace'::TextureInterpolator<struct ccl::half4>::interp_3d_tricubic(struct A0x9A03A44B::TextureInfo const &,float,float,float))
2E3 00000000 SECTE1 notype ()    Static       | ?interp_3d_tricubic@?$TextureInterpolator@Uuchar4@ccl@@@?A0x9A03A44B@ccl@@SA?AUfloat4@2@AEBUTextureInfo@2@MMM@Z (public: static struct A0x9A03A44B::float4 __cdecl ccl::`anonymous namespace'::TextureInterpolator<struct ccl::uchar4>::interp_3d_tricubic(struct A0x9A03A44B::TextureInfo const &,float,float,float))
2E8 00000000 SECTE2 notype ()    Static       | ?interp_3d_tricubic@?$TextureInterpolator@Uushort4@ccl@@@?A0x9A03A44B@ccl@@SA?AUfloat4@2@AEBUTextureInfo@2@MMM@Z (public: static struct A0x9A03A44B::float4 __cdecl ccl::`anonymous namespace'::TextureInterpolator<struct ccl::ushort4>::interp_3d_tricubic(struct A0x9A03A44B::TextureInfo const &,float,float,float))
2ED 00000000 SECTE3 notype ()    Static       | ?interp_3d_tricubic@?$TextureInterpolator@Ufloat4@ccl@@@?A0x9A03A44B@ccl@@SA?AUfloat4@2@AEBUTextureInfo@2@MMM@Z (public: static struct A0x9A03A44B::float4 __cdecl ccl::`anonymous namespace'::TextureInterpolator<struct ccl::float4>::interp_3d_tricubic(struct A0x9A03A44B::TextureInfo const &,float,float,float))

Diff Detail

Repository

rB Blender

Branch

tmp_cycles_odr_fix (branched from master)

Build Status

Buildable 4143
Build 4143: arc lint + arc unit

Event Timeline

Ray Molenkamp (LazyDodo) created this revision.Jul 23 2019, 7:06 PM

Harbormaster completed remote builds in B4143: Diff 16569.Jul 23 2019, 7:06 PM

Brecht Van Lommel (brecht) accepted this revision.Jul 23 2019, 7:29 PM

Brecht Van Lommel (brecht) added inline comments.

intern/cycles/kernel/kernels/cpu/kernel_cpu_image.h
21	Add a comment like: /* Make template functions private so symbols don't conflict between kernels with different instruction sets. */

This revision is now accepted and ready to land.Jul 23 2019, 7:29 PM

This is quite dangerous thing to do and is not something to be spreaded into more code, so it is to be explicitly stated.

Did anyone try to define CCL_NAMESPACE_BEGIN to namespace ccl { namespace <CPU_MICROARCH> { instead? That seems more correct approach to me.

Played with it around the time i submitted T55054, I think i ran into a of build/linker errors (but I had to admit, it's been a while, all i remember was it did not have a 'happy' time right away), but taking a fresh look this week, a 2 line line anonymous namespace fix seemed more elegant.

I can take another look at namespaces if you want?

For a longer term i think one of us would end up into looking namespaces solution. Quite sure this issue will keep re-appearing with the growths of kernel.

For a shorter term i'm fine with this solution, but add a comment /TODO around it.

Short term this really doesn't need a fix, this has been broken for years, and unless changes MS their linker behavior or we drastically change our cmake organisation, this will keep on working this just fine. (disclaimer: until it doesn't)

if we rather go the namespace route when we have more time, it probably be better just to abandon this diff.

We should definitely also look into the undesirable math.h behavior outlined here , it's beyond creepy

Hold on, i am a bit confused. Is there a known crash which is happening? And was it only localized to texture interpolator?
If the crash is fixed with this change, i don't see a reason to discard this patch. In this specific case having multiple private implementations of interpolator is better than crash. And in this specific case i don't see other risks. My initial concern was about making it clear in a comment that this is not proven-to-be-always-way-to-go solution to the issue.

if we rather go the namespace route when we have more time

As you've pointed out, math and other functions also needs to be sanitized. It just feels that putting microarchitecture into CCL_NAMESPACE_BEGIN is the safest way to go. But i can see how it might cause some re-organization to have C++ code implemented in a non-headers linked correctly (util_simd.cpp, util_debug.cpp, and Embree/OpenVDB integration are coming in mind).

The unknowness to me here is the "more time" part. But that shouldn't be a stopper for a simpler/existing fix for a bug.

In D5326#120321, @Sergey Sharybin (sergey) wrote:

Hold on, i am a bit confused. Is there a known crash which is happening? And was it only localized to texture interpolator?

There was a crash, when building with clang on windows, an avx2 instruction would leak into the avx kernel we mitigated it by forcing clang to inline the function (like we do with gcc, but not with msvc since it balloons the build time per kernel to 20 minutes)

when building with regular msvc, the same ODR violation exists but the linker reliably picks the sse2 variant of interp_3d_tricubic which is while not great, it's not the end of the world either.

this patch just gives all kernels their own implementation again, if we want to the namespace route, we can safely discard this diff and nothing bad will (*crosses fingers* should) happen.

Ray Molenkamp (LazyDodo) mentioned this in D5345: Cleanup: Const correctness of several large arrays..Jul 25 2019, 7:50 PM

Closed by commit rBea8e0df67271: Fix T55054: possible use of unsupported instructions in Cycles texture code (authored by Ray Molenkamp (LazyDodo), committed by Brecht Van Lommel (brecht)). · Explain WhyAug 16 2019, 4:49 PM

This revision was automatically updated to reflect the committed changes.

Brecht Van Lommel (brecht) added a commit: rBea8e0df67271: Fix T55054: possible use of unsupported instructions in Cycles texture code.

Brecht Van Lommel (brecht) added a commit: rC9933b10a15c2: Fix T55054: possible use of unsupported instructions in Cycles texture code.Aug 22 2019, 5:21 PM