unittests/unicoap: statically allocate option buffers #21610

mguetschow · 2025-07-17T12:30:12Z

Contribution description

unicoap unittests would currently fail due to stack overflow on most hardware. They indeed allocate much more memory than needed.

The default unittest stacksize is set to THREAD_STACKSIZE_LARGE which is 2048 for cortexm_common, so 900 bytes will largely fit. However, some other architectures (e.g., atmega8) define much smaller stack sizes per default, where the reduced buffer would not fit. Should we rather make them static so they work for all architectures? We then wouldn't be able to use the convenience macro anymore.

EDIT: Changed to statically allocated buffers now using a new convenience macro UNICOAP_OPTIONS_ALLOC_STATIC.

Testing procedure

make -C tests/unittests BOARD=nrf52840dk tests-unicoap flash term

on master:

Welcome to pyterm!
Type '/exit' to exit.
r
2025-07-17 14:28:14,931 # READY
s
2025-07-17 14:28:16,637 # START
2025-07-17 14:28:16,643 # main(): This is RIOT! (Version: 2025.07-devel-571-gd06b6)
2025-07-17 14:28:16,648 # Help: Press s to start test, r to print it is ready

with this PR:

Welcome to pyterm!
Type '/exit' to exit.
r
2025-07-17 14:26:48,657 # READY
s
2025-07-17 14:26:49,469 # START
2025-07-17 14:26:49,471 # ................
2025-07-17 14:26:49,473 # OK (16 tests)

Issues/PRs references

Found while testing #21582

the default unittest stacksize is set to THREAD_STACKSIZE_LARGE which is 2048 for cortexm_common

carl-tud

Thanks, this was my mistake.
For anyone else wondering: The size of the options buffer allocated should be slightly (well, maybe not by 1.3 KB) larger than needed so tests where unicoap writes too much into buffer can fail gracefully instead of memfaulting immediately.

carl-tud · 2025-07-17T12:34:52Z

As a quick fix for making the tests run, modifying the buffer size is the right thing to do.
I think I should a add a macro UNICOAP_OPTIONS_ALLOC_STATIC for these instances. These buffer should've been static from the get-go.

mguetschow · 2025-07-17T13:19:30Z

As a quick fix for making the tests run, modifying the buffer size is the right thing to do. I think I should a add a macro UNICOAP_OPTIONS_ALLOC_STATIC for these instances. These buffer should've been static from the get-go.

Should I adapt the PR to provide that macro instead? Much cleaner solution indeed.

carl-tud · 2025-07-17T13:22:29Z

Okay, then we have two options:

// 1
UNICOAP_OPTIONS_ALLOC_STATIC(my_options, 100);

// or 
// 2
UNICOAP_OPTIONS_ALLOC(my_options, 100, static);

I think I would prefer 2 over 1.

mguetschow · 2025-07-17T13:24:44Z

I'd prefer 1, but that's probably more personal taste.

carl-tud · 2025-07-17T13:25:11Z

@miri64

crasbe · 2025-07-17T15:25:02Z

sys/include/net/unicoap/options.h

@@ -193,7 +193,21 @@ static inline void unicoap_options_clear(unicoap_options_t* options)
 * the given capacity, then calls @ref unicoap_options_t::unicoap_options_init.
 */
 #define UNICOAP_OPTIONS_ALLOC(name, capacity) \
-    _UNICOAP_OPTIONS_ALLOC(_CONCAT3(name, _storage, __LINE__), name, capacity)
+    _UNICOAP_OPTIONS_ALLOC(_CONCAT3(name, _storage, __LINE__), name, capacity,)


Suggested change

_UNICOAP_OPTIONS_ALLOC(_CONCAT3(name, _storage, __LINE__), name, capacity,)

_UNICOAP_OPTIONS_ALLOC(_CONCAT3(name, _storage, __LINE__), name, capacity)

I guess?

No, in that case we don't want to emit anything before the type (aka, no static).

carl-tud · 2025-07-17T15:40:46Z

sys/include/net/unicoap/options.h

