Warning compiling CommonBase.cpp

[mvousden]

Forgive my laziness, here's the email I sent around:

Exciting new warning on gcc 12.1.0:

In file included from /usr/include/c++/12.1.0/map:60,
                 from ../../Generics/map2.h:6,
                 from ../../Generics/Msg_p.hpp:4,
                 from ../../Source/Common/PMsg_p.hpp:5,
                 from ../../Source/Common/CommonBase.h:8,
                 from ../../Source/Common/CommonBase.cpp:3:
In member function ‘std::_Rb_tree<_Key, _Val, _KeyOfValue, _Compare, _Alloc>::iterator std::_Rb_tree<_Key, _Val, _KeyOfValue, _Compare, _Alloc>::_M_lower_bound(_Link_type, _Base_ptr, const _Key&) [with _Key = int; _Val = std::pair<const int, std::__cxx11::basic_string<char> >; _KeyOfValue = std::_Select1st<std::pair<const int, std::__cxx11::basic_string<char> > >; _Compare = std::less<int>; _Alloc = std::allocator<std::pair<const int, std::__cxx11::basic_string<char> > >]’,
    inlined from ‘std::_Rb_tree<_Key, _Val, _KeyOfValue, _Compare, _Alloc>::iterator std::_Rb_tree<_Key, _Val, _KeyOfValue, _Compare, _Alloc>::lower_bound(const key_type&) [with _Key = int; _Val = std::pair<const int, std::__cxx11::basic_string<char> >; _KeyOfValue = std::_Select1st<std::pair<const int, std::__cxx11::basic_string<char> > >; _Compare = std::less<int>; _Alloc = std::allocator<std::pair<const int, std::__cxx11::basic_string<char> > >]’ at /usr/include/c++/12.1.0/bits/stl_tree.h:1270:30,
    inlined from ‘std::map<_Key, _Tp, _Compare, _Alloc>::iterator std::map<_Key, _Tp, _Compare, _Alloc>::lower_bound(const key_type&) [with _Key = int; _Tp = std::__cxx11::basic_string<char>; _Compare = std::less<int>; _Alloc = std::allocator<std::pair<const int, std::__cxx11::basic_string<char> > >]’ at /usr/include/c++/12.1.0/bits/stl_map.h:1307:32,
    inlined from ‘std::map<_Key, _Tp, _Compare, _Alloc>::mapped_type& std::map<_Key, _Tp, _Compare, _Alloc>::operator[](const key_type&) [with _Key = int; _Tp = std::__cxx11::basic_string<char>; _Compare = std::less<int>; _Alloc = std::allocator<std::pair<const int, std::__cxx11::basic_string<char> > >]’ at /usr/include/c++/12.1.0/bits/stl_map.h:507:28,
    inlined from ‘unsigned int CommonBase::OnSystPingAck(PMsg_p*)’ at ../../Source/Common/CommonBase.cpp:164:28:
/usr/include/c++/12.1.0/bits/stl_tree.h:1951:9: warning: ‘TgtR’ may be used uninitialized [-Wmaybe-uninitialized]
 1951 |         if (!_M_impl._M_key_compare(_S_key(__x), __k))
      |         ^~
../../Source/Common/CommonBase.cpp: In member function ‘unsigned int CommonBase::OnSystPingAck(PMsg_p*)’:
../../Source/Common/CommonBase.cpp:162:5: note: ‘TgtR’ was declared here
  162 | int TgtR;                              // Target rank
      |     ^~~~

Looking at the source, in CommonBase.cpp:

unsigned CommonBase::OnSystPingAck(PMsg_p * Z)
// Ping acknowledge?
{
int cnt;
double ZT0 = 0.0;                      // MPI times
double * pZT0 = Z->Get<double>(0,cnt); // Unload the REQ launch time
if (pZT0!=0) ZT0 = *pZT0;
double trip = Z->Ztime(1)-ZT0;         // Round trip time
int * pTgtR = Z->Get<int>(0,cnt);      // Unload target rank
int TgtR;                              // Target rank
if (pTgtR!=0) TgtR = * pTgtR;
string TgtS = pPmap->M[TgtR];          // Target name
[...]

Looks as though, if pTgtR=0, TgtR is used uninitialised, which could happen if the PMsg_p has no data at index zero, looking at Msg_p.tpp:

template <class T> T * Msg_p::Get(int k,int & cnt)
// Can return 0, so you need to test it before you dereference it
{
cnt = 0;
int keyT = t2imap[typeid(T).name()];   // Data type key
                                       // Legitimate response - unknown type
if (Tmap.find(keyT)==Tmap.end()) return (T *)0;
typemap * pTM = Tmap[keyT];
if (pTM==0) return (T *)0;             // Bad response - unknown type
                                       // Legitimate response - unknown key
if (pTM->Dmap.find(k)==pTM->Dmap.end()) return (T *)0;

SS * pSS = pTM->Dmap[k];               // Data stepping stone
cnt = pSS->c / pTM->typelen;           // Number of things
return new(pSS->p) T;                  // Reinterpret ->p as type T
}

Thoughts? Maybe post something angry and return 0 if pTgtR==0?

...and ADB's response:

The snappy answer is this should never happen (ho ho ho). OnSystPingAck is only ever generated in response to a OnSystPingReq (not unreasonably), and the last few lines of OnSystPingReq contain

Pkt.Put<int>(0,&Urank);         // My (sending) rank. Why, Lord?
Pkt.Src(Urank);                        // My sending rank.
Pkt.Send();
return 0;
}

so the index zero field is always defined.

Ways forward:

1. Initialise TgtR in OnSystPingAck: int TgtR; becomes int TgtR = Q::NAP; // Not a process It'll shut the compiler up, but if it ever did happen that index zero was undefined - and it can't ever be - I have no idea what this will do further down the road. Also, it's a bit dangerous because from the code above it looks like it could happen and this is the mechanism to handle it.

2. Post an Unrecoverable error. That is exactly what they're there for, and that is the right thing to do in this circumstance.

3. Initialise with somehting anodyne and obviously wrong: int TgtR = -1; Uncool. It'll shut the compiler up, but it'd be Bad Code, and we don't write Bad Code.

It's a cracking error message, though, one must admit. gcc at its finest......