Skip to content
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

Changes to fix data types mismatches for armv7 build #395

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rvroge
Copy link

@rvroge rvroge commented Sep 4, 2024

We are running the server and core in a armv7 device.
Build for platform linux/arm/v7 failed because the code was using 64 bit size variables and we have 32 bit.

@@ -255,7 +255,7 @@ CacheEntry::SerializeResponseOutput(
// Pack everything into provided buffer
uint64_t position = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

out of curiosity, why this line doesn't cause issues

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@oandreeva-nv you have a good point. I'll elaborate a bit more on this one. The real issue is with the following lines. SerializeResponseOutput is expecting a size_t pointer:
CacheEntry::SerializeResponseOutput( const InferenceResponse::Output& output, Byte* buffer, size_t* output_size)

For our platform there is a mismatch between these two types:
error: cannot convert ‘uint64_t*’ {aka ‘long long unsigned int*’} to ‘size_t*’ {aka ‘unsigned int*’}

@@ -255,7 +255,7 @@ CacheEntry::SerializeResponseOutput(
// Pack everything into provided buffer
uint64_t position = 0;
// Total serialized output size
memcpy(buffer + position, &total_byte_size, sizeof(uint64_t));
memcpy(buffer + position, &total_byte_size, sizeof(size_t));
position += sizeof(uint64_t);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants