EPICS Base

SunPro C++ compiler warnings on 3.15 branch

Bug #885802 reported by Andrew Johnson
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
EPICS Base
Won't Fix
Low
Andrew Johnson
EPICS Base 3.15.branch

Bug Description

I'm seeing the following warnings from the SunPro compiler when building the 3.15 branch code, which occur because src/libCom/test/epicsAtomicTest.cpp is passing pointers to functions with C++ linkage into epicsThreadCreate() which expects an extern "C" function. The result does run properly everywhere I've tested it, although there might be platforms out there where it won't:

make[1]: Entering directory `/home/phoebus3/ANJ/epics/base/mirror-3.15/src/libCom/test/O.solaris-sparc'
/opt/SUNWspro/bin/CC -c -D_POSIX_C_SOURCE=199506L -D_XOPEN_SOURCE=500 -DUNIX -DSOLARIS=10 -mt -D__EXTENSIONS__ -O +w -I. -I../O.Common -I. -I. -I.. -I../../../../include/compiler/solStudio -I../../../../include/os/solaris -I../../../../include ../epicsAtomicTest.cpp
"../epicsAtomicTest.cpp", line 141: Warning (Anachronism): Formal argument funptr of type extern "C" void(*)(void*) in call to epicsThreadCreate(const char*, unsigned, unsigned, extern "C" void(*)(void*), void*) is being passed void(*)(void*).
"../epicsAtomicTest.cpp", line 225: Where: While instantiating "testIncrDecr<int>()".
"../epicsAtomicTest.cpp", line 225: Where: Instantiated from non-template code.
"../epicsAtomicTest.cpp", line 143: Warning (Anachronism): Formal argument funptr of type extern "C" void(*)(void*) in call to epicsThreadCreate(const char*, unsigned, unsigned, extern "C" void(*)(void*), void*) is being passed void(*)(void*).
"../epicsAtomicTest.cpp", line 225: Where: While instantiating "testIncrDecr<int>()".
"../epicsAtomicTest.cpp", line 225: Where: Instantiated from non-template code.
"../epicsAtomicTest.cpp", line 141: Warning (Anachronism): Formal argument funptr of type extern "C" void(*)(void*) in call to epicsThreadCreate(const char*, unsigned, unsigned, extern "C" void(*)(void*), void*) is being passed void(*)(void*).
"../epicsAtomicTest.cpp", line 226: Where: While instantiating "testIncrDecr<unsigned>()".
"../epicsAtomicTest.cpp", line 226: Where: Instantiated from non-template code.
"../epicsAtomicTest.cpp", line 143: Warning (Anachronism): Formal argument funptr of type extern "C" void(*)(void*) in call to epicsThreadCreate(const char*, unsigned, unsigned, extern "C" void(*)(void*), void*) is being passed void(*)(void*).
"../epicsAtomicTest.cpp", line 226: Where: While instantiating "testIncrDecr<unsigned>()".
"../epicsAtomicTest.cpp", line 226: Where: Instantiated from non-template code.
"../epicsAtomicTest.cpp", line 173: Warning (Anachronism): Formal argument funptr of type extern "C" void(*)(void*) in call to epicsThreadCreate(const char*, unsigned, unsigned, extern "C" void(*)(void*), void*) is being passed void(*)(void*).
"../epicsAtomicTest.cpp", line 227: Where: While instantiating "testAddSub<int>()".
"../epicsAtomicTest.cpp", line 227: Where: Instantiated from non-template code.
"../epicsAtomicTest.cpp", line 175: Warning (Anachronism): Formal argument funptr of type extern "C" void(*)(void*) in call to epicsThreadCreate(const char*, unsigned, unsigned, extern "C" void(*)(void*), void*) is being passed void(*)(void*).
"../epicsAtomicTest.cpp", line 227: Where: While instantiating "testAddSub<int>()".
"../epicsAtomicTest.cpp", line 227: Where: Instantiated from non-template code.
"../epicsAtomicTest.cpp", line 173: Warning (Anachronism): Formal argument funptr of type extern "C" void(*)(void*) in call to epicsThreadCreate(const char*, unsigned, unsigned, extern "C" void(*)(void*), void*) is being passed void(*)(void*).
"../epicsAtomicTest.cpp", line 228: Where: While instantiating "testAddSub<unsigned>()".
"../epicsAtomicTest.cpp", line 228: Where: Instantiated from non-template code.
"../epicsAtomicTest.cpp", line 175: Warning (Anachronism): Formal argument funptr of type extern "C" void(*)(void*) in call to epicsThreadCreate(const char*, unsigned, unsigned, extern "C" void(*)(void*), void*) is being passed void(*)(void*).
"../epicsAtomicTest.cpp", line 228: Where: While instantiating "testAddSub<unsigned>()".
"../epicsAtomicTest.cpp", line 228: Where: Instantiated from non-template code.
"../epicsAtomicTest.cpp", line 208: Warning (Anachronism): Formal argument funptr of type extern "C" void(*)(void*) in call to epicsThreadCreate(const char*, unsigned, unsigned, extern "C" void(*)(void*), void*) is being passed void(*)(void*).
"../epicsAtomicTest.cpp", line 229: Where: While instantiating "testCAS<int>()".
"../epicsAtomicTest.cpp", line 229: Where: Instantiated from non-template code.
"../epicsAtomicTest.cpp", line 208: Warning (Anachronism): Formal argument funptr of type extern "C" void(*)(void*) in call to epicsThreadCreate(const char*, unsigned, unsigned, extern "C" void(*)(void*), void*) is being passed void(*)(void*).
"../epicsAtomicTest.cpp", line 230: Where: While instantiating "testCAS<unsigned>()".
"../epicsAtomicTest.cpp", line 230: Where: Instantiated from non-template code.
"../epicsAtomicTest.cpp", line 208: Warning (Anachronism): Formal argument funptr of type extern "C" void(*)(void*) in call to epicsThreadCreate(const char*, unsigned, unsigned, extern "C" void(*)(void*), void*) is being passed void(*)(void*).
"../epicsAtomicTest.cpp", line 231: Where: While instantiating "testCAS<void*>()".
"../epicsAtomicTest.cpp", line 231: Where: Instantiated from non-template code.
"../epicsAtomicTest.cpp", line 51: Warning: Could not find source for TestDataAddSub<int>::delta.
"../epicsAtomicTest.cpp", line 51: Warning: Could not find source for TestDataAddSub<unsigned>::delta.
13 Warning(s) detected.
/opt/SUNWspro/bin/CC -o epicsAtomicTest -L/home/phoebus3/ANJ/epics/base/mirror-3.15/lib/solaris-sparc -R/home/phoebus3/ANJ/epics/base/mirror-3.15/lib/solaris-sparc -L/usr/local/lib -mt -z ignore -z combreloc -z lazyload -R/usr/local/lib epicsAtomicTest.o -lCom
make[1]: Leaving directory `/home/phoebus3/ANJ/epics/base/mirror-3.15/src/libCom/test/O.solaris-sparc'

I get a similar warning from src/ca/client/acctstRegister.cpp which I can resolve by wrapping the static void acctstCallFunc() definition inside an extern "C" {} block, but I can't do that for epicsAtomicTest.cpp as I then get these errors:

"../epicsAtomicTest.cpp", line 29: Error: Template declarations cannot have extern "C" linkage.
"../epicsAtomicTest.cpp", line 38: Error: Template declarations cannot have extern "C" linkage.
"../epicsAtomicTest.cpp", line 48: Error: Template declarations cannot have extern "C" linkage.
"../epicsAtomicTest.cpp", line 57: Error: Template declarations cannot have extern "C" linkage.
"../epicsAtomicTest.cpp", line 110: Error: Template declarations cannot have extern "C" linkage.

mdavidsaver (mdavidsaver)
Changed in epics-base:
assignee: nobody → Andrew Johnson (anj)
Revision history for this message
Ralph Lange (ralph-lange) wrote :

@Andrew, is this still relevant?

Revision history for this message
Andrew Johnson (anj) wrote :

Yes, the warnings still occur because the code is passing a series of template functions (which must have C++ linkage) to the routine epicsThreadCreate() that wants a pointer to a function with C linkage.

I have a solution (attached), which is to add a new C++-only epicsThreadCreateCpp() API that takes a function pointer with C++ linkage and arranges for it to be called properly in the new thread. For completeness it also catches any exceptions thrown in the target thread, and converts thread-creation failures into exceptions (instead of returning a NULL epicsThreadId).

Any objections to my adding this?

Revision history for this message
mdavidsaver (mdavidsaver) wrote :

> Any objections to my adding this?

Yes, I think there is a mis-understanding here about what role 'extern "C"' plays in passing a function pointer, with a side order of template fun. I've had to deal with this in the past, as I use c++ frequently, and have never needed additional API calls, or special wrappers (w/ gcc, clang, or msvc).

Revision history for this message
mdavidsaver (mdavidsaver) wrote :

With gcc, clang, and msvc, an 'extern "C"' only controls name mangling, not calling convention. Is SunPro different?

Also, should this prove necessary to placate SunPro, this seems like a case where c++ argument overloading could avoid an API change.

Revision history for this message
Andrew Johnson (anj) wrote :

There is no requirement in the C++ standard that the ABI calling convention for a C-language function be compatible with that for a C++-language function, only that both be supported by the compiler, controlled by the language linkage declared for that function. The ABI implementations you are used to don't distinguish between the two and this is actually also true of the Solaris ABI as well, but the SunPro compiler is giving warnings because this could be a portability issue.
    https://stackoverflow.com/questions/19422857/
    https://gcc.gnu.org/bugzilla/show_bug.cgi?id=2316

I tried using argument overloading but gcc gives a multiple definition error (it doesn't even distinguish between the two function pointer types, see the above bugzilla link).

I added the exception handling to make it possible to remove similar code elsewhere, although Base-3.15 only has one C++ file outside of the test programs that calls epicsThreadCreate().

Revision history for this message
mdavidsaver (mdavidsaver) wrote :

> ... this could be a portability issue.

Theoretically, yes you are correct as far as I can tell. I'm not convinced that this is worth worrying about though. Let alone making API changes for. (How long will you be supporting SunPro?).

> ... Base-3.15 only has one C++ file outside of the test programs that calls epicsThreadCreate().

How about switching these to use class epicsThread? This also has the benefit of catching any c++ exceptions.

Also, I have an allergy to being forced to use extern "C" for what should be static/non-visible symbols.

Revision history for this message
Andrew Johnson (anj) wrote :

epicsThreadTest.cpp contains:

extern "C" {
static void thread(void *arg)
{
    ...
}
}

tux% nm epicsThreadTest.o | grep thread
00000000000000e0 t thread

Thus when using this construct the symbol is not visible externally.

Anyway, the point of my adding epicsThreadCreateCpp() was to allow us to *not* have to use the extern "C" that we are supposed to have with epicsThreadCreate(). Any C++ function pointer matching
    void (*)(void*)
can be passed to this, and the function can even safely throw exceptions, which will be logged before the thread exits.

Yes it's a new API that can't be used with external code that has to build against older Base versions etc., but I think it's worth having the Cpp() version as well for the throw safety (which isn't present in the old version and shouldn't be added since older releases don't have it).

If you still object I will drop this.

Revision history for this message
mdavidsaver (mdavidsaver) wrote :

> to allow us to *not* have to use the extern "C"

class epicsThread already accomplishes this in a more c++ friendly way, though epicsRunnable is a bit awkward.
Rather than adding a new (C-like) function, how about another constructor for class epicsThread? (a la. epics::pvData::Thread).

https://github.com/epics-base/pvDataCPP/blob/a1c0e432ee605c8abaa8f34185503c2886e12545/src/misc/pv/thread.h#L185

Revision history for this message
Andrew Johnson (anj) wrote :

How would you manage the new epicsThread objects that creates without leaking memory?

I converted my code to implement that, but that API turns out to be more complicated to use because the epicsThread destructor explicitly waits for the thread to exit, so unless the thread has a very limited lifetime you either have to leak epicsThread objects (27,200 bytes each) or collect all the pointers and destroy them all later.

Revision history for this message
mdavidsaver (mdavidsaver) wrote :

> How would you manage the new epicsThread objects that creates without leaking memory?

See how a good c++ API forces you to think about resource cleanup! For unittest code the answer is usually easy, and epicsAtomicTest is no exception. Keep them on the stack.

The test cases each start thread(s), wait, then test to if the thread did the right thing. This is naturally the point where the threads would join, and also serves to enforce isolation of each test case (which reuse globals).

imo, best (as in ideal) C++ API design practice would ultimately tie all resource allocation to a stack frame. This has a number of benefits, including exception safety and ease of unit testing. And of course it is pleasing when valgrind tells you "no leaks are possible".

Revision history for this message
Andrew Johnson (anj) wrote :

Having thought more about resource cleanup as you suggest, I now realize that the epicsThread class is not compatible with your "another constructor" idea without major internal changes, which I am not willing to spend time on.

While callers would be responsible for managing their epicsThread objects, the new constructor has to allocate an internal 'struct runner' object derived from epicsThreadRunable that holds the function pointer and void *parm, and provides the run() method that epicsThreadCallEntryPoint() calls. The epicsThread holds a reference to this object.

In my original I can delete the equivalent 'struct caller' object immediately after calling the function, but in epicsThread the reference to the runner exists for the life of the epicsThread object, so user code could still call epicsThread::show(2) after the thread has exited, which calls the runner's virtual show() routine so *bang*. There is nowhere to do the cleanup of this object within the current epicsThread implementation.

My original idea adds about 40 lines of straightforward new code to epicsThread.cpp. Yours would require significant internal surgery to get working safely.

Revision history for this message
mdavidsaver (mdavidsaver) wrote :

> not compatible with your "another constructor" idea without major internal changes

umm.. Did you see how I accomplished this with epics::pvData::Thread (which is a sub-class of epicsThread)?
cf. 'p_runner' vs. 'p_owned_runner'.

> My original idea adds about 40 lines of straightforward new code to epicsThread.cpp. ...

I think epicsThreadCreateCpp() encourages bad C++ practice.

Andrew Johnson (anj)
Changed in epics-base:
status: Confirmed → Won't Fix
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.
Subscribing...

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.