-      unicoap_options_t _name;                          \
+#  define _UNICOAP_OPTIONS_ALLOC(_buf, _name, capacity, _static)    \
+      _static uint8_t _buf[capacity];                               \
+      unicoap_options_t _name;                                      \
      unicoap_options_init(&_name, _buf, capacity);


1, I think the static version should work on the root level of a translation unit, too. So I fear we have to replace unicoap_options_init with unicoap_options_t _name = { .entries->data = _buf, .storage_capacity = capacity };

3, And why wouldn't we want to statically allocate the unicoap_options_t, too? Is there a good reason not to? It would make sense because of the lookup array in the options.

Good point, adapted it accordingly. Note that I do the same for both _STATIC and non-static, with the slight difference that more memory will be zero-initialized for the non-static version compared to using the unicoap_options_init function. But it keeps the macros in sync.

Sure, would make sense, especially if used outside of functions. Adapted.

sys/include/net/unicoap/options.h

riot-ci · 2025-07-17T18:33:28Z

Murdock results

✔️ PASSED

8623772 unittests/unicoap: statically allocate option buffers

Success	Failures	Total	Runtime
10525	0	10525	11m:01s

Artifacts

Documentation preview

mguetschow · 2025-07-18T07:15:42Z

I've also added UNICOAP_OPTIONS_ALLOC_STATIC_DEFAULT akin to UNICOAP_OPTIONS_ALLOC_DEFAULT.

sys/include/net/unicoap/options.h

carl-tud · 2025-07-18T07:55:41Z

maybe you could update the pr description to reflect the change

mguetschow · 2025-07-18T10:55:56Z

maybe you could update the pr description to reflect the change

Done.

mguetschow · 2025-07-23T16:27:34Z

Sorry, had forgotten to push 🙈 please tell me when to squash

Teufelchen1

I somewhat wonder if the macros are worth the hassle...

Teufelchen1 · 2025-07-24T09:12:55Z

Yes squash please

Teufelchen1 · 2025-07-24T09:23:19Z

@carl-tud is your review done?

carl-tud

Looks good, thanks.

sys/include/net/unicoap/options.h

Teufelchen1 · 2025-07-24T13:40:06Z

Please run dist/tools/insufficient_memory/update_insufficient_memory_board.sh :>

mguetschow · 2025-07-24T13:40:29Z

@crasbe would you mind checking for the necessary Makefile.ci content?

crasbe · 2025-07-24T13:40:51Z

@crasbe would you mind checking for the necessary Makefile.ci content?

Already on it :)

crasbe · 2025-07-24T13:53:56Z

buechse@skyleaf:~/RIOTstuff/riot-unicoap/RIOT/tests/unittests$ git diff Makefile.ci
diff --git a/tests/unittests/Makefile.ci b/tests/unittests/Makefile.ci
index 3490841797..6e9fdfedef 100644
--- a/tests/unittests/Makefile.ci
+++ b/tests/unittests/Makefile.ci
@@ -79,6 +79,7 @@ BOARD_INSUFFICIENT_MEMORY := \
     pba-d-01-kw2x \
     remote-pa \
     remote-reva \
+    remote-revb \
     samd10-xmini \
     samd20-xpro \
     samd21-xpro \

crasbe · 2025-07-24T13:55:08Z

So annoying that it failed at the last minute...

mguetschow · 2025-07-24T13:59:22Z

Should run through now 🙏

mguetschow · 2025-07-24T22:43:25Z

Backport provided in #21622

unittests/unicoap: reduce buffer size to fit on stack

ec3ca22

the default unittest stacksize is set to THREAD_STACKSIZE_LARGE which is 2048 for cortexm_common

mguetschow requested a review from miri64 as a code owner July 17, 2025 12:30

mguetschow requested review from carl-tud and removed request for miri64 July 17, 2025 12:30

github-actions bot added the Area: tests Area: tests and testing framework label Jul 17, 2025

carl-tud approved these changes Jul 17, 2025

View reviewed changes

github-actions bot added Area: network Area: Networking Area: CoAP Area: Constrained Application Protocol implementations Area: sys Area: System labels Jul 17, 2025

crasbe added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jul 17, 2025

