Skip to content

StoredRange can store an optionally-deleted unique_ptr#3332

Merged
jwpeterson merged 1 commit intolibMesh:develfrom
jwpeterson:stored_range_unique_ptr
Jul 5, 2022
Merged

StoredRange can store an optionally-deleted unique_ptr#3332
jwpeterson merged 1 commit intolibMesh:develfrom
jwpeterson:stored_range_unique_ptr

Conversation

@jwpeterson
Copy link
Copy Markdown
Member

  • Also add some libmesh_asserts in StoredRange::reset() to catch the case
    where a range is reset() but the _objs pointer was not initialized.
  • Use lambdas to define the custom deleter. The decision to delete/not
    is made at construction time, and does not change during the object's
    lifetime.
  • One thing I am not 100% happy with is that we still need to call 'new'
    when allocating the class unique_ptr, since one cannot really use
    std::make_unique when a custom deleter is employed.
    https://stackoverflow.com/questions/21788066/using-stdmake-unique-with-a-custom-deleter
  • Use ptr_type typedef for improved readability
  • Add required C++ includes.

* Also add some libmesh_asserts in StoredRange::reset() to catch the case
  where a range is reset() but the _objs pointer was not initialized.
* Use lambdas to define the custom deleter. The decision to delete/not
  is made at construction time, and does not change during the object's
  lifetime.
* One thing I am not 100% happy with is that we still need to call 'new'
  when allocating the class unique_ptr, since one cannot really use
  std::make_unique when a custom deleter is employed.
  https://stackoverflow.com/questions/21788066/using-stdmake-unique-with-a-custom-deleter
* Use ptr_type typedef for improved readability
* Add required C++ includes.
@moosebuild
Copy link
Copy Markdown

Job Coverage on 97fafb9 wanted to post the following:

Coverage

efa6ff #3332 97fafb
Total Total +/- New
Rate 55.73% 55.72% -0.00% 100.00%
Hits 44901 44899 -2 5
Misses 35674 35674 - 0

Diff coverage report

Full coverage report

This comment will be updated on new commits.

@jwpeterson
Copy link
Copy Markdown
Member Author

So we have a randomly failing Min clang target, is that correct?

@jwpeterson jwpeterson merged commit e35f3fc into libMesh:devel Jul 5, 2022
@jwpeterson jwpeterson deleted the stored_range_unique_ptr branch July 5, 2022 15:16
@roystgnr
Copy link
Copy Markdown
Member

roystgnr commented Jul 5, 2022

Yup. In MetaPhysicL. I'll see if I can replicate it locally.

@roystgnr
Copy link
Copy Markdown
Member

roystgnr commented Jul 5, 2022

Well, that's fun. With clang-11, I can run MetaPhysicL test/instantiations_unit in a loop and get a SEGFPE within seconds, after 60 or 70 successful runs. This is supposed to be deterministic code. Looks like I can get the same with clang 12, so just seeing it from the Min clang target may have been luck. Let me see what other compilers work vs fail.

@roystgnr
Copy link
Copy Markdown
Member

roystgnr commented Jul 5, 2022

I'm getting the same non-deterministic failures from clang 13 and 14, and gcc 9 and 11; this has got to be our problem, not a compiler issue.

Unless it's an issue with something else in the environment? I went to an older Linux box to try clang 10, the compiler in our failing CI runs, and that turns out to be happy to make thousands of runs without a hiccup.

@roystgnr
Copy link
Copy Markdown
Member

roystgnr commented Jul 5, 2022

... instantiations_unit is outputting uninitialized values. That's undefined behavior, and it's non-deterministic in the heap-allocated cases because the heap might be pulling memory we've already used for something else, and some of that memory might be effectively getting a reinterpret_cast to NaN.

I'll fix it.

@jwpeterson
Copy link
Copy Markdown
Member Author

OK, sounds good. From what I could see, the metaphysicl test suite doesn't yet output the name of the test which failed (?) but that would be useful for trying to diagnose problems via CIVET... since I don't think the unit test log can be downloaded as part of the results.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants