this compared to NULL in CarpetLib

Create issue
Issue #1986 closed
Roland Haas created an issue

I get

/home/rhaas/Cactus/arrangements/Carpet/CarpetLib/src/gdata.cc: In member function ‘void gdata::copy_from(comm_state&, const gdata*, const ibbox&, const ibbox&, const islab*, int, int)’:
/home/rhaas/Cactus/arrangements/Carpet/CarpetLib/src/gdata.cc:185:53: warning: nonnull argument ‘this’ compared to NULL [-Wnonnull-compare]
   int const order_space = (this ? cent : src->cent) == vertex_centered ? 1 : 0;
                           ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~

which leads me to suspect that the compiler is allowed to assume this != NULL and will optimize out one half of the condition.

Keyword:

Comments (9)

  1. Erik Schnetter
    • changed status to open
    • removed comment

    I have a two-line correction for this that I am currently testing:

    diff --git a/CarpetLib/src/gdata.cc b/CarpetLib/src/gdata.cc
    index 3d448fd..1ff8cc6 100644
    --- a/CarpetLib/src/gdata.cc
    +++ b/CarpetLib/src/gdata.cc
    @@ -183,7 +183,7 @@ void gdata::copy_from(comm_state &state, gdata const *const src,
       vector<gdata const *> const srcs(1, src);
       CCTK_REAL const time = 0.0;
       vector<CCTK_REAL> const times(1, time);
    -  int const order_space = (this ? cent : src->cent) == vertex_centered ? 1 : 0;
    +  int const order_space = cent == vertex_centered ? 1 : 0;
       int const order_time = 0;
       transfer_from(state, srcs, times, dstbox, srcbox, slabinfo, dstproc, srcproc,
                     time, order_space, order_time);
    @@ -221,7 +221,7 @@ void gdata::transfer_from(comm_state &state, vector<gdata const *> const &srcs,
           assert(srcs.AT(t)->has_storage());
         }
       }
    -  gdata const *const src = is_src ? srcs.AT(0) : NULL;
    +  gdata const *const src = srcs.AT(0);
    
       operator_type const my_transport_operator =
           is_dst ? transport_operator : src->transport_operator;
    
  2. Roland Haas reporter
    • removed comment

    That will take care of the NULL this I guess as long as srcs is never empty (in which case I'd expect AT to assert() or if not we'll have undefined behaviour since the pointer src [while presumably not "really" used], is not a pointer to a gdata object).

    I am not sure how much can be reviewed in this. If this patch works for the test cases then "Please apply". Should we include the patch in the test suites on at least some machines?

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

    This fails all the time for me. The first line:

    int const order_space = cent == vertex_centered ? 1 : 0;
    

    will fail when this is NULL and this happens to me of the periodiccarpet test testperiodicinterp using 2 MPI ranks.

  4. Erik Schnetter
    • removed comment

    Wasn't there a proposed solution that replaces cent by this ? cent : src->cent?

  5. Roland Haas reporter
    • removed comment

    No,

    this ? cent : src->cent
    

    is what the current (in master) code does and that gives the "this compared to NULL warning". The underlying issue is using a NULL this pointer to call member functions. In the case of PeriodicCarpet this happens when doing dst->copy_from(...) with a NULL dst when dst is not a local component.

    I had a fix for that but it involved calling new_typed_data() when dst would otherwise be NULL. Initially the thought was that new_typed_data() may be too expensive. I am no longer so sure since transfer_from itself already creates a new gdata for its buf object all the time. I am however now working on a (not thread safe...) replacement that uses a single typed_data for all such occurrences.

  6. Roland Haas reporter
    • removed comment

    I re-opened #1821 which had the original proposed fix. I'll put the updated fix with only a single new_typed_data into the old pull request.

  7. Log in to comment