Carpet may call object methods with this == NULL

Create issue
Issue #1821 closed
Roland Haas created an issue

Comments (13)

  1. Roland Haas reporter
    • changed status to open
    • removed comment

    Fails in CarpetPeriodic test testperiodicinterp. Related to #1821 which is however only a warning.

  2. Erik Schnetter
    • removed comment

    I now recall the correction I put into place. The fix is applied at the calling side (obviously). Instead of creating a temporary object, we're using another object that already exists at the caller, e.g. src instead of dst.

    I thought this fix was already committed. Maybe I only applied it to a branch, or maybe I missed a calling site?

  3. Erik Schnetter
    • removed comment

    (1) I don't think that CarpetLib itself calls this method, so the changes to ggf.cc should not be necessary. (2) In PeriodicCarpet, you could use src instead of fake_data_pointer(). (3) The initialization of the static variable in fake_data_pointer is not thread-safe. This has actually become an issue when using Qthreads for parallelization.

  4. Roland Haas reporter
    • removed comment

    (1) I don't think that CarpetLib itself calls this method, so the changes to ggf.cc should not be necessary.

    I added a comment to the code diff: https://bitbucket.org/eschnett/carpet/pull-requests/7/carpet-avoid-using-null-pointer-for-member/diff#comment-33242870

    (2) In PeriodicCarpet, you could use src instead of fake_data_pointer(). Could be done. That seems odd though.

    (3) The initialization of the static variable in fake_data_pointer is not thread-safe. This has actually become an issue when using Qthreads for parallelization. Correct. At the time I reported this CarpetLib was not yet thread safe (ie none of the Timers was) so it did not matter. Thread safe would be either using new() or the src trick that you suggest or making transfer_from a static member function that takes the current dst as eg its first argument.

  5. Roland Haas reporter
    • changed status to open
    • removed comment

    I updated the pull request with a version that introduces static functions for gdata::copy_from and gdata::transfer_from and renames them to copy_data and transfer_data. This requires changes in all code using those functions since the originals are gone (could be restored but the current state brings attention to possible code parts that may also pass in a NULL this pointer).

  6. Roland Haas reporter
    • removed comment

    Erik: I think this should be reviewed before the next release as otherwise CarpetPeriodic fails to run when compiled with newer compilers (gcc 6.0 at least).

  7. Log in to comment