Re: arm-none-eabi, nested function trampolines and caching

[Ed Robbins <edd.robbins@googlemail.com>]

On Tue, 28 Nov 2023 at 18:00, David Brown <david.brown@hesbynett.no> wrote:
>
> On 28/11/2023 10:51, Ed Robbins via Gcc-help wrote:
> > On Tue, 28 Nov 2023 at 07:21, Matthias Pfaller <leo@marco.de> wrote:
> >>
> >> On 2023-11-27 16:16, Ed Robbins via Gcc-help wrote:
> >>> Hello,
> >>> I am using gcc-arm-none-eabi with a cortex M7 device, with caches
> >>> (data/instruction) enabled. Nested function calls result in a usage fault
> >>> because there is no clear cache call for this platform.
> >>>
>
> >> I have lots of code with nested functions. When switching to gcc-12 I got random
> >> crashes on my cortex-m7 targets. In order to get that working again I had to patch
> >> gcc/config/arm/arm.h:
> >>
>
>
> Can I ask (either or both of you) why you are using are using nested
> functions like this?  This is possibly the first time I have heard of
> anyone using them, certainly the first time in embedded development.
> Even when I programmed in Pascal, where nested functions are part of the
> language, I did not use them more than a couple of times.
>
> What benefit do you see in nested functions in C, compared to having
> separate functions?  Have you considered moving to C++ and using
> lambdas, which are more flexible, standard, and can be very much more
> efficient?
>
> This is, of course, straying from the topicality of this mailing list.
> But I am curious, and I doubt if I am the only one who is.
>
> David
>

In the simplest case you may want to do something like:

struct sometype* find_thing_with_value(struct list *things, uint32_t value) {
    bool match(void *thing) {
        return ((struct sometype*)thing)->field == value;
    }
    return (struct sometype*)list_find(things, &match);
}

In general though dependency inversion can be quite powerful. We use
nested functions that do not access local variables fairly frequently
in our codebase, mostly as a scoping mechanism. We would like to be
able to access locals, and are aware of the implications, but only
very recently decided to address the issue.

I think that rather than switch to C++ lambdas we would be more
inclined to move to clang blocks and stick with C. But there is no
impetus for either move: Nested functions are just fine for what we
need to do, and the syntax is very clean. We have thoughts about a HLL
shift but it wouldn't be to C++.

Security implications are not an issue for us because there is no way
for an outsider to communicate with the devices, let alone get a stack
overflow and code execution.

I think that it would be sensible if there was a -mflush_func option
for ARM targets, as for MIPS, which would resolve the caching issues
that require a patched gcc build in this case.

Thanks to Matthias for guidance. I have ended up implementing this a
bit differently, as in the patch below, as it's then totally
self-contained. I've tested by rebuilding the toolchain using the
script from [1] and it is working well.

Best regards,
Ed

[1] https://github.com/FreddieChopin/bleeding-edge-toolchain/tree/master

From 569177e29cdc777e84e5e8cf633ebef761e82f83 Mon Sep 17 00:00:00 2001
From: Ed Robbins <edd.robbins@gmail.com>
Date: Tue, 5 Dec 2023 16:56:36 +0000
Subject: [PATCH] Define CLEAR_INSN_CACHE in arm.h and implement for v7em
 targets

---
 gcc/config/arm/arm.h | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/gcc/config/arm/arm.h b/gcc/config/arm/arm.h
index f479540812a..666d98611bc 100644
--- a/gcc/config/arm/arm.h
+++ b/gcc/config/arm/arm.h
@@ -2518,4 +2518,37 @@ const char *arm_be8_option (int argc, const char **argv);
    representation for SHF_ARM_PURECODE in GCC.  */
 #define SECTION_ARM_PURECODE SECTION_MACH_DEP

+#ifndef CLEAR_INSN_CACHE
+/* When defined CLEAR_INSN_CACHE is called by __clear_cache in libgcc/libgcc2.c
+   It needs to always be _defined_, otherwise
maybe_emit_call_builtin___clear_cache
+   from gcc/builtins.cc will not generate a call to __clear_cache, however we
+   only want it _implemented_ for the multilib version of libgcc.a built for
+   v7em targets. */
+#ifdef __ARM_ARCH_7EM__
+#define CLEAR_INSN_CACHE(BEG, END) \
+ { \
+ const void *scb_base = (const void*)0xe000e000; \
+ const unsigned dccmvac_offset = 0x268; \
+ const unsigned icimvau_offset = 0x258; \
+ const unsigned cache_line_size = 32; \
+ void *addr = (void*)((unsigned)BEG & ~(cache_line_size - 1)); \
+ void *end = (void*)((unsigned)(END + cache_line_size - 1) &
~(cache_line_size - 1)); \
+ __asm__ __volatile__("dsb" : : : "memory"); \
+ while (addr < end) { \
+ *(unsigned**)(scb_base + dccmvac_offset) = addr; \
+ addr += cache_line_size; \
+ } \
+ __asm__ __volatile__("dsb; isb" : : : "memory"); \
+ addr = (void*)((unsigned)BEG & ~(cache_line_size - 1)); \
+ while (addr < end) { \
+ *(unsigned**)(scb_base + icimvau_offset) = addr; \
+ addr += cache_line_size; \
+ } \
+ __asm__ __volatile__("dsb; isb" : : : "memory"); \
+ }
+#else
+#define CLEAR_INSN_CACHE(BEG, END) ;
+#endif
+#endif
+
 #endif /* ! GCC_ARM_H */
-- 
2.34.1