feat(core): Add external data and destructor feature #9112
Conversation
|
Hi @liamHowatt, I've made some adjustments based on our previous discussion and have verified them on a virtual machine. PTAL. |
|
Hi 👋, thank you for your PR! We've run benchmarks in an emulated environment. Here are the results: ARM Emulated 32b - lv_conf_perf32b
Detailed Results Per Scene
ARM Emulated 64b - lv_conf_perf64b
Detailed Results Per Scene
Disclaimer: These benchmarks were run in an emulated environment using QEMU with instruction counting mode. 🤖 This comment was automatically generated by a bot. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
11 issues found across 31 files
Prompt for AI agents (all 11 issues)
Understand the root cause of the following 11 issues and fix them.
<file name="src/core/lv_obj_class.h">
<violation number="1" location="src/core/lv_obj_class.h:82">
lv_obj_set_external_data dereferences ext_data without validating the array pointer (and only asserts obj). Because the new comment promises NULL-safe handling, callers will pass NULL and hit a crash when asserts are off. Please bail out early when obj or ext_data is NULL before iterating.</violation>
<violation number="2" location="src/core/lv_obj_class.h:82">
Passing NULL destructor leaves the previous cleanup callback active, so lv_obj_destruct can double-free new external data.</violation>
</file>
<file name="src/others/observer/lv_observer.h">
<violation number="1" location="src/others/observer/lv_observer.h:95">
The documentation for lv_subject_set_external_data labels the first parameter as an observer even though the API takes an lv_subject_t *. Please update the parameter description to match the function signature so callers know this attaches data to a subject.</violation>
</file>
<file name="src/core/lv_obj_class.c">
<violation number="1" location="src/core/lv_obj_class.c:141">
Destructor is invoked on NULL ext_data slots, violating the API contract and causing callbacks that dereference their input to crash.</violation>
</file>
<file name="src/others/observer/lv_observer.c">
<violation number="1" location="src/others/observer/lv_observer.c:106">
Overwriting subject->ext_data without invoking the previous destructor leaks any resource already associated with the subject.</violation>
<violation number="2" location="src/others/observer/lv_observer.c:519">
Destructing subject->ext_data during every observer removal breaks remaining observers and can trigger double-cleanup; only run the destructor when the subject itself is being destroyed or when the last observer goes away.</violation>
</file>
<file name="src/display/lv_display.c">
<violation number="1" location="src/display/lv_display.c:1256">
Overwriting ext_data/destructor here drops the previously registered destructor without running it, so repeated lv_display_set_external_data calls leak the old payload and skip its cleanup hook.</violation>
</file>
<file name="src/themes/default/lv_theme_default.c">
<violation number="1" location="src/themes/default/lv_theme_default.c:671">
Resetting the theme’s destructor pointer to NULL on every init wipes out any destructor previously attached (e.g., via lv_theme_set_external_data). When lv_theme_default_init() runs again during a resolution-change re-init, the old destructor is lost and its resource can never be released.</violation>
<violation number="2" location="src/themes/default/lv_theme_default.c:672">
Clearing theme->base.ext_data on every init discards the previously registered external data pointer during re-init, so even if a destructor remains, it will receive NULL and the external resource leaks.</violation>
</file>
<file name="src/themes/lv_theme.c">
<violation number="1" location="src/themes/lv_theme.c:101">
Calling the setter drops any previously registered destructor/ext_data without running it, so clearing or replacing data leaks the original resources. Please invoke the old destructor with the prior ext_data before overwriting these fields.</violation>
</file>
<file name="src/misc/lv_event.c">
<violation number="1" location="src/misc/lv_event.c:190">
Calling the descriptor destructor while the list is still being traversed frees ext_data that the active callback continues to expose via e->ext_data, leading to immediate use-after-free when callbacks remove themselves.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| * object deletion. Receives single data pointer as parameter. | ||
| * NULL means no automatic cleanup. | ||
| */ | ||
| void lv_obj_set_external_data(lv_obj_t * obj, void * ext_data[], int ext_data_num, |
There was a problem hiding this comment.
lv_obj_set_external_data dereferences ext_data without validating the array pointer (and only asserts obj). Because the new comment promises NULL-safe handling, callers will pass NULL and hit a crash when asserts are off. Please bail out early when obj or ext_data is NULL before iterating.
Prompt for AI agents
Address the following comment on src/core/lv_obj_class.h at line 82:
<comment>lv_obj_set_external_data dereferences ext_data without validating the array pointer (and only asserts obj). Because the new comment promises NULL-safe handling, callers will pass NULL and hit a crash when asserts are off. Please bail out early when obj or ext_data is NULL before iterating.</comment>
<file context>
@@ -61,6 +61,28 @@ bool lv_obj_is_editable(lv_obj_t * obj);
+ * object deletion. Receives single data pointer as parameter.
+ * NULL means no automatic cleanup.
+ */
+void lv_obj_set_external_data(lv_obj_t * obj, void * ext_data[], int ext_data_num,
+ void (* destructor)(void * ext_data));
+#endif
</file context>
| * object deletion. Receives single data pointer as parameter. | ||
| * NULL means no automatic cleanup. | ||
| */ | ||
| void lv_obj_set_external_data(lv_obj_t * obj, void * ext_data[], int ext_data_num, |
There was a problem hiding this comment.
Passing NULL destructor leaves the previous cleanup callback active, so lv_obj_destruct can double-free new external data.
Prompt for AI agents
Address the following comment on src/core/lv_obj_class.h at line 82:
<comment>Passing NULL destructor leaves the previous cleanup callback active, so lv_obj_destruct can double-free new external data.</comment>
<file context>
@@ -61,6 +61,28 @@ bool lv_obj_is_editable(lv_obj_t * obj);
+ * object deletion. Receives single data pointer as parameter.
+ * NULL means no automatic cleanup.
+ */
+void lv_obj_set_external_data(lv_obj_t * obj, void * ext_data[], int ext_data_num,
+ void (* destructor)(void * ext_data));
+#endif
</file context>
src/others/observer/lv_observer.h
Outdated
| * - Contextual data storage for observer callbacks | ||
| * - Proper memory management for observer-related resources | ||
| * | ||
| * @param observer Pointer to the observer object (must not be NULL) |
There was a problem hiding this comment.
The documentation for lv_subject_set_external_data labels the first parameter as an observer even though the API takes an lv_subject_t *. Please update the parameter description to match the function signature so callers know this attaches data to a subject.
Prompt for AI agents
Address the following comment on src/others/observer/lv_observer.h at line 95:
<comment>The documentation for lv_subject_set_external_data labels the first parameter as an observer even though the API takes an lv_subject_t *. Please update the parameter description to match the function signature so callers know this attaches data to a subject.</comment>
<file context>
@@ -78,6 +82,27 @@ typedef void (*lv_observer_cb_t)(lv_observer_t * observer, lv_subject_t * subjec
+ * - Contextual data storage for observer callbacks
+ * - Proper memory management for observer-related resources
+ *
+ * @param observer Pointer to the observer object (must not be NULL)
+ * @param ext_data User-defined data pointer to associate (may be NULL)
+ * @param destructor Cleanup function called when:
</file context>
| * @param observer Pointer to the observer object (must not be NULL) | |
| * @param subject Pointer to the subject object (must not be NULL) |
| #if LV_EXTERNAL_DATA_AND_DESTRUCTOR | ||
| for(int i = 0; i < LV_EXT_DATA_MAX_NUM; i++) { | ||
| if(obj->destructor) { | ||
| obj->destructor(obj->ext_data[i]); |
There was a problem hiding this comment.
Destructor is invoked on NULL ext_data slots, violating the API contract and causing callbacks that dereference their input to crash.
Prompt for AI agents
Address the following comment on src/core/lv_obj_class.c at line 141:
<comment>Destructor is invoked on NULL ext_data slots, violating the API contract and causing callbacks that dereference their input to crash.</comment>
<file context>
@@ -135,6 +135,15 @@ void lv_obj_class_init_obj(lv_obj_t * obj)
+#if LV_EXTERNAL_DATA_AND_DESTRUCTOR
+ for(int i = 0; i < LV_EXT_DATA_MAX_NUM; i++) {
+ if(obj->destructor) {
+ obj->destructor(obj->ext_data[i]);
+ obj->ext_data[i] = NULL;
+ }
</file context>
| observer->subject->notify_restart_query = 1; | ||
|
|
||
| #if LV_EXTERNAL_DATA_AND_DESTRUCTOR | ||
| if(observer->subject->destructor) { |
There was a problem hiding this comment.
Destructing subject->ext_data during every observer removal breaks remaining observers and can trigger double-cleanup; only run the destructor when the subject itself is being destroyed or when the last observer goes away.
Prompt for AI agents
Address the following comment on src/others/observer/lv_observer.c at line 519:
<comment>Destructing subject->ext_data during every observer removal breaks remaining observers and can trigger double-cleanup; only run the destructor when the subject itself is being destroyed or when the last observer goes away.</comment>
<file context>
@@ -505,6 +515,13 @@ void lv_observer_remove(lv_observer_t * observer)
observer->subject->notify_restart_query = 1;
+#if LV_EXTERNAL_DATA_AND_DESTRUCTOR
+ if(observer->subject->destructor) {
+ observer->subject->destructor(observer->subject->ext_data);
+ observer->subject->ext_data = NULL;
</file context>
| { | ||
| LV_ASSERT_NULL(disp); | ||
|
|
||
| disp->ext_data = ext_data; |
There was a problem hiding this comment.
Overwriting ext_data/destructor here drops the previously registered destructor without running it, so repeated lv_display_set_external_data calls leak the old payload and skip its cleanup hook.
Prompt for AI agents
Address the following comment on src/display/lv_display.c at line 1256:
<comment>Overwriting ext_data/destructor here drops the previously registered destructor without running it, so repeated lv_display_set_external_data calls leak the old payload and skip its cleanup hook.</comment>
<file context>
@@ -1238,6 +1248,16 @@ int32_t lv_display_dpx(const lv_display_t * disp, int32_t n)
+{
+ LV_ASSERT_NULL(disp);
+
+ disp->ext_data = ext_data;
+ disp->destructor = destructor;
+}
</file context>
| theme->base.flags = dark ? MODE_DARK : 0; | ||
| #if LV_EXTERNAL_DATA_AND_DESTRUCTOR | ||
| theme->base.destructor = NULL; | ||
| theme->base.ext_data = NULL; |
There was a problem hiding this comment.
Clearing theme->base.ext_data on every init discards the previously registered external data pointer during re-init, so even if a destructor remains, it will receive NULL and the external resource leaks.
Prompt for AI agents
Address the following comment on src/themes/default/lv_theme_default.c at line 672:
<comment>Clearing theme->base.ext_data on every init discards the previously registered external data pointer during re-init, so even if a destructor remains, it will receive NULL and the external resource leaks.</comment>
<file context>
@@ -667,6 +667,10 @@ lv_theme_t * lv_theme_default_init(lv_display_t * disp, lv_color_t color_primary
theme->base.flags = dark ? MODE_DARK : 0;
+#if LV_EXTERNAL_DATA_AND_DESTRUCTOR
+ theme->base.destructor = NULL;
+ theme->base.ext_data = NULL;
+#endif
</file context>
| theme->base.ext_data = NULL; | |
| if(!theme->inited) theme->base.ext_data = NULL; |
| theme->base.apply_cb = theme_apply; | ||
| theme->base.flags = dark ? MODE_DARK : 0; | ||
| #if LV_EXTERNAL_DATA_AND_DESTRUCTOR | ||
| theme->base.destructor = NULL; |
There was a problem hiding this comment.
Resetting the theme’s destructor pointer to NULL on every init wipes out any destructor previously attached (e.g., via lv_theme_set_external_data). When lv_theme_default_init() runs again during a resolution-change re-init, the old destructor is lost and its resource can never be released.
Prompt for AI agents
Address the following comment on src/themes/default/lv_theme_default.c at line 671:
<comment>Resetting the theme’s destructor pointer to NULL on every init wipes out any destructor previously attached (e.g., via lv_theme_set_external_data). When lv_theme_default_init() runs again during a resolution-change re-init, the old destructor is lost and its resource can never be released.</comment>
<file context>
@@ -667,6 +667,10 @@ lv_theme_t * lv_theme_default_init(lv_display_t * disp, lv_color_t color_primary
theme->base.apply_cb = theme_apply;
theme->base.flags = dark ? MODE_DARK : 0;
+#if LV_EXTERNAL_DATA_AND_DESTRUCTOR
+ theme->base.destructor = NULL;
+ theme->base.ext_data = NULL;
+#endif
</file context>
| theme->base.destructor = NULL; | |
| if(!theme->inited) theme->base.destructor = NULL; |
| { | ||
| LV_ASSERT_NULL(theme); | ||
|
|
||
| theme->ext_data = ext_data; |
There was a problem hiding this comment.
Calling the setter drops any previously registered destructor/ext_data without running it, so clearing or replacing data leaks the original resources. Please invoke the old destructor with the prior ext_data before overwriting these fields.
Prompt for AI agents
Address the following comment on src/themes/lv_theme.c at line 101:
<comment>Calling the setter drops any previously registered destructor/ext_data without running it, so clearing or replacing data leaks the original resources. Please invoke the old destructor with the prior ext_data before overwriting these fields.</comment>
<file context>
@@ -93,6 +93,16 @@ lv_color_t lv_theme_get_color_secondary(lv_obj_t * obj)
+{
+ LV_ASSERT_NULL(theme);
+
+ theme->ext_data = ext_data;
+ theme->destructor = destructor;
+}
</file context>
| if(event == dsc) { | ||
| #if LV_EXTERNAL_DATA_AND_DESTRUCTOR | ||
| if(dsc->destructor) { | ||
| dsc->destructor(dsc->ext_data); |
There was a problem hiding this comment.
Calling the descriptor destructor while the list is still being traversed frees ext_data that the active callback continues to expose via e->ext_data, leading to immediate use-after-free when callbacks remove themselves.
Prompt for AI agents
Address the following comment on src/misc/lv_event.c at line 190:
<comment>Calling the descriptor destructor while the list is still being traversed frees ext_data that the active callback continues to expose via e->ext_data, leading to immediate use-after-free when callbacks remove themselves.</comment>
<file context>
@@ -168,6 +185,12 @@ bool lv_event_remove_dsc(lv_event_list_t * list, lv_event_dsc_t * dsc)
if(event == dsc) {
+#if LV_EXTERNAL_DATA_AND_DESTRUCTOR
+ if(dsc->destructor) {
+ dsc->destructor(dsc->ext_data);
+ dsc->ext_data = NULL;
+ }
</file context>
There was a problem hiding this comment.
Pull Request Overview
This pull request introduces an external data and destructor feature for LVGL, enabling automatic cleanup of user-provided data when LVGL objects are destroyed. This enhancement is designed to support WebAssembly (WASM) use cases where LVGL runs in a virtual machine and needs to manage external resources.
Key changes:
- Adds conditional compilation support via
LV_EXTERNAL_DATA_AND_DESTRUCTORconfig option - Implements destructor callbacks and external data storage across core LVGL components
- Objects support multiple external data pointers (configurable via
LV_EXT_DATA_MAX_NUM)
Reviewed Changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| lv_conf_template.h | Adds configuration option for external data feature with max data count setting |
| Kconfig | Defines build configuration for external data/destructor with help documentation |
| src/lv_conf_internal.h | Internal configuration processing for the new feature flags |
| src/core/lv_obj_private.h | Adds destructor callback and external data array to object structure |
| src/core/lv_obj_class.h/c | Implements object-level external data setter with array management |
| src/core/lv_group_private.h/h/c | Adds destructor support to group objects |
| src/display/lv_display_private.h/h/c | Adds destructor support to display objects |
| src/indev/lv_indev_private.h/h/c | Adds destructor support to input device objects |
| src/misc/lv_event_private.h/h/c | Adds destructor support to event descriptors and event objects |
| src/misc/lv_timer_private.h/h/c | Adds destructor support to timer objects |
| src/misc/lv_anim.h/c | Adds destructor support to animation objects |
| src/others/observer/lv_observer.h/c | Adds destructor support to observer subjects |
| src/themes/*.c | Initializes and cleans up destructors in theme implementations |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/misc/lv_anim.h
Outdated
| /** | ||
| * Get the currently running animation. | ||
| * @return pointer to an animation that is currently being processed. | ||
| */ | ||
| lv_anim_t * lv_anim_get_running_anim(void); | ||
|
|
There was a problem hiding this comment.
The function lv_anim_get_running_anim is documented but not implemented in the corresponding .c file. Either implement this function or remove its documentation.
| /** | |
| * Get the currently running animation. | |
| * @return pointer to an animation that is currently being processed. | |
| */ | |
| lv_anim_t * lv_anim_get_running_anim(void); |
src/core/lv_obj_class.c
Outdated
| if(destructor) { | ||
| obj->destructor = destructor; | ||
| } |
There was a problem hiding this comment.
The destructor is only set when the new destructor parameter is non-NULL. This means if a user calls this function with destructor=NULL to clear external data, the old destructor would still be invoked for the new data. Either always update obj->destructor or check if the existing destructor should be cleared when setting new data.
| if(destructor) { | |
| obj->destructor = destructor; | |
| } | |
| obj->destructor = destructor; |
| #if LV_EXTERNAL_DATA_AND_DESTRUCTOR | ||
| lv_event_dsc_t * dsc = lv_event_get_dsc(list, i); | ||
| if(dsc && dsc->destructor) { | ||
| dsc->destructor(dsc->ext_data); | ||
| dsc->ext_data = NULL; | ||
| } | ||
| #endif | ||
| event_mark_deleting(list, *event_array_at(list, i)); |
There was a problem hiding this comment.
In lv_event_remove_all, lv_event_get_dsc is called unnecessarily when LV_EXTERNAL_DATA_AND_DESTRUCTOR is enabled. The descriptor is already available via *event_array_at(list, i) on line 257. Store this value once and reuse it to avoid the redundant function call.
| #if LV_EXTERNAL_DATA_AND_DESTRUCTOR | |
| lv_event_dsc_t * dsc = lv_event_get_dsc(list, i); | |
| if(dsc && dsc->destructor) { | |
| dsc->destructor(dsc->ext_data); | |
| dsc->ext_data = NULL; | |
| } | |
| #endif | |
| event_mark_deleting(list, *event_array_at(list, i)); | |
| lv_event_dsc_t * dsc = *event_array_at(list, i); | |
| #if LV_EXTERNAL_DATA_AND_DESTRUCTOR | |
| if(dsc && dsc->destructor) { | |
| dsc->destructor(dsc->ext_data); | |
| dsc->ext_data = NULL; | |
| } | |
| #endif | |
| event_mark_deleting(list, dsc); |
| #if LV_EXTERNAL_DATA_AND_DESTRUCTOR | ||
| void (* destructor)(void * ext_data); | ||
| void * ext_data; | ||
| #endif |
There was a problem hiding this comment.
The _lv_event_t structure has destructor and ext_data fields added, but there's no code that initializes or uses these fields in the event structure itself. The feature is only implemented for _lv_event_dsc_t. If these fields are not needed in _lv_event_t, they should be removed to avoid confusion and unnecessary memory overhead.
| #if LV_EXTERNAL_DATA_AND_DESTRUCTOR | |
| void (* destructor)(void * ext_data); | |
| void * ext_data; | |
| #endif |
| for(int c = 0; c < LV_EXT_DATA_MAX_NUM; c++) { | ||
| if(obj->ext_data[c] == NULL) { | ||
| obj->ext_data[c] = ext_data[i]; | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The duplicate check and insertion logic doesn't handle the case where all slots are full and a non-duplicate item cannot be inserted. When all LV_EXT_DATA_MAX_NUM slots are occupied, new non-NULL and non-duplicate ext_data[i] values are silently ignored without any warning or error, which could lead to resource leaks or unexpected behavior.
| for(int c = 0; c < LV_EXT_DATA_MAX_NUM; c++) { | |
| if(obj->ext_data[c] == NULL) { | |
| obj->ext_data[c] = ext_data[i]; | |
| break; | |
| } | |
| } | |
| } | |
| } | |
| bool inserted = false; | |
| for(int c = 0; c < LV_EXT_DATA_MAX_NUM; c++) { | |
| if(obj->ext_data[c] == NULL) { | |
| obj->ext_data[c] = ext_data[i]; | |
| inserted = true; | |
| break; | |
| } | |
| } | |
| if(!inserted) { | |
| LV_LOG_WARN("Failed to insert ext_data[%d]: all slots are full.", i); | |
| } | |
| } |
src/core/lv_obj_class.c
Outdated
| LV_ASSERT_NULL(obj); | ||
| if(ext_data_num > LV_EXT_DATA_MAX_NUM) { | ||
| LV_LOG_WARN("ext_data_num (%d) exceeds LV_EXT_DATA_MAX_NUM (%d), truncating input array.", | ||
| ext_data_num, LV_EXT_DATA_MAX_NUM); | ||
| } |
There was a problem hiding this comment.
Missing null check for the ext_data parameter. If ext_data is NULL but ext_data_num > 0, the function will dereference a null pointer in the loop at line 197. Add a null check for ext_data before proceeding with the loop operations.
|
Hi, Thank you for the new PR. I see that the lv_timer_set_ext_data(lv_timer_t * t, void * ext_data); //Set for each timer
lv_timer_set_ext_data_destructor_cb(lv_ext_data_destructor_cb_t cb); //Set globally for every timer
//Same for groups, animations, etc |
Hi @kisvegabor the destructor is typically the same, but theoretically, it can also be different, depending on whether you bind different destructor functions when creating the timer. |
cwespressif
force-pushed
the
feature/add_external_data_and_destructor
branch
from
83d5ff2 to
ce4a21f
Compare
Signed-off-by: chenwen@espressif.com <chenwen@espressif.com>
cwespressif
force-pushed
the
feature/add_external_data_and_destructor
branch
from
ce4a21f to
fe814b7
Compare
I see, thank you. For maximal flexibility let's keep the desctroctor for every instance, as you have suggested. A few thing about formatting and naming:
typedef struct {
void * data;
void (*free_cb)(void * data);
}lv_ext_data_t;and use it for consistency and maintainability. Please also not that names I suggested as these are more inline with how we usually name things. Please also mark the irrelevant AI comments as resolved. |
3 participants
Part of the LVGL app's code was compiled into a WASM file, while the core LVGL code was placed in the WAMR virtual machine, we have tested LVGL examples using esp-wasmachine and esp-wdf, and all tests passed.
Notes
lv_conf_template.hrun lv_conf_internal_gen.py and update Kconfig.scripts/code-format.py(astyle v3.4.12needs to installed by runningcd scripts; ./install_astyle.sh) and follow the Code Conventions.