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

(small fix) Use memcpy instead of copying bytes manually #549

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

Conversation

cadouthat
Copy link

This is cleaner and more efficient, but more importantly it fixes this annoying GCC warning:

hfp_hf.c:1561:26: warning: writing 1 byte into a region of size 0 [-Wstringop-overflow=]
 1561 |         hfp_hf_codecs[i] = codecs[i];
      |         ~~~~~~~~~~~~~~~~~^~~~~~~~~~~

This is cleaner and more efficient, but more importantly it fixes this annoying GCC warning:
```
hfp_hf.c:1561:26: warning: writing 1 byte into a region of size 0 [-Wstringop-overflow=]
 1561 |         hfp_hf_codecs[i] = codecs[i];
      |         ~~~~~~~~~~~~~~~~~^~~~~~~~~~~
```
@mringwal
Copy link
Member

Thanks for the fix/workaround. I'd like to understand this better first.

Which gcc gives you this warning? Also, looking hard at the code, I don't see the problem. There's an global array of HFP_MAX_NUM_CODECS. Do you have btstack_assert enabled?

Could you try
for (i=0; i<codecs_nr && i < HFP_MAX_NUM_CODECS; i++){ .. }?

@cadouthat
Copy link
Author

I'm using GCC 10.3.1 arm-none-eabi (Pico SDK). Agreed that there doesn't appear to be anything wrong with the code, my guess is that GCC somehow mis-interprets hfp_hf_codecs as a null-terminated string, and believes that we may not be leaving room for the terminator.

btstack_assert does appear to be enabled, and still no luck with && i < HFP_MAX_NUM_CODECS

@hegdi
Copy link
Collaborator

hegdi commented Nov 28, 2023

Well I'm afraid this is a bug but not on our side, checkout
https://gcc.gnu.org/g:b48d4e6818674898f90d9358378c127511ef0f9f
for explanation/resolution or
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101854
for a little back story, fixed in GCC 12 and I can confirm GCC 13.2.0 is not affected.
Well I would suggest you update your arm-none-eabi toolchain the current Debian is at GCC 12.2.1 for arm, from 2022, so GCC 10.3.1 is pretty old

@hegdi hegdi requested a review from mringwal November 28, 2023 20:03
@hegdi hegdi assigned mringwal and unassigned hegdi Nov 28, 2023
@cadouthat
Copy link
Author

Thanks for the info! Unfortunately I think my version is the most recent via Homebrew, so I'd have to install manually

But regardless of the GCC warning, I still think the proposed change is good in itself

@mringwal
Copy link
Member

mringwal commented Nov 30, 2023

@cadouthat Interestingly, my manually installed arm-none-eabi toolchain is 10.3-2021.10, while brew provides an up-to-date 13.2.0. I guess I'll switch to the one provided by brew - which is even compile for ARM64 instead of emulating x64 on my M2.

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

Successfully merging this pull request may close these issues.

3 participants