crasbe reviewed Jul 17, 2025

View reviewed changes

carl-tud requested changes Jul 17, 2025

View reviewed changes

carl-tud reviewed Jul 18, 2025

View reviewed changes

sys/include/net/unicoap/options.h Outdated Show resolved Hide resolved

mguetschow changed the title ~~unittests/unicoap: reduce buffer size to fit on stack~~ unittests/unicoap: statically allocate option buffers Jul 18, 2025

mguetschow added the Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch label Jul 23, 2025

Teufelchen1 approved these changes Jul 24, 2025

View reviewed changes

sys/net/unicoap: add UNICOAP_OPTIONS_ALLOC_STATIC

50a5d17

mguetschow force-pushed the unicoap-test-stack-overflow branch from dd80c05 to 64352cb Compare July 24, 2025 09:18

carl-tud approved these changes Jul 24, 2025

View reviewed changes

Teufelchen1 added this pull request to the merge queue Jul 24, 2025

benpicco reviewed Jul 24, 2025

View reviewed changes

sys/include/net/unicoap/options.h Show resolved Hide resolved

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 24, 2025

crasbe added this pull request to the merge queue Jul 24, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 24, 2025

unittests/unicoap: statically allocate option buffers

8623772

mguetschow force-pushed the unicoap-test-stack-overflow branch from 64352cb to 8623772 Compare July 24, 2025 13:58

mguetschow enabled auto-merge July 24, 2025 13:59

mguetschow added this pull request to the merge queue Jul 24, 2025

Merged via the queue into RIOT-OS:master with commit f572bbe Jul 24, 2025
25 checks passed

mguetschow deleted the unicoap-test-stack-overflow branch July 24, 2025 22:29

mguetschow mentioned this pull request Jul 24, 2025

unittests/unicoap: statically allocate option buffers [backport 2025.07] #21622

Merged

	_UNICOAP_OPTIONS_ALLOC(_CONCAT3(name, _storage, __LINE__), name, capacity,)
	_UNICOAP_OPTIONS_ALLOC(_CONCAT3(name, _storage, __LINE__), name, capacity)

unittests/unicoap: statically allocate option buffers #21610

unittests/unicoap: statically allocate option buffers #21610

Uh oh!

Conversation

mguetschow commented Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

carl-tud left a comment

Choose a reason for hiding this comment

Uh oh!

carl-tud commented Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mguetschow commented Jul 17, 2025

Uh oh!

carl-tud commented Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mguetschow commented Jul 17, 2025

Uh oh!

carl-tud commented Jul 17, 2025

Uh oh!

crasbe Jul 17, 2025

Choose a reason for hiding this comment

Uh oh!

mguetschow Jul 18, 2025

Choose a reason for hiding this comment

Uh oh!

carl-tud Jul 17, 2025

Choose a reason for hiding this comment

Uh oh!

mguetschow Jul 18, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

riot-ci commented Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Murdock results

Artifacts

Uh oh!

mguetschow commented Jul 18, 2025

Uh oh!

Uh oh!

carl-tud commented Jul 18, 2025

Uh oh!

mguetschow commented Jul 18, 2025

Uh oh!

mguetschow commented Jul 23, 2025

Uh oh!

Teufelchen1 left a comment

Choose a reason for hiding this comment

Uh oh!

Teufelchen1 commented Jul 24, 2025

Uh oh!

Teufelchen1 commented Jul 24, 2025

Uh oh!

carl-tud left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Teufelchen1 commented Jul 24, 2025

Uh oh!

mguetschow commented Jul 24, 2025

Uh oh!

crasbe commented Jul 24, 2025

Uh oh!

crasbe commented Jul 24, 2025

Uh oh!

crasbe commented Jul 24, 2025

Uh oh!

mguetschow commented Jul 24, 2025

Uh oh!

Uh oh!

mguetschow commented Jul 24, 2025

Uh oh!

mguetschow commented Jul 17, 2025 •

edited

Loading

carl-tud commented Jul 17, 2025 •

edited

Loading

carl-tud commented Jul 17, 2025 •

edited

Loading

riot-ci commented Jul 17, 2025 •

edited

Loading