Virtual destructor for base class "Print"

[Ivan-Perez]

Abstract classes must have virtual destructors, so when you delete an object through a pointer the proper destructor can be called.

Lack of this virtual constructor gives two possible compiler warnings in some situations:

• deleting object of abstract class type 'Print' which has non-virtual destructor will cause undefined behaviour [-Wdelete-non-virtual-dtor]
• deleting object of polymorphic class type '...' which has non-virtual destructor might cause undefined behaviour [-Wdelete-non-virtual-dtor]

More info: http://stackoverflow.com/a/461224/1852787

Example of undefined behaviour code without the virtual destructor:

#include <SoftwareSerial.h>

void setup()
{
  Print* printStream = new SoftwareSerial(1, 2);
  delete printStream;// will not call destructor for SoftwareSerial

  SoftwareSerial* serial = new SoftwareSerial(3, 4);
  delete serial;// this causes another warning because of undefined behaviour
}
void loop() { }

This pull request fixes both possible warnings.

[NicoHood]

Doesnt this add a lot overhead?

[Ivan-Perez]

There are lots of virtual functions across all Arduino's base libraries. For example, in the same file, the write function is virtual. This virtual destructor is far less frequently used than the print or write functions.

What's more, I think it is more important that these base libraries are correct, so compiling any project using them should not raise any compiler warning. What is it that the library has no overheads if its behavior is not correct?

But anyway, overhead added here will be minimal. No overhead would appear if you don't use pointers.

[NicoHood]

Please check that again. I am not sure if it will not add any overhead. From my experience it will always add an overhead, even if not used. Thats the only thing that I am worried about.

And those other virtual functions in the code base are essential. But they mostly avoid it for a good reason. Not sure if there are other solutions to fix your problem.

[Ivan-Perez]

But compared to how many times the write function is called, which already has the same overhead that will be added here, how often you'll delete these type of objects? In small projects or on those projects which do not create and delete objects derived from Print - never. The same as the write function is essential to be declared virtual, the destructor too, if not defined as virtual how can the compiler delete properly a derived object?

There's lot of information where virtual destructors should be used, and one of the causes is when on class that will be derived, with some exceptions which are not this case. References: (1) (2) and more

Without virtual, when I delete a SoftwareSerial object through a pointer to its base class (Print) the compiler will only call the base class destructor, which can cause a memory leak if SoftwareSerial has reserved some memory. In the other hand, if I hold a reference to a SoftwareSerial object in a SoftwareSerial pointer then the compiler may not call the destructor chain (a compiler warning is shown - undefined behaviour).

There are no other solutions to fix my code. Then, my question is: Isn't it important that the library has the correct behaviour?

Thanks

[NicoHood]

What i mean:
It adds some overhead, even if you do not use the deconstructor. That is what i am asking. Does it add overhead only when used or always adds this overhead? Please test that, because I am not sure about that.

If its just in the case of using it: why not. Good idea then.

[Chris--A]

@NicoHood
It always adds overhead.

Here is a small test case:

struct Base{
  virtual ~Base(){ Serial.println("base dtor"); }
  virtual void foo() = 0;
};

struct Derived : Base{
  ~Derived(){ Serial.println("derived dtor"); }  
  virtual void foo(){ Serial.println("derived foo"); }
};

Then there are three basic usage scenarios.

1. An object, which is how things are most commonly used:

Derived d;
d.foo();

This worked fine with or without virtual on Base::~Base. However it adds 656 bytes in my test (most common usage).

---

2. Then there is dynamic memory.

Derived *d = new Derived;
d->foo();
delete d;

This also worked fine, with or without a virtual base. It added 60 bytes.

---

3. Lastly there is dynamic memory using a Base pointer:

Base *b = new Derived;
b->foo();
delete b;

This does not call Derived::~Derived unless Base::~Base is virtual. It also adds 60 bytes.

---

The last example does not seem well suited to the Print class as you'll slice off any custom features in Derived. You couldn't begin Serial without having access to the derived object. However, C++ does allow it, for reasons that might not seem suitable for this object.

There is also nothing stopping a collective group of Base pointers while using a Derived object.
Print *arr[] = { &Serial, &mySoftSerial, &LCD };. And the objects will be deleted properly when their lifetime ends.

However, if people are going to delete using a base pointer, then we'll need virtual dtors. Its just a pain as 99.91% of Arduino users will have either local or global objects. Many people use Print/Stream pointers to pass objects to generic functions, but this is the first I have seen someone mention deleting via the Print base. What is more, any objects that support reading (Serial) actually use Stream, so the virtual dtor overhead is even more as it must go through multiple tiers to get the fully derived object.

I think the code will have to be fixed, as I do not see us having a good enough way to warn the user of certain restricted code patterns. Or....

---

A possible solution:

class Base{
  protected:
    ~Base(){ Serial.println("base dtor"); }
  public:
    virtual void foo() = 0;
};

class Derived : public Base{
  public:
    ~Derived(){ Serial.println("derived dtor"); }  
    virtual void foo(){ Serial.println("derived foo"); }
};

Now the last example (3) will fail on delete b; as the destructor is not accessible.
The other methods do work as protected members are accessible by derived objects.

