Page MenuHome

Windows: Support building with clang.
ClosedPublic

Authored by LazyDodo (LazyDodo) on May 9 2018, 7:50 PM.

Details

Summary

This is the bare minimum to get it to build.

Things that work:

  • It's able to make a build and run the test suite with just freestyle_stroke_material.blend failing (but that also fails with regular msvc)
  • It's faster than msvc (bmw27 on my polluted dev box, msvc 10:28.21 vs clang 09:55.08)

Things that are broken:

  • BF_alembic + BF_openvdb
    • Just boost unhappy about some things, I can either patch this in our boost version or we can update to a newer boost (I think this got fixed in 1.62) but i can imagine the other platforms won't be thrilled about a boost update.
  • OSL
    • has some kind of linking error, incompatibilities due to __cplusplus being set to '199711L' for the pre-compiled libs and clang has a higher value, causing a conflict between std::shared_ptr and boost::shared_ptr, I can work around this in the osl headers.
  • Bullet
    • has some sse related issues, couldn't get this building easily, but didn't look too hard either.
  • Cycles
    • the avx kernel has a stray andn instruction ( from the bmi instruction set , available on haswell and up) even adding -no-bmi doesn't seem to change things) causing issues in my ivy-bridge. See T55054 for details. fixed, but the perf regression for msvc remains, but thats better handled in T55054.
  • So many warnings, will have to do check the flags that are being passed. down from > 10.000 to a couple of hundreds that seem like genuine warns to me.

things that'll probably be misunderstood:

  • This doesn't free us from msvc. Clang on windows uses the msvc headers+libs+c runtime and is faking the ms compiler by supplying it's own stub for cl.exe,

I tested with the following setup, others may work, but that's untested for now.

  • msvc 2017
  • llvm/clang 6.0.0
  • llvm-vs2017-integration [1]

I played a little with asan, it seems to work for /MT but not /MTd, there's some linker issues with it and it doesn't resolve addresses to line numbers yet, but i hope to get this working. asan works, have added an 'asan' configuration in the visual studio solution so it's as easy as changing from debug->release, however for line number information you need a copy of llvm-symbolizer which for some reason doesn't ship with the official llvm distro.

[1] https://github.com/arves100/llvm-vs2017-integration

Diff Detail

Repository
rB Blender

Event Timeline

LazyDodo (LazyDodo) edited the summary of this revision. (Show Details)May 9 2018, 8:49 PM
  • windows: fix building bullet with clang on windows.

the BT_ALLOW_SSE4 define is a perculiar one, aparently we've been emitting sse4 intrinsics for bullet and nobody noticed..

LazyDodo (LazyDodo) edited the summary of this revision. (Show Details)May 9 2018, 11:04 PM
  • add asan configuration and clean up some warnings.
LazyDodo (LazyDodo) edited the summary of this revision. (Show Details)May 12 2018, 5:37 PM
LazyDodo (LazyDodo) edited the summary of this revision. (Show Details)May 12 2018, 9:31 PM
LazyDodo (LazyDodo) edited the summary of this revision. (Show Details)
LazyDodo (LazyDodo) edited the summary of this revision. (Show Details)May 13 2018, 9:58 PM
  • fix debug build with clang and bullet
Campbell Barton (campbellbarton) requested changes to this revision.EditedMay 16 2018, 9:54 AM

While Clang support on windows could be useful and Asan is certainly handy, I'd rather not add Asan as a new configuration type, at the very least it could be separated from supporting clang on windows.

Asan has multiple incompatible options, so it's not just a configuration you enable or not.

Also, you can use asan with RelWithDebInfo or Debug.. etc.

This revision now requires changes to proceed.May 16 2018, 9:54 AM

While Clang support on windows could be useful and Asan is certainly handy, I'd rather not add Asan as a new configuration type, at the very least it could be separated from supporting clang on windows.

I don't understand, do you mean like commiting asan support in a seperate commit? or do you want it hidden behind a cmake option?

Asan has multiple incompatible options, so it's not just a configuration you enable or not.

Can you elaborate on this?

Also, you can use asan with RelWithDebInfo or Debug.. etc.

