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

[BUG] Fix Clang/GCC compiler warnings. #558

Closed
phelter opened this issue Sep 25, 2022 · 8 comments
Closed

[BUG] Fix Clang/GCC compiler warnings. #558

phelter opened this issue Sep 25, 2022 · 8 comments
Labels
bug Something isn't working

Comments

@phelter
Copy link
Contributor

phelter commented Sep 25, 2022

Describe the bug
Cleanup the Clang and GNU compiler warnings when -Wall, -Weverything, and -Wpedantic are used.

Start using clang for checking doxygen and other C based linting violations - can use in gitub-actions.

Target

  • Development board: Linux
  • Instruction Set Architecture: Linux
  • IDE and version: Visual Code 1.71.1
  • Toolchain and version: Clang-13 and GCC 7.5.0

Host

  • Host OS: Linux
  • Version: Ubuntu 18.04

To Reproduce

Expected behavior
Code cleaned up and a CI call to Clang and GCC to ensure code quality is adhered to.

Screenshots
N/A

Wireshark logs
N/A

Additional context
The primary reason for doing this is to use Clang and GCC to provide better and more descriptive code.

@phelter phelter added the bug Something isn't working label Sep 25, 2022
@kstribrnAmzn
Copy link
Member

Going to dump what I was able to my setup information and what warnings I was able to find here.

Setup:

  • Mac Pro, i7, Ventura 13.2
  • Clang version (clang-1400.0.29.202), x86_64-apple-darwin22.3.0

I was able to add the warning flags you requested (-Wall -Weverything -Wpedantic) to the default config used in the CI. I built this and saw many warnings but was able to boil them down to a couple things...

  1. I was pretty lazy in adding the flags. The warning flags were also applying to the FreeRTOS/Kernel files when the +TCP library was being built. I overcame this by refining the CMake file I had created to ONLY care about the +TCP source files.
  2. The #pragma packing push/pop directives were causing the bulk of the warnings I would see. For me, the errors would look something like...
/workspace/forks/FreeRTOS-Plus-TCP/source/include/FreeRTOS_DNS_Globals.h:134:14: warning: the current #pragma pack alignment value is modified in the included file [-Wpragma-pack]
    #include "pack_struct_start.h"
             ^
/workspace/forks/FreeRTOS-Plus-TCP/source/portable/Compiler/MSVC/pack_struct_start.h:35:9: note: previous '#pragma pack' directive that modifies alignment is here
#pragma pack( push, 1 )
        ^
In file included from /Users/kstribrn/workspace/forks/FreeRTOS-Plus-TCP/test/build-combination/Common/main.c:38:
In file included from /Users/kstribrn/workspace/forks/FreeRTOS-Plus-TCP/source/include/FreeRTOS_IP.h:448:
In file included from /Users/kstribrn/workspace/forks/FreeRTOS-Plus-TCP/source/include/FreeRTOS_IP_Utils.h:62:
In file included from /Users/kstribrn/workspace/forks/FreeRTOS-Plus-TCP/source/include/FreeRTOS_DNS.h:38:

I think the only way to overcome this is to suppress the -Wpramga-pack warning. I did this by adding the following lines to the pack_struct_start.h file

#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wpragma-pack"

I suspect my strategy here is a little too loose as when I now build, I'm not seeing any warnings. I'll chat with others tomorrow to get some ideas for how this can be refined.

@kstribrnAmzn
Copy link
Member

@phelter Can you elaborate on the Clang/GCC compiler warnings you were seeing? Were they all related to the packing directives? Or were there others?

@phelter
Copy link
Contributor Author

phelter commented Mar 6, 2023

@kstribrnAmzn -

They were:

@kstribrnAmzn
Copy link
Member

Thanks @phelter! I'll take a look at what you've done, pull what I can in, and come up with a PR. I'll probably break this into several PRs so that the fixes flow faster.

@kstribrnAmzn kstribrnAmzn mentioned this issue Mar 15, 2023
2 tasks
@kstribrnAmzn
Copy link
Member

GCC warnings fixes - #798

@kstribrnAmzn
Copy link
Member

The Clang fixes are a little trickier here as there are more false positives that aren't as easily resolved.

Example 1:

wait_for_event.h:30:9: error: macro name is a reserved identifier [-Werror,-Wreserved-macro-identifier]
#define _WAIT_FOR_EVENT_H_

From this file in the Kernel. This is because Clang (and other compilers) reserve macros starting with an underscore and capitalized. I have not found a Clang diagnostic flag for disabling the reserved macro warning.

@phelter
Copy link
Contributor Author

phelter commented Mar 16, 2023

@kstribrnAmzn those false positives are easily resolved by search and replacing each without the prefix of the _ (preferred change).

eg:

#ifndef WAIT_FOR_EVENT_H
#define WAIT_FOR_EVENT_H

Clang diagnostic flag for disabling this is like all other clang diagnostics by prefixing the diagnostic with -Wno-.

So for this scenario it would be -Wno-reserved-macro-identifier

@kstribrnAmzn
Copy link
Member

Thanks for the advice @phelter. I actually found some things couldn't be disabled with the -Wno- prefix. IIRC the reserved macro identifier was them which surprised me.

Anyway I have posted a Clang warnings fix PR and my GCC warnings fix has been approved. When you look at these you're probably going to wonder why I left out the pedantic warnings. This was a conscious decision by the team. Pedantic was removed for a couple reasons: other solutions decreased readability and would have caused customer pain either in consuming our library or debugging application problems with it.

If you're happy, then there is no further action on your part. When the Clang PR is merged, I'll close out this issue.

kstribrnAmzn added a commit that referenced this issue Mar 24, 2023
Corrects several warnings from Clang flags
for Clang 13.

Inspired by @phelter's bug report
#558
moninom1 pushed a commit to moninom1/FreeRTOS-Plus-TCP that referenced this issue Apr 17, 2023
Corrects several warnings from Clang flags
for Clang 13.

Inspired by @phelter's bug report
FreeRTOS#558
moninom1 pushed a commit to moninom1/FreeRTOS-Plus-TCP that referenced this issue Apr 17, 2023
Corrects several warnings from Clang flags
for Clang 13.

Inspired by @phelter's bug report
FreeRTOS#558
moninom1 pushed a commit to moninom1/FreeRTOS-Plus-TCP that referenced this issue Apr 17, 2023
Corrects several warnings from Clang flags
for Clang 13.

Inspired by @phelter's bug report
FreeRTOS#558
moninom1 pushed a commit to moninom1/FreeRTOS-Plus-TCP that referenced this issue Apr 18, 2023
Corrects several warnings from Clang flags
for Clang 13.

Inspired by @phelter's bug report
FreeRTOS#558
moninom1 pushed a commit to moninom1/FreeRTOS-Plus-TCP that referenced this issue Apr 18, 2023
Corrects several warnings from Clang flags
for Clang 13.

Inspired by @phelter's bug report
FreeRTOS#558
moninom1 pushed a commit to moninom1/FreeRTOS-Plus-TCP that referenced this issue Apr 18, 2023
Corrects several warnings from Clang flags
for Clang 13.