Now all we need is a comment and documentation mentioning that you cannot delete directly from Print, you must use a derived class (and probably the same for Stream).

Note: cannot be private, must be protected.

[matthijskooijman]

However it adds 656 bytes in my test (most common usage).

You probably meant 65 or 66 bytes there? I also think this is the full snippet, not the overhead of adding the virtual keyword?

As for the overhead of adding the virtual keyword, I believe adding a virtual method or destructor adds one pointer to the Print vtable of all subclasses, so I expect that will add at least 2 bytes (both RAM and flash), more if you have more Print subclasses. Additionally, the Print destructor (even if empty) will also add a bit of flash space. AFAICS it will always be included if a subclass is accessed through a pointer, even if it is not actually used. In some cases, the compiler might be able to devirtualize things entirely, but not always I expect.

Can someone try a few example sketches using serial or ethernet (or some other Print subclass) and see how much bigger the sketch gets by adding the virtual destructor?

What is more, any objects that support reading (Serial) actually use Stream, so the virtual dtor overhead is even more as it must go through multiple tiers to get the fully derived object.

I'm not sure if this is true - if Stream does not actually need to do anything in its destructor, it can just leave it undefined and subclasses will just use the Print destructor. Since both Stream and Print are (I think) abstract, they should not need a vtable of their own, so no overhead there.

In general, objects like Serial in Arduino are only allocated once globally and never de-allocated, making a virtual destructor not very useful. Even if objects are allocated and deallocated dynamically, the responsibility of deallocating falls to the same code that allocated the object, and which will know the actual subclass, removing the need for a virtual destructor. @Ivan-Perez, could you expand a bit on your usecase?

@Chris--A your last suggestion, using a protected destructor, will essentially just enforce the current behaviour of not supporting a virtual destructor / deleting things through a Print*, right? If we decide not to add the virtual destructor, I think this would be a good way to prevent things from silently failing indeed.

[Ivan-Perez]

@matthijskooijman Here it is my usecase. This is an oversimplified logger class, which logs messages to some streams at the same time, of different kinds:

class MultiStream {
private:
  static const uint8_t MAX = 12;
  Stream* streams[MAX];
  uint8_t count;
public:
  MultiStream() {
    this->count = 0;
  }
  ~MultiStream() {
    for (uint8_t i = 0; i < this->count; i++) {
      this->streams[i]->flush();
      delete this->streams[i];
    }
  }
  void add(Stream* stream) {
    this->streams[this->count] = stream;
    this->count++;
  }
  void print(String msg) {
    for (uint8_t i = 0; i < this->count; i++) {
       this->streams[i]->print(msg);
    }
  }
};

MultiStream* ms = new MultiStream();
ms->add(new File("foo.log", O_WRITE));// write to a file
ms->add(new SoftwareSerial(1, 2));// write to gprs port
ms->print(F("testing..."));
delete ms;

In this case, the deletion is not closing nor the file neither the software serial. It is also not freeing the allocated memory -if any- from derived classes.

In my project, where I use lots of classes and inheritance between them (and of course virtual methods and destructors), the overhead of the compiled program when adding this virtual destructor is less than 100 bytes (from 117,278 to 117,346). Maybe the compiler can share some machine code for the v-tables, so for my project there isn't so much overhead as there is for simple programs.

[Chris--A]

@matthijskooijman

Sorry, forgot about this one.

You probably meant 65 or 66 bytes there? I also think this is the full snippet, not the overhead of adding the virtual keyword?

No, 656. Compiling the sketch below (Mega) is 2858 bytes with a virtual base destructor, or 2202 without it.

struct Base{
  virtual ~Base(){ Serial.println("base dtor"); }
  virtual void foo() = 0;
};

struct Derived : Base{
  ~Derived(){ Serial.println("derived dtor"); }  
  virtual void foo(){ Serial.println("derived foo"); }
};

void setup() {
  Serial.begin(9600);
  Derived d;
  d.foo();
}

void loop() {}

I'm not sure if this is true - if Stream does not actually need to do anything in its destructor, it can just leave it undefined and subclasses will just use the Print destructor. Since both Stream and Print are (I think) abstract, they should not need a vtable of their own, so no overhead there.

This is what I thought also, they appear to not be implemented how I'd expect. From inspection of the generated assembly, the compiler generates something for each class in the hierarchy, even if it is empty or not implemented in code.

