lv_obj_del fails in the same place upon trying to delete and object.
What MCU/Processor/Board and compiler are you using?
PC Simulator
What do you want to achieve?
Delete a not longer used object
What have you tried so far?
lv_obj_del and lv_obj_del_async
Code to reproduce
Theres alot of code, but here is the gist of it.
When a tree node is created, it creates 3 sibling lv_obj_t*, these are stored in a vector for the node.
When I am deleting the node, I do:
for (int i=0; i<lvObjects.size(); ++i)
{
if(lvObjects[i]!=nullptr)
lv_obj_del_async(lvObjects[i]);
}
This always fails in the same place:
void lv_group_remove_obj(lv_obj_t * obj)
{
lv_group_t * g = obj->group_p;
if(g == NULL) return;
if(g->obj_focus == NULL) return; /*Just to be sure (Not possible if there is at least one object in the group)*/ <- This line :(
I havent added the obj to any groups, so I a at a bit of a loss how to destroy them
The exception data:
Unhandled exception thrown: read access violation.
g was 0xFFFFFFFFFFFFFFE7.
One of those two should be working. My guess is that there’s some sort of memory corruption, since 0xFFFFFFFFFFFFFFE7 is a very oddly numbered address.
I assume that in a simple demo project this issue doesn’t happen? Also, ensure that you are not deleting an object that has already been deleted.
Well, the code sample you showed does not set lvObjects[i] to nullptr after deletion, so I would expect that. Unless you were doing that in your code and you just didn’t show it?
One option would be for us to add an lv_obj_valid function that scans the linked list of objects to see if the passed pointer is valid. That would allow you to add a debug assertion that verifies that the object is valid (i.e. not a bogus pointer or deleted).
Oh it was a litany if errors on my part that lead to it.
An idea maybe, how about merging your idea of lv_obj_valid into this. Maybe have an lv_obj_del_safe that deletes and obj and children, checking each for validity before deleting.
@rohmer Maybe. My only concern with that is that it would unconditionally add extra “debugging” code into a hot path of the library itself, whereas ASSERT(lv_obj_valid(obj)) could be done in someone’s project and wouldn’t pollute LittlevGL.
OTOH, we do have lv_mem_assert used in lots of places already, so maybe another “safety check” isn’t a big deal. But a null pointer on memory allocation is a more likely crash and worth checking for.
My thought was this would be a NEW option for lv_obj_del_safe, so other than a little added code, it wouldnt be in a hot path of the library.
Second… I can see both sides of this issue to be honest. I would not want to use the safe delete during development, as it would hide issues (Like mine, I deleted objects from a vector but didnt clear the vector, so it had a bunch of links to dead objects).
OTOH, I believe it would be awesome for “Production Code”. Here is my scenario:
I create a piece of code that runs indefinitely. Sure ive been programming for a long time, but you cant test every code path, esp in a UI where you can bifurcate almost to infinity. So in this case, you could end up with some stale pointers or what not. So having a safe delete would be a really good thing.
I was considering for a while to add a LV_DEBUG_MODE config option. My idea was to perform a type check at the beginning of all API functions. E.g. lv_label_set_text really gets a label. However, we can do other things like “validity check” too.
To make it extendible there can be a macro with the “check” functions to call:
#define LV_TEST_OBJ(obj, type_str) \
if(type_check(obj, type_str)) LV_DEBUG_HALT(); \
if(alive_check(obj)) LV_DEBUG_HALT(); \
// and so on
Before you continue reading, I just want to note that I’m not rejecting any particular implementation when I say that I disagree.
It wouldn’t be too difficult, provided that we assume the printing code is still going to work when LV_DEBUG_HALT is called.
Interesting. I have the exact opposite opinion. Maybe we have different interpretations of lv_obj_del_safe? My theoretical implementation would have asserted that the object existed and printed an error/aborted the whole program if it didn’t.
It depends on the use case.
I would argue that production code needs to run as fast as possible. Checking whether an object is valid is a fairly slow operation which would run in O(n) time (I’m pretty sure it would have to traverse the linked list for every single object).
That means that an application would be significantly slowed down when validity checks are in place. This is great for development (and when you are trying to find a bug), but would probably have a significantly negative effect on performance.
I like this idea. This is the approach I use inside my own projects. When debug is enabled, assertions are turned on (and I use a lot of assertions). Since adopting this approach, I think I’ve almost never encountered a fault from the processor, as the assertion always discovers the issue before it happens.
So for debugging, I generally want to see all of the issues. Not hide them.
For production, in general I would use lv_obj_del, but lets use my tree example.
Trees grow and shrink thru usage, and all that growing/pruning/grafting/etc could leave dangling stuff.
So if I had a use case where I had a tree that I wanted to create, use a lot, then delete. I could see using the lv_obj_del_safe on the root node’s lv_obj, and clean out the children safely.
I guess there is no harm in adding lv_obj_del_safe for a case like that.
As a personal preference, though, I would prefer to fix the code in the event that someone left dangling pointers, rather than relying on the library to spot it for me. But that’s just my opinion.
I agree. If you relied on the library will spot the dirty pointer then we would need lv_label_create_safe() (what if the parent is invalid) lv_label_set_text_safe (what if the label is invalid) and so on.
IMO, helping the user to find the issue is more reasonable.
@kisvegabor I disagree with this assertion:
" I agree. If you relied on the library will spot the dirty pointer then we would need lv_label_create_safe() (what if the parent is invalid) lv_label_set_text_safe (what if the label is invalid) and so on."
2 points:
First of all - I am not suggesting reliance on the library, not at all (To ref @embeddedt’s comment, I agree totally. That is why I would never use this code path in development. Thats where you WANT to see bugs). What I am suggesting, is in complicated, long running systems, bugs surface that you had no clue you were there. Back to the code path comment.
Second - The application would be slowed in the event of this code path, and I would not suggest it for every case, but in the case of a complicated thing, that changed a lot, I would indeed want to use it for safety.
agreed, I too would want to fix the code as well. And this would be a good place for telemetry, to tell me that the safe delete spotted something that was amiss.
However, issues can occur in other functions too. Why do we pick only lv_obj_del?
Having issues with lv_obj_get_parent() or lv_obj_get_width is similarly likely.
If the goal is to leave certain protection in production code too it’d be possible to make the checks configurable. E.g.:
