Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test for dynamic_cast compiler bug #4015

Open
wants to merge 1 commit into
base: devel
Choose a base branch
from

Conversation

roystgnr
Copy link
Member

See issue #4014.

I considered adding a workaround in another commit here, where we move all our defaulted virtual destructors into the .C files to try to convince the new XCode that that really is the proper place to create a single RTTI instantiation for each class. One downside would have been that this would prevent inlining of the destructors of final classes, but a bigger downside is that grep says we have 144 header files with defaulted virtual destructors that would need to be moved. Gross.

See issue 4014, or the URL in the comment.
@moosebuild
Copy link

Job Coverage, step Generate coverage on 972d353 wanted to post the following:

Coverage

9631bd #4015 972d35
Total Total +/- New
Rate 62.33% 62.33% -0.00% -
Hits 72642 72641 -1 0
Misses 43901 43902 +1 0

Diff coverage report

Full coverage report

This comment will be updated on new commits.

@jwpeterson
Copy link
Member

I considered adding a workaround in another commit here, where we move all our defaulted virtual destructors into the .C files

I don't think the issue is specific to whether the virtual destructor is inlined or not, right?

Those StackOverflow answers are suggesting that XCode is now generating RTTI information in the compilation unit with the definition of the first non-inline virtual function and that the problem thus only occurs for classes with no non-inline virtual functions, and that can't be true here because NumericVector and PetscVector do have non-inline virtual functions

The SO article doesn't include an example where the virtual destructor is declared out-of-line, but other virtual functions are still declared inline (the case we have with NumericVector), so I'm curious if the compiler would still have the bug in that case. If so, then moving the destructors out-of-line wouldn't change anything...

@jwpeterson
Copy link
Member

BTW, there is at least one good reason to declare destructors out-of-line. If your class X contains a std::unique_ptr<T>, then you only need a forward declaration of T in X.h unless you try to define ~X() (e.g. by defaulting it) in the header file. As I understand it, the X destructor automatically generates a call to the destructor of T, which then requires a complete type for T to be available.

Copy link
Member

@jwpeterson jwpeterson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a good start, but I still think we should have a configure test (ideally not one based on executing test code, but based on static version checks of the OSX command line tools version) that sets libmesh_final to an empty string when the bad version is detected.

@colegruninger97
Copy link

colegruninger97 commented Nov 23, 2024

I considered adding a workaround in another commit here, where we move all our defaulted virtual destructors into the .C files

I don't think the issue is specific to whether the virtual destructor is inlined or not, right?

Those StackOverflow answers are suggesting that XCode is now generating RTTI information in the compilation unit with the definition of the first non-inline virtual function and that the problem thus only occurs for classes with no non-inline virtual functions, and that can't be true here because NumericVector and PetscVector do have non-inline virtual functions

The SO article doesn't include an example where the virtual destructor is declared out-of-line, but other virtual functions are still declared inline (the case we have with NumericVector), so I'm curious if the compiler would still have the bug in that case. If so, then moving the destructors out-of-line wouldn't change anything...

In the petsc_vector.h header file, I tried the following potential solutions:

  1. Moving virtual destructors out-of-line (to .C files): dynamic_cast fails
  2. Moving both virtual destructors AND virtual functions out-of-line: dynamic_cast fails
  3. Removing the "final" keyword from the class declaration works in all cases, regardless of whether functions are inline or out-of-line

This suggests the issue with the compiler might have something to do specifically with the "final" keyword.

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.

4 participants