#1986: this compared to NULL in CarpetLib --------------------+------------------------------------------------------- Reporter: rhaas | Owner: eschnett Type: defect | Status: new Priority: minor | Milestone: Component: Carpet | Version: development version Keywords: | --------------------+------------------------------------------------------- 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.
#1986: this compared to NULL in CarpetLib ---------------------+------------------------------------------------------ Reporter: rhaas | Owner: eschnett Type: defect | Status: review Priority: minor | Milestone: Component: Carpet | Version: development version Resolution: | Keywords: ---------------------+------------------------------------------------------ Changes (by eschnett):
* status: new => review
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; }}}
#1986: this compared to NULL in CarpetLib ---------------------+------------------------------------------------------ Reporter: rhaas | Owner: eschnett Type: defect | Status: review Priority: minor | Milestone: Component: Carpet | Version: development version Resolution: | Keywords: ---------------------+------------------------------------------------------
Comment (by rhaas):
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?
#1986: this compared to NULL in CarpetLib ---------------------+------------------------------------------------------ Reporter: rhaas | Owner: eschnett Type: defect | Status: reviewed_ok Priority: minor | Milestone: Component: Carpet | Version: development version Resolution: | Keywords: ---------------------+------------------------------------------------------ Changes (by eschnett):
* status: review => reviewed_ok
#1986: this compared to NULL in CarpetLib ---------------------+------------------------------------------------------ Reporter: rhaas | Owner: eschnett Type: defect | Status: reopened Priority: minor | Milestone: Component: Carpet | Version: development version Resolution: | Keywords: ---------------------+------------------------------------------------------ Changes (by rhaas):
* status: reviewed_ok => reopened
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.
#1986: this compared to NULL in CarpetLib ---------------------+------------------------------------------------------ Reporter: rhaas | Owner: eschnett Type: defect | Status: reopened Priority: minor | Milestone: Component: Carpet | Version: development version Resolution: | Keywords: ---------------------+------------------------------------------------------
Comment (by eschnett):
Wasn't there a proposed solution that replaces {{{cent}}} by {{{this ? cent : src->cent}}}?
#1986: this compared to NULL in CarpetLib ---------------------+------------------------------------------------------ Reporter: rhaas | Owner: eschnett Type: defect | Status: reopened Priority: minor | Milestone: Component: Carpet | Version: development version Resolution: | Keywords: ---------------------+------------------------------------------------------
Comment (by rhaas):
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.
#1986: this compared to NULL in CarpetLib ---------------------+------------------------------------------------------ Reporter: rhaas | Owner: eschnett Type: defect | Status: reopened Priority: minor | Milestone: Component: Carpet | Version: development version Resolution: | Keywords: ---------------------+------------------------------------------------------
Comment (by rhaas):
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.
#1986: this compared to NULL in CarpetLib ---------------------+------------------------------------------------------ Reporter: rhaas | Owner: eschnett Type: defect | Status: closed Priority: minor | Milestone: Component: Carpet | Version: development version Resolution: fixed | Keywords: ---------------------+------------------------------------------------------ Changes (by rhaas):
* status: reopened => closed * resolution: => fixed
Comment:
Fixed along with #1821 in git hash 474a09de0c1cd56c00492400fe5304c8e9591cbd "PeriodicCarpet: avoid using NULL this pointer for gdata::copy_from" of Carpet.
trac@lists.einsteintoolkit.org
-
Einstein Toolkit