From 9662a76ebd1c8e5cbf0014ef669d76b3502ac709 Mon Sep 17 00:00:00 2001 From: Kari Argillander Date: Thu, 22 Aug 2024 15:50:20 +0300 Subject: [PATCH] Make clean build with MSVC /Wall (#740) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * ci: Fix compile warning as errors was not correct We want to enable warning as errors both Windows and Linux. This is easiest to do with cmake option as -Werror does not work with MSVC. Also it is self explaining what it does so no comment needed. * dlmstp_linux: Fix -Wdeclaration-after-statement compiler warnings Make dlmstp_linux C89/C90 combatible ``` /bacnet-stack/ports/linux/dlmstp_linux.c: In function ‘Timer_Silence’: /bacnet-stack/ports/linux/dlmstp_linux.c:56:5: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement] 56 | int32_t res; | ^~~~~~~ /bacnet-stack/ports/linux/dlmstp_linux.c: In function ‘dlmstp_init’: /bacnet-stack/ports/linux/dlmstp_linux.c:795:5: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement] 795 | struct termios newtio; | ^~~~~~ ``` * Fix warnings produces by MSVC Now that we have enabled /Wall for MSVC we get some warnings with it which can be easily fixed. We get following warnings: ``` src\bacnet\bacstr.c(223,39): warning C4127: conditional expression is constant apps\router-mstp\main.c(1123,1): warning C4702: unreachable code apps\epics\main.c(885,53): warning C4459: declaration of 'myState' hides global declaration ``` * cmake: Use /Wall with MSVC Make MSVC to build cleanly with Wall. This might matter for some Windows developers. And you never know if MSVC will find more bugs. * cmake: Improve router build Router build gives some warnings as it is not C90 compatible. It is ok that example is not following C90 rules. Also it is annoing to new user to build this cmake as first error usually is that libconfig is not found. Let's just give warning about this so first build will usually go smoother. --------- Co-authored-by: Kari Argillander --- .github/workflows/main.yml | 8 +-- CMakeLists.txt | 115 +++++++++++++++++++++++++------- apps/epics/main.c | 14 ++-- apps/mstpcap/main.c | 2 - apps/router-mstp/main.c | 2 - ports/linux/dlmstp_linux.c | 5 +- ports/win32/bacport.h | 6 +- src/bacnet/bacstr.c | 6 +- src/bacnet/basic/sys/platform.h | 6 -- 9 files changed, 106 insertions(+), 58 deletions(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 42e3033f..8a4b9e32 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -56,18 +56,14 @@ jobs: source_dir=$GITHUB_WORKSPACE fi - c_flags="" - if [[ "$RUNNER_OS" != "Windows" ]]; then - # Warnings as errors. - c_flags="$c_flags -Werror" - fi + cmake_options="-DCMAKE_COMPILE_WARNING_AS_ERROR=ON" # Compile as much as possible to at least compile test code. c_flags="$c_flags -DBACFILE=ON" c_flags="$c_flags -DPRINT_ENABLED=1" c_flags="$c_flags -DBACNET_TIME_MASTER=ON" c_flags="$c_flags -DBACAPP_COLOR_RGB_CONVERSION_ENABLED=ON" - cmake_options="-DCMAKE_C_FLAGS=$c_flags" + cmake_options="$cmake_options -DCMAKE_C_FLAGS=$c_flags" # TODO: Add BACDL_BIP6=ON when it builds withous errors. cmake_options="$cmake_options -DBACDL_BIP=ON" diff --git a/CMakeLists.txt b/CMakeLists.txt index 7407adf0..b380ed68 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -138,6 +138,36 @@ if (CMAKE_C_COMPILER_ID MATCHES "Clang" OR CMAKE_C_COMPILER_ID MATCHES "AppleCla add_compile_options(-Wno-float-conversion) add_compile_options(-Wno-missing-declarations) add_compile_options(-Wno-unused-but-set-variable) +elseif(MSVC) + add_compile_options(/Wall) + + # Potentially uninitialized local variable. Definetly fix these. There might + # be false positives but it is better to fix them also. + add_compile_options(/wd4701) + + # Function is unsafe (strtok, sscanf, fopen, strerror). + add_compile_options(/wd4996) + + # Handle all enums values in switch. + add_compile_options(/wd4061) + + # Possible loss of data. + add_compile_options(/wd4242 /wd4244 /wd4267) + + # Signed/unsigned mismatch. + add_compile_options(/wd4018 /wd4388 /wd4389) + + # Not defined as a preprocessor macro. + add_compile_options(/wd4668) + + # Function not inlined. + add_compile_options(/wd4710) + + # Compiler adds padding to structures. + add_compile_options(/wd4820) + + # Might be slower if builded with /Qspectre + add_compile_options(/wd5045) endif() if (CMAKE_CXX_COMPILER_ID MATCHES "Clang" OR CMAKE_CXX_COMPILER_ID MATCHES "AppleClang" OR CMAKE_CXX_COMPILER_ID MATCHES "GNU") @@ -618,10 +648,6 @@ elseif(WIN32) $<$:ws2_32> $<$:iphlpapi>) - if(MSVC) - add_definitions("/W3 /D_CRT_SECURE_NO_WARNINGS /wd4244 /wd4018 /wd4267") - endif() - target_sources(${PROJECT_NAME} PRIVATE ports/win32/bacport.h $<$:ports/win32/bip6.c> @@ -712,9 +738,24 @@ if(BACNET_STACK_BUILD_APPS) if(BACDL_MSTP) add_executable(mstpcap apps/mstpcap/main.c) target_link_libraries(mstpcap PRIVATE ${PROJECT_NAME}) - + target_compile_options(mstpcap PRIVATE + # NOTE: Might be that this example currently doesn't work on Windows because + # of the following warning: + + # 'strncasecmp': macro redefinition + $<$:/wd4005> + # 'gettimeofday' undefined; assuming extern returning int + $<$:/wd4013> + ) + add_executable(mstpcrc apps/mstpcrc/main.c) target_link_libraries(mstpcrc PRIVATE ${PROJECT_NAME}) + target_compile_options(mstpcrc PRIVATE + # NOTE: Might be that this example currently doesn't work on Windows because + # of the following warning: + # 'gettimeofday' undefined; assuming extern returning int + $<$:/wd4013> + ) endif() if(BACNET_BUILD_PIFACE_APP) @@ -729,6 +770,10 @@ if(BACNET_STACK_BUILD_APPS) src/bacnet/basic/client/bac-data.c src/bacnet/basic/client/bac-rw.c) target_link_libraries(bacpoll PRIVATE ${PROJECT_NAME}) + target_compile_options(bacpoll PRIVATE + # Unreachable code because we have endless loop. + $<$:/wd4702> + ) endif(BACNET_BUILD_BACPOLL_APP) if(BACNET_BUILD_BACDISCOVER_APP) @@ -736,7 +781,11 @@ if(BACNET_STACK_BUILD_APPS) apps/server-discover/main.c src/bacnet/basic/client/bac-discover.c src/bacnet/basic/client/bac-rw.c) - target_link_libraries(bacdiscover PRIVATE ${PROJECT_NAME}) + target_link_libraries(bacdiscover PRIVATE ${PROJECT_NAME}) + target_compile_options(bacdiscover PRIVATE + # Unreachable code because we have endless loop. + $<$:/wd4702> + ) endif(BACNET_BUILD_BACDISCOVER_APP) if(BACDL_BIP OR BACDL_BIP6) @@ -766,25 +815,39 @@ if(BACNET_STACK_BUILD_APPS) target_link_libraries(reinit PRIVATE ${PROJECT_NAME}) if(BACDL_MSTP AND NOT WIN32) - add_executable( - router - apps/router/ipmodule.c - apps/router/ipmodule.h - apps/router/main.c - apps/router/msgqueue.c - apps/router/msgqueue.h - apps/router/mstpmodule.c - apps/router/mstpmodule.h - apps/router/network_layer.c - apps/router/network_layer.h - apps/router/portthread.c - apps/router/portthread.h) + find_library(LIBCONFIG_LIBRARIES NAMES config) + if(NOT LIBCONFIG_LIBRARIES) + message(WARNING "BACNET: Will not build apps/router as libconfig not found") + return() + else() + add_executable( + router + apps/router/ipmodule.c + apps/router/ipmodule.h + apps/router/main.c + apps/router/msgqueue.c + apps/router/msgqueue.h + apps/router/mstpmodule.c + apps/router/mstpmodule.h + apps/router/network_layer.c + apps/router/network_layer.h + apps/router/portthread.c + apps/router/portthread.h) - target_link_libraries( - router - PRIVATE ${PROJECT_NAME} - # needs libconfig - -lconfig) + target_link_libraries( + router + PRIVATE ${PROJECT_NAME} + # needs libconfig + -lconfig) + + target_compile_options(router PRIVATE + # These make this example not totally C90 compatible but it is ok. + + -Wno-declaration-after-statement + -Wno-overlength-strings + -Wno-variadic-macros + ) + endif() endif() if(BACDL_BIP6) @@ -797,6 +860,10 @@ if(BACNET_STACK_BUILD_APPS) add_executable(server apps/server/main.c) target_link_libraries(server PRIVATE ${PROJECT_NAME}) + target_compile_options(server PRIVATE + # Unreachable code because we have endless loop. + $<$:/wd4702> + ) add_executable(timesync apps/timesync/main.c) target_link_libraries(timesync PRIVATE ${PROJECT_NAME}) diff --git a/apps/epics/main.c b/apps/epics/main.c index 89315b1c..31a83a50 100644 --- a/apps/epics/main.c +++ b/apps/epics/main.c @@ -876,13 +876,13 @@ static uint8_t Read_Properties( * If the present state is GET_HEADING_RESPONSE, store the results * in globals for later use. * @param rpm_data [in] The list of RPM data received. - * @param myState [in] The current state. + * @param state [in] The current state. * @return The next state of the EPICS state machine, normally NEXT_OBJECT * if the RPM got good data, or GET_PROPERTY_REQUEST if we have to * singly process the list of Properties. */ static EPICS_STATES ProcessRPMData( - BACNET_READ_ACCESS_DATA *rpm_data, EPICS_STATES myState) + BACNET_READ_ACCESS_DATA *rpm_data, EPICS_STATES state) { BACNET_READ_ACCESS_DATA *old_rpm_data; BACNET_PROPERTY_REFERENCE *rpm_property; @@ -890,7 +890,7 @@ static EPICS_STATES ProcessRPMData( BACNET_APPLICATION_DATA_VALUE *value; BACNET_APPLICATION_DATA_VALUE *old_value; bool bSuccess = true; - EPICS_STATES nextState = myState; /* assume no change */ + EPICS_STATES nextState = state; /* assume no change */ /* Some flags to keep the output "pretty" - * wait and put these object lists at the end */ bool bHasObjectList = false; @@ -902,7 +902,7 @@ static EPICS_STATES ProcessRPMData( while (rpm_property) { /* For the GET_LIST_OF_ALL_RESPONSE case, * just keep what property this was */ - if (myState == GET_LIST_OF_ALL_RESPONSE) { + if (state == GET_LIST_OF_ALL_RESPONSE) { switch (rpm_property->propertyIdentifier) { case PROP_OBJECT_LIST: bHasObjectList = true; /* Will append below */ @@ -924,7 +924,7 @@ static EPICS_STATES ProcessRPMData( value = value->next; free(old_value); } - } else if (myState == GET_HEADING_RESPONSE) { + } else if (state == GET_HEADING_RESPONSE) { Property_Value_List[i++].value = rpm_property->value; /* copy this pointer. * On error, the pointer will be null @@ -946,10 +946,10 @@ static EPICS_STATES ProcessRPMData( } /* Now determine the next state */ - if (myState == GET_HEADING_RESPONSE) { + if (state == GET_HEADING_RESPONSE) { nextState = PRINT_HEADING; /* press ahead with or without the data */ - } else if (bSuccess && (myState == GET_ALL_RESPONSE)) { + } else if (bSuccess && (state == GET_ALL_RESPONSE)) { nextState = NEXT_OBJECT; } else if (bSuccess) { /* and GET_LIST_OF_ALL_RESPONSE */ /* Now append the properties we waited on. */ diff --git a/apps/mstpcap/main.c b/apps/mstpcap/main.c index 8bd0895d..ae63b706 100644 --- a/apps/mstpcap/main.c +++ b/apps/mstpcap/main.c @@ -895,8 +895,6 @@ static BOOL WINAPI CtrlCHandler(DWORD dwCtrlType) Sleep(100); } exit(0); - - return TRUE; } #else static void sig_int(int signo) diff --git a/apps/router-mstp/main.c b/apps/router-mstp/main.c index 15725ee2..fcb6702b 100644 --- a/apps/router-mstp/main.c +++ b/apps/router-mstp/main.c @@ -1119,8 +1119,6 @@ static BOOL WINAPI CtrlCHandler(DWORD dwCtrlType) Sleep(100); } exit(0); - - return TRUE; } static void control_c_hooks(void) diff --git a/ports/linux/dlmstp_linux.c b/ports/linux/dlmstp_linux.c index 32b48f46..c2a8415a 100644 --- a/ports/linux/dlmstp_linux.c +++ b/ports/linux/dlmstp_linux.c @@ -42,6 +42,7 @@ } static uint32_t Timer_Silence(void *poPort) { + int32_t res; struct timeval now, tmp_diff; SHARED_MSTP_DATA *poSharedData; struct mstp_port_struct_t *mstp_port = (struct mstp_port_struct_t *)poPort; @@ -53,8 +54,6 @@ static uint32_t Timer_Silence(void *poPort) return -1; } - int32_t res; - gettimeofday(&now, NULL); timersub(&poSharedData->start, &now, &tmp_diff); res = ((tmp_diff.tv_sec) * 1000 + (tmp_diff.tv_usec) / 1000); @@ -764,6 +763,7 @@ bool dlmstp_init(void *poPort, char *ifname) unsigned long hThread = 0; int rv = 0; SHARED_MSTP_DATA *poSharedData; + struct termios newtio; struct mstp_port_struct_t *mstp_port = (struct mstp_port_struct_t *)poPort; if (!mstp_port) { return false; @@ -792,7 +792,6 @@ bool dlmstp_init(void *poPort, char *ifname) exit(1); } - struct termios newtio; printf("RS485: Initializing %s", poSharedData->RS485_Port_Name); /* Open device for reading and writing. diff --git a/ports/win32/bacport.h b/ports/win32/bacport.h index 41e8781c..8053ee31 100644 --- a/ports/win32/bacport.h +++ b/ports/win32/bacport.h @@ -38,10 +38,10 @@ #ifdef __MINGW32__ #include #else +#pragma warning( push ) +#pragma warning(disable: 4101 4191) #include -/* Microsoft has deprecated some CRT and C++ Standard Library functions -and globals in favor of more secure versions. */ -#pragma warning(disable : 4996) +#pragma warning( pop ) /* add winmm.lib to our build */ #pragma comment(lib, "winmm.lib") #endif diff --git a/src/bacnet/bacstr.c b/src/bacnet/bacstr.c index 18d9a70f..5ac3d462 100644 --- a/src/bacnet/bacstr.c +++ b/src/bacnet/bacstr.c @@ -220,11 +220,7 @@ bool bitstring_set_bits_used( unsigned bitstring_bits_capacity(BACNET_BIT_STRING *bit_string) { if (bit_string) { - if ((MAX_BITSTRING_BYTES * 8) <= (UINT8_MAX + 1)) { - return (MAX_BITSTRING_BYTES * 8); - } else { - return (UINT8_MAX + 1); - } + return min((MAX_BITSTRING_BYTES * 8), (UINT8_MAX + 1)); } else { return 0; } diff --git a/src/bacnet/basic/sys/platform.h b/src/bacnet/basic/sys/platform.h index df28266a..ea680ce7 100644 --- a/src/bacnet/basic/sys/platform.h +++ b/src/bacnet/basic/sys/platform.h @@ -106,10 +106,4 @@ __inline int c99_snprintf(char *outBuf, size_t size, const char *format, ...) #define BACNET_STACK_FALLTHROUGH() /* fall through */ #endif -#if defined(_MSC_VER) -/* Silence the warnings about unsafe versions of library functions */ -/* as we need to keep the code portable */ -#pragma warning(disable : 4996) -#endif - #endif