Details

Reviewers

Brecht Van Lommel (brecht)
Campbell Barton (campbellbarton)
Sergey Sharybin (sergey)

Commits

rBS9d876960e891: This option limits visibility of the glew.h header to just bf_gpu and…
rB9d876960e891: This option limits visibility of the glew.h header to just bf_gpu and…
rC3042128c651e: Windows: Add support for building with clang
rBac4eec8cb9c6: Windows: Add support for building with clang.
rBS81060ff6b26c: Windows: Add support for building with clang.
rB81060ff6b26c: Windows: Add support for building with clang.

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

Branch

blender2.8_clang (branched from blender2.8)

Build Status

Buildable 1523
Build 1523: arc lint + arc unit

Event Timeline

Ray Molenkamp (LazyDodo) created this revision.May 9 2018, 7:50 PM

Ray Molenkamp (LazyDodo) added reviewers: Brecht Van Lommel (brecht), Campbell Barton (campbellbarton), Sergey Sharybin (sergey).May 9 2018, 7:50 PM

Ray Molenkamp (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..

Harbormaster completed remote builds in B1523: Diff 10756.May 9 2018, 11:03 PM

Ray Molenkamp (LazyDodo) edited the summary of this revision. (Show Details)May 9 2018, 11:04 PM

add asan configuration and clean up some warnings.

Ray Molenkamp (LazyDodo) edited the summary of this revision. (Show Details)May 12 2018, 5:37 PM

Germano Cavalcante (mano-wii) awarded a token.May 12 2018, 5:56 PM

Ray Molenkamp (LazyDodo) edited the summary of this revision. (Show Details)May 12 2018, 9:31 PM

Ray Molenkamp (LazyDodo) edited the summary of this revision. (Show Details)

Ray Molenkamp (LazyDodo) mentioned this in T55054: Cyles: TextureInterpolator Undefined linker behavior - performance regression. .May 13 2018, 9:56 PM

Ray Molenkamp (LazyDodo) edited the summary of this revision. (Show Details)May 13 2018, 9:58 PM

fix debug build with clang and bullet

Stefan Werner (swerner) added a subscriber: Stefan Werner (swerner).May 14 2018, 9:37 PM

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

In D3304#78662, @Campbell Barton (campbellbarton) wrote:

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..

In D3304#78789, @Ray Molenkamp (LazyDodo) wrote:

In D3304#78662, @Campbell Barton (campbellbarton) wrote:

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.

In D3304#78916, @Campbell Barton (campbellbarton) wrote:

In D3304#78789, @Ray Molenkamp (LazyDodo) wrote:

In D3304#78662, @Campbell Barton (campbellbarton) wrote:

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.

Ray Molenkamp (LazyDodo) edited the summary of this revision. (Show Details)May 17 2018, 4:06 PM

Ray Molenkamp (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
1512–1519	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

Ray Molenkamp (LazyDodo) marked 2 inline comments as done.May 17 2018, 4:51 PM

Ray Molenkamp (LazyDodo) added inline comments.

CMakeLists.txt
1512–1519	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.

Ray Molenkamp (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

Campbell Barton (campbellbarton) added inline comments.May 22 2018, 4:04 PM

CMakeLists.txt
1512–1519	space after `#` elsewhere too.
source/blender/blenkernel/intern/rigidbody.c
1661 ↗	(On Diff #11076)	This change is missing for pop.

Updated with feedback.

Ray Molenkamp (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.

Campbell Barton (campbellbarton) added inline comments.May 27 2018, 9:57 AM

CMakeLists.txt
1511–1512	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 ---
1512–1519	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.

Ray Molenkamp (LazyDodo) marked 2 inline comments as done.May 27 2018, 6:22 PM

Ray Molenkamp (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.

Campbell Barton (campbellbarton) accepted this revision.May 27 2018, 6:32 PM

This revision is now accepted and ready to land.May 27 2018, 6:32 PM

Include lib patches needed

Disable clang for X86.

Closed by commit rB81060ff6b26c: Windows: Add support for building with clang. (authored by Ray Molenkamp (LazyDodo)). · Explain WhyMay 28 2018, 10:35 PM

This revision was automatically updated to reflect the committed changes.

Windows: Support building with clang.
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 10756

CMakeLists.txt

build_files/cmake/platform/platform_win32.cmake

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

extern/bullet2/src/LinearMath/btScalar.h

intern/cycles/CMakeLists.txt

make.bat

source/blender/bmesh/CMakeLists.txt

Windows: Support building with clang.ClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 10756

CMakeLists.txt

build_files/cmake/platform/platform_win32.cmake

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

extern/bullet2/src/LinearMath/btScalar.h

intern/cycles/CMakeLists.txt

make.bat

source/blender/bmesh/CMakeLists.txt

Windows: Support building with clang.
ClosedPublic
Actions

Revision Contents
Changeset List