Skip to content

Fix heap_4 Cast Conversion Issue - #778

Closed
Skptak wants to merge 2 commits into
FreeRTOS:mainfrom
Skptak:Fix_Heap_4_Warning
Closed

Fix heap_4 Cast Conversion Issue#778
Skptak wants to merge 2 commits into
FreeRTOS:mainfrom
Skptak:Fix_Heap_4_Warning

Conversation

@Skptak

@Skptak Skptak commented Sep 4, 2023

Copy link
Copy Markdown
Contributor

Description

Pull Request #771 introduced the following warning in heap_4.c:

/home/runner/work/FreeRTOS-Plus-TCP/FreeRTOS-Plus-TCP/build/_deps/freertos_kernel-src/portable/MemMang/heap_4.c: In function ‘prvHeapInit’:
  /home/runner/work/FreeRTOS-Plus-TCP/FreeRTOS-Plus-TCP/build/_deps/freertos_kernel-src/portable/MemMang/heap_4.c:473:55: error: conversion to ‘long unsigned int’ from ‘intptr_t’ {aka ‘long int’} may change the sign of the result [-Werror=sign-conversion]
    473 |     uxAddress = ( portPOINTER_SIZE_TYPE ) ( uxAddress + xTotalHeapSize );
        |                                                       ^
  cc1: all warnings being treated as errors

The reason of the warning is that the types two variables being added, namely uxAddress and xTotalHeapSize, differ in signedness. This PR adds a cast to avoid this warning.

Test Steps

The warning is no longer there after the fix:

# cmake -S . -B build
-DFREERTOS_PLUS_TCP_TEST_CONFIGURATION=ENABLE_ALL; cmake --build build
--target freertos_plus_tcp_build_test
-- Detected UNIX/Posix system setting FREERTOS_PLUS_TCP_NETWORK_IF =
POSIX
-- Using FreeRTOS-Plus-TCP Test Configuration : ENABLE_ALL
CMake Warning at CMakeLists.txt:168 (message):
  FreeRTOS-Kernel configuration settings are configured by FreeRTOS-
Plus-TCP


-- Configuring done
-- Generating done
-- Build files have been written to:
/home/ANT.AMAZON.COM/skptak/repos/fork/FreeRTOS-Plus-TCP_Soren/build
[  4%] Built target freertos_kernel_port
[  5%] Building C object _deps/freertos_kernel-
build/CMakeFiles/freertos_kernel.dir/portable/MemMang/heap_4.c.o
[  7%] Linking C static library libfreertos_kernel.a
.
.
.
[ 98%] Building C object test/build-
combination/CMakeFiles/freertos_plus_tcp_build_test.dir/Common/main.c.o
[100%] Linking C executable freertos_plus_tcp_build_test
[100%] Built target freertos_plus_tcp_build_test

Checklist:

  • I have tested my changes. No regression in existing tests.
  • I have modified and/or added unit-tests to cover the code changes in this Pull Request.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@Skptak
Skptak requested a review from a team as a code owner September 4, 2023 20:17
@codecov

codecov Bot commented Sep 4, 2023

Copy link
Copy Markdown

Codecov Report

Patch and project coverage have no change.

Comparison is base (231278e) 94.35% compared to head (f4a9a41) 94.35%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #778   +/-   ##
=======================================
  Coverage   94.35%   94.35%           
=======================================
  Files           6        6           
  Lines        2446     2446           
  Branches      598      598           
=======================================
  Hits         2308     2308           
  Misses         85       85           
  Partials       53       53           
Flag Coverage Δ
unittests 94.35% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@amazonKamath

Copy link
Copy Markdown
Contributor

is the Github actions file changes related/necessary for the warning fix?

@aggarg

aggarg commented Sep 5, 2023

Copy link
Copy Markdown
Member

If the Workflow files changes are unrelated, please do them in a separate PR.

@Skptak
Skptak force-pushed the Fix_Heap_4_Warning branch from e70abda to 3778278 Compare September 5, 2023 16:53
@Skptak

Skptak commented Sep 5, 2023

Copy link
Copy Markdown
Contributor Author

Was just annoyed with the dozen or so warnings we get on every workflow run since we use checkout@v2, which is deprecated and soon to be dis-continued. Didn't think it would be an issue to add it onto this since it's a small change to the workflow file. Removed it per requests.

@aggarg

aggarg commented Sep 5, 2023

Copy link
Copy Markdown
Member

Thank you @Skptak! As just reported by a user on PR 771, there is another issue in the PR which causes the malloc to fail. I have created a PR to address that issue and in that PR I addresses this issue as well - #781.

I think we can close this PR in favor of 781.

@Skptak

Skptak commented Sep 5, 2023

Copy link
Copy Markdown
Contributor Author

Sounds good! Taking a look at your #781 PR now

@Skptak Skptak closed this Sep 5, 2023
@Skptak Skptak mentioned this pull request Sep 5, 2023
2 tasks
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