Move heap implementation to :platform:heap and make optional #364

salkinium · 2020-03-21T03:39:22Z

This moves the heap implementation from the :platform:core modules to it's own :platform:heap modules, which allows to build apps without dynamic memory at all (which is the vast majority of modm's examples). The Block allocator is moved to :driver:block.allocator where it belongs much better.

Unfortunately when not including the module and then calling malloc() or new, there aren't any compiler errors, only linker errors, often cryptic (relating to missing sbrk). Not sure if this is a great usibility improvement. But not including this code by default make a difference for tiny targets.

The goal is to add better memory allocators in future, since TLSF has a huge static overhead (several kBs) and newlib's allocator can't deal with mutiple memory regions and is pretty heavy handed too. The Block allocator should be retired.

Fixes #357, fixes #368.

TODO:

Find all examples that require :platform:heap
Linkstep error message when using malloc/new without implementation
Better documentation

rleh · 2020-03-21T11:51:36Z

Nice! 👍

calling malloc() or new, there aren't any compiler errors, only linker errors, often cryptic (relating to missing sbrk)

As far a I know you could explicitly delete the new operators.

Not sure if this is a great usability improvement.

Definitively not.
Couldn't we by default include the :platform:heap module (as dependency of :platform:core or :architecture:memory) unless the user disables heap via lbuild option?
I think it would save a lot of people from getting annoying mistakes.

salkinium · 2020-03-21T21:53:53Z

As far a I know you could explicitly delete the new operators.

GCC disagrees 🤨

In file included from modm/src/modm/platform.hpp:18,
                 from modm/src/modm/board/board.hpp:16,
                 from modm/src/modm/board.hpp:12,
                 from main.cpp:12:
modm/src/modm/heap/cortex/newdelete_noheap.hpp:15:7: error: deleted definition of 'void* operator new(std::size_t)'
   15 | void* operator new(std::size_t size) = delete;
      |       ^~~~~~~~
In file included from modm/src/modm/heap/cortex/newdelete_noheap.hpp:12,
                 from modm/src/modm/platform.hpp:18,
                 from modm/src/modm/board/board.hpp:16,
                 from modm/src/modm/board.hpp:12,
                 from main.cpp:12:
/usr/local/Cellar/arm-gcc-bin/9-2019-q4-major/arm-none-eabi/include/c++/9.2.1/new:125:26: note: previous declaration of 'void* operator new(std::size_t)'
  125 | _GLIBCXX_NODISCARD void* operator new(std::size_t) _GLIBCXX_THROW (std::bad_alloc)
      |                          ^~~~~~~~

salkinium · 2020-03-21T22:00:43Z

But I can mark the prototypes as deprected! And at least that way you get a good error message:

[[deprecated("Heap is not implemented!")]]
void* operator new(std::size_t size);
// all others new() types too

Results in

main.cpp: In function 'int main()':
main.cpp:30:34: warning: 'void* operator new [](std::size_t)' is deprecated: Heap is not implemented! [-Wdeprecated-declarations]
   30 |  uint32_t *ptr = new uint32_t[100];
      |                                  ^
In file included from modm/src/modm/platform.hpp:26,
                 from modm/src/modm/board/board.hpp:16,
                 from modm/src/modm/board.hpp:12,
                 from main.cpp:12:
modm/src/modm/platform/core/newdelete.hpp:18:7: note: declared here
   18 | void* operator new[](std::size_t size);
      |       ^~~~~~~~
main.cpp:30:34: warning: 'void* operator new [](std::size_t)' is deprecated: Heap is not implemented! [-Wdeprecated-declarations]
   30 |  uint32_t *ptr = new uint32_t[100];
      |                                  ^
In file included from modm/src/modm/platform.hpp:26,
                 from modm/src/modm/board/board.hpp:16,
                 from modm/src/modm/board.hpp:12,
                 from main.cpp:12:
modm/src/modm/platform/core/newdelete.hpp:18:7: note: declared here
   18 | void* operator new[](std::size_t size);
      |       ^~~~~~~~
main.cpp:30:12: warning: unused variable 'ptr' [-Wunused-variable]
   30 |  uint32_t *ptr = new uint32_t[100];
      |            ^~~

rleh · 2020-03-21T23:01:44Z

#pragma GCC poison malloc new

https://stackoverflow.com/questions/18365804/is-it-possible-to-completely-disable-the-default-c-new-operator/37823315#37823315

salkinium · 2020-03-22T04:59:59Z

#pragma GCC poison malloc new

Interesting, learned something new today.

However, it's actually worse than deprecation, I cannot add a message why it was poisened, the pragma doesn't allow adding a comment. So you get only this without context:

main.cpp:31:18: error: attempt to use poisoned "new"
   31 |  uint32_t *ptr = new uint32_t[100];
      |                  ^
main.cpp:32:15: error: attempt to use poisoned "malloc"
   32 |  void *cptr = malloc(100);
      |               ^

You can however still combine it with the deprecation notice, so that it doesn't compile (what we want) and shows why (also what we want).

There's just one problem, the pragma poisons every call to malloc and new including calls that are never used. This is an issue with header files, say I want to use std::addressof or some other utility from <memory> in a modm module, the header may get included after the #pragma declaration like so:

#pragma GCC poison malloc new // included unknowingly via <modm/platform.hpp> for example
#include <memory>

Results in this shitshow:

In file included from /usr/local/Cellar/arm-gcc-bin/9-2019-q4-major/arm-none-eabi/include/c++/9.2.1/memory:64,
                 from main.cpp:13:
/usr/local/Cellar/arm-gcc-bin/9-2019-q4-major/arm-none-eabi/include/c++/9.2.1/bits/stl_construct.h:75:9: error: attempt to use poisoned "new"
   75 |     { ::new(static_cast<void*>(__p)) _T1(std::forward<_Args>(__args)...); }
      |         ^
/usr/local/Cellar/arm-gcc-bin/9-2019-q4-major/arm-none-eabi/include/c++/9.2.1/bits/stl_construct.h:90:9: error: attempt to use poisoned "new"
   90 |     { ::new(static_cast<void*>(__p)) _T1; }
      |         ^
// ... many more lines of this ...

This doesn't happen with the deprecation notice, it only warns about used calls (or, rather non-optimized out calls).

However, even if you could use this #pragma, you may still link against the static libraries of newlib or stdlibc++ that call malloc/new, and this isn't cought neither with poison NOR with deprecation. Example:

int res = at_quick_exit([](){/*tumbleweed*/});

Results in:

Linking········ modm/build/nucleo_f103rb/blink/release/blink.elf
arm-none-eabi/bin/ld: arm-none-eabi/lib/thumb/v7-m/nofp/libnosys.a(sbrk.o): in function `_sbrk':
sbrk.c:(.text._sbrk+0x18): undefined reference to `end'
collect2: error: ld returned 1 exit status

However, since I had some trouble finding a libc function that actually uses malloc (and arguably this one isn't applicable for embedded systems) I assume this case is fairly rare, and the stdlibc++ too is mostly in headers and little in pure source code, so most of it won't be linked in either.

What I would do now is:

Use deprecation only, it gets included with <modm/platform.hpp>, which you must include somewhere in your application.
Add the linker errors in the description of :platform:heap, so that you can find it through via lbuild search and the Doxypress and homepage search fields.

cc @chris-durand You're more knowledgeable of the stdlibc++, any better ideas?

salkinium · 2020-03-22T06:15:14Z

This doesn't happen with the deprecation notice, it only warns about used calls (or, rather non-optimized out calls).

Actually it does happen, it's just that for system headers, the warnings are ignored.
So for example, including the modm:utils headers but not using them results in this:

In file included from modm/src/modm/utils/allocator.hpp:17,
                 from modm/src/modm/container/dynamic_array.hpp:20,
                 from modm/src/modm/math/geometry/point_set_2d.hpp:17,
                 from modm/src/modm/math/geometry/circle_2d.hpp:20,
                 from modm/src/modm/math/geometry.hpp:18,
                 from main.cpp:17:
modm/src/modm/utils/allocator/dynamic.hpp: In member function 'T* modm::allocator::Dynamic<T>::allocate(size_t)':
modm/src/modm/utils/allocator/dynamic.hpp:64:56: warning: 'void* operator new(std::size_t)' is deprecated: Heap is not implemented! Please include the `:platform:heap` module in your project! [-Wdeprecated-declarations]
   64 |     return static_cast<T*>(::operator new(n * sizeof(T)));
      |                                                        ^
In file included from modm/src/modm/platform.hpp:26,
                 from modm/src/modm/board/board.hpp:18,
                 from modm/src/modm/board.hpp:12,
                 from main.cpp:16:
modm/src/modm/platform/core/no_heap.hpp:18:7: note: declared here
   18 | void* operator new(std::size_t);
      |       ^~~~~~~~

So this is a false positive… Maybe we can inject error messages into the linker? 😓

Furthermore, code that doesn't include the deprecation or #pragma header inside modm compile fine of course. The :freertos module doesn't include anything of course, so the link step fails with an error message.

salkinium · 2020-03-22T06:28:07Z

Maybe we can inject error messages into the linker?

🤔🤔🤔🤔🤔🤔 What if…

modm_section(".Heap_is_not_implemented__Please_include_the__platform_heap__module_in_your_project")
void* operator new(std::size_t) { return (void*)-1; }

And in the linkerscript:

/DISCARD/ :
{
	*(.Heap_is_not_implemented__Please_include_the__platform_heap__module_in_your_project)
}

Results in:

Linking········ modm/build/nucleo_f103rb/blink/release/blink.elf
`_Znaj' referenced in section `.text.startup.main' of modm/build/nucleo_f103rb/blink/release/main.o: defined in discarded section `.Heap_is_not_implemented__Please_include_the__platform_heap__module_in_your_project' of modm/build/nucleo_f103rb/blink/release/modm/libmodm.a(no_heap.o)
collect2: error: ld returned 1 exit status

It… works? I should not be allowed near C++…

salkinium · 2020-03-22T09:58:20Z

src/modm/platform/heap/cortex/no_heap.c.in

+
+// ----------------------------------------------------------------------------
+modm_section("{{ no_heap_section }}") modm_weak
+void* _sbrk_r(struct _reent *r, ptrdiff_t size)


This is actually perfect, since _sbrk_r is not implemented in newlib at all, so I can just implement it here as a weak function with just the section attribute and no other linker flags (particularly no function wrapping).

This means the user can supply their own malloc implementation if they do not include the :platform:heap module, by either just simply overwriting _sbrk_r to use the default newlib allocator or by additionally wrapping the _malloc_r etc function via the project collectors settings.

chris-durand · 2020-03-22T13:11:21Z

cc @chris-durand You're more knowledgeable of the stdlibc++, any better ideas?

I don't have any better ideas.

salkinium · 2020-03-23T04:04:36Z

examples/nucleo_g071rb/custom_allocator/main.cpp

+    	while(1) ;
+    }
+    return (void*) heap;
+}


This is now all that's needed to define your own heap region.

salkinium · 2020-03-23T04:46:40Z

ext/gcc/new_delete.cpp.in

+		 * null pointer, at which time it throws std::bad_alloc.
+		 */
+		if (std::get_new_handler()) std::get_new_handler()();
+		else break;


@chris-durand is this correctly implemented?

salkinium · 2020-03-23T04:58:58Z

I'm done now, and tested this on a bunch of hardware.

salkinium · 2020-03-24T07:04:04Z

Absent of dissenting reviews, I'll be merging this this afternoon.

salkinium added advanced 🤯 feature 🚧 labels Mar 21, 2020

chris-durand mentioned this pull request Mar 22, 2020

[c++] Recovery from out-of-heap-memory condition is not possible #368

Closed

salkinium force-pushed the feature/heap branch 2 times, most recently from 7b29a04 to 1f9b3de Compare March 22, 2020 09:53

salkinium commented Mar 22, 2020

View reviewed changes

salkinium force-pushed the feature/heap branch 5 times, most recently from 430fa3b to 4e64b7a Compare March 22, 2020 10:51

[driver] Move BlockAllocator out of :architecture:heap

e42f52a

salkinium force-pushed the feature/heap branch 4 times, most recently from 63cef92 to a12bf3f Compare March 23, 2020 02:55

salkinium marked this pull request as ready for review March 23, 2020 02:57

salkinium force-pushed the feature/heap branch from a12bf3f to 0a061d7 Compare March 23, 2020 03:10

salkinium commented Mar 23, 2020

View reviewed changes

salkinium force-pushed the feature/heap branch from 0a061d7 to 1b189c2 Compare March 23, 2020 04:45

salkinium commented Mar 23, 2020

View reviewed changes

[platform] Move heap to :platform:heap module

5bd3596

salkinium added 4 commits March 23, 2020 05:57

[platform] Compute heap defaults from device file

4272570

[heap] Add nothrow new with MemoryTraits

611abc6

[examples] Adapt all and add allocator example

ae6316a

[ci] Always force upgrade all modm dependencies

83e5374

salkinium force-pushed the feature/heap branch from 1b189c2 to 83e5374 Compare March 23, 2020 04:57

salkinium merged commit b8648be into modm-io:develop Mar 25, 2020

salkinium deleted the feature/heap branch March 25, 2020 03:13

rleh mentioned this pull request Apr 29, 2020

STM32 DMA rework #371

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move heap implementation to :platform:heap and make optional #364

Move heap implementation to :platform:heap and make optional #364

salkinium commented Mar 21, 2020 •

edited

Loading

rleh commented Mar 21, 2020

salkinium commented Mar 21, 2020

salkinium commented Mar 21, 2020

rleh commented Mar 21, 2020

salkinium commented Mar 22, 2020 •

edited

Loading

salkinium commented Mar 22, 2020 •

edited

Loading

salkinium commented Mar 22, 2020 •

edited

Loading

salkinium Mar 22, 2020

chris-durand commented Mar 22, 2020

salkinium Mar 23, 2020

salkinium Mar 23, 2020

salkinium commented Mar 23, 2020

salkinium commented Mar 24, 2020

Move heap implementation to :platform:heap and make optional #364

Move heap implementation to :platform:heap and make optional #364

Conversation

salkinium commented Mar 21, 2020 • edited Loading

rleh commented Mar 21, 2020

salkinium commented Mar 21, 2020

salkinium commented Mar 21, 2020

rleh commented Mar 21, 2020

salkinium commented Mar 22, 2020 • edited Loading

salkinium commented Mar 22, 2020 • edited Loading

salkinium commented Mar 22, 2020 • edited Loading

salkinium Mar 22, 2020

Choose a reason for hiding this comment

chris-durand commented Mar 22, 2020

salkinium Mar 23, 2020

Choose a reason for hiding this comment

salkinium Mar 23, 2020

Choose a reason for hiding this comment

salkinium commented Mar 23, 2020

salkinium commented Mar 24, 2020

salkinium commented Mar 21, 2020 •

edited

Loading

salkinium commented Mar 22, 2020 •

edited

Loading

salkinium commented Mar 22, 2020 •

edited

Loading

salkinium commented Mar 22, 2020 •

edited

Loading