Bug found (and fixed) in touchgfx 4.15 and 4.16, AbstractDataGraph.cpp

[VSimu.1]

Hi,

So far in AbstractDataGraph.cpp the library had:

namespace touchgfx
{
AbstractDataGraph::AbstractDataGraph(int16_t capacity)
    : dataScale(1), alpha(255), topPadding(0), leftPadding(0), rightPadding(0), bottomPadding(0),
      maxCapacity(capacity), usedCapacity(0), gapBeforeIndex(0), clickAction()

but it should be:

namespace touchgfx
{
AbstractDataGraph::AbstractDataGraph(int16_t capacity)
    : dataScale(1), alpha(255), topPadding(0), leftPadding(0), rightPadding(0), bottomPadding(0),
      maxCapacity(capacity), usedCapacity(0), gapBeforeIndex(0), clickAction(), dragAction()

i.e. the problem was that "dragAction" was never initialized and our application would crash. But not every time you dragged a Graph widget, but only sometimes (when you enter that screen and everything is initialized for maybe the 2nd or 3rd time, then with the wrong garbage data, it would crash)

Here is our stacktrack where it would always crash. dragAction->isValid() would be called, even though dragAction was never initialized. i.e. you are dereferencing an invalid pointer. The reason why the "!dragAction" check passed, was because dragAction had garbage data (i.e. it was not set to nullptr upon initialization i.e. it was never initialized).

Cheers

[VSimu.1]

Also everywhere in this code, at least our linter, is saying there is no virtual destructor declared anywhere for any *Graph classes, but it has not caused trouble for us yet, so we have not done anything regarding this...

[Romain DIELEMAN · Accepted solution]

Hi,

Thank you very much for reporting this !

/Romain

[Soren Pingel DALSGAARD]

Dear VSimu.1

Can you please be more specific. When I check the source code for TouchGFX I find that

class DataGraphWrapAndClear : public AbstractDataGraphWithY : public AbstractDataGraph : public Container : public Drawable which has a virtual destructor.

class GraphElementDots : public AbstractGraphElement : public CanvasWidget : public Widget : public Drawable which has a virtual destructor.

Since we might roll out a 4.16.1 soon, if you can be more specific, we might be able to look into this and include a fix in 4.16.1.

1) Which specific class is reported to have the problem?

2) Which linter are you using?

3) Are you actually casting a pointer to a base class before deleting it - otherwise it should not cause any problems. TouchGFX does not use dynamic allocation, so there is no problem there.

Thanks for taking the time to report these problems to us.

Regards,

Søren

[VSimu.1]

Dear Søren,

Okay thank you for the information.

Answers to your questions:

1) GraphScroll

2) I do not know this. I do not even know where to find this. We use Eclipse and the code linter that checks the source files there shows this. See screenshot:

 

TouchGFX only had "Warnings". Nothing was in "Errors" tab. 

3) Okay. I do not think that we are for this case. But maybe that may change? We are also not dynamically allocating anything either, we are trying to avoid using "new" if possible.

[Soren Pingel DALSGAARD]

HI VSimu.1

I created a small program to show what happens:

#include <cstdio>
#include <new>
 
class A
{
public:
  A() : x(0) { printf("New A\n"); }
  virtual ~A() { printf("Bye A\n"); }
  virtual void setup(int v) { x = v; }
protected:
  int x;
};
 
class B: public A
{
public:
  B() : A() { printf("New B\n"); }
};
 
class C: public B
{
public:
  C() : B() { printf("New C\n"); }
  ~C() { printf("Bye C\n"); }
  virtual void bar(int v) { x = v * v; }
};
 
int main(int argc, char ** argv)
{
  printf("Crating A\n");
  A* a = new A;
  printf("Deleting A\n");
  delete a;
 
  printf("\n");
 
  printf("Crating B\n");
  A* b = new B;
  printf("Deleting A\n");
  delete b;
 
  printf("\n");
 
  printf("Crating C\n");
  A* c = new C;
  printf("Deleting C\n");
  delete c;
 
  return 0;
}

