From e9539d4560c6a559135c7ec65f0b707b091d8f6d Mon Sep 17 00:00:00 2001 From: morris Date: Mon, 17 Nov 2025 17:55:08 +0800 Subject: [PATCH] refactor: avoid function calls inside MIN/MAX macros --- components/efuse/src/esp_efuse_fields.c | 6 ++- components/esp_driver_i2c/i2c_master.c | 6 ++- components/esp_driver_i2s/lp_i2s.c | 6 ++- components/esp_driver_ledc/src/ledc.c | 6 ++- components/esp_driver_uart/src/uart.c | 3 +- components/esp_hw_support/dma/gdma.c | 1 + components/esp_hw_support/esp_clk.c | 3 +- components/esp_hw_support/sleep_modes.c | 4 +- .../port/arch/xtensa/debug_helpers.c | 3 +- components/esp_timer/src/esp_timer.c | 6 ++- .../esp_timer/test_apps/main/test_esp_timer.c | 3 +- .../esp_usb_cdc_rom_console/vfs_cdcacm.c | 3 +- components/hal/systimer_hal.c | 3 +- .../mbedtls/port/aes/dma/esp_aes_dma_core.c | 6 ++- components/spi_flash/spi_flash_os_func_app.c | 3 +- .../no_function_call_in_min_max_macro.yml | 37 +++++++++++++++++++ 16 files changed, 78 insertions(+), 21 deletions(-) create mode 100644 tools/ci/sg_rules/no_function_call_in_min_max_macro.yml diff --git a/components/efuse/src/esp_efuse_fields.c b/components/efuse/src/esp_efuse_fields.c index 9bb94cea08..3ac433e62f 100644 --- a/components/efuse/src/esp_efuse_fields.c +++ b/components/efuse/src/esp_efuse_fields.c @@ -44,7 +44,8 @@ void esp_efuse_reset(void) uint32_t esp_efuse_read_secure_version(void) { uint32_t secure_version = 0; - size_t size = MIN(APP_SEC_VER_SIZE_EFUSE_FIELD, esp_efuse_get_field_size(ESP_EFUSE_SECURE_VERSION)); + size_t field_size = esp_efuse_get_field_size(ESP_EFUSE_SECURE_VERSION); + size_t size = MIN(APP_SEC_VER_SIZE_EFUSE_FIELD, field_size); esp_efuse_read_field_blob(ESP_EFUSE_SECURE_VERSION, &secure_version, size); return __builtin_popcount(secure_version & ((1ULL << size) - 1)); } @@ -66,7 +67,8 @@ bool esp_efuse_check_secure_version(uint32_t secure_version) esp_err_t esp_efuse_update_secure_version(uint32_t secure_version) { - size_t size = MIN(APP_SEC_VER_SIZE_EFUSE_FIELD, esp_efuse_get_field_size(ESP_EFUSE_SECURE_VERSION)); + size_t field_size = esp_efuse_get_field_size(ESP_EFUSE_SECURE_VERSION); + size_t size = MIN(APP_SEC_VER_SIZE_EFUSE_FIELD, field_size); if (size < secure_version) { ESP_LOGE(TAG, "Max secure version is %u. Given %"PRIu32" version can not be written.", (unsigned)size, secure_version); return ESP_ERR_INVALID_ARG; diff --git a/components/esp_driver_i2c/i2c_master.c b/components/esp_driver_i2c/i2c_master.c index 023122659f..94fbae5610 100644 --- a/components/esp_driver_i2c/i2c_master.c +++ b/components/esp_driver_i2c/i2c_master.c @@ -190,7 +190,8 @@ static bool s_i2c_write_command(i2c_master_bus_handle_t i2c_master, i2c_operatio uint8_t data_fill = 0; // data_fill refers to the length to fill the data - data_fill = MIN(remaining_bytes, I2C_FIFO_LEN(i2c_master->base->port_num) - *address_fill); + uint32_t fifo_len = I2C_FIFO_LEN(i2c_master->base->port_num); + data_fill = MIN(remaining_bytes, fifo_len - *address_fill); write_pr = i2c_operation->data + i2c_operation->bytes_used; i2c_operation->bytes_used += data_fill; hw_cmd.byte_num = data_fill + *address_fill; @@ -286,7 +287,8 @@ static bool s_i2c_read_command(i2c_master_bus_handle_t i2c_master, i2c_operation i2c_bus_handle_t handle = i2c_master->base; i2c_ll_hw_cmd_t hw_cmd = i2c_operation->hw_cmd; - *fifo_fill = MIN(remaining_bytes, I2C_FIFO_LEN(i2c_master->base->port_num) - i2c_master->read_len_static); + uint32_t fifo_len = I2C_FIFO_LEN(i2c_master->base->port_num); + *fifo_fill = MIN(remaining_bytes, fifo_len - i2c_master->read_len_static); i2c_master->rx_cnt = *fifo_fill; hw_cmd.byte_num = *fifo_fill; diff --git a/components/esp_driver_i2s/lp_i2s.c b/components/esp_driver_i2s/lp_i2s.c index 2c46566170..9b1d6fc196 100644 --- a/components/esp_driver_i2s/lp_i2s.c +++ b/components/esp_driver_i2s/lp_i2s.c @@ -169,7 +169,8 @@ esp_err_t lp_i2s_channel_read(lp_i2s_chan_handle_t chan, lp_i2s_trans_t *trans, return ESP_ERR_TIMEOUT; } - size_t len = MIN(lp_i2s_ll_get_rx_mem_fifo_cnt(chan->ctlr->hal.dev), trans->buflen); + size_t fifo_cnt = lp_i2s_ll_get_rx_mem_fifo_cnt(chan->ctlr->hal.dev); + size_t len = MIN(fifo_cnt, trans->buflen); lp_i2s_ll_read_buffer(chan->ctlr->hal.dev, trans->buffer, len); trans->received_size = len; portENTER_CRITICAL(&g_i2s.spinlock); @@ -305,7 +306,8 @@ static void IRAM_ATTR s_i2s_default_isr(void *arg) ESP_DRAM_LOGD(TAG, "in isr, rx_mem_fifo_cnt: %d bytes", lp_i2s_ll_get_rx_mem_fifo_cnt(ctlr->hal.dev)); if (ctlr->rx_chan->cbs.on_request_new_trans) { - size_t len = MIN(lp_i2s_ll_get_rx_mem_fifo_cnt(ctlr->hal.dev), ctlr->rx_chan->trans.buflen); + size_t fifo_cnt = lp_i2s_ll_get_rx_mem_fifo_cnt(ctlr->hal.dev); + size_t len = MIN(fifo_cnt, ctlr->rx_chan->trans.buflen); ESP_DRAM_LOGD(TAG, "len: %d", len); lp_i2s_ll_read_buffer(ctlr->hal.dev, ctlr->rx_chan->trans.buffer, len); lp_i2s_ll_rx_clear_interrupt_status(ctlr->hal.dev, LP_I2S_LL_EVENT_RX_MEM_THRESHOLD_INT); diff --git a/components/esp_driver_ledc/src/ledc.c b/components/esp_driver_ledc/src/ledc.c index 04bada46c8..4166f1c3c8 100644 --- a/components/esp_driver_ledc/src/ledc.c +++ b/components/esp_driver_ledc/src/ledc.c @@ -1176,11 +1176,13 @@ uint32_t ledc_find_suitable_duty_resolution(uint32_t src_clk_freq, uint32_t time { // Highest resolution is calculated when LEDC_CLK_DIV = 1 (i.e. div_param = 1 << LEDC_LL_FRACTIONAL_BITS) uint32_t div = (src_clk_freq + timer_freq / 2) / timer_freq; // rounded - uint32_t duty_resolution = MIN(ilog2(div), SOC_LEDC_TIMER_BIT_WIDTH); + uint32_t ilog2_div = ilog2(div); + uint32_t duty_resolution = MIN(ilog2_div, SOC_LEDC_TIMER_BIT_WIDTH); uint32_t div_param = ledc_calculate_divisor(src_clk_freq, timer_freq, 1 << duty_resolution); if (LEDC_IS_DIV_INVALID(div_param)) { div = src_clk_freq / timer_freq; // truncated - duty_resolution = MIN(ilog2(div), SOC_LEDC_TIMER_BIT_WIDTH); + ilog2_div = ilog2(div); + duty_resolution = MIN(ilog2_div, SOC_LEDC_TIMER_BIT_WIDTH); div_param = ledc_calculate_divisor(src_clk_freq, timer_freq, 1 << duty_resolution); if (LEDC_IS_DIV_INVALID(div_param)) { duty_resolution = 0; diff --git a/components/esp_driver_uart/src/uart.c b/components/esp_driver_uart/src/uart.c index fbf4f7c2eb..1c3a87cc5c 100644 --- a/components/esp_driver_uart/src/uart.c +++ b/components/esp_driver_uart/src/uart.c @@ -1618,7 +1618,8 @@ static int uart_tx_all(uart_port_t uart_num, const char *src, size_t size, bool } xRingbufferSend(p_uart_obj[uart_num]->tx_ring_buf, (void *) &evt, sizeof(uart_tx_data_t), portMAX_DELAY); while (size > 0) { - size_t send_size = MIN(size, xRingbufferGetCurFreeSize(p_uart_obj[uart_num]->tx_ring_buf)); + size_t free_size = xRingbufferGetCurFreeSize(p_uart_obj[uart_num]->tx_ring_buf); + size_t send_size = MIN(size, free_size); xRingbufferSend(p_uart_obj[uart_num]->tx_ring_buf, (void *)(src + offset), send_size, portMAX_DELAY); size -= send_size; offset += send_size; diff --git a/components/esp_hw_support/dma/gdma.c b/components/esp_hw_support/dma/gdma.c index 1c2a6a0dff..b2648aaefa 100644 --- a/components/esp_hw_support/dma/gdma.c +++ b/components/esp_hw_support/dma/gdma.c @@ -198,6 +198,7 @@ err: if (alloc_rx_channel) { free(alloc_rx_channel); } + // coverity[dead_error_condition] - defensive check for code maintainability if (pair) { gdma_release_pair_handle(pair); // pair release will also release group if it's the last reference diff --git a/components/esp_hw_support/esp_clk.c b/components/esp_hw_support/esp_clk.c index 3eaefc4cf8..63c5041f04 100644 --- a/components/esp_hw_support/esp_clk.c +++ b/components/esp_hw_support/esp_clk.c @@ -87,7 +87,8 @@ int IRAM_ATTR esp_clk_apb_freq(void) { // TODO: IDF-5173 Require cleanup, implementation should be unified #if CONFIG_IDF_TARGET_ESP32 || CONFIG_IDF_TARGET_ESP32S2 || CONFIG_IDF_TARGET_ESP32S3 || CONFIG_IDF_TARGET_ESP32C3 || CONFIG_IDF_TARGET_ESP32C2 - return MIN(s_get_cpu_freq_mhz() * MHZ, APB_CLK_FREQ); + uint32_t cpu_freq_hz = s_get_cpu_freq_mhz() * MHZ; + return MIN(cpu_freq_hz, APB_CLK_FREQ); #else // for all later targets return rtc_clk_apb_freq_get(); #endif diff --git a/components/esp_hw_support/sleep_modes.c b/components/esp_hw_support/sleep_modes.c index 2cae1213ba..6d902a2c21 100644 --- a/components/esp_hw_support/sleep_modes.c +++ b/components/esp_hw_support/sleep_modes.c @@ -1578,9 +1578,9 @@ esp_err_t esp_light_sleep_start(void) * min_protect(2); * 4. All the adjustment time which is s_config.sleep_time_adjustment below. */ + uint32_t rtc_slowclk_us = rtc_time_slowclk_to_us(RTC_MODULE_SLEEP_PREPARE_CYCLES + RTC_CNTL_MIN_SLP_VAL_MIN, s_config.rtc_clk_cal_period); const uint32_t vddsdio_pd_sleep_duration = MAX(FLASH_PD_MIN_SLEEP_TIME_US, - flash_enable_time_us + s_config.sleep_time_adjustment - + rtc_time_slowclk_to_us(RTC_MODULE_SLEEP_PREPARE_CYCLES + RTC_CNTL_MIN_SLP_VAL_MIN, s_config.rtc_clk_cal_period)); + flash_enable_time_us + s_config.sleep_time_adjustment + rtc_slowclk_us); if (can_power_down_vddsdio(sleep_flags, vddsdio_pd_sleep_duration)) { if (s_config.sleep_time_overhead_out < flash_enable_time_us) { diff --git a/components/esp_system/port/arch/xtensa/debug_helpers.c b/components/esp_system/port/arch/xtensa/debug_helpers.c index 95bd85c2db..e4bb15fb41 100644 --- a/components/esp_system/port/arch/xtensa/debug_helpers.c +++ b/components/esp_system/port/arch/xtensa/debug_helpers.c @@ -192,7 +192,8 @@ esp_err_t ESP_SYSTEM_IRAM_ATTR esp_backtrace_print_all_tasks(int depth) &ctrl.cur_tasks[core_id].next_pc); // Get snapshot of all tasks in the system - const UBaseType_t num_snapshots = MIN(num_tasks, uxTaskGetSnapshotAll(task_snapshots, num_tasks, NULL)); + UBaseType_t snapshot_count = uxTaskGetSnapshotAll(task_snapshots, num_tasks, NULL); + const UBaseType_t num_snapshots = MIN(num_tasks, snapshot_count); // Print the backtrace of every task in the system for (UBaseType_t task_idx = 0; task_idx < num_snapshots; task_idx++) { bool cur_running = false; diff --git a/components/esp_timer/src/esp_timer.c b/components/esp_timer/src/esp_timer.c index 3c21f995dd..28f70e73a6 100644 --- a/components/esp_timer/src/esp_timer.c +++ b/components/esp_timer/src/esp_timer.c @@ -161,7 +161,8 @@ esp_err_t ESP_TIMER_IRAM_ATTR esp_timer_restart(esp_timer_handle_t timer, uint64 * - if the alarm was a one-shot one, i.e. `period` is 0, it remains non-periodic. */ if (period != 0) { /* Remove function got rid of the alarm and period fields, restore them */ - const uint64_t new_period = MAX(timeout_us, esp_timer_impl_get_min_period_us()); + const uint64_t min_period = esp_timer_impl_get_min_period_us(); + const uint64_t new_period = MAX(timeout_us, min_period); timer->alarm = now + new_period; timer->period = new_period; } else { @@ -218,7 +219,8 @@ esp_err_t ESP_TIMER_IRAM_ATTR esp_timer_start_periodic(esp_timer_handle_t timer, if (!is_initialized()) { return ESP_ERR_INVALID_STATE; } - period_us = MAX(period_us, esp_timer_impl_get_min_period_us()); + uint64_t min_period = esp_timer_impl_get_min_period_us(); + period_us = MAX(period_us, min_period); int64_t alarm = esp_timer_get_time() + period_us; esp_timer_dispatch_t dispatch_method = timer->flags & FL_ISR_DISPATCH_METHOD; esp_err_t err; diff --git a/components/esp_timer/test_apps/main/test_esp_timer.c b/components/esp_timer/test_apps/main/test_esp_timer.c index 899096240b..31976d26fe 100644 --- a/components/esp_timer/test_apps/main/test_esp_timer.c +++ b/components/esp_timer/test_apps/main/test_esp_timer.c @@ -457,7 +457,8 @@ static void timer_test_monotonic_values_task(void* arg) state->pass = false; } state->avg_diff += diff; - state->max_error = MAX(state->max_error, llabs(diff)); + int64_t abs_diff = llabs(diff); + state->max_error = MAX(state->max_error, abs_diff); state->test_cnt++; } state->avg_diff /= state->test_cnt; diff --git a/components/esp_usb_cdc_rom_console/vfs_cdcacm.c b/components/esp_usb_cdc_rom_console/vfs_cdcacm.c index e242d81450..3026c86d25 100644 --- a/components/esp_usb_cdc_rom_console/vfs_cdcacm.c +++ b/components/esp_usb_cdc_rom_console/vfs_cdcacm.c @@ -191,7 +191,8 @@ static ssize_t cdcacm_read(int fd, void *data, size_t size) } else { /* process pending interrupts before requesting available data */ esp_usb_console_poll_interrupts(); - size = MIN(size, cdcacm_data_length_in_buffer()); + size_t available = cdcacm_data_length_in_buffer(); + size = MIN(size, available); } while (received < size) { diff --git a/components/hal/systimer_hal.c b/components/hal/systimer_hal.c index 658bfaad47..b141202367 100644 --- a/components/hal/systimer_hal.c +++ b/components/hal/systimer_hal.c @@ -92,7 +92,8 @@ void systimer_hal_set_alarm_target(systimer_hal_context_t *hal, uint32_t alarm_i { int64_t offset = hal->us_to_ticks(1) * 2; uint64_t now_time = systimer_hal_get_counter_value(hal, 0); - systimer_counter_value_t alarm = { .val = MAX(hal->us_to_ticks(timestamp), now_time + offset) }; + uint64_t timestamp_ticks = hal->us_to_ticks(timestamp); + systimer_counter_value_t alarm = { .val = MAX(timestamp_ticks, now_time + offset) }; do { systimer_ll_enable_alarm(hal->dev, alarm_id, false); systimer_ll_set_alarm_target(hal->dev, alarm_id, alarm.val); diff --git a/components/mbedtls/port/aes/dma/esp_aes_dma_core.c b/components/mbedtls/port/aes/dma/esp_aes_dma_core.c index 4cb5cd4429..d07a310af4 100644 --- a/components/mbedtls/port/aes/dma/esp_aes_dma_core.c +++ b/components/mbedtls/port/aes/dma/esp_aes_dma_core.c @@ -244,8 +244,10 @@ static int esp_aes_process_dma_ext_ram(esp_aes_context *ctx, const unsigned char #ifdef SOC_GDMA_EXT_MEM_ENC_ALIGNMENT if (efuse_hal_flash_encryption_enabled()) { if (esp_ptr_external_ram(input) || esp_ptr_external_ram(output) || esp_ptr_in_drom(input) || esp_ptr_in_drom(output)) { - input_alignment = MAX(get_cache_line_size(input), SOC_GDMA_EXT_MEM_ENC_ALIGNMENT); - output_alignment = MAX(get_cache_line_size(output), SOC_GDMA_EXT_MEM_ENC_ALIGNMENT); + size_t input_cache_line_size = get_cache_line_size(input); + size_t output_cache_line_size = get_cache_line_size(output); + input_alignment = MAX(input_cache_line_size, SOC_GDMA_EXT_MEM_ENC_ALIGNMENT); + output_alignment = MAX(output_cache_line_size, SOC_GDMA_EXT_MEM_ENC_ALIGNMENT); input_heap_caps = MALLOC_CAP_8BIT | (esp_ptr_external_ram(input) ? MALLOC_CAP_SPIRAM : MALLOC_CAP_DMA | MALLOC_CAP_INTERNAL); output_heap_caps = MALLOC_CAP_8BIT | (esp_ptr_external_ram(output) ? MALLOC_CAP_SPIRAM : MALLOC_CAP_DMA | MALLOC_CAP_INTERNAL); diff --git a/components/spi_flash/spi_flash_os_func_app.c b/components/spi_flash/spi_flash_os_func_app.c index 0b658ab811..e10976b77d 100644 --- a/components/spi_flash/spi_flash_os_func_app.c +++ b/components/spi_flash/spi_flash_os_func_app.c @@ -217,7 +217,8 @@ static void* get_buffer_malloc(void* arg, size_t reqest_size, size_t* out_size) unsigned retries = 5; size_t read_chunk_size = reqest_size; while(ret == NULL && retries--) { - read_chunk_size = MIN(read_chunk_size, heap_caps_get_largest_free_block(MALLOC_CAP_INTERNAL | MALLOC_CAP_8BIT)); + size_t largest_free = heap_caps_get_largest_free_block(MALLOC_CAP_INTERNAL | MALLOC_CAP_8BIT); + read_chunk_size = MIN(read_chunk_size, largest_free); read_chunk_size = (read_chunk_size + 3) & ~3; ret = heap_caps_malloc(read_chunk_size, MALLOC_CAP_INTERNAL | MALLOC_CAP_8BIT); } diff --git a/tools/ci/sg_rules/no_function_call_in_min_max_macro.yml b/tools/ci/sg_rules/no_function_call_in_min_max_macro.yml new file mode 100644 index 0000000000..cbc7aafbf4 --- /dev/null +++ b/tools/ci/sg_rules/no_function_call_in_min_max_macro.yml @@ -0,0 +1,37 @@ +id: no-function-call-in-min-max +message: "Avoid calling functions inside MIN/MAX macros" +severity: error +note: "MIN/MAX macros may evaluate arguments multiple times. Store function results in a variable first." +language: C +files: + - "components/**/*" +ignores: + - "components/bt/**/*" + - "components/spiffs/**/*" +rule: + kind: call_expression + pattern: $FUNC($ARG1, $ARG2) + any: + - has: + kind: call_expression + field: arguments + stopBy: end + position: 0 + not: + has: + kind: identifier + regex: '^(MIN|MAX)$' + field: function + - has: + kind: call_expression + field: arguments + stopBy: end + position: 1 + not: + has: + kind: identifier + regex: '^(MIN|MAX)$' + field: function +constraints: + FUNC: + regex: '^(MIN|MAX)$'