ttlappalainen / NMEA2000

NMEA2000 library for Arduino
531 stars 220 forks source link

delete ( (tNMEA_SocketCAN *)) issues #213

Open lsoltero opened 3 years ago

lsoltero commented 3 years ago

the following code

`

include

include

include

using namespace std;

int main() { tNMEA2000_SocketCAN t=new tNMEA2000_SocketCAN((char )"can0"); delete (t); // causes illigal instruction in N2kStream... so live with the memory leak return 0; } ` causes the following compiler warning.

g++ -c -std=c++11 -g -Wall -Werror -I /usr/local/include/nmea0183 -I /usr/local/include/n2k nmead.cpp -o nmead.o nmead.cpp: In function ‘bool setup_n2k()’: nmead.cpp:367:16: error: deleting object of polymorphic class type ‘tNMEA2000_SocketCAN’ which has non-virtual destructor might cause undefined behaviour [-Werror=delete-non-virtual-dtor] delete (t); // causes illigal instruction in N2kStream... so live with the memory leak `

easy enough to fix by adding a virtual destructor to tNMEA2000_SocketCAN. After doing so the compile is happy but on the execution of

delete (t);

you get the following

` (gdb) b main Breakpoint 1 at 0x4013f1: file test.cpp, line 15. (gdb) r Starting program: /home/lsoltero/openwrt.ee/local_packages/nmead/src/test [Thread debugging using libthread_db enabled] Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".

Breakpoint 1, main () at test.cpp:15 15 tNMEA2000_SocketCAN t=new tNMEA2000_SocketCAN((char )"can0"); (gdb) n 16 delete (t); // causes illigal instruction in N2kStream... so live with the memory leak (gdb) p t $1 = (tNMEA2000_SocketCAN ) 0x627cf0 (gdb) p t $2 = { = {_vptr.tNMEA2000 = 0x410a60 <vtable for tNMEA2000_SocketCAN+16>, static FwdModeBit_EnableForward = 1, static FwdModeBit_SystemMessages = 2, static FwdModeBit_OnlyKnownMessages = 4, static FwdModeBit_OwnMessages = 8, static HandleModeBit_OnlyKnownMessages = 16, dbMode = tNMEA2000::dm_None, N2kMode = tNMEA2000::N2km_ListenOnly, ForwardType = tNMEA2000::fwdt_Actisense, ForwardMode = 11, ForwardStream = 0x0, MsgHandlers = 0x0, DeviceReady = false, AddressChanged = false, DeviceInformationChanged = false, Devices = 0x0, DeviceCount = 1, LocalConfigurationInformationData = 0x0, ConfigurationInformation = { ManufacturerInformation = 0x410ca0 "NMEA2000 library, https://github.com/ttlappalainen/NMEA2000", InstallationDescription1 = 0x410cdc "", InstallationDescription2 = 0x410cdd ""}, SingleFrameMessages = {0x0, 0x0}, FastPacketMessages = {0x0, 0x0}, N2kCANMsgBuf = 0x0, MaxN2kCANMsgs = 0 '\000', CANSendFrameBuf = 0x0, MaxCANSendFrames = 40, CANSendFrameBufferWrite = 0, CANSendFrameBufferRead = 0, MaxCANReceiveFrames = 0, MsgHandler = 0x0, ISORqstHandler = 0x0, pGroupFunctionHandlers = 0x0, InstallationDescriptionChanged = false}, skt = 0, _CANport = 0x410960 "can0"} (gdb) n

Program received signal SIGILL, Illegal instruction. 0x0000000000410b1a in typeinfo for N2kStream () (gdb) where

0 0x0000000000410b1a in typeinfo for N2kStream ()

1 0x000000000040142d in main () at test.cpp:16

`

not sure why...

comment out the delete() and everything works fine...

ttlappalainen commented 3 years ago

You are the first one who complains about delete; I have not met any real case yet, where you would need to delete NMEA2000 object. Normally library will be used on devices communicting with N2 bus. So they will be turned on and turned of - delete has nothing to do there, when you turn it off. That is simple reason why there is no destructor and I did not even tried to think how and in which order everything must be cleared. It would be mostly just wasting of time and code space.

lsoltero commented 3 years ago

Yes.. this is really not much of a problem since usually there is one object created. In my case I created an NMEA2000 object using "new" for a second CAN bus interface, then copied it, and when done thought I would delete is a good house keeping measure. Was very surprised to see that I could not delete an object I had just created with new.... Here is a very simple full program that shows the problem... I just compiled and tested to make sure that it wasn't anything silly on my end.

as you see there is nothing to it. new followed by delete causes a core dump. comment out the delete and the application exits without issue.

Anyway... no action needed here unless there is an underlying problem that could cause other issues down the line.

` lsoltero@ubuntu16:~/openwrt.ee/local_packages/nmead/src$ cat test.cpp

include

include

include

using namespace std;

int main() { tNMEA2000_SocketCAN t=new tNMEA2000_SocketCAN((char )"can0"); delete (t); // causes illigal instruction in N2kStream... so live with the memory leak

if ( t == NULL ) return 1;
return 0;

}

`

ttlappalainen commented 3 years ago

I do not understand why do you need to create new and then delete it? If you have second interface, don't you need it over the program. Or even if you do not, just leave it. When your program closes, os will clean everything for that process.

lsoltero commented 3 years ago

the board I am using has a number of different CAN interfaces all running different protocols (n2k, rv-c, j1939, ...) so the application opens each interface and in this case specifically looking for N2K data. If it finds no N2K on the current interface it goes on to the next. in doing so it closes and deletes the NMEA2000 instance since it no longer needs it.

But... it's not a big deal... the embedded system I am working with has plenty of RAM so I can leave the unused instances of NMEA2000 open and live with the small memory usage required for each.

So no need to fix it and the issue is now documented for others.

Take care.