parsec

parsec copied to clipboard

Describe the bug

Building TTG with GCC 11.4.0 raises a bunch of warnings from PaRSEC:

/home/jschuchart/src/ttg/ttg/build/_deps/parsec-src/parsec/class/list.h: In function 'parsec_list_item_t* parsec_list_nolock_pop_back(parsec_list_t*)':
/home/jschuchart/src/ttg/ttg/build/_deps/parsec-src/parsec/class/list_item.h:326:16: warning: '--' expression of 'volatile'-qualified type is deprecated [-Wvolatile]
  326 |         _item->refcount--;                                              \
      |         ~~~~~~~^~~~~~~~
/home/jschuchart/src/ttg/ttg/build/_deps/parsec-src/parsec/class/list.h:1147:9: note: in expansion of macro 'PARSEC_ITEM_DETACH'
 1147 |         PARSEC_ITEM_DETACH(ITEM);                                        \
      |         ^~~~~~~~~~~~~~~~~~
/home/jschuchart/src/ttg/ttg/build/_deps/parsec-src/parsec/class/list.h:1195:5: note: in expansion of macro '_RET_NULL_GHOST'
 1195 |     _RET_NULL_GHOST(list, item);
      |     ^~~~~~~~~~~~~~~

C++20 has deprecated compound operations on volatile variables.

A fix is to change

_item->refcount--;

into

_item->refcount = _item->refcount - 1;

Arguably, it's only C++ that deprecated these operations and C apparently has not (yet). However, PaRSEC is now consumed by C++ projects. In the greater scheme of things, refcount is probably an atomic variable and should be marked as such (instead of volatile).

Not sure the two step replacement is still 'atomic', dependent upon what assembly is generated with the double evaluation.

We abused volatile because there was no good C99 way of saying its an atomic variable and the compiler shouldn't reorder around it, even if we are not doing fetch_and_op on it. In C11 there are all the good types we need, and requiring c11 is not unreasonable nowadays.

There is a difference between ((volatile int)v)++ and ((atomic int)a)++: the later will substitute for a fetch_and_add automatically, the former will only enforce that no reordering and register reload is done but will use a normal ADD assembly. That may have performance implications, whether that matters is to be seen.

Not sure the two step replacement is still 'atomic', dependent upon what assembly is generated with the double evaluation.

The current version is not atomic either. I assume it doesn't have to be because it is used in parsec_list_nolock_pop_back. I read that as assuming that there is a lock taken somewhere by the caller...

We abused volatile because there was no good C99 way of saying its an atomic variable and the compiler shouldn't reorder around it, even if we are not doing fetch_and_op on it. In C11 there are all the good types we need, and requiring c11 is not unreasonable nowadays.

Agreed. BUT: since PaRSEC interfaces with C++ (TTG) and those headers are used publicly the _Atomic prefix is not compatible with C++ :/

Accepting a = a - 1 while refusing a-- sounds pretty random from the C++ compiler.

1. The refcount shall not be atomic, because it is illegal to add or remove simultaneously an item to and from multiple lists.
2. we are using this field only when DEBUG_PARANOID is on, as an additional checkup. Having the object as _Atomic would be an overkill.
3. we need it marked as volatile to prevent the compiler from doing any optimizations on this variable.

Anyway, the following patch should make the C++ compiler quiet.

diff --git a/parsec/class/list_item.h b/parsec/class/list_item.h
index 0e82ac12f..cc94780f2 100644
--- a/parsec/class/list_item.h
+++ b/parsec/class/list_item.h
@@ -109,7 +109,7 @@ parsec_list_item_ring( parsec_list_item_t* first, parsec_list_item_t* last )
         parsec_list_item_t* item = first;
         do {
             assert( item->belong_to == first->belong_to );
-            item->refcount--;
+            item->refcount = item->refcount - 1;
             assert( 0 == item->refcount );
             item = (parsec_list_item_t*)item->list_next;
         } while(item != first);
@@ -203,7 +203,7 @@ parsec_list_item_ring_chop( parsec_list_item_t* item )
     item->list_prev->list_next = item->list_next;
     item->list_next->list_prev = item->list_prev;
 #if defined(PARSEC_DEBUG_PARANOID)
-    if(item->refcount) item->refcount--;
+    if(item->refcount) item->refcount = item->refcount - 1;
     item->list_prev = (parsec_list_item_t*)(void*)0xdeadbeefL;
     item->list_next = (parsec_list_item_t*)(void*)0xdeadbeefL;
 #endif
@@ -285,7 +285,7 @@ parsec_list_item_ring_push_sorted( parsec_list_item_t* ring,
 #define PARSEC_ITEM_ATTACH(LIST, ITEM)                                  \
     do {                                                                \
         parsec_list_item_t *_item_ = (ITEM);                            \
-        _item_->refcount++;                                             \
+        _item_->refcount = _item_->refcount + 1;                        \
         assert( 1 == _item_->refcount );                                \
         _item_->belong_to = (LIST);                                     \
     } while(0)
@@ -309,7 +309,7 @@ parsec_list_item_ring_push_sorted( parsec_list_item_t* ring,
         assert( _item->belong_to != (void*)_item );                     \
         _item->list_prev = (parsec_list_item_t*)(void*)0xdeadbeefL;     \
         _item->list_next = (parsec_list_item_t*)(void*)0xdeadbeefL;     \
-        _item->refcount--;                                              \
+        _item->refcount = _item->refcount - 1;                          \
         assert( 0 == _item->refcount );                                 \
     } while (0)
 #else

Do we want to promote this to v4.0 since we have code ready to apply to the problem?