Fix warnings from recent compiler versions

[Andrew Johnson]

Builds of EPICS Base using recent versions of gcc, clang and MSVC generate compiler warnings, which we try to minimize if possible. Some of these warnings can be prevented with suitable code modifications, although the code must still build on the older compilers and other architectures that we support.

Both the 3.15 and 7.0 branches can be examined for warnings; fixes made on the 3.15 branch get propagated to the 7.0 branch during periodic up-merges, so there is no need to make the same changes to both branches. Some code only exists on one branch; for example the gdd module was unbundled after 3.15, and there are many new modules in 7.0.

[Freddie Akeroyd (freddie-akeroyd)]

Happy to work on some MSVC ones during codeathon

[Gabriel Fedel (fedel)]

I will work on this too, with gcc (8.0.3) to start.

[Freddie Akeroyd (freddie-akeroyd)]

This is for 3.15 (after a bit of trimming with sort and uniq) - I will looks at the most important sounding ones specific to the os/WIN32 area first then we shoudl have a chat Gabriel so we don't start looking at the same class of problem.

https://gist.github.com/FreddieAkeroyd/f0d899c50d086db972e3ec0d25dbb9ab

[Freddie Akeroyd (freddie-akeroyd)]

And preference on how to handle the following:

'gethostbyaddr': Use getnameinfo() or GetNameInfoW() instead or define _WINSOCK_DEPRECATED_NO_WARNINGS

Moving to getnameinfo restricts to Windows XP ot later

GetVersionEx (deprecated ) used to test for a function at xp or higher in osdMutex, if we only support XP or higher versions then the test could be removed

epicsSocketAccept should return SOCKET rather than int - on most systems SOCKET is a typedef to int, but not on Windows. There it is UINT_PTR so 64bit size on win64. However i believe only the lower 32 bits are currently used (like for HANDLE) so there may be more risk in breaking an existing API by fixing the data type

[Freddie Akeroyd (freddie-akeroyd)]

For "and preference" read "any preference"

[Gabriel Fedel (fedel)]

I'm currently trying to address these kind of warnings from gcc 8:

../../../src/libCom/error/errSymLib.c:165:17: warning: ‘strncpy’ output truncated copying between 1 and 5 bytes from a string of length 15 [-Wstringop-truncation]
                 strncpy ( pBuf,"<err copy fail>", bufLength );
                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

[Freddie Akeroyd (freddie-akeroyd)]

volatile has changed meaning in c++11 - should we be checking for this and using std::atomic ?

C:\devel\epics-base\src\libCom\osi\os\default\osdMessageQueue.cpp(43): warning C5220: 'threadNode::eventSent': a non-static data member with a volatile qualified type no longer implies
         that compiler generated copy/move constructors and copy/move assignment operators are not trivial
C:\devel\epics-base\src\libCom\osi\os\default\osdMessageQueue.cpp(42): note: see declaration of 'threadNode::eventSent'

[mdavidsaver (mdavidsaver)]

I don't think the volatile in osdMessageQueue.c is presently necessary (and probably never was). These accesses are guarded by a mutex.

[Andrew Johnson (anj)]

Re volatile in osdMessageQueue.c: Agreed, these can be dropped.

[Gabriel Fedel (fedel)]

There are some warnings from gcc 8.3.0 (and above) on biRecord.c and on boRecord.c similar to this one (from 3.15 branch):

../../../src/std/rec/biRecord.c: In function ‘get_enum_str’:
../../../src/std/rec/biRecord.c:165:35: warning: argument to ‘sizeof’ in ‘strncpy’ call is the same expression as the source; did you mean to use the size of the destination? [-Wsizeof-pointer-memaccess]
  strncpy(pstring,prec->znam,sizeof(prec->znam));

---

I think it looks a place where a problem could occur if the size of znam is greater than pstring. If we are sure this is not an issue, we could solve the warning using a fixed length (on the biRecord and boRecord case, znam and onam have length 26).
On that case, could make sense to have a constant for onam, znam (and others similar) length.