Inspired by @phelter's bug report
FreeRTOS#558
moninom1 added a commit that referenced this issue Jul 5, 2023
* Update mainline to reflect changes after the release. (#563)

* Update README.md

* Update History.txt

* Update version number macros

* Update manifest.yml

* IPv4/single SAME70 emac race condition (#567)

* Implemented Maty's solution

* Added a new statistic 'tx_write_fail'

* Uncrustify: triggered by comment.

Co-authored-by: Hein Tibosch <hein@htibosch.net>
Co-authored-by: GitHub Action <action@github.com>

* IPv4/Single: Add a SocketID to a socket (#546)

* IPv4/Single: Add a SocketID to a socket

* Change in comment

* Applied uncrustify to format the source code

* Added a few entries to lexicon.txt

* Removed the 'ipconfigUSE_SetSocketID' option

* Change to lexicon.txt

* Add unit tests for the newly added API

Co-authored-by: Hein Tibosch <hein@htibosch.net>
Co-authored-by: alfred gedeon <28123637+alfred2g@users.noreply.github.com>
Co-authored-by: Aniruddha Kanhere <60444055+AniruddhaKanhere@users.noreply.github.com>

* IPv4/single: SAME70 EMAC buffer sizes (#568)

* Implemented Maty's solution

* Added a new statistic 'tx_write_fail'

* Uncrustify: triggered by comment.

* Increase NETWORK_BUFFER_SIZE in order to include the 'ipBUFFER_PADDING' bytes

* ICMP checksum calculated manually

* Uncrustify: triggered by comment.

* Update gmac_SAM.c

Co-authored-by: Hein Tibosch <hein@htibosch.net>
Co-authored-by: GitHub Action <action@github.com>
Co-authored-by: Aniruddha Kanhere <60444055+AniruddhaKanhere@users.noreply.github.com>

* Eliminate some warnings (#578)

* Eliminate some warnings related to print statements

Authored-by:  Pete Bone  <pete-pjb@users.noreply.github.com >

* Add MISRA justification for use of dynamic memory (#581)

* Update deprecated macros in network driver files (#579)

* Update deprecated macros in network driver files

* Fix typo in RX driver.

* Replace #warning with #error on test for deprecated macro.

* Fix doxygen check

Co-authored-by: PeterB <PeterB@PETE-LT.otg.local>
Co-authored-by: Aniruddha Kanhere <60444055+AniruddhaKanhere@users.noreply.github.com>

* Fix Network-interface of the Xilinx UltraScale port (#588)

The underlying issue was when the port would be used with Jumbo frames.
During receives of Jumbo packets the data length was always set
incorrectly, which then would cause buffer allocation issues and
subsequently corrupted data would be sent to the IP-task.

After some inspection in the Xilinx UltraScale port, I found out
that when the data length would be set, the wrong mask from the
Xilinx Ethernet MAC driver would be used. By using the right mask
(XEMACPS_RXBUF_LEN_JUMBO_MASK) when Jumbo Frame support is enabled
the issue was resolved

* Fix Windows thread calling vTaskSuspendAll / xTaskResumeAll. (#592)

Co-authored-by: Jason Carroll <czjaso@amazon.com>

* Updated comments for FreeRTOS_select return value (#596)

* Updated comments for FreeRTOS_select return value

* Updated the function brief for FreeRTOS_select

* Uncrustify: triggered by comment.

* Updating FreeRTOS_select function @brief

* Updated function brief for FreeRTOS_SignalSocket

* Uncrustify: triggered by comment.

* Update source/FreeRTOS_Sockets.c

Co-authored-by: Ubuntu <ubuntu@ip-172-31-22-210.ec2.internal>
Co-authored-by: GitHub Action <action@github.com>
Co-authored-by: Aniruddha Kanhere <60444055+AniruddhaKanhere@users.noreply.github.com>

* Fixed readme script to build and run unit tests (#644)

* Minor warning fixes (#589)

* Eliminate compiler unused parameter warning

* Eliminate compiler unused variable warnings

* Eliminate compiler unused function warning

The function pcGetPHIName(...) would be called only in the
FreeRTOS_printf message, however FreeRTOS_printf maybe be defined
to nothing e.g. release builds, which then the warning would come up

* Rework callback setups in the EMAC-driver of the Xilinx UltraScale port

The calls to the function XEmacPs_SetHandler would trigger the
pedantic warning:

"ISO C forbids conversion of object pointer to function pointer type"

The reason for this, is that the second parameter of the function
XEmacPS_SetHandler is declared as pointer to a void type, but the
function "expects" a function pointer, which in setup_isr rightly
happens.

However IMHO, this is just bad code from the side of Xilinx, as not
on all architectures the size of a data pointer is identical to the
size of a function pointer, which also is correctly recognised by
the compiler.

Instead of using the "bad" function XEmacPs_SetHandler, we can set the
handlers manually to the EmacPS-instance.

* Uncrustify: triggered by comment.

* Update source/portable/NetworkInterface/xilinx_ultrascale/x_emacpsif_hw.c

Co-authored-by: Paul Bartell <paul.bartell@gmail.com>

* Apply suggestions from code review

Co-authored-by: Paul Bartell <paul.bartell@gmail.com>

* Address comments from reviews

Co-authored-by: GitHub Action <action@github.com>
Co-authored-by: Paul Bartell <paul.bartell@gmail.com>

* Use CBMC XML output to enable VSCode debugger (#673)

Prior to this commit, CBMC would emit logging information in plain text
format, which does not contain information required for the CBMC VSCode
debugger. This commit makes CBMC use XML instead of plain text.

Co-authored-by: Mark Tuttle <tuttle@acm.org>

* Remove need of token

* Use vTaskDelay for sleep in the network-interface of xilinx_ultrascale (#698)

The issue here is that, the FreeRTOS IP-task would block all other
tasks during PHY-link speed negotiations, as it was using busy
waiting. However this is not really ideal. A much more suitable
function for such a task would be `vTaskDelay`.

* Make sure that a TCP socket is closed only once (#707)

* Make sure that a TCP socket is closed only once

* Fix failing test cases for FreeRTOS_TCP_IP unit test modules post PR#705 changes

* Uncrustify: triggered by comment.

* Fix failing test cases for FreeRTOS_TCP_IP unit test modules post PR - 705 changes

---------

Co-authored-by: Hein Tibosch <hein_tibosch@yahoo.es>
Co-authored-by: GitHub Action <action@github.com>

* Remove Dup function HAL_ETH_SetMDIOClockRange. (#711)

* Update PR template to include checkbox for ut change (#734)

* Main/TCP4 : ACK number in TCP RESET reply to SYN packet (#724)

* Main/TCP4 : ACK number in TCP RESET reply to SYN packet

* Typo fix

* Add unit-test for coverage; Fix ntohl to htonl

* Fix unit-test

---------

Co-authored-by: Nikhil Kamath <110539926+amazonKamath@users.noreply.github.com>
Co-authored-by: Aniruddha Kanhere <60444055+AniruddhaKanhere@users.noreply.github.com>

* #556 Initial Cmake Module definition. (#557)

* #556 Initial Cmake Module definition.

* Fixing CI builds, rely on pcap. (#556)

* Updating tested configurations and minor clean-up of missing network interfaces (#555)

* Further clean-up based on testing with build environment. (#555)

* Using single definition for libraries everywhere. (#555)

* Fixing A_CUSTOM_NETWORK_IF compile option.

* Identifying and fixing compile issues.

* Adding in additional warnings for GNU to ignore for now.

* Fixing formatting issues with uncrustify.

* More warnings for GNU used by CI/CD pipeline.

* Assuming custom for build tests and using latest freertos-kernel code.  Updated readme for how to consume at project level.

* Fixing up issues identified in the PR. Making the build_test EXCLUDE_FROM_ALL so only compiled if requested.

* Changing to support C89 instead of C99. Renaming tcp_tools to tcp_utilities to mimic the directory.

* Using C90 ISO.  Fixing compiler warnings.

* Fixing non C90 compliant declaration after statement

* Separating out CMakeLists so each port is independent.

* Updating warning list in code.

* Fixed formatting with uncrustify.

* Fix failing tests

* Fix failing unit-test

* Fix a typo.

---------

Co-authored-by: Aniruddha Kanhere <60444055+AniruddhaKanhere@users.noreply.github.com>

* CMake: Fix GIT_REPOSITORY and GIT_TAG (#742)

* Allow use of loopback addresses in IP stack (127.0.0.0/8) (#754)

Authored-by: Adam St. Amand <astamand@amazon.com>

* Add release candidate automation (#761)

This is a minimal subset of release automation which only creates a tag
and verifies it.

Signed-off-by: Gaurav Aggarwal <aggarg@amazon.com>

* Add CBMC-running GitHub Action;

This commit adds a GitHub Action that runs the CBMC proofs in this
repository upon pushes and pull requests

* Copy CBMC output directory to CI location

This commit ensures that the output directory for CBMC proofs is in the
correct location expected by the FreeRTOS CI-CD repository.

* rx: Read mac address using FreeRTOS_GetMACAddress() rather than using the defines (#765)

* Read mac address using FreeRTOS_GetMACAddress() rather than using the defines
---------
Co-authored-by: GitHub Action <action@github.com>

* cmake: Remove add_subdirectory( cbmc ) call

CBMC proofs cannot currently be run using CMake.

fixes #753

* FreeRTOS_IP.h: Fix build error introduced by 55658e1 in FreeRTOS-Kernel

* Add Nxp1060 network interface (#774)

* Update PR template to include checkbox for ut change

* Create NetworkInterface.c

* Uncrustify: triggered by comment.

* Address PR comments

* Uncrustify: triggered by comment.

* Update NetworkInterface.c

* Uncrustify: triggered by comment.

* Update copyright year

* Refactor the init function. Add 'brief'. Cleanup.

* Uncrustify: triggered by comment.

* Update global link status only when the network is quiet

* Uncrustify: triggered by comment.

* Update copyright yeat

* Update the driver to deal with network cable disconnects

* Uncrustify: triggered by comment.

* Update NetworkInterface.c

* Clean up and address PR comments

* More cleanup and address PR comments

* Uncrustify: triggered by comment.

* Empty-Commit

* Address issue comments

* Uncrustify: triggered by comment.

* Empty-Commit to trigger workflow

* Remove Full-Duplex restriction

* Uncrustify: triggered by comment.

* Empty-Commit to trigger workflow

---------

Co-authored-by: GitHub Action <action@github.com>

* Correct GCC warnings (#798)

* Correct GCC warnings

Corrects warnings with current GCC flags
for GCC 7.5.0. The only suppressed warning pertains
to function to object pointer conversion which is
required and common for socket callbacks.

* PR feedback

---------

Co-authored-by: Ubuntu <ubuntu@ip-10-0-137-67.ec2.internal>
Co-authored-by: Nikhil Kamath <110539926+amazonKamath@users.noreply.github.com>

* Cleanup of NXP1060 network driver (#801)

* Update PR template to include checkbox for ut change

* Empty-Commit to trigger workflow

* Fix issues pointed out in PR comments

* Uncrustify: triggered by comment.

* Empty-Commit to trigger workflow

---------

Co-authored-by: GitHub Action <action@github.com>

* Fix Clang warnings (#809)

Corrects several warnings from Clang flags
for Clang 13.

Inspired by @phelter's bug report
#558

* uncrustify yml fix (#815)

* Add NetworkDown notification to NetworkInterface.c [PR: #671] (#812)

* Add NetworkDown notification to EMAC task

* Add NetworkDown notification to NetworkInterface.c

* Uncrustify: triggered by comment.

* Introduce ipconfigSUPPORT_NETWORK_DOWN_EVENT compile flag

* Fix formatting

* Uncrustify: triggered by comment.

---------

Co-authored-by: Filip Oleszek <filip.oleszek@o2.pl>
Co-authored-by: zipperowiec <35423392+zipperowiec@users.noreply.github.com>
Co-authored-by: GitHub Action <action@github.com>

* Uncrustify bot command fix (#816)

* fix uncrustify run command

* test uncrustify

* Revert "test uncrustify"

This reverts commit f660ab4.

* Fix uncrustify bot command - disable install prompt (#819)

* fix uncrustify run command

* test uncrustify

* Revert "test uncrustify"

This reverts commit f660ab4.

* removing apt-get prompt while installing git

* Removing deprecated set-output command from uncrustify bot run yml (#820)

* fix uncrustify run command

* test uncrustify

* Revert "test uncrustify"

This reverts commit f660ab4.

* removing apt-get prompt while installing git

* removing the deprecated set-output command from uncrustify bot run yml, use latest git

* IPv4/Single: Let send() stop blocking after a connection reset (#561)

* IPv4/Single: Let send() stop after a protocol error

* Remove token need

* Repaired unit-testing

* Added the cunftion test_FreeRTOS_send_DisconnectionOccursDuringWait()

* Added a comment for unit-test function test_FreeRTOS_send_DisconnectionOccursDuringWait()

* Added an item to lexicon.txt

* Restored original tcp_utilities

* Restored original tcp_utilities, once more

---------

Co-authored-by: Hein Tibosch <hein@htibosch.net>
Co-authored-by: Aniruddha Kanhere <60444055+AniruddhaKanhere@users.noreply.github.com>
Co-authored-by: Nikhil Kamath <110539926+amazonKamath@users.noreply.github.com>

* Add logs to print random number generation failure (#908)

Add logs to print random number generation failure for better debugging of issue.

* Update usage of uint64_t according to C90 standard (#907)

Co-authored-by: kar-rahul-aws <118818625+kar-rahul-aws@users.noreply.github.com>

* Fix pragma pack in CCS compiler to push/pop (#906)

`#pragma pack(1)` would make it so that all structs inserted after pack_struct_start.h
was included for the TI arm compiler would be packed, leading to potential unaligned memory access error.
Refer: https://www.ti.com/lit/ug/spnu151w/spnu151w.pdf SECTION 5.11.23

* Modified libslirp backend file to cover different libslirp library versions (#929)

Authored-by: Xiaodong Li <xiaodonn@amazon.com>

* Update according to devIntegration

* Update links to point to main directory

---------

Signed-off-by: Gaurav Aggarwal <aggarg@amazon.com>
Co-authored-by: Aniruddha Kanhere <60444055+AniruddhaKanhere@users.noreply.github.com>
Co-authored-by: Hein Tibosch <hein_tibosch@yahoo.es>
Co-authored-by: Hein Tibosch <hein@htibosch.net>
Co-authored-by: GitHub Action <action@github.com>
Co-authored-by: alfred gedeon <28123637+alfred2g@users.noreply.github.com>
Co-authored-by: Pete Bone <pete-pjb@users.noreply.github.com>
Co-authored-by: PeterB <PeterB@PETE-LT.otg.local>
Co-authored-by: ChristosZosi <76208460+ChristosZosi@users.noreply.github.com>
Co-authored-by: jasonpcarroll <23126711+jasonpcarroll@users.noreply.github.com>
Co-authored-by: Jason Carroll <czjaso@amazon.com>
Co-authored-by: Tony Josi <117763118+tony-josi-aws@users.noreply.github.com>
Co-authored-by: Ubuntu <ubuntu@ip-172-31-22-210.ec2.internal>
Co-authored-by: Paul Bartell <paul.bartell@gmail.com>
Co-authored-by: Kareem Khazem <karkhaz@amazon.com>
Co-authored-by: Mark Tuttle <tuttle@acm.org>
Co-authored-by: Tony Josi <tonyjosi@amazon.com>
Co-authored-by: ActoryOu <jay2002824@gmail.com>
Co-authored-by: Nikhil Kamath <110539926+amazonKamath@users.noreply.github.com>
Co-authored-by: phelter <paulheltera@gmail.com>
Co-authored-by: Adam St. Amand <adam.stamand@gmail.com>
Co-authored-by: Gaurav-Aggarwal-AWS <33462878+aggarg@users.noreply.github.com>
Co-authored-by: sayyadumar <sayyadumar@users.noreply.github.com>
Co-authored-by: Paul Bartell <pbartell@amazon.com>
Co-authored-by: Kody Stribrny <89810515+kstribrnAmzn@users.noreply.github.com>
Co-authored-by: Ubuntu <ubuntu@ip-10-0-137-67.ec2.internal>
Co-authored-by: Filip Oleszek <filip.oleszek@o2.pl>
Co-authored-by: zipperowiec <35423392+zipperowiec@users.noreply.github.com>
Co-authored-by: kar-rahul-aws <118818625+kar-rahul-aws@users.noreply.github.com>
Co-authored-by: Rahul Arasikere <arasikere.rahul@gmail.com>
Co-authored-by: Xiaodong Li <85011700+ChaiTowKway@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants