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.

Sign up for GitHub

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

Jump to bottom

Fix possible conversion errors by using __builtin_ffsll (IDFGH-9541) #10895

Merged
merged 1 commit into from
May 8, 2023
Merged

Fix possible conversion errors by using __builtin_ffsll (IDFGH-9541) #10895

merged 1 commit into from
May 8, 2023

Conversation

term-est
Copy link
Contributor

@term-est term-est commented Mar 3, 2023

Using C++20 with -Wsign-conversion causes my build to break as the function xt_utils_set_watchpoint takes size argument as of type size_t and passes it to __builtin_ffs(unsigned int), which takes an unsigned int parameter.

Using the function __builtin_ffsll instead should fix this.

Thank you for taking your time to review this pull request and have a great day!

@term-est
Fix possible conversion errors by using __builtin_ffsll instead of __…
14724d1 
…builtin_ffs

Signed-off-by: term_est <62337595+term-est@users.noreply.github.com>
@CLAassistant
Copy link

CLAassistant commented Mar 3, 2023 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@espressif-bot espressif-bot added the Status: Opened Issue is new label Mar 3, 2023
@github-actions github-actions bot changed the title Fix possible conversion errors by using __builtin_ffsll Fix possible conversion errors by using __builtin_ffsll (IDFGH-9541) Mar 3, 2023
@igrr
Copy link
Member

igrr commented Mar 3, 2023

@term-est Could you please describe how to reproduce the build issue? This is so that we could add a regression test along with your PR.

@term-est
Copy link
Contributor Author

term-est commented Mar 3, 2023

@term-est Could you please describe how to reproduce the build issue? This is so that we could add a regression test along with your PR.

I am sorry for the misunderstanding. __builtin_ffs breaks my build, and it's not an issue on ESP's part, nor the ESP's build system.

We are using -Werror and -Wsign-conversion as part of our build process in our company for our C++ builds. and adding uart.h adds xt_utils.h, which triggers the build error for us.

I still think possible sign conversion errors might be avoided in the ESP-IDF repository, so I deiced to open up the pull request.
For clarification, this is not an issue on the ESP-IDF build system part, I am sorry for the misunderstanding.

@espressif-bot espressif-bot added Status: Selected for Development Issue is selected for development and removed Status: Opened Issue is new labels Mar 8, 2023
@0xjakob
Copy link
Collaborator

0xjakob commented Apr 25, 2023

sha=14724d1cb967f252936bff160a69148489ad43bb

@0xjakob 0xjakob added the PR-Sync-Merge Pull request sync as merge commit label Apr 25, 2023
@espressif-bot espressif-bot added Status: Reviewing Issue is being reviewed and removed Status: Selected for Development Issue is selected for development labels Apr 25, 2023
@espressif-bot espressif-bot added Status: Done Issue is done internally Resolution: NA Issue resolution is unavailable and removed Status: Reviewing Issue is being reviewed labels May 4, 2023
@espressif-bot espressif-bot merged commit 14724d1 into espressif:master May 8, 2023
espressif-bot pushed a commit that referenced this pull request May 13, 2023
@0xjakob
cxx/esp_hw_support: added build test, changed parameter types
48ab527 
Changed rv_utils_intr_edge_ack and esp_cpu_intr_edge_ack to
take uint32_t instead of int to avoid build errors.

The test is to test in particular that __builtin_ffsll, used in
xt_utils.h, which is included via esp_cpu.h, compiles fine
in C++20 with -Wsign-conversion enabled.

Closes #10895
espressif-bot pushed a commit that referenced this pull request May 20, 2023
@0xjakob
cxx/esp_hw_support: added build test, changed parameter types
5946c6a 
Changed rv_utils_intr_edge_ack and esp_cpu_intr_edge_ack to
take uint32_t instead of int to avoid build errors.

The test is to test in particular that __builtin_ffsll, used in
xt_utils.h, which is included via esp_cpu.h, compiles fine
in C++20 with -Wsign-conversion enabled.

Closes #10895
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@0xjakob 0xjakob

Labels
PR-Sync-Merge Pull request sync as merge commit Resolution: NA Issue resolution is unavailable Status: Done Issue is done internally
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@term-est @CLAassistant @igrr @0xjakob @espressif-bot