Does it make sense?
Any other suggestion ?

[Gabriel Fedel (fedel)]

Another set of warnings on branch 3.15 is related to gdd, the warnings look like this:

In file included from ../gdd.h:511,
                 from ../gddAppTable.h:18, from ../genApps.cc:14:
../gddI.h: In member function ‘gdd& gdd::operator=(const gdd&)’: ../gddI.h:117:30: warning: ‘void* memcpy(void*, const void*, size_t)’ writing to an object of type ‘class gdd’ with no trivial copy-assignment; use copy-assignment or copy-initialization instead [-Wclass-memaccess] { memcpy(this,&v,sizeof(gdd)); return *this; }
                              ^

Is it worthy to fix them?
I'm asking because the gdd code is in a folder called legacy.

[Andrew Johnson (anj)]

So gdd is code that we would have thrown away years ago if we didn't need it to support the CA Gateway and any other CA server tools that use the PCAS. Those libraries were moved to an external module https://github.com/epics-modules/pcas before we released EPICS 7.0.1 so the community still owns and needs to keep the code working, but as to whether it's worth fixing these I would put on the low end of the priority order.

About the bi/bo string thing, there's a hole in our APIs because the get_enum_str() method gets passed a pointer to a buffer without also telling it how big that buffer is. That's probably why the code is using the source's size, because the author had no way to know – this dates all the way back to the first commit of this record code in October 1990. This method probably doesn't get used very much, I actually had a hard time working out how to get the code to call it, but yet it is a potential problem which the Core group will need to decide what to do about. I will file a separate bug about that, so please leave that code as it is, and thanks for raising the question.

[Gabriel Fedel (fedel)]

Thank you Andrew for the explanations.

I've tried to solve the gdd warnings, but I haven't found a solution yet.
My first try was to use the copy method:

--- a/src/ca/legacy/gdd/gddI.h
+++ b/src/ca/legacy/gdd/gddI.h
@@ -114,7 +114,7 @@ inline void gdd::setStatSevr(aitInt16 stat, aitInt16 sevr)
         { status.s.aitStat = stat; status.s.aitSevr = sevr; }

 inline gdd& gdd::operator=(const gdd& v)
- { memcpy(this,&v,sizeof(gdd)); return *this; }
+ { this->copy(&v); return *this; }

 inline int gdd::isScalar(void) const { return dimension()==0?1:0; }
 inline int gdd::isContainer(void) const

But with this change the code doesn't compile, and I don't know why. I got this:

make[3]: *** No rule to make target '../O.Common/gddApps.h', needed by '../../../../../include/gddApps.h'. Stop.
make[2]: *** [../../../../configure/RULES_ARCHS:58: install.linux-x86_64] Error 2
make[1]: *** [../configure/RULES_DIRS:85: ca/legacy/gdd.install] Error 2
make: *** [configure/RULES_DIRS:85: src.install] Error 2

[Gabriel Fedel (fedel)]

Also, I was working on the strncpy warnings but it looks my compiler outputs is not exactly the same as from the ci and from Michael:

https://github.com/epics-base/epics-base/pull/124

I'm basically using :
make 2> warnings.log

From a fresh code. Does someone know if I should do any extra step to replicate the same approach than the ci code?

[Jerzy Tarasiuk (tarasiukj)]

I have tried EPICS base compilation on three different
Ubuntu versions (using LTS = Long Term Support ones only):

Ubuntu 16.04 = GCC and G++ 5.5.0, make 4.1, perl v5.22.1

Ubuntu 18.04 = gcc/g++ 7.3.0, make 4.1, perl v5.26.1
            (there is a gcc 7.5.0 available, but this
             lower version was used for the compilation)

Ubuntu 20.04 = gcc/g++ 9.3.0, make 4.2.1, perl v5.30.0

The base could be compiled on every of them; hoever,
the newer Ubuntu version was used, the more warnings
were produced during the compilation.

I wrote a bug report about warnings showing that some
Perl script could not be fount (it came out that it was
on the beginning of the compilation only and later
the script was used, and it was on every Ubuntu version);
here I am saying about some other warnings I was getting.

One of them was warning about a possibility of buffer
overflow: it was not shown on Ubuntu 16.04, it was shown
for modules/pvAccess/testApp/remote/testServer.cpp
function createNTTable declares char sbuf[16] and uses
a format which can consume more buffer space, at least
theoretically, as the columnsCount is probably small,
so the format may specify %hd instead %d (max=32767).

This warning is shown for Ubuntu 18 and 20, on 18 it is
reported for the first line of the function, on 20 for
the line containing the sprintf.

On all three Ubuntu-s 8 deprecated declarations are
reported.

On the newest only 4 warnings about output truncated
are shown - possibly they are caused by strncpy
(inlined from string.h), however when I tried to get
such a warning from a simple .cpp program, compiling it
using g++ with -Wall -Wstringop-truncation it was not
shown; such a warning are for:
modules/libcom/src/osi/epicsTime.cpp in lines 668 and 680
modules/libcom/src/osi/osiSock.c in line 76
modules/database/src/ioc/dbStatic/dbStaticLib.c in lines
 663, 730 and 745 (one common warning for 3 places - but
 for the last file I do not see the strcpy there - maybe
 functions were inlined and lines are shown incorrectly)

As well as I understand it, such a warning means that
a string used as source for strncpy will _always_ be
truncated (i.e. the compiler predicts it) - sometimes
a string of a length 1 is to be copied without its
terminator and the compiler warns about it!

An alarmistic warning (without a real reason) is shown
for modules/libcom/test/epicsStackTraceTest.c line 96.
The compiles sees that 'sz' gets source length, but
overlooks the fact that it is changed when it exceeds
space in the destination; maybe some change can help?
Unfortunately, when I extracted the code from the EPICS
and compiled it alone, no warning was shown.

Well: to show the warning, I need to use options:
 -O3 -Wall -Werror-implicit-function-declaration
(the last is not needed in this case; -Wpedantic
 and -Wextra can be used, the warning is the same).

Now I tried the code from the EPICS and some modified
- no luck, I am still getting the warning. The code:

#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <ctype.h>

typedef struct { char buf[40]; size_t pos; } TDD, *TestData;

static void logClient(void *ptr, const char *msg)
{
    TestData td = ptr;
    size_t sz...

[Jerzy Tarasiuk (tarasiukj)]

Replace the 3rd arg of strncpy by 'mx' - it compiles without warning,
maybe it becomes less optimal, as end of the source string is to be
detected twice (by strlen and by strncpy) instead once.

But my attempts to get the "output truncated" warning in a simple
program (to find what caused it in EPICS) remain unsuccesful.

[Jerzy Tarasiuk (tarasiukj)]

The arg change mentioned previously is for the program from #15, containing
a function from modules/libcom/test/epicsStackTraceTest.c for which the GCC
issues a warning " specified bound depends on the length of the source
 argument [-Wstringop-overflow=]". Now on another kind of warning, like:
"specified bound 64 equals destination size [-Wstringop-truncation]"
this or similar warning is produced from the following:

modules/ca/src/client/msgForMultiplyDefinedPV.cpp lines 44 and 46
    strncpy ( this->acc, pAcc, sizeof ( this->acc ) );
    strncpy ( this->channel, pChannelName, sizeof ( this->channel ) );
modules/ca/src/perl/Cap5.xs lines 602 and 694 (here 40, not 64)
- both: strncpy(data.dbr_string, SvPV_nolen(val), MAX_STRING_SIZE);

If the stored value is to contain a terminator, may specify one character
less as arg3 od strncpy, and store 0 in the last character - the message
means that storing the entire destination was tried (leaving no space for
the terminator). Or... disable the -Wstringop-truncation ?

[Jerzy Tarasiuk (tarasiukj)]

modules/ca/src/client/caRepeater.cpp line 93
- a chdir("/"); alone causes a warning;
  an old construction (void)chdir("/"); caused a warning, too;
  to avoid the warning, need use: if(chdir("/")) {} ;)

modules/database/src/ioc/dbtemplate/msi.cpp line 306
- signed/unsigned comparision: maybe declare 'i' as unsigned?
  as an array index unsigned is OK, maybe need use typecast:
  cmdind = i; => cmdint = (int)i; (the cmdint must be int)
an alternative can be a code like:
  for (cmdint = NELEMENTS(cmdNames); cmdint >=0; cmdint--) {
    if (strstr(command, cmdNames[cmdint])) break;
  }

[Jerzy Tarasiuk (tarasiukj)]

Yet another machine used to compile EPICS base: NI's Compact RIO 9039.
Operating system: NI Linux Real-Time 6.0, with installed software:
GNU Make 4.2.1, perl v5.24.1, gcc/g++ 6.3.0; 9 warnings.
The attachment contains saved stderr from the compilation.
(my plan is to build EPICS IOC on the Compact RIO, with an interface
 to a control program in its FPGA, these Ubuntu-s were for tests)

[Jerzy Tarasiuk (tarasiukj)]

Another warning summary (for u16,u18,u20,crio):
1. comparison between signed and unsigned integer expressions
   - 1x except u20? for u20 there is another text
2. deprecated - 8x in all; crio has no more warnings
3. osi/os/posix/osdTime.cpp:64 "done" defined but not used u16 (only!)
4. caRepeater.cpp:93 ignoring return value of chdir("/") u16 u18 u20
5. Cap5.xs:646 CA_put :739 CA_put_callback "p.dbr" may be uninitialized*2 u16 u20
6. testApp/remote/testServer.cpp:265 may write a terminating nul
   past the end of the destination u18*2 u20*2
7. output truncated u20*4
8. specified bound depends on the length of the source argument u20*1
9. specified bound xx equals destination size u20*4
10. rec/biRecord.c argument to 'sizeof' in 'strncpy' call is the same expression
  as the source; did you mean to use the size of the destination? u20*6
11. mbbioDirectTest.c:44 '.B' directive writing 2 bytes into a region of size
  between 1 and 40 u20*2

Total warnings: crio=9 u16=13 u18=12 u20=31
Abbreviations: crio = Compact RIO (NI-9039 w/ NI Linux Real-Time),
               u16/u18/u20 = Ubuntu 16.04 / 18.04 / 20.04 LTS.

More details about some warnings:
1,4,6,8 - a fix was proposed earlier, in #18,#18,#15,#16, respectively
3. the "done" in posix/osdTime.cpp line 64 is really unused; just remove it?
   (or comment it out by putting // - maybe it is to be used in some future)
5. possibly uninitialized p.dbr in Cap5.xs
   there is a best_type() function, and its return value is used in a 'case'
   statement; for 4 values, the p.dbr will be initialized, for any other it
   will remain uninitialized; what can the best_type() function return?
9. bound xx equals - see #17; is the value stored to have a null terminator?

Still poorly understood or not examined at all:
7. output truncated - see #6, #15, #16
2. 8*deprecated
10. argument to 'sizeof' in 'strncpy' call (biRecord.c)
11. '.B' directive writing 2 bytes into a region (mbbioDirectTest.c:44)

[Jerzy Tarasiuk (tarasiukj)]

Shortly, these 8 'deprecated' warnings were caused
by using functions purposely marked as deprecated.
Possibly they are planned for removing. More below.

The warning 'deprecated' occur for 2 files only:
1. modules/pvData/testApp/misc/testByteBuffer.cpp lines 164 and 191
   testOk1(strncmp(buff->getArray(),&src[2],6)==0);
   testOk1(strncmp(buf->getArray(),expect,6)==0);
- the warning is when the getArray() is invoked and used as
  strncmp() arg (the warning place is the ')' of the getArray())
  the buff/buf are of type ByteBuffer(32) #include <pv/byteBuffer.h>
- see modules/pvData/src/misc/pv/byteBuffer.h for the getArray() definition
  EPICS_ALWAYS_INLINE const char* getArray() const EPICS_DEPRECATED
- modules/libcom/src/osi/compiler/clang/compilerSpecific.h
  #define EPICS_DEPRECATED __attribute__((deprecated))
- obviously the warning is generated purposely

2. modules/pvAccess/testApp/remote/testServer.cpp lines 2261 2270 2279
    epics::pvData::StructureConstPtr s =
      getFieldCreate()->createFieldBuilder()->
=> addBoundedString("value", 8)->
      add("timeStamp", getStandardField()->timeStamp())->
      createStructure();
     epics::pvData::StructureConstPtr s =
       getFieldCreate()->createFieldBuilder()->
=> addBoundedArray("value", pvDouble, 8)->
       add("timeStamp", getStandardField()->timeStamp())->
       createStructure();
     epics::pvData::StructureConstPtr s =
       getFieldCreate()->createFieldBuilder()->
=> addFixedArray("value", pvDouble, 8)->
       add("timeStamp", getStandardField()->timeStamp())->
       createStructure();
the warning place is always on the ')'.
=> https://github.com/epics-base/pvDataCPP/issues/52
 [-Wdeprecated-declarations]
- modules/pvData/src/pv/pvIntrospect.h
  #define PVD_DEPRECATED_52 PVD_DEPRECATED(\
"See https://github.com/epics-base/pvDataCPP/issues/52")
- these functions addBoundedString(), addBoundedArray(),
  and addFixedArray() are marked by the PVD_DEPRECATED_52
