Feature: shmem init error checking #1997

holke · 2025-11-26T09:15:58Z

Describe your changes here:

Related to #1985
The shared memory was often not initialized properly but we did not detect it.

Merge Bugfix: Ignore warnings when executing CMake tests #1996 first.

I added an error code to t8_shmem_init following the convention of sc_shmem_init.
This actually caused many of our tests to fail giving more reason to this feature.
The errors are fixed with #1996 which should be merged first.

When the error code is met, we currently abort the program.

The reasoning is that shared memory should always be available to us, since it is part of MPI >=3.0

(This creates the new issue of requiring MPI 3.0 in our build system).

All these boxes must be checked by the AUTHOR before requesting review:

The PR is small enough to be reviewed easily. If not, consider splitting up the changes in multiple PRs.
The title starts with one of the following prefixes: Documentation:, Bugfix:, Feature:, Improvement: or Other:.
If the PR is related to an issue, make sure to link it.
The author made sure that, as a reviewer, he/she would check all boxes below.

All these boxes must be checked by the REVIEWERS before merging the pull request:

As a reviewer please read through all the code lines and make sure that the code is fully understood, bug free, well-documented and well-structured.

General

The reviewer executed the new code features at least once and checked the results manually.
The code follows the t8code coding guidelines.
New source/header files are properly added to the CMake files.
The code is well documented. In particular, all function declarations, structs/classes and their members have a proper doxygen documentation.
All new algorithms and data structures are sufficiently optimal in terms of memory and runtime (If this should be merged, but there is still potential for optimization, create a new issue).

Tests

The code is covered in an existing or new test case using Google Test.
The code coverage of the project (reported in the CI) should not decrease. If coverage is decreased, make sure that this is reasonable and acceptable.
Valgrind doesn't find any bugs in the new code. This script can be used to check for errors; see also this wiki article.

If the Pull request introduces code that is not covered by the github action (for example coupling with a new library):

Should this use case be added to the github action?
If not, does the specific use case compile and all tests pass (check manually).

Scripts and Wiki

If a new directory with source files is added, it must be covered by the script/find_all_source_files.scp to check the indentation of these files.
If this PR introduces a new feature, it must be covered in an example or tutorial and a Wiki article.

License

The author added a BSD statement to doc/ (or already has one).

CMakeLists.txt

…it_error_checking

codecov · 2025-11-26T10:02:20Z

Codecov Report

❌ Patch coverage is 90.90909% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 77.05%. Comparing base (f74354f) to head (49e5eb7).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/t8_cmesh/t8_cmesh_io/t8_cmesh_save.cxx	0.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1997   +/-   ##
=======================================
  Coverage   77.05%   77.05%           
=======================================
  Files         112      112           
  Lines       18959    18962    +3     
=======================================
+ Hits        14608    14611    +3     
  Misses       4351     4351

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

…it_error_checking

…ror_checking

…' into feature-shmem_init_error_checking

…g' into feature-shmem_init_error_checking

…it_error_checking

sandro-elsweijer · 2025-12-02T11:51:33Z

src/t8_data/t8_shmem.c

+#ifndef T8_ENABLE_MPI
+  // If we do not use MPI, there is nothing to do.
+  // We only have a single process.
+  return 1;
+#endif
+#ifndef SC_ENABLE_MPICOMMSHARED
+  SC_ABORT ("Trying to use shared memory but SC_ENABLE_MPICOMMSHARED is not set. This should not happen if you use MPI "
+            "v.3.0 or higher. Maybe related to https://github.com/DLR-AMR/t8code/pull/1996.");
+#endif


Is this only the case when MPI is not linked or also when the number of ranks is 1?

When the number of ranks is 1, but linked against MPI, it is save to use shared memory.
SC_ENABLE_MPICOMMSHARED will be defined and that case and this error will not occur.

Davknapp · 2025-12-04T08:27:59Z

test/t8_cmesh/t8_gtest_cmesh_set_partition_offsets.cxx

   * that concentrates all trees at one process. */
-  t8_shmem_init (sc_MPI_COMM_WORLD);
+  const int intranode_size = t8_shmem_init (sc_MPI_COMM_WORLD);
+  ASSERT_GT (intranode_size, 0) << "Could not initialize shared memory.";


Suggested change

ASSERT_GT (intranode_size, 0) << "Could not initialize shared memory.";

ASSERT_NE (intranode_size, 0) << "Could not initialize shared memory.";

I feel more comfortable keeping GT, since >0 is a more restrictive condition then !=0.
If the intranode_size should be <0 then something went wrong and the memory was not initialized.

Davknapp · 2025-12-04T08:28:11Z

test/t8_cmesh/t8_gtest_cmesh_set_partition_offsets.cxx

   * that concentrates all trees at one process. */
-  t8_shmem_init (comm);
+  const int intranode_size = t8_shmem_init (comm);
+  ASSERT_GT (intranode_size, 0) << "Could not initialize shared memory.";


Suggested change

ASSERT_GT (intranode_size, 0) << "Could not initialize shared memory.";

ASSERT_NE (intranode_size, 0) << "Could not initialize shared memory.";

See comment above.

Davknapp · 2025-12-04T08:28:21Z

test/t8_data/t8_gtest_shmem.cxx

  /* setup shared memory usage */
-  t8_shmem_init (comm);
+  const int intrasize_from_init = t8_shmem_init (comm);
+  ASSERT_GT (intrasize_from_init, 0) << "Error in t8_shmem_init. No intranode communicator set.";


Suggested change

ASSERT_GT (intrasize_from_init, 0) << "Error in t8_shmem_init. No intranode communicator set.";

ASSERT_NE (intrasize_from_init, 0) << "Error in t8_shmem_init. No intranode communicator set.";

See comment above.

Davknapp · 2025-12-04T08:28:29Z

test/t8_data/t8_gtest_shmem.cxx

  /* setup shared memory usage */
-  t8_shmem_init (comm);
+  const int intranode_size = t8_shmem_init (comm);
+  ASSERT_GT (intranode_size, 0) << "Could not initialize shared memory.";


Suggested change

ASSERT_GT (intranode_size, 0) << "Could not initialize shared memory.";

ASSERT_NE (intranode_size, 0) << "Could not initialize shared memory.";

See comment above.

Davknapp · 2025-12-04T08:28:38Z

test/t8_data/t8_gtest_shmem.cxx

  /* setup shared memory usage */
-  t8_shmem_init (comm);
+  const int intranode_size = t8_shmem_init (comm);
+  ASSERT_GT (intranode_size, 0) << "Could not initialize shared memory.";


Suggested change

ASSERT_GT (intranode_size, 0) << "Could not initialize shared memory.";

ASSERT_NE (intranode_size, 0) << "Could not initialize shared memory.";

See comment above.

Davknapp · 2025-12-04T08:33:07Z

src/t8_forest/t8_forest_partition.cxx

  if (forest->global_first_desc == NULL) {
    /* Set the shmem array type of comm */
-    t8_shmem_init (comm);
+    SC_CHECK_ABORT (t8_shmem_init (comm) > 0, "Error in shared memory setup. Could not partition forest.");


Suggested change

SC_CHECK_ABORT (t8_shmem_init (comm) > 0, "Error in shared memory setup. Could not partition forest.");

SC_CHECK_ABORT (t8_shmem_init (comm) != 0, "Error in shared memory setup. Could not partition forest.");

See comment above

Davknapp · 2025-12-04T08:46:28Z

src/t8_forest/t8_forest_partition.cxx

  if (forest->tree_offsets == NULL) {
    /* Set the shmem array type of comm */
-    t8_shmem_init (comm);
+    SC_CHECK_ABORT (t8_shmem_init (comm) > 0, "Error in shared memory setup. Could not partition forest.");


Suggested change

SC_CHECK_ABORT (t8_shmem_init (comm) > 0, "Error in shared memory setup. Could not partition forest.");

SC_CHECK_ABORT (t8_shmem_init (comm) != 0, "Error in shared memory setup. Could not partition forest.");

See comment above

Davknapp · 2025-12-04T08:46:38Z

src/t8_forest/t8_forest_partition.cxx

  T8_ASSERT (forest->element_offsets == NULL);
  /* Set the shmem array type to comm */
-  t8_shmem_init (comm);
+  SC_CHECK_ABORT (t8_shmem_init (comm) > 0, "Error in shared memory setup. Could not partition forest.");


Suggested change

SC_CHECK_ABORT (t8_shmem_init (comm) > 0, "Error in shared memory setup. Could not partition forest.");

SC_CHECK_ABORT (t8_shmem_init (comm) != 0, "Error in shared memory setup. Could not partition forest.");

See comment above

Davknapp · 2025-12-04T08:47:11Z

src/t8_cmesh/t8_cmesh_io/t8_cmesh_save.cxx


  /* Try to set the comm type */
-  t8_shmem_init (comm);
+  SC_CHECK_ABORT (t8_shmem_init (comm) > 0, "Error in shared memory setup. Could not load cmesh.");


Suggested change

SC_CHECK_ABORT (t8_shmem_init (comm) > 0, "Error in shared memory setup. Could not load cmesh.");

SC_CHECK_ABORT (t8_shmem_init (comm) != 0, "Error in shared memory setup. Could not load cmesh.");

Davknapp · 2025-12-04T08:47:20Z

src/t8_cmesh/t8_cmesh_internal/t8_cmesh_partition.cxx

  tree_offset = cmesh->first_tree_shared ? -cmesh->first_tree - 1 : cmesh->first_tree;
  if (cmesh->tree_offsets == NULL) {
-    t8_shmem_init (comm);
+    SC_CHECK_ABORT (t8_shmem_init (comm) > 0, "Error in shared memory setup.");


Suggested change

SC_CHECK_ABORT (t8_shmem_init (comm) > 0, "Error in shared memory setup.");

SC_CHECK_ABORT (t8_shmem_init (comm) != 0, "Error in shared memory setup.");

I think that the != 0 fits better the true/false version of C, e.g. true being not zero.

holke · 2026-01-14T13:36:23Z

Thanks for the review.

I argue for keeping the >0 condition everywhere since it will catch more error cases.
Thus, there should be no need to change code from my side.

There was one comment by @sandro-elsweijer which i believe i have answered positively.
If you agree, you can merge.

holke added 4 commits November 25, 2025 11:16

Let t8_shmem_init return the size of the intranode communicator

9a82481

Use return value of t8_shmem_init when called

1233b2d

Dont initialize shmem if MPI is not enabled

f2f69cd

Ignore warnings when executing CMake tests

a7cf3ff

holke commented Nov 26, 2025

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

holke added 2 commits November 26, 2025 10:19

Merge branch 'feature-ignore_warnings_in_tests' into feature-shmem_in…

bd6410f

…it_error_checking

Merge branch 'feature-ignore_warnings_in_tests' into feature-shmem_in…

19fac25

…it_error_checking

holke added 6 commits November 27, 2025 11:17

Merge branch 'feature-ignore_warnings_in_tests' into feature-shmem_in…

cfb5711

…it_error_checking

Merge remote-tracking branch 'origin/main' into feature-shmem_init_er…

999fa7b

…ror_checking

Merge remote-tracking branch 'origin/feature-ignore_warnings_in_tests…

eb704d6

…' into feature-shmem_init_error_checking

Added a clear error message when shared memory is not initialized

d6a3a9b

Merge remote-tracking branch 'origin/feature-shmem_init_error_checkin…

904ccf9

…g' into feature-shmem_init_error_checking

Merge branch 'feature-ignore_warnings_in_tests' into feature-shmem_in…

3d91658

…it_error_checking

Base automatically changed from feature-ignore_warnings_in_tests to main November 28, 2025 10:41

Merge branch 'main' into feature-shmem_init_error_checking

231f112

sandro-elsweijer reviewed Dec 2, 2025

View reviewed changes

Davknapp reviewed Dec 4, 2025

View reviewed changes

Davknapp assigned Davknapp and holke and unassigned Davknapp Dec 4, 2025

holke assigned Davknapp and unassigned holke Jan 14, 2026

Merge branch 'main' into feature-shmem_init_error_checking

49e5eb7

	ASSERT_GT (intranode_size, 0) << "Could not initialize shared memory.";
	ASSERT_NE (intranode_size, 0) << "Could not initialize shared memory.";

	ASSERT_GT (intrasize_from_init, 0) << "Error in t8_shmem_init. No intranode communicator set.";
	ASSERT_NE (intrasize_from_init, 0) << "Error in t8_shmem_init. No intranode communicator set.";

	SC_CHECK_ABORT (t8_shmem_init (comm) > 0, "Error in shared memory setup. Could not partition forest.");
	SC_CHECK_ABORT (t8_shmem_init (comm) != 0, "Error in shared memory setup. Could not partition forest.");

Feature: shmem init error checking #1997

Are you sure you want to change the base?

Feature: shmem init error checking #1997

Uh oh!

Conversation

holke commented Nov 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

General

Tests

Scripts and Wiki

License

Uh oh!

Uh oh!

codecov bot commented Nov 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

holke commented Jan 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

holke commented Nov 26, 2025 •

edited

Loading

codecov bot commented Nov 26, 2025 •

edited

Loading