From db7475e8fc4bfd1062c8c3c1cd82afddfe038c6d Mon Sep 17 00:00:00 2001 From: Marc Herbert Date: Fri, 30 Apr 2021 00:54:20 +0000 Subject: [PATCH 1/2] Remove all function calls in arguments to MIN() and MAX() macros For consistency with Zephyr, MIN and MAX macros are "downgraded" to the basic, compile-time, standard compliant and unsafe implementation that Zephyr uses. "Unsafe" because it can evaluate its arguments twice which is obviously not desired when the argument has side-effects: https://wiki.sei.cmu.edu/confluence/display/c/PRE31-C.+Avoid+side+effects+in+arguments+to+unsafe+macros I reviewed every single invocation of MIN() and MAX() and found no increment or assignment but I found a number of function calls in their arguments. This commit removes them all. Note I did _not_ check for volatile variables. Most of these functions do not appear to have side-effects _but_ it's much simpler and faster for the reader or static analyzer not to even have to wonder about the possibility of side-effects and it's also more future-proof; no risk of a function accidentally gaining side effects. Note the following files are not compiled in the default configurations and the changes in them were NOT compile-tested locally: src/audio/codec_adapter/codec_adapter.c src/audio/drc/drc_generic.c Signed-off-by: Marc Herbert --- src/audio/codec_adapter/codec_adapter.c | 9 +++++---- src/audio/dai.c | 10 +++++----- src/audio/drc/drc_generic.c | 5 +++-- src/audio/host.c | 17 +++++++++-------- src/audio/kpb.c | 4 +++- src/audio/mixer.c | 11 +++++++---- src/audio/mux/mux.c | 7 ++++--- src/schedule/timer_domain.c | 5 ++--- 8 files changed, 38 insertions(+), 30 deletions(-) diff --git a/src/audio/codec_adapter/codec_adapter.c b/src/audio/codec_adapter/codec_adapter.c index 2fd0ef88bc97..c4cc1ffab5b8 100644 --- a/src/audio/codec_adapter/codec_adapter.c +++ b/src/audio/codec_adapter/codec_adapter.c @@ -297,8 +297,8 @@ codec_adapter_copy_from_source_to_lib(const struct audio_stream *source, size_t bytes) { /* head_size - available data until end of local buffer */ - uint32_t head_size = MIN(bytes, audio_stream_bytes_without_wrap(source, - source->r_ptr)); + const int without_wrap = audio_stream_bytes_without_wrap(source, source->r_ptr); + uint32_t head_size = MIN(bytes, without_wrap); /* tail_size - residue data to be copied starting from the beginning * of the buffer */ @@ -319,8 +319,9 @@ codec_adapter_copy_from_lib_to_sink(const struct codec_processing_data *cpd, size_t bytes) { /* head_size - free space until end of local buffer */ - uint32_t head_size = MIN(bytes, audio_stream_bytes_without_wrap(sink, - sink->w_ptr)); + const int without_wrap = + audio_stream_bytes_without_wrap(sink, sink->w_ptr); + uint32_t head_size = MIN(bytes, without_wrap); /* tail_size - rest of the bytes that needs to be written * starting from the beginning of the buffer */ diff --git a/src/audio/dai.c b/src/audio/dai.c index 5547e7762379..fede3eb6696c 100644 --- a/src/audio/dai.c +++ b/src/audio/dai.c @@ -1067,6 +1067,7 @@ static int dai_copy(struct comp_dev *dev) { struct dai_data *dd = comp_get_drvdata(dev); uint32_t dma_fmt = dd->dma_buffer->stream.frame_fmt; + const uint32_t sampling = get_sample_bytes(dma_fmt); struct comp_buffer *buf = dd->local_buffer; uint32_t avail_bytes = 0; uint32_t free_bytes = 0; @@ -1091,20 +1092,19 @@ static int dai_copy(struct comp_dev *dev) /* calculate minimum size to copy */ if (dev->direction == SOF_IPC_STREAM_PLAYBACK) { src_samples = audio_stream_get_avail_samples(&buf->stream); - sink_samples = free_bytes / get_sample_bytes(dma_fmt); + sink_samples = free_bytes / sampling; samples = MIN(src_samples, sink_samples); } else { - src_samples = avail_bytes / get_sample_bytes(dma_fmt); + src_samples = avail_bytes / sampling; sink_samples = audio_stream_get_free_samples(&buf->stream); samples = MIN(src_samples, sink_samples); /* limit bytes per copy to one period for the whole pipeline * in order to avoid high load spike */ - samples = MIN(samples, dd->period_bytes / - get_sample_bytes(dma_fmt)); + samples = MIN(samples, dd->period_bytes / sampling); } - copy_bytes = samples * get_sample_bytes(dma_fmt); + copy_bytes = samples * sampling; buffer_unlock(buf, flags); diff --git a/src/audio/drc/drc_generic.c b/src/audio/drc/drc_generic.c index d210ab645c2c..6987e3847f00 100644 --- a/src/audio/drc/drc_generic.c +++ b/src/audio/drc/drc_generic.c @@ -225,6 +225,7 @@ void drc_update_envelope(struct drc_state *state, const struct sof_drc_params *p db_per_frame = Q_MULTSR_32X32((int64_t)db_per_frame, p->kSpacingDb, 30, 0, 24); envelope_rate = db2lin_fixed(db_per_frame); /* Q12.20 */ } else { + int32_t sat32; /* Attack mode - compression_diff_db should be positive dB */ /* Fix gremlins. */ @@ -234,9 +235,9 @@ void drc_update_envelope(struct drc_state *state, const struct sof_drc_params *p /* As long as we're still in attack mode, use a rate based off * the largest compression_diff_db we've encountered so far. */ + sat32 = sat_int32(Q_SHIFT_LEFT((int64_t)compression_diff_db, 21, 24)); state->max_attack_compression_diff_db = - MAX(state->max_attack_compression_diff_db, - sat_int32(Q_SHIFT_LEFT((int64_t)compression_diff_db, 21, 24))); + MAX(state->max_attack_compression_diff_db, sat32); eff_atten_diff_db = MAX(HALF_Q24, state->max_attack_compression_diff_db); /* Q8.24 */ diff --git a/src/audio/host.c b/src/audio/host.c index 1ef4d0cc2e56..d65679f5bf91 100644 --- a/src/audio/host.c +++ b/src/audio/host.c @@ -347,17 +347,18 @@ static uint32_t host_get_copy_bytes_normal(struct comp_dev *dev) buffer_lock(hd->local_buffer, &flags); /* calculate minimum size to copy */ - if (dev->direction == SOF_IPC_STREAM_PLAYBACK) + if (dev->direction == SOF_IPC_STREAM_PLAYBACK) { /* limit bytes per copy to one period for the whole pipeline * in order to avoid high load spike */ - copy_bytes = MIN(hd->period_bytes, - MIN(avail_bytes, - audio_stream_get_free_bytes(&hd->local_buffer->stream))); - else - copy_bytes = MIN( - audio_stream_get_avail_bytes(&hd->local_buffer->stream), free_bytes); - + const uint32_t free_bytes = + audio_stream_get_free_bytes(&hd->local_buffer->stream); + copy_bytes = MIN(hd->period_bytes, MIN(avail_bytes, free_bytes)); + } else { + const uint32_t avail_bytes = + audio_stream_get_avail_bytes(&hd->local_buffer->stream); + copy_bytes = MIN(avail_bytes, free_bytes); + } buffer_unlock(hd->local_buffer, flags); /* copy_bytes should be aligned to minimum possible chunk of diff --git a/src/audio/kpb.c b/src/audio/kpb.c index ef40be9dc8be..95a130b52cf7 100644 --- a/src/audio/kpb.c +++ b/src/audio/kpb.c @@ -624,6 +624,7 @@ static int kpb_copy(struct comp_dev *dev) size_t sample_width = kpb->config.sampling_width; uint32_t flags = 0; struct draining_data *dd = &kpb->draining_task_data; + uint32_t avail_bytes; comp_dbg(dev, "kpb_copy()"); @@ -756,7 +757,8 @@ static int kpb_copy(struct comp_dev *dev) /* In draining and init draining we only buffer data in * the internal history buffer. */ - copy_bytes = MIN(audio_stream_get_avail_bytes(&source->stream), kpb->hd.free); + avail_bytes = audio_stream_get_avail_bytes(&source->stream); + copy_bytes = MIN(avail_bytes, kpb->hd.free); ret = PPL_STATUS_PATH_STOP; if (copy_bytes) { buffer_invalidate(source, copy_bytes); diff --git a/src/audio/mixer.c b/src/audio/mixer.c index f97d69841a8d..11ed68239740 100644 --- a/src/audio/mixer.c +++ b/src/audio/mixer.c @@ -298,8 +298,10 @@ static int mixer_copy(struct comp_dev *dev) /* write zeros if all sources are inactive */ if (num_mix_sources == 0) { + uint32_t free_frames; buffer_lock(sink, &flags); - frames = MIN(frames, audio_stream_get_free_frames(&sink->stream)); + free_frames = audio_stream_get_free_frames(&sink->stream); + frames = MIN(frames, free_frames); sink_bytes = frames * audio_stream_frame_bytes(&sink->stream); buffer_unlock(sink, flags); @@ -315,10 +317,11 @@ static int mixer_copy(struct comp_dev *dev) /* check for underruns */ for (i = 0; i < num_mix_sources; i++) { + uint32_t avail_frames; buffer_lock(sources[i], &flags); - frames = MIN(frames, - audio_stream_avail_frames(sources_stream[i], - &sink->stream)); + avail_frames = audio_stream_avail_frames(sources_stream[i], + &sink->stream); + frames = MIN(frames, avail_frames); buffer_unlock(sources[i], flags); } diff --git a/src/audio/mux/mux.c b/src/audio/mux/mux.c index ce94dad2f470..9fe7429fcecd 100644 --- a/src/audio/mux/mux.c +++ b/src/audio/mux/mux.c @@ -571,11 +571,12 @@ static int mux_copy(struct comp_dev *dev) } for (i = 0; i < MUX_MAX_STREAMS; i++) { + uint32_t avail_frames; if (!sources[i]) continue; - frames = MIN(frames, - audio_stream_avail_frames(sources_stream[i], - &sink->stream)); + avail_frames = audio_stream_avail_frames(sources_stream[i], + &sink->stream); + frames = MIN(frames, avail_frames); buffer_unlock(sources[i], flags); } diff --git a/src/schedule/timer_domain.c b/src/schedule/timer_domain.c index b9360d4bfa44..1f2b33775b20 100644 --- a/src/schedule/timer_domain.c +++ b/src/schedule/timer_domain.c @@ -228,10 +228,9 @@ static void timer_domain_set(struct ll_schedule_domain *domain, uint64_t start) uint64_t ticks_tout = timer_domain->timeout; uint64_t ticks_set; #ifndef __ZEPHYR__ - uint64_t ticks_req; - /* make sure to require for ticks later than tout from now */ - ticks_req = MAX(start, platform_timer_get_atomic(timer_domain->timer) + ticks_tout); + const uint64_t time = platform_timer_get_atomic(timer_domain->timer); + const uint64_t ticks_req = MAX(start, time + ticks_tout); ticks_set = platform_timer_set(timer_domain->timer, ticks_req); #else From ee344a1b3f79c1e8d55910677fb4ea3fe38bf620 Mon Sep 17 00:00:00 2001 From: Marc Herbert Date: Fri, 30 Apr 2021 01:06:34 +0000 Subject: [PATCH 2/2] numbers.h: downgrade MIN() and MAX() to unsafe versions For compatibility with Zephyr, MIN and MAX macros are downgraded to the basic, compile-time, standard compliant and unsafe implementation. This means the code will now be the same whether it's compiled with Zephyr versus not. Fixes https://github.com/zephyrproject-rtos/zephyr/issues/33567 which means it's now possible to use MIN() or MAX() in either Zephyr's BUILD_ASSERT() or in SOF's equivalent STATIC_ASSERT() or in C11's _Static_assert() This implementation is unsafe because it can evaluate its arguments twice which is obviously not desired when the argument has side-effects: https://wiki.sei.cmu.edu/confluence/display/c/PRE31-C.+Avoid+side+effects+in+arguments+to+unsafe+macros Signed-off-by: Marc Herbert --- src/include/sof/math/numbers.h | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/src/include/sof/math/numbers.h b/src/include/sof/math/numbers.h index 7e08e3e9afa3..78be3239fd76 100644 --- a/src/include/sof/math/numbers.h +++ b/src/include/sof/math/numbers.h @@ -16,23 +16,17 @@ #ifdef MIN #undef MIN #endif - -#define MIN(a, b) ({ \ - typeof(a) __a = (a); \ - typeof(b) __b = (b); \ - __a > __b ? __b : __a; \ -}) +/* Unsafe and portable macros for consistency with Zephyr. + * See SEI CERT-C PRE31-C + */ +#define MIN(a, b) ((a) < (b) ? (a) : (b)) /* Zephyr defines this - remove local copy once Zephyr integration complete */ #ifdef MAX #undef MAX #endif +#define MAX(a, b) ((a) < (b) ? (b) : (a)) -#define MAX(a, b) ({ \ - typeof(a) __a = (a); \ - typeof(b) __b = (b); \ - __a < __b ? __b : __a; \ -}) #define ABS(a) ({ \ typeof(a) __a = (a); \ __a < 0 ? -__a : __a; \