-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: change position and size parameters to size_t to avoid 64-bit system compiler warnings #5873
Conversation
…ystem access to size_t
May I ask which platform are you using? I'm unable to get the warning on my ubuntu22.04. |
I'm getting these warnings using Microsoft Visual Studio 2022. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! I've opened #5904 to have a Windows based CI too.
src/misc/lv_fs.c
Outdated
@@ -203,9 +203,9 @@ static lv_fs_res_t lv_fs_read_cached(lv_fs_file_t * file_p, char * buf, uint32_t | |||
/*If remaining data chunk is smaller than buffer size, then read into cache buffer*/ | |||
res = file_p->drv->read_cb(file_p->drv, file_p->file_d, (void *)buffer, buffer_size, &bytes_read_to_buffer); | |||
file_p->cache->start = file_p->cache->end + 1; | |||
file_p->cache->end = file_p->cache->start + bytes_read_to_buffer - 1; | |||
file_p->cache->end = (uint32_t)(file_p->cache->start + bytes_read_to_buffer - 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to change cache->start, cache->end to size_t
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see no issue with changing them to size_t
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@glueckm Do you have time to complete this modification? I believe the merger will be completed soon :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I can do it, but I'm not 100% sure how to proceed?
Should I change the start(end back to uint32_t and add the cast or do we keep the start/end at size_t and >I just make sure that the pull request merges with the current master?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is recommended to unify to size_t:
typedef struct {
size_t start;
size_t end;
size_t file_position;
void * buffer;
} lv_fs_file_cache_t;
We need some feedback on this pull request. Now we mark this as "stale" because there was no activity here for 14 days. Remove the "stale" label or comment else this will be closed in 7 days. |
Looks good to me. Is there anything else to add here? |
@glueckm |
src/misc/cache/_lv_cache_lru_rb.c
Outdated
@@ -440,7 +440,8 @@ static lv_cache_reserve_cond_res_t reserve_cond_cb(lv_cache_t * cache, const voi | |||
|
|||
uint32_t data_size = key ? lru->get_data_size_cb(key) : 0; | |||
if(data_size > lru->cache.max_size) { | |||
LV_LOG_ERROR("data size (%" LV_PRIu32 ") is larger than max size (%" LV_PRIu32 ")", data_size, lru->cache.max_size); | |||
LV_LOG_ERROR("data size (%" LV_PRIu32 ") is larger than max size (%" LV_PRIu32 ")", data_size, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use %zu
to print size_t
type without casting.
See: https://stackoverflow.com/questions/2524611/how-can-one-print-a-size-t-variable-portably-using-the-printf-family
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use
%zu
to printsize_t
type without casting. See: https://stackoverflow.com/questions/2524611/how-can-one-print-a-size-t-variable-portably-using-the-printf-family
Did not know that this exist. Should have tried tried to google it..
Should I add a new LV_PRIsize_t
define for it, oder just use the the raw %zu
format string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use %zu
directly. It directly matches size_t
and does not require adding macros for conversion.
src/misc/lv_fs.c
Outdated
res = file_p->drv->tell_cb(file_p->drv, file_p->file_d, &tmp_position); | ||
|
||
if(res == LV_FS_RES_OK) { | ||
file_p->cache->file_position = tmp_position; | ||
file_p->cache->file_position = (uint32_t)tmp_position; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please consider removing any unnecessary casts.
I have no idea why the microypthon test failed. Has nothing to do with my changes |
res = lv_fs_read(&file, dest, palette_size_bytes, &palette_br); | ||
if(res != LV_FS_RES_OK || palette_br != palette_size_bytes) { | ||
LV_LOG_ERROR("read %s (palette: %" LV_PRIu32 ", br: %" LV_PRIu32 ") failed", | ||
path, palette_size_bytes, palette_br); | ||
path, palette_size_bytes, (uint32_t)palette_br); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
%zu
res = lv_fs_read(&file, src_temp, src_stride, &br); | ||
if(res != LV_FS_RES_OK || br != src_stride) { | ||
LV_LOG_ERROR("read %s (y: %" LV_PRIu32 ", src_stride: %" LV_PRIu32 ", br: %" LV_PRIu32 ") failed", | ||
path, y, src_stride, br); | ||
path, y, src_stride, (uint32_t)br); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use %zu
@@ -915,7 +915,8 @@ static lv_result_t decode_compressed(lv_image_decoder_t * decoder, lv_image_deco | |||
} | |||
|
|||
if(compressed->compressed_size != compressed_len) { | |||
LV_LOG_WARN("Compressed size mismatch: %" LV_PRIu32" != %" LV_PRIu32, compressed->compressed_size, compressed_len); | |||
LV_LOG_WARN("Compressed size mismatch: %" LV_PRIu32" != %" LV_PRIu32, compressed->compressed_size, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
%zu
@@ -947,7 +948,8 @@ static lv_result_t decode_compressed(lv_image_decoder_t * decoder, lv_image_deco | |||
lv_memcpy(compressed, image->data, len); | |||
compressed->data = image->data + len; | |||
if(compressed->compressed_size != compressed_len) { | |||
LV_LOG_WARN("Compressed size mismatch: %" LV_PRIu32" != %" LV_PRIu32, compressed->compressed_size, compressed_len); | |||
LV_LOG_WARN("Compressed size mismatch: %" LV_PRIu32" != %" LV_PRIu32, compressed->compressed_size, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
%zu
lv_fs_read(&f, &jpg_signature, sizeof(jpg_signature), &rn); | ||
lv_fs_close(&f); | ||
|
||
if(rn != sizeof(jpg_signature)) { | ||
LV_LOG_WARN("file: %s signature len = %" LV_PRIu32 " error", fn, rn); | ||
LV_LOG_WARN("file: %s signature len = %" LV_PRIu32 " error", fn, (uint32_t)rn); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
%zu
@@ -252,23 +252,22 @@ static lv_fs_res_t lv_fs_read_cached(lv_fs_file_t * file_p, char * buf, uint32_t | |||
} | |||
|
|||
if(res == LV_FS_RES_OK) { | |||
file_p->cache->file_position += *br; | |||
file_p->cache->file_position += * br; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
file_p->cache->file_position += *br;
It seems that memory allocation failed. Try pushing again to see if re-triggering CI can solve the problem.
|
I reproduced it. |
src/misc/lv_fs.c
Outdated
@@ -364,7 +364,7 @@ lv_fs_res_t lv_fs_seek(lv_fs_file_t * file_p, size_t pos, lv_fs_whence_t whence) | |||
res = file_p->drv->tell_cb(file_p->drv, file_p->file_d, &tmp_position); | |||
|
|||
if(res == LV_FS_RES_OK) { | |||
file_p->cache->file_position = (uint32_t)tmp_position; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can make the micropython test pass by adding this cast back. But it wasn't here before. The problem is from something else in this PR. Still looking.
I see the problem. @kdschlosser can we update the mpy binding to handle the case when Edit: Or for some APIs like FS we could just keep the u32. Here is what I did to get it to work. Obviously this isn't the fix, though. diff --git a/gen/gen_mpy.py b/gen/gen_mpy.py
index b8ec6e2..ef2e2ed 100644
--- a/gen/gen_mpy.py
+++ b/gen/gen_mpy.py
@@ -693,8 +693,7 @@ register_int_ptr_type('u32ptr',
'unsigned *',
'unsigned int *',
'unsigned long *',
- 'unsigned long int *',
- 'size_t *')
+ 'unsigned long int *')
register_int_ptr_type('i32ptr',
'int32_t *',
@@ -710,7 +709,8 @@ register_int_ptr_type('u64ptr',
'int64_t *',
'signed long long *',
'long long *',
- 'long long int *')
+ 'long long int *',
+ 'size_t *')
register_int_ptr_type('i64ptr',
'uint64_t *',
diff --git a/lib/fs_driver.py b/lib/fs_driver.py
index bac84b3..e5a8b1a 100644
--- a/lib/fs_driver.py
+++ b/lib/fs_driver.py
@@ -38,7 +38,7 @@ def fs_read_cb(drv, fs_file, buf, btr, br):
try:
tmp_data = fs_file.__cast__()['file'].read(btr)
buf.__dereference__(btr)[0:len(tmp_data)] = tmp_data
- br.__dereference__(4)[0:4] = struct.pack("<L", len(tmp_data))
+ br.__dereference__(8)[0:8] = struct.pack("<Q", len(tmp_data))
except OSError as e:
raise RuntimeError("fs_read_callback(%s) exception %s" % (fs_file.__cast__()['path'], e))
@@ -57,7 +57,7 @@ def fs_seek_cb(drv, fs_file, pos, whence):
def fs_tell_cb(drv, fs_file, pos):
try:
tpos = fs_file.__cast__()['file'].tell()
- pos.__dereference__(4)[0:4] = struct.pack("<L", tpos)
+ pos.__dereference__(8)[0:8] = struct.pack("<Q", tpos)
except OSError as e:
raise RuntimeError("fs_tell_callback(%s) exception %s" % (fs_file.__cast__()['path'], e))
@@ -67,7 +67,7 @@ def fs_tell_cb(drv, fs_file, pos):
def fs_write_cb(drv, fs_file, buf, btw, bw):
try:
wr = fs_file.__cast__()['file'].write(buf.__dereference__(btw)[0:btw])
- bw.__dereference__(4)[0:4] = struct.pack("<L", wr)
+ bw.__dereference__(8)[0:8] = struct.pack("<Q", wr)
except OSError as e:
raise RuntimeError("fs_write_callback(%s) exception %s" % (fs_file.__cast__()['path'], e))
|
I am failing to understand why there is a need to access a file that has a size of 18446744073709551615 bytes from LVGL. I don't even see the need to access a file that is 4294967295bytes in size. That's 3.99GB which is what a 32but unsigned integer is able to hold. In the end you are still only dealing with files on a byte by byte basis. so reading and writing to files should only be calculated using 8 bits at a time anywho. The compiler using 64 bit or 32 bit doesn't change anything when using any type that isn't a long type. so a uint32_t is 4 bytes no matter what. memory addresses change size from 4 bytes to 8 bytes but that has zero effect on the size of the underlying data. If the underlying file system driver is using say a uint32_t for telling the driver what the position is to be in the file and that structure is expecting to get a data type that is 8 bits in size I recommend using The datatype that is used when calling the callback should be casting to a |
the issue is in LVGL not in the binding. That is where the problem needs to be corrected. |
look here. Line 290 in lv_fs.c you have this code.
the issue is passing a pointer that has to be filled for the bytes that have been written is bonkers. the treturn type from the callback should be an in32_t. if the number is zero or greater that is the number of bytes written and if it is less than zero that indicates an error. you should not be passing a pointer that needs to be filled for an integer when the mechanics are in place to return the value that would be put into that pointer anyhow. It causes excess complications that shouldn't have to be dealt with. I think that LVGL having the capacity to handle a file that is 2147483647 bytes in size is more than sufficient. That would be the maximum that could be returned from the callback function. and again errors would be anything less than zero. |
lets use the seek function from the posix driver
This code can have the potential to have the compiler complain. This is because of the type that is set for the pos parameter for the function. It is being passed uncasted to a function that is not under the control of LVGL. If that functions type changes based on 32 or 64 bit some complaining will take place. there needs to be a specific cast made to that parameters data type. If the data type is a
in ubuntu this is the declaration for lseek
|
Maybe the best thing is to remove |
Where the problem is stemming from with the file system drivers is the pointer that gets passed to the callback function so it can be filled with the bytes that are written or read. How you do that on the Python side of things ends up being dependent on the memory address size which changes if compiled 32bit vs 64bit. what I am saying is instead of passing that pointer that gets filled why not just return the value from the callback instead. Set any file system errors to negative values and set the return type to int32_t. Granted you will be only to return a written or read size of 2GB but who on earth is going to be loading a 2GB file using LVGL? No one would. |
I could write wrappers around the file system drivers to handle this but that is going to make the binding even more complicated and prone to having errors. It will also need to have code in it that ends up being LVGL specific which is something that has been avoided so far. You don't want to have code in it that looks for specific function names to wrap code to modify the behavior. |
What about detecting if MP is running on a 32 or 64 bit system, and add an if on_32bit:
bw.__dereference__(4)[0:4] = struct.pack("<L", wr)
else:
bw.__dereference__(8)[0:8] = struct.pack("<Q", wr) |
You might be able to do that IF micropython has the bits added to it to be able to>> import sys
is_64bit = sys.maxsize > 2 ** 32 import ctypes
is_64bit = ctypes.sizeof(ctypes.c_void_p) == 8 There is another one that you can do using the struct module as well. I don't remember what that one is off the top of my head tho. Or you will need to specifically add an integer constant in the LVGL module that is able to provide the bitness that things were compiled in. The issue with doing this Is you are only solving an issue in a single place. There are may other places in LVGL where a pointer gets passed as a parameter to be filled and that is where the problem stems from. If it is a primitive type like an int and not an array or a struct being able to access the memory location properly is what ends up causing problems. The simple solution for this is to return the value instead of putting it into a pointer. In the majority of cases where something like this needs to be done it is done when reading or writing some kind of a buffer or file like object this is also the why errors those kinds of operations are negative numbers. Well originally that was the intention was to be be able to return the position or size as a positive number and the errors as a negative. Once memory and file sizes get up into the Gigabytes that's when that was done away with and replaced by using the parameter because you cannot hold an integer that is > 2GB in an For some crazy reason I cannot foresee the need to have the ability to deal with buffers or file objects that have a size > 2GB. That's a very large amount of data. a 2GB image that is RGBA using RAW pixel data is going to have 536,870,912 pixels. That is the maximum that a signed integer can handle . That's a HUGE image. 536 megapixel of RGBA data. |
I see, however passing a pointer to do something with it is quite normal. E.g. So how does it work for |
while I know that is normal for C operations it becomes a problem because LVGL is being bound to for higher level programming languages that do not know how to deal with pointers properly. You have to remember that operations in higher level languages are going to be about 200 times slower so each new line of code that has to be added to handle these kinds of things is going to weigh it down. Simple data types are not an issue but when dealing with the mechanics that have to be added to handle pointers it really causes problems. I do not know why the micropython binding was written to return a pointer as a structure of some kind instead of returning a memoryview object. A memoryview object is how python deals with pointers. With the memoryview object there should not need to be any knowledge of the bit depth for the memory address that is handled internally. As an example say you have a pointer that points to address 0xAC015632 this address is pointing to a single location and that location is able to hold 8 bits of data. That is where the size of what is being stored comes into play to determine what the end point is. When a uint32_t type gets passed into python code as a pointer the memoryview object that gets created to represent it is going to point to the start address and it is going to have a length of 4 bytes. so now we know the start and stop addresses start address = 0xAC015632 what needs to be done is the data being stored has to be split into the 4 bytes of data and that gets stored as you would in a C array. There is no knowledge of the side of the pointer address. What is causing the problem is the mechanics in the binding when dealing with pointers and having the pointer wrapped in a structure which masks the underlying memory location. In Python there is the ability to pass a pointer to a python object as user data using the "id" of that object. The ID is the memory location. If that ID gets passed from C code back into Python it can be converted back into the Python object the ID points to. Currently this ability doesn't exist in the binding. I have no way of passing the instance of a class into C code and then back again. That is why |
I understand the issue (mostly), but in this particular PR we need to solve only the issue of I would like to keep the use of |
We need some feedback on this pull request. Now we mark this as "stale" because there was no activity here for 14 days. Remove the "stale" label or comment else this will be closed in 7 days. |
Not stale |
We need some feedback on this pull request. Now we mark this as "stale" because there was no activity here for 14 days. Remove the "stale" label or comment else this will be closed in 7 days. |
Change the type of the position and size parameters from uint32_t to size_t to ignore compiler warnings on 64bit systems