When using the print class, there may be quite a few calls: Print->Stream->HardwareSerial->(anything else, like my PrintEx lib for instance)`.

It is quite difficult to follow using an Arduino example, my ASM skills are not great, I'm a C++ man. However you can see the execution jumping all over the place before it gets to the print destructor.

Even this sketch has a difference of 720 bytes when using a virtual destructor.

void setup() {
  Serial.begin(9600);
}

void loop() {
  Serial.print("I hate traffic lights");
}

your last suggestion, using a protected destructor, will essentially just enforce the current behaviour of not supporting a virtual destructor / deleting things through a Print*, right?

Exactly!

[matthijskooijman]

I had a closer look at the difference in generated asm. It seems that when adding the virtual destructor to Print, and when compiling the last sketch in the previous example, some extra things appear:

• The HardwareSerial destructor (two versions - one that calls free() and one that does not do anything, I think it is standard in C++ to have multiple)
• A list of global destructors to call (list of pointer addresses between __ctors_end and __dtors_end), as well as some code (__do_global_dtors) to iterate the list and call each of the pointers in turn (just one here). The actual pointer called is _GLOBAL__sub_D___vector_18, which probably loads &Serial into this and calls the destructor, but I suspect it is all inlined, so this is just an empty function.
• malloc and free are added

Adding malloc() and free() is what gives the bulk of the overhead. The reason they are present, is that both the destructors mentioned above are present in the vtable. Since one of the constructors references free() the library compilation unit that contains free() is included in the link, which also pulls in mallo() (I assume). You would expect at least malloc() to be removed, since it really is unused, but the linker only removes unused sections, not unused symbols, and I assume that malloc() and free() live in the same section, so they both stay.

As you would expect from this, if you have a sketch that already references malloc(), the overhead is reduced to a few dozen bytes instead of hundreds, since then malloc() and free() will be present with or without the virtual destructor. If the Print destructor is not virtual, the global dtors overhead will be added, but the actual destructors and malloc()/free() are not added (probably optimized away, since there is no vtable referencing them).

I was hoping that the 1.6.10 IDE version, with LTO enabled, would perhaps solve this issue. However, it turns out the above testing already was running with LTO enabled (my Arduino setup is a bit of a mixture between versions), so that does not seem to help.

In that light, I think that adding a virtual destructor isn't something we can do, at least not until the compiler becomes smarter about this. Adding hundreds of bytes to every sketch that uses Print but not malloc() does not seem acceptable to me.

Adding a non-virtual, protected destructor still adds the global dtors overhead (26 bytes), but not the rest. However, using this provides the same result, but without any overhead:

  protected:
    ~Print() = default;

I think that adding this would be a good way to at least document the absence of a virtual destructor.

@Ivan-Perez, for your usecase, I would suggest adding a wrapper class
that has a virtual destructor and a template subclass to call the actual
destructor you need. e.g.:

class StreamWrapper {
    protected:
        template <typename T>
        class Sub : StreamWrapper {
            Sub(T *stream) : StreamWrapper(stream) { }
            ~Sub() {
                delete ((T*)(this->stream));
            }
        };

        StreamWrapper(Stream *stream) : stream(stream) { }
    public:
        Stream *stream;
        virtual ~StreamWrapper() { }
        template <typename T>
        static Sub<T> wrap(T *stream) {
            return new Sub<T>(stream);
        }
    }
}

In your logging code, you can then use StreamWrapper* instead of
Stream* (and access its stream attribute if you need the underlying
stream) and StreamWrapper::wrap() to wrap a Stream* that is on the
heap along with (through the vtable of the Sub class) a way to call
the actual destructor.

Note that I have not tested the above classes (not even compile-tested), so
it might not serve as more than inspiration :-)

[nomis]

Couldn't this be optional behind an #ifdef in all the affected classes, e.g. ARDUINO_USE_VIRTUAL_DTORS?

[blazoncek]

Hi.
5 years leater and we still don't have virtual destructors. This prohibits derived classes to allocate any memory since it that memory cannot be automatically deallocated.
The workaround above puts a burden on a programmer's shoulders where it shouldn't be the case (since C++ was developed so that programmers shouldn't track when memory was allocated).
@matthijskooijman If there is indeed a need to save 656 bytes of memory I would agree with @nomis that this can be solved by #defines and #ifdefs but instead I would opt for virtual destructors by default.

[nomis]

@matthijskooijman If there is indeed a need to save 656 bytes of memory I would agree with @nomis that this can be solved by #defines and #ifdefs but instead I would opt for virtual destructors by default.

That's 25% of the RAM on an Arduino Micro.

[blazoncek]

That's 25% of the RAM on an Arduino Micro.

In that case you should probably not use C++. ;)

[matthijskooijman]

That's 25% of the RAM on an Arduino Micro.

Reading back, I think those 656 bytes refer to flash usage, not RAM usage.

[blazoncek]

@matthijskooijman is there a chance to get virtual destructors into Print class?

[nomis]

That's 25% of the RAM on an Arduino Micro.

Reading back, I think those 656 bytes refer to flash usage, not RAM usage.

You're too quick for me to correct my error. It's 2% of the flash.

In that case you should probably not use C++. ;)

This is a one time overhead for making any use of virtual destructors.

Making the destructor protected looks like a reasonable solution but the lack of a virtual destructor causes problems for other derived class hierarchies that just want to be Printable.

[matthijskooijman]

@matthijskooijman is there a chance to get virtual destructors into Print class?

It's not my call, but personally I would be hesitant if this adds 600bytes of flash usage to all sketches that are not yet using dynamic memory (which is how I read the earlier discussion). Maybe new compiler versions are smarter about this and manage to optimize this stuff more, that could be interesting to recheck maybe.

[blazoncek]

Perhaps adding virtual destructors on platforms that are not short on memory?
Again #ifdef could help.