Debug won't work on windows, asan only supports /MT and /MD, the debug crt's are currently not supported, Release is useless given it lacks symbol information, so the only thing you can really use it with it on is a RelWithDebInfo build with the optimizer disabled (so you can still debug and inspect variables etc), which will be what most people will want to get usable results.

The configuration I added as a convenience given it's the right settings for 'most' developers, without them having to figure out the limitations and oddities. More experienced developers are still free not to use this and fiddle with the cflags own their own to enable asan (any of the other supported sanatizers)

Just to be sure there's no mis-understandings, when i say 'add an asan configuration' what I mean is , add asan to this list of build configurations.

Only on windows, and even then only when building with clang, none of the other build configurations and platforms will even be aware this exists..

While Clang support on windows could be useful and Asan is certainly handy, I'd rather not add Asan as a new configuration type, at the very least it could be separated from supporting clang on windows.

I don't understand, do you mean like commiting asan support in a seperate commit? or do you want it hidden behind a cmake option?

I'd rather review this separately. Even before discussing how best to expose it - if at all.

Asan has multiple incompatible options, so it's not just a configuration you enable or not.

Can you elaborate on this?

eg:

-fsanitize=thread can't be used with -fsanitize=address/leak

Also, you can use asan with RelWithDebInfo or Debug.. etc.

Debug won't work on windows, asan only supports /MT and /MD, the debug crt's are currently not supported, Release is useless given it lacks symbol information, so the only thing you can really use it with it on is a RelWithDebInfo build with the optimizer disabled (so you can still debug and inspect variables etc), which will be what most people will want to get usable results.
The configuration I added as a convenience given it's the right settings for 'most' developers, without them having to figure out the limitations and oddities. More experienced developers are still free not to use this and fiddle with the cflags own their own to enable asan (any of the other supported sanatizers)
Just to be sure there's no mis-understandings, when i say 'add an asan configuration' what I mean is , add asan to this list of build configurations.


Only on windows, and even then only when building with clang, none of the other build configurations and platforms will even be aware this exists..

Why have asan for windows only? .. but again, this is reason not to mix clang & asan in patch.

-based on master so tests can be run
-removed asan
-fixed cycles stray avx2 instruction.

While Clang support on windows could be useful and Asan is certainly handy, I'd rather not add Asan as a new configuration type, at the very least it could be separated from supporting clang on windows.

I don't understand, do you mean like commiting asan support in a seperate commit? or do you want it hidden behind a cmake option?

I'd rather review this separately. Even before discussing how best to expose it - if at all.

Ok I removed it for the sake of moving things forward, however I'd like to see this back sooner rather than later, enabling asan on windows is *NOT* as easy as just passing a flag like it is on linux (ie not all runtimes work, you have to manually link the asan support lib, asan's hook trampoline is too big and ruins the execution of wcslen causing instant crash when you start blender)

Asan has multiple incompatible options, so it's not just a configuration you enable or not.

Can you elaborate on this?

eg:
-fsanitize=thread can't be used with -fsanitize=address/leak

I had only exposed -fsanitize=address only since it is what most developper will need, if you feel experimental or know what you are doing there's nothing stopping a more experienced dev from fiddling with the cflags and getting a build just right to their specifications, however just because more experienced devs exist is no good reason to hide 'this will work for most' configuration from everybody else.

Only on windows, and even then only when building with clang, none of the other build configurations and platforms will even be aware this exists..

Why have asan for windows only? .. but again, this is reason not to mix clang & asan in patch.

I'm the windows maintainer, I do windows things,I don't have linux to test on, I don't have mac to test on, linux devs might be comfortable fiddling with cmake and environment flags, but most windows devs certainly aren't, that's why we have pre-compiled libs, and a make.bat that pretty much takes all guessing work out of getting a working build.

So when I add new compiler support and there are features that are handy, but complex to setup, that I can make easily available in the IDE , I add them! The lower the barrier is for people to write great code, or fix bugs the better!!

On top of that, I'm having a hard enough time getting any code reviews (*cough D3079 *cough* D3039 *cough*) so if it's slightly related i have bigger odds combining the features into a single patch.

LazyDodo (LazyDodo) edited the summary of this revision. (Show Details)May 17 2018, 4:06 PM
LazyDodo (LazyDodo) edited the summary of this revision. (Show Details)
LazyDodo (LazyDodo) edited the summary of this revision. (Show Details)
  • fix stray character in platform/platform_win32.cmake
Campbell Barton (campbellbarton) requested changes to this revision.May 17 2018, 4:39 PM
Campbell Barton (campbellbarton) added inline comments.
CMakeLists.txt
1625

Why not use this, does clang not support gnu extensions on windows?

extern/bullet2/src/LinearMath/btScalar.h
16

Should be added to extern/bullet2/patches

source/creator/CMakeLists.txt
649 ↗(On Diff #11014)

not needed

This revision now requires changes to proceed.May 17 2018, 4:39 PM
  • -fix stray ; in source/creator/CMakeLists.txt
  • add bullet patches
LazyDodo (LazyDodo) marked 2 inline comments as done.May 17 2018, 4:51 PM
LazyDodo (LazyDodo) added inline comments.
CMakeLists.txt
1625

It's 'complicated' since it is still mostly bolted on top of the msvc headers+libs they ship a 'drop in' replacement for microsofts cl.exe that'll understand *some* but not *all* of the parameters visual studio will emit and will translate them into their clang variants, it'll also understand *some*, but not *all* clang parameters.

Since they are going out of their way to emulate the ms compiler where there is no way to turn off C++11/14 support (always on!) these switches aren't supported.

updated with feedback.

since there's a healthy back and forth going on, here are the patches required for the libs.

P689

the one in OSL is absolutely required, and we can't upstream , it is due to our binary libs being build with regular msvc which uses the boost headers, and clang trying to use std::shared_ptr .

for the boost patches there's 2 options, 1) we change it locally in our boost 1.60 headers, 2) we upgrade to a newer boost.

any patches we apply to our libs will need to be integrated into blender\build_files\build_environment and should be part of this diff if we chose to go that way.

  • fix unused variable warnings coming out of bf_rna
  • deal with the bulk of the warnings

This deals with the most chatty of warnings, there's still a bunch left but those seem genuine problems.

LazyDodo (LazyDodo) edited the summary of this revision. (Show Details)May 17 2018, 9:15 PM
  • Merge remote-tracking branch 'origin/master' into clang_master
  • Fix copying of the runtime dll's for clang
CMakeLists.txt
1625

space after # elsewhere too.

source/blender/blenkernel/intern/rigidbody.c
1661

This change is missing for pop.

LazyDodo (LazyDodo) marked an inline comment as done.
  • Updated with feedback.
LazyDodo (LazyDodo) marked an inline comment as done.May 22 2018, 4:20 PM
  • Merge remote-tracking branch 'origin/master' into clang_master

catch up with master

  • prevent building with clang and ninja for now.
CMakeLists.txt
1614–1615

Would rather include this in the conditional, eg:

if(
    CMAKE_COMPILER_IS_GNUCC OR
    (CMAKE_C_COMPILER_ID MATCHES "Clang" AND (NOT MSVC)) OR
    (CMAKE_C_COMPILER_ID MATCHES "Intel")
)
    # Use C99 + GNU extensions, works with GCC, Clang, ICC
    --- snip ---
1625

Think its worth noting this in a short comment, since its not obvious.

extern/Eigen3/Eigen/src/Core/arch/SSE/PacketMath.h
337

Should be added to extern/Eigen3/patches.

  • Updated with feedback.
  • Added eigen3 patch.
LazyDodo (LazyDodo) marked 2 inline comments as done.May 27 2018, 6:22 PM
LazyDodo (LazyDodo) added inline comments.
extern/Eigen3/Eigen/src/Core/arch/SSE/PacketMath.h
337

added the patch, no need to upstream since they already fixed it on their own.

This revision is now accepted and ready to land.May 27 2018, 6:32 PM
LazyDodo (LazyDodo) marked an inline comment as done.
  • Include lib patches needed
  • Disable clang for X86.
This revision was automatically updated to reflect the committed changes.