Page MenuHome

Fix failing cycles_render_graph_finalize_test when building with clang.
ClosedPublic

Authored by Ray molenkamp (LazyDodo) on Jan 17 2020, 6:08 PM.

Details

Summary

Story time!

I am unsure why this was allowed to happen and why none of the compiler/linkers have warned us about this.

However the root of where it all goes wrong is in gflags_declare.h where this little gem is hiding

#ifndef GFLAGS_IS_A_DLL
#  define GFLAGS_IS_A_DLL 1
#endif

// We always want to import the symbols of the gflags library.
#ifndef GFLAGS_DLL_DECL
#  if GFLAGS_IS_A_DLL && defined(_MSC_VER)
#    define GFLAGS_DLL_DECL __declspec(dllimport)
#  else
#    define GFLAGS_DLL_DECL
#  endif
#endif

Which implies that unless you define GFLAGS_DLL_DEFINE_FLAG it is going to cram a __declspec(dllimport) on all functions
which for the compiler means changing the function signature from somefunction to __imp_somefunction and normally when that happens and you try to link you get a wonderful linker error saying "I can't find this import."

Now for some reason for gflags this did not work, symbol got renamed, but... no linker error. The MS linker somehow found the static lib and fudged it and make it work. (I'm unsure about what the exact logic here is, or how, or why this is even allowed)

Now clang's lld linker did the same thing but seemingly has a bug, not sure what triggers it, but for just SetCommandLineOption it generates a jmp to __imp_SetCommandLineOption however at that call-site, there is no code, no forwarding jmp, no nothing! It starts executing random garbage and we end up crashing the test.

So who's at fault here? Mixed bag...

  1. US: we should not have put dllimports on functions that has no business getting imported.
  2. MSVC: linker warning/error altering us we were doing "the stupid" would have been nice.
  3. Clang: lld seems buggy

Now this is a fix in /extern so what I did is probably not the right way to get this fix into the codebase

@Sergey Sharybin (sergey) do you want to take it from here?

Diff Detail

Repository
rB Blender

Event Timeline

extern/gflags/README.blender has a line saying Forced disabled GFLAGS_IS_A_DLL, which seems was only done in extern/gflags/src/gflags/config.h but not extern/gflags/src/gflags/gflags_declare.h.

I'd say update that comment to indicate it needs to be changed in two files and this is good to go.

  • updated README.blender
This revision is now accepted and ready to land.Jan 17 2020, 6:53 PM