- modules/pvData/src/misc/pv/serialize.h
  defines PVD_DEPRECATED(msg) if defined(PVD_INTERNAL)
  (without the PVD_INTERNAL it is defined as do-nothing)

[Dirk Zimoch (dirk.zimoch)]

In my opiniton it would make sense to disable all DEPRECATED macros for EPICS internal calls. At least for building the tests.

[Jerzy Tarasiuk (tarasiukj)]

'.B' directive writing 2 bytes into a region (mbbioDirectTest.c:44)

modules/database/test/std/rec/mbbioDirectTest.c
- line 38: use a larger 'field', e.g. char field[44]; ?
  but: the 'rec' arg can be up to "do-NNNNNNNNNN" (13-char)
  only and the sprintf in void testmbbioFields() can produce
  at most 4 more chars +terminator - 17 total; this is a G++
  bug - it sees the 'rec' declared size and assumes the whole
  size is to be used; need 'field' size to be 4 char bigger,
  but the 'rec' size can be 16-char, and the 'field' 20-char.
  (this assumes 'int' is 32-bit; for 64=bit int need 24/28)

The code which causes the warning is:

static
void testmbbioFields(const char* rec, unsigned int value)
{
    char field[40];
    unsigned int i;

    testdbGetFieldEqual(rec, DBF_ULONG, value);
    for (i=0; i < 32; i++)
    {
        sprintf(field,"%s.B%X", rec, i);
        testdbGetFieldEqual(field, DBF_ULONG, (value>>i)&1);
    }
}

static
void testmbbioRecords(unsigned int count, unsigned int value)
{
    char rec[40];
    unsigned int i;

    for (i = 1; i <= count; i++)
    {
        sprintf(rec, "do%d", i);
        testDiag(" ### %s ###", rec);
        testmbbioFields(rec, value);
        sprintf(rec, "di%d", i);
        testmbbioFields(rec, value);
    }
}

[Jerzy Tarasiuk (tarasiukj)]

modules/database/src/std/rec/biRecord.c
  argument to 'sizeof' in 'strncpy' call is the same
  expression as the source; did you mean to use the size
  of the destination? [-Wsizeof-pointer-memaccess]
  line:char 184:42 187:42 201:43 203:43 331:42 334:42

A sizeof znam/onam field size is used as the size limit
for strncpy; GCC suspects it to be a mistake and warns.
Unfortunately, the size is specified as a number in
biRecord.h - a fix should change the generator to use
a name and #define the name, and then use the name
instead the sizeof (using a number in the biRecord.c
would remove the warning, but the code would become
wrong without a warning if the size was changed later).