Where

• A is the base class with virtual function and virtual destructor.
• B is derived from A with no virtual functions and no destructor.
• C is derived from B with a virtual function and a NON-virtual destructor.

In main we create and delete an A, B and C, but we only store an A pointer. So is the base class destructor called when destructing each of A, B, C?

Output from the program, when run, is:

Crating A
New A
Deleting A
Bye A
 
Crating B
New A
New B
Deleting A
Bye A
 
Crating C
New A
New B
New C
Deleting C
Bye C
Bye A

So the destructor is called as expected. BUT if you change the destructor in the BASE class to be non-virtual, the output is as follows:

Crating A
New A
Deleting A
Bye A
 
Crating B
New A
New B
Deleting A
Bye A
 
Crating C
New A
New B
New C
Deleting C
Bye A

and as you can see, the destructor for C is not called when deleting the C object.

So the problem lies in the linter. The rule is that the destructor in the base class must be virtual. Even though it does not take up a lot of space, we do not want to introduce the unnecessary virtual destructors in all classes in TouchGFX. I hope this helps understanding the issue.

Feel free to write again if you have more questions.

Regards,

Søren

[VSimu.1]

Hi Søren,

Thank you for clearing up this concern. That makes sense. Yes the Linter has a lot of false positives.

I do actually have 1 further feedback point. Since you mentioned you will be doing a 4.16.1 release soon. We were happy to find in 4.16.0 that the SwipeContainer.hpp file got a new public function "uint8_t getSelectedPage() const;". We were hard-coding this ourselves because we needed it. But now we do not have to modify touchgfx because of this. Because touchgfx now provides this function.

But, we are still modifying the touchgfx library for something else. Inside SwipeContainer.hpp, we add this one-liner to the end of the class:

public: const PageIndicator& getPageIndicatorInstance() { return pageIndicator; }

We want read-only access to the PageIndicator of the swipe container. The use case is the following. We statically create in TouchGFX Designer 5 Pages in the swipe container. But based on application configuration, we simply "remove" pages, if we do not want so many. This makes the "Page Indicator" not horizontally-centered. So then we added this one-liner to take care of that, which explains our need to modify SwipeContainer.hpp again:

 swipeContainer1.setPageIndicatorXYWithCenteredX(swipeContainer1.getWidth()/2.0, swipeContainer1.getPageIndicatorInstance().getY()); // Center the page indicator. The position changes depending on how many pages are configured, so need to "recenter" via this function. You see we just want the Y-coordinate to stay the same. We do not want to change it. But this is the only way to get what the Y coordinate value is. All we are interested in is horizontally-centered.

So what I am trying to say is: if you can add "public: const PageIndicator& getPageIndicatorInstance() { return pageIndicator; }" to 4.16.1, that would be nice.

Cheers!

[Soren Pingel DALSGAARD]

Hi again,

Release 4.16.1 is a bugfix release, so new feature will make it into 4.16.1. We will take your input into account during our work on 4.17.0 which is for release this spring.

Regards,

Søren

[Soren Pingel DALSGAARD]

If you use SwipeContainer::remove(page), does the pageIndicator not get repositioned and centered properly?

Regards,

Søren

[VSimu.1]

Just tested it to make sure (I did not test with 4.16.0 before you asked). It does not reposition the pageIndicator after SwipeContainer::remove(page).

You see, without the code that I mentioned, the pageIndicator just stays fixed at the same XY position. Regardless of how many pages I remove. The only reason "5 pages" looks good, is because I statically set 5 pages in TouchGFX Designer. So in that case it does not make a difference.

But maybe if you have the code that I wrote previously, at the end of your SwipeContainer::remove() method, then that would work.

I guess since we are talking about this, do the Destructors of all widgets that were on a "page" get called after you call SwipeContainer::remove(page)?