fix baremetal runtime: rename malloc and co. export names

[hectorchu]

The original intention was the Go code would use CGo to call into C code, and the exports would allow C code to call malloc without needing modification. But calling the export malloc is wrong as it clashes when linking the Go code as a static library to esp-idf C environment. Moreover, C.CString calls malloc to allocate memory to return to C. I have found that it links the libc_malloc function instead. This means C.CString returns GC-allocated memory to C, so if C calls free on that, it bombs. I think a better name would be tinygo_malloc. And if the aim was to avoid modification of existing C code, it is very easy for the developer to add the aliasing function on the C side.

[deadprogram]

Thanks for working on this @hectorchu

A few problems:

• This breaks C.CString as well as all other CGo malloc usage on baremetal.
• Specifically when calling C.malloc() on baremetal the linker needs to find a symbol named malloc. This PR removes that symbol entirely.
• It also causes some problems in wasm
• No tests

Since I think the problem you are actually trying to solve is related to the esp-idf integration, perhaps you might try this instead:

• Use __attribute__((weak)) (see espradio for examples of this)
• Use build tag like //go:build baremetal && !esp
• Use linker --wrap=malloc to redirect. We are already doing this here https://github.com/tinygo-org/tinygo/blob/dev/targets/esp32s3.json#L11
• Only rename for esp targets

[aykevl]

TinyGo includes a C library, and also a C heap implementation using the Go heap (and therefore provides malloc/free to C). In this mode, you can't link to an external C library like the one in ESP-IDF - a small change like this might get it to work but it will eventually break down the line.

If you want to link to a different C library, TinyGo should be made to not use picolibc but instead use the external C library. We don't really support this right now - on all platforms we use our own C library or at least our own headers (which we use to link to a system C library on MacOS and Windows).

[hectorchu]

TinyGo includes a C library, and also a C heap implementation using the Go heap (and therefore provides malloc/free to C). In this mode, you can't link to an external C library like the one in ESP-IDF - a small change like this might get it to work but it will eventually break down the line.

If you want to link to a different C library, TinyGo should be made to not use picolibc but instead use the external C library. We don't really support this right now - on all platforms we use our own C library or at least our own headers (which we use to link to a system C library on MacOS and Windows).

Ah, but in my buildmode=c-archive PR (#5316), I skip linking of the C library, so I don't pick up any problems to do with clashing with symbols in that.

[hectorchu]

Once you remove the libc.a and rename the exports in baremetal.go, the malloc references resolve to the esp-idf one when linking the complete static library into the esp-idf project. It does work. And it isn't fragile.

[hectorchu]

• This breaks C.CString as well as all other CGo malloc usage on baremetal.
• Specifically when calling C.malloc() on baremetal the linker needs to find a symbol named malloc. This PR removes that symbol entirely.

I still think the way the current code does it is the wrong choice. Because malloc is supposed to not return GC-alloc'ed memory. But here C.CString returns memory from the GC heap - which is semantically wrong.

My suggestion was for consumer C code to define malloc themselves that just calls the exported libc_malloc.

[hectorchu]

The Go wiki says:

One important thing to remember is that C.CString() will allocate a new string of the appropriate length, and return it. That means the C string is not going to be garbage collected and it is up to you to free it.

It follows that the current TinyGo behaviour is to return GC-alloced memory. Which might get garbage collected when it goes out of scope. You don't need to call free on it.

[aykevl]

Because malloc is supposed to not return GC-alloc'ed memory. But here C.CString returns memory from the GC heap - which is semantically wrong.

And why do you say this? It is perfectly legal for a C compiler to return malloc'ed memory, in fact the GCC compiler itself has used a garbage collector internally for a long time (they might still do this, I haven't checked). The only requirement is that the memory remains available until a call to free, which is the case here since the TinyGo runtime is able to scan all the memory.

[aykevl]

You are proposing a rather radical change in how memory is managed for CGo in TinyGo, which needs a good explanation as to why this is needed and why it's still correct.

[hectorchu]

Okay so here is the failure mode I observed:

• originally, I linked my .a into the esp-idf project and got error: duplicate definition for malloc
• my first workaround was to set the tinygo's malloc to internal linkage. This fixed the esp-idf link
• in this configuration, C.CString returns GC memory
• the app_main.c calls a CGo exported function which returns the result of C.CString
• app_main.c later calls free() on this pointer. free() being the esp-idf version. It crashes.
• the memory may or may not be reclaimed by GC, depending on what stackTop is set to.

[hectorchu]

You are proposing a rather radical change in how memory is managed for CGo in TinyGo, which needs a good explanation as to why this is needed and why it's still correct.

I'm not actually suggesting CGo in TinyGo doesn't pass around GC memory. I'm just saying give some more flexibility here to accommodate different linking modes. As I said, if C-land wants to handle GC-alloc'ed memory, it should explicitly alias malloc to tinygo's malloc.

[hectorchu]

But, perhaps the correct way of looking at this is that buildmode=c-archive introduces a new "mode", but I'm not entirely sure how to "switch" on that "mode", since buildmode isn't a build tag, right?

[hectorchu]

In build.go, I'm wondering if its possible, for the "non c-archive" mode, to manually insert aliases from malloc to tinygo_malloc. In that location we definitely know whether its c-archive mode or not.

[aykevl]

This should definitely be part of #5316 and not done separately.

[hectorchu]

Ok I think I figured out the best workaround with this cute sequence:

	for _, name := range []string{"malloc", "calloc", "free"} {
		if fn := mod.NamedFunction(name); !fn.IsNil() {
			fn2 := llvm.AddFunction(mod, "__"+name, fn.Type())
			fn2.SetLinkage(llvm.ExternalLinkage)
			fn.ReplaceAllUsesWith(fn2)
			fn.EraseFromParentAsFunction()
			fn2.SetName(name)
		}
	}

Closing.

[hectorchu]

I hate to keep posting comments about this, but there is still the issue that two different functions are prefixed with //export malloc. See

https://github.com/tinygo-org/tinygo/blob/dev/src/runtime/baremetal.go#L41

and

https://github.com/tinygo-org/tinygo/blob/dev/src/runtime/runtime.go#L70

How is this legal in Go?

[deadprogram]

@hectorchu basically the //export directive does two things in TinyGo. It both creates the C-visible linker symbol and also links the Go-side declaration to the body.

[aykevl]

How is this legal in Go?

Why wouldn't it be legal in Go?

[hectorchu]

How is this legal in Go?

Why wouldn't it be legal in Go?

I'm no expert but I asked GPT and they said you can't have two different functions with the same export. But in this case, one of the functions is just a declaration not a definition. So in the end, I think malloc and libc_malloc end up as being "one". So maybe the law is "fuzzy" in this case.

[aykevl]

but I asked GPT

Yeah I don't believe anything these tools say, all they do is give you an answer that sounds vaguely correct even if it isn't.

Anyway, you already figured out the reason why this is possible in TinyGo:

But in this case, one of the functions is just a declaration not a definition.