Fix #112933: Bug crash 17.0+ MSVC builds with function passed-by-value arguments captured in lambdas #112616

Merged

Ray molenkamp merged 10 commits from mod_moder/blender:tmp_fix_msvc_bug into main

2023-09-29 16:57:12 +02:00

Iliya Katushenock commented

2023-09-20 10:23:56 +02:00

Member

MSVC builder (Windows platform) have a bug that cause the crash of
the Split Edges node from Geometry Nodes.
This bug already being investigated by Microsoft team [1].
Until compiler bug be fixed (and it will be guaranteed that the blender
will not build on damaged versions), this fix should be used in blender.
Bug can be occurred in other places, so this fix might be reused in other places.
The define used to not violate the code style guide and simplify
future cleaning to delete this.

https://developercommunity.visualstudio.com/t/VS-1772-C-17:-Function-arguments-in/10448291

MSVC builder (Windows platform) have a bug that cause the crash of the Split Edges node from Geometry Nodes. This bug already being investigated by Microsoft team [1]. Until compiler bug be fixed (and it will be guaranteed that the blender will not build on damaged versions), this fix should be used in blender. Bug can be occurred in other places, so this fix might be reused in other places. The define used to not violate the code style guide and simplify future cleaning to delete this. 1. https://developercommunity.visualstudio.com/t/VS-1772-C-17:-Function-arguments-in/10448291

Iliya Katushenock added the

Platforms, Builds & Tests

Nodes & Physics

labels 2023-09-20 10:23:56 +02:00

Iliya Katushenock added 1 commit 2023-09-20 10:24:08 +02:00

5f8ae1b455 init

Iliya Katushenock added the

label 2023-09-20 10:25:03 +02:00

Iliya Katushenock added 1 commit 2023-09-20 10:26:14 +02:00

5b608bfc16 fix typo

Ray molenkamp requested changes 2023-09-20 15:08:16 +02:00

Ray molenkamp left a comment

Member

ms offered 2 workarounds, however they do not do the same thing

the bug in the ms compiler is in the inliner, by putting __declspec(noinline) on the function, the inliner doesn't run and we don't get bad code.
the bug in the ms compiler, more specifically is with how the inliner handles struct parameters by value, by passing these parameters by reference, the bug doesn't trigger and we don't get bad code.

passing these parameters by reference, does NOT disable the inliner, as your patch currently suggest.

Fix: get rid of the define, just pass by reference (given @HooglyBoogly / @JacquesLucke sign off on that)

if we are determined we want to use some form of macro here it'll have to be guarded so it'll only affect msvc 17.7+

ms offered [2 workarounds](https://developercommunity.visualstudio.com/t/VS-1772-C-17:-Function-arguments-in/10448291#T-N10470009), however they do _not_ do the same thing 1) the bug in the ms compiler is in the inliner, by putting __declspec(noinline) on the function, the inliner doesn't run and we don't get bad code. 2) the bug in the ms compiler, more specifically is with how the inliner handles struct parameters by value, by passing these parameters by reference, the bug doesn't trigger and we don't get bad code. passing these parameters by reference, does NOT disable the inliner, as your patch currently suggest. Fix: get rid of the define, just pass by reference (given @HooglyBoogly / @JacquesLucke sign off on that) if we are determined we want to use some form of macro here it'll have to be guarded so it'll only affect msvc 17.7+

Hans Goudey commented

2023-09-20 15:26:05 +02:00

Member

I'd rather use __declspec(noinline) or even better BLI_NOINLINE here. This function is only called once for the entire mesh, inlining it shouldn't be important. And for consistency I'd rather keep passing these arguments by value.

I'd rather use `__declspec(noinline)` or even better `BLI_NOINLINE` here. This function is only called once for the entire mesh, inlining it shouldn't be important. And for consistency I'd rather keep passing these arguments by value.

Iliya Katushenock added 2 commits 2023-09-24 16:47:55 +02:00

9f1f7ec98b Merge branch 'main' into tmp_fix_msvc_bug

0a85496ae2 progress

Iliya Katushenock requested review from Ray molenkamp 2023-09-24 16:48:45 +02:00

Ray molenkamp requested changes 2023-09-24 18:44:40 +02:00

Ray molenkamp left a comment

Member

BLI_NOINLINE_MS sounds very generic, but it only applies to very specific MSVC versions, that's not OK. Just use BLI_NOINLINE and put the logic at the call site in mesh_split_edges.cc

`BLI_NOINLINE_MS` sounds very generic, but it only applies to very specific MSVC versions, that's not OK. Just use `BLI_NOINLINE` and put the logic at the call site in `mesh_split_edges.cc`

Iliya Katushenock commented

2023-09-24 18:47:27 +02:00

Author

Member

BLI_NOINLINE does not work. The error may appear somewhere else. This must be explicitly declared for general use.

`BLI_NOINLINE` does not work. The error may appear somewhere else. This must be explicitly declared for general use.

Ray molenkamp commented

2023-09-24 19:04:22 +02:00

Member

I see why you would think that, but no, a generic solution is not needed, this is a temporary issue at a limited number of call sites (currently 1) solve it at the call site

I see why you would think that, but no, a generic solution is not needed, this is a temporary issue at a limited number of call sites (currently 1) solve it at the call site

Guillermo Venegas referenced this pull request

2023-09-27 05:00:41 +02:00

Crash on use split edges node #112933

Philipp Oeser referenced this pull request

2023-09-27 11:15:39 +02:00

Crash on use split edges node #112933

Pratik Borhade referenced this pull request

2023-09-27 11:54:24 +02:00

Crash on use split edges node #112933

Philipp Oeser commented

2023-09-27 12:41:34 +02:00

Member

Note: this would also fix #112933

Note: this would also fix #112933

Iliya Katushenock requested review from Ray molenkamp 2023-09-27 18:13:36 +02:00

Iliya Katushenock changed title from ~~Fix: Bug crash 17.0+ MSVC builds with function passed-by-value arguments captured in lambdas~~ to Fix #112933: Bug crash 17.0+ MSVC builds with function passed-by-value arguments captured in lambdas

2023-09-27 18:13:45 +02:00

Iliya Katushenock added 2 commits 2023-09-27 18:13:51 +02:00

d5980e6a9d Merge branch 'main' into tmp_fix_msvc_bug

83ab8c407f g

Hans Goudey reviewed 2023-09-27 18:19:53 +02:00

Hans Goudey left a comment

Member

Does it not work to just use BLI_NOINLINE for this function and add a comment? I don't understand why this has to be more complicated than that.

Does it not work to just use `BLI_NOINLINE` for this function and add a comment? I don't understand why this has to be more complicated than that.

Iliya Katushenock commented

2023-09-27 18:27:23 +02:00

Author

Member

BLI_NOINLINE does not work.

`BLI_NOINLINE` does not work.

Ray molenkamp commented

2023-09-27 18:39:52 +02:00

Member

try this

diff --git a/source/blender/blenlib/BLI_compiler_compat.h b/source/blender/blenlib/BLI_compiler_compat.h
index 8945c15a4c4..b7a6ce548d9 100644
--- a/source/blender/blenlib/BLI_compiler_compat.h
+++ b/source/blender/blenlib/BLI_compiler_compat.h
@@ -40,6 +40,8 @@ template<typename T> static inline T decltype_helper(T x)

 #if defined(__GNUC__)
 #  define BLI_NOINLINE __attribute__((noinline))
+#elif defined(_MSC_VER)
+#  define BLI_NOINLINE __declspec(noinline)
 #else
 #  define BLI_NOINLINE
 #endif

try this ```Diff diff --git a/source/blender/blenlib/BLI_compiler_compat.h b/source/blender/blenlib/BLI_compiler_compat.h index 8945c15a4c4..b7a6ce548d9 100644 --- a/source/blender/blenlib/BLI_compiler_compat.h +++ b/source/blender/blenlib/BLI_compiler_compat.h @@ -40,6 +40,8 @@ template<typename T> static inline T decltype_helper(T x) #if defined(__GNUC__) # define BLI_NOINLINE __attribute__((noinline)) +#elif defined(_MSC_VER) +# define BLI_NOINLINE __declspec(noinline) #else # define BLI_NOINLINE #endif ````

Iliya Katushenock added 1 commit 2023-09-27 19:03:29 +02:00

d087b54b07 progress

Guillermo Venegas commented

2023-09-27 21:42:05 +02:00

Contributor

Why not capturing lambda arguments as values instead of references?

diff --git a/source/blender/geometry/intern/mesh_split_edges.cc b/source/blender/geometry/intern/mesh_split_edges.cc
index b78b7922890..54058ca2904 100644
--- a/source/blender/geometry/intern/mesh_split_edges.cc
+++ b/source/blender/geometry/intern/mesh_split_edges.cc
@@ -203,17 +203,18 @@ static Array<Vector<CornerGroup>> calc_all_corner_groups(const OffsetIndices<int
                                                          const IndexMask &affected_verts)
 {
   Array<Vector<CornerGroup>> corner_groups(affected_verts.size(), NoInitialization());
-  affected_verts.foreach_index(GrainSize(512), [&](const int vert, const int mask) {
-    new (&corner_groups[mask])
-        Vector<CornerGroup>(calc_corner_groups_for_vertex(faces,
-                                                          corner_verts,
-                                                          corner_edges,
-                                                          edge_to_corner_map,
-                                                          corner_to_face_map,
-                                                          split_edges,
-                                                          vert_to_corner_map[vert],
-                                                          vert));
-  });
+  affected_verts.foreach_index(
+      GrainSize(512), [=, &corner_groups](const int vert, const int mask) {
+        new (&corner_groups[mask])
+            Vector<CornerGroup>(calc_corner_groups_for_vertex(faces,
+                                                              corner_verts,
+                                                              corner_edges,
+                                                              edge_to_corner_map,
+                                                              corner_to_face_map,
+                                                              split_edges,
+                                                              vert_to_corner_map[vert],
+                                                              vert));
+      });
   return corner_groups;
 }

Why not capturing lambda arguments as values instead of references? ```diff diff --git a/source/blender/geometry/intern/mesh_split_edges.cc b/source/blender/geometry/intern/mesh_split_edges.cc index b78b7922890..54058ca2904 100644 --- a/source/blender/geometry/intern/mesh_split_edges.cc +++ b/source/blender/geometry/intern/mesh_split_edges.cc @@ -203,17 +203,18 @@ static Array<Vector<CornerGroup>> calc_all_corner_groups(const OffsetIndices<int const IndexMask &affected_verts) { Array<Vector<CornerGroup>> corner_groups(affected_verts.size(), NoInitialization()); - affected_verts.foreach_index(GrainSize(512), [&](const int vert, const int mask) { - new (&corner_groups[mask]) - Vector<CornerGroup>(calc_corner_groups_for_vertex(faces, - corner_verts, - corner_edges, - edge_to_corner_map, - corner_to_face_map, - split_edges, - vert_to_corner_map[vert], - vert)); - }); + affected_verts.foreach_index( + GrainSize(512), [=, &corner_groups](const int vert, const int mask) { + new (&corner_groups[mask]) + Vector<CornerGroup>(calc_corner_groups_for_vertex(faces, + corner_verts, + corner_edges, + edge_to_corner_map, + corner_to_face_map, + split_edges, + vert_to_corner_map[vert], + vert)); + }); return corner_groups; } ```

Ray molenkamp requested changes 2023-09-27 21:44:09 +02:00

Ray molenkamp left a comment

Member

just put BLI_NOINLINE on that method, you're making it much more difficult than it needs to be

just put `BLI_NOINLINE` on that method, you're making it much more difficult than it needs to be

Iliya Katushenock requested review from Ray molenkamp 2023-09-29 10:58:07 +02:00

Iliya Katushenock added 2 commits 2023-09-29 10:58:28 +02:00

009243cbd1 Merge branch 'main' into tmp_fix_msvc_bug

93e97800fc f

Ray molenkamp reviewed 2023-09-29 15:16:36 +02:00

source/blender/geometry/intern/mesh_split_edges.cc Outdated

						
				@ -201,3 +196,1 @@

				                                                         const Span<int> corner_to_face_map,

				                                                         const BitSpan split_edges,

				                                                         const IndexMask &affected_verts)

				BLI_NOINLINE static Array<Vector<CornerGroup>> calc_all_corner_groups(

Ray molenkamp commented

2023-09-29 15:16:36 +02:00

Member

/* BLI_NOINLINE because MSVC 17.7 has a codegen bug here, given there is only a single call to this function, not inlining it for all platforms won't affect performance. See https://developercommunity.visualstudio.com/t/10448291 for details. */

/* BLI_NOINLINE because MSVC 17.7 has a codegen bug here, given there is only a single call to this function, not inlining it for all platforms won't affect performance. See https://developercommunity.visualstudio.com/t/10448291 for details. */

mod_moder marked this conversation as resolved

Iliya Katushenock added 1 commit 2023-09-29 15:23:31 +02:00

d011ae6a27 add comment

Ray molenkamp approved these changes 2023-09-29 16:49:43 +02:00

Ray molenkamp merged commit 6c14764f32 into main

2023-09-29 16:57:12 +02:00

Ray molenkamp referenced this issue from a commit

2023-09-29 16:57:15 +02:00

Fix #112933: Fix codegen issue on MSVC 17.7

Iliya Katushenock deleted branch tmp_fix_msvc_bug

2023-09-29 16:58:50 +02:00

Vasilis-Milios referenced this issue from a commit

2023-10-04 14:15:43 +02:00

Fix #112933: Fix codegen issue on MSVC 17.7

Vasilis-Milios referenced this issue from a commit

2023-10-04 14:26:47 +02:00

Fix #112933: Fix codegen issue on MSVC 17.7

Richard Antalik referenced this issue from a commit

2023-10-04 19:17:52 +02:00

Fix #112933: Fix codegen issue on MSVC 17.7

Joseph Eagar referenced this issue from a commit

2023-10-07 21:46:16 +02:00

Fix #112933: Fix codegen issue on MSVC 17.7

Joseph Eagar referenced this issue from a commit

2023-10-07 21:46:17 +02:00

Fix #112933: Fix codegen issue on MSVC 17.7

Colin Basnett referenced this issue from a commit

2023-10-08 23:38:52 +02:00

Fix #112933: Fix codegen issue on MSVC 17.7

Sign in to join this conversation.

No reviewers

No Label

Animation & Rigging

Asset Browser Project Overview

Automated Testing

Blender Asset Bundle

Dependency Graph

Development Management

EEVEE & Viewport

Images & Movies

Motion Tracking

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Render Pipeline

Sculpt, Paint & Texture

Video Sequencer

Virtual Reality

Blender 2.8 Project

Milestone 1: Basic, Local Asset Browser

Good First Issue

Animation & Rigging

Development Management

EEVEE & Viewport

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Sculpt, Paint & Texture

Needs Info from Developers

Needs Information from User

No Milestone

No project

No Assignees

5 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#112616

No description provided.