Add LLVM libc sample #538

smithp35 · 2024-10-16T15:30:39Z

This will be built into the LLVM libc overlay package. Sample installs to samples/src/llvmlibc/baremetal-semihosting.

Add llvmlibc-samples directory to contain LLVM libc specific samples.
Add single baremetal-semihosting sample using LLVM libc.

Sample contains a number of calls to LLVM libc functionality:

heap.
printf.
string library.
math library.

A special linker script is used to set the bounds of the heap. This uses different linker defined symbols to picolibc.

The crt0.S from the existing llvmlibc-support can't be used directly as the RW and ZI from the linker script need initializing.

This will be built into the LLVM libc overlay package. Sample installs to samples/src/llvmlibc/baremetal-semihosting. * Add llvmlibc-samples directory to contain LLVM libc specific samples. * Add single baremetal-semihosting sample using LLVM libc. Sample contains a number of calls to LLVM libc functionality: * heap. * printf. * string library. * math library. A special linker script is used to set the bounds of the heap. This uses different linker defined symbols to picolibc. The crt0.S from the existing llvmlibc-support can't be used directly as the RW and ZI from the linker script need initializing.

voltur01 · 2024-10-16T15:38:38Z

llvmlibc-samples/src/llvmlibc/baremetal-semihosting/Makefile

@@ -0,0 +1,37 @@
+#
+# Copyright (c) 2020-2023, Arm Limited and affiliates.


Suggested change

# Copyright (c) 2020-2023, Arm Limited and affiliates.

# Copyright (c) 2020-2024, Arm Limited and affiliates.

OK, I'll add this change in. Given I'm making other changes I'll update in a follow up commit.

voltur01 · 2024-10-16T15:41:19Z

llvmlibc-samples/src/llvmlibc/baremetal-semihosting/microbit-llvmlibc.ld

+/*
+ * SPDX-License-Identifier: BSD-3-Clause
+ *
+ * Copyright © 2019 Keith Packard


Do we need to add Arm copyright as well? Presumably this was tweaked to meet the needs of the sample.

It was a trivial modification, but it is probably best to add the additional copyright anyway.

pratlucas · 2024-10-16T15:41:36Z

What's the motivation for using a separate directory structure instead of placing the new samples inside the existing samples directory?

smithp35 · 2024-10-16T15:51:01Z

What's the motivation for using a separate directory structure instead of placing the new samples inside the existing samples directory?

A couple of reasons, neither particularly strong:

Easier to avoid name clashing with the existing picolibc samples as the overlay package is developed separately. This could be resolved by explicitly adding llvmlibc to each sample name, for example baremetal-semihosting-llvmlibc.
It made it easier for me to remove the llvm-libc samples when repeatedly testing the package, but I think that's resolvable.

I'm open to opinions on what's best?

* Update copyright date on Makefile. * Add additional copyright message on linker script. * Add missing ) in half-open interval comment in linker script.

statham-arm · 2024-10-17T08:37:24Z

llvmlibc-samples/src/llvmlibc/baremetal-semihosting/crt0llvmlibc.c

+extern char __tls_end[];
+
+void _start(void) {
+  memcpy(__data_start, __data_source, (uintptr_t) __data_size);


Pedantry: formally, memcpy and memset take a size_t, not a uintptr_t.

Thanks will fix.

statham-arm · 2024-10-17T08:41:34Z

llvmlibc-samples/src/llvmlibc/baremetal-semihosting/hello.c

+#include <string.h>
+#include <math.h>
+
+/* Example that uses heap, string and math library */


I'm not sure you have used the math library, have you? I can only see a call to abs, which is defined in stdlib.h, not math.h.

The problem with the actual math.h functions in embedded LLVM libc is that they won't make a good demo, because you can't printf the floating-point results, because FP printf is left out in the embedded build profile!

I suppose if you really wanted to demonstrate math.h functions, you could do something that has an integer final result, along the lines of lround(400000 * atan(1)) to get 314159. But it's probably also worth adding a comment explaining why you picked that example, so that when the user inevitably tries tweaking it to printf a floating-point value, they're warned why it won't work.

That will teach me for using the list of math functions in cppreference. Was trying to find one that returned an integer.

I've used atanf as llvm-libc doesn't define atan. This also showed up that I hadn't defined int *__llvm_libc_errno() so I've added that to the docs aswell.

pratlucas · 2024-10-17T09:16:22Z

What's the motivation for using a separate directory structure instead of placing the new samples inside the existing samples directory?

A couple of reasons, neither particularly strong:

Easier to avoid name clashing with the existing picolibc samples as the overlay package is developed separately. This could be resolved by explicitly adding llvmlibc to each sample name, for example baremetal-semihosting-llvmlibc.

It made it easier for me to remove the llvm-libc samples when repeatedly testing the package, but I think that's resolvable.

I'm open to opinions on what's best?

For consistency I'd suggest keeping everything under samples. It also helps to keep the directory structure simple and clear.

Could we maybe have separate sub-sirectories inside samples for each library package we provide? E.g.:

LLVM-embedded-toolchain-for-Arm/
├─ samples/
│  ├─ picolibc/
│  ├─ newlib/
│  ├─ llvmlibc/
├─ ...

* Use a function from the math library, rather than a math like function not defined in math.h. * Use size_t in memcpy/memset in startup code. * Define int *__llvm_libc_errno() as math library function requires it. * Add to documentation that you may need to define errno.

smithp35 · 2024-10-17T10:42:16Z

What's the motivation for using a separate directory structure instead of placing the new samples inside the existing samples directory?

A couple of reasons, neither particularly strong:

Easier to avoid name clashing with the existing picolibc samples as the overlay package is developed separately. This could be resolved by explicitly adding llvmlibc to each sample name, for example baremetal-semihosting-llvmlibc.

It made it easier for me to remove the llvm-libc samples when repeatedly testing the package, but I think that's resolvable.

I'm open to opinions on what's best?

For consistency I'd suggest keeping everything under samples. It also helps to keep the directory structure simple and clear.

Could we maybe have separate sub-sirectories inside samples for each library package we provide? E.g.:
LLVM-embedded-toolchain-for-Arm/
├─ samples/
│  ├─ picolibc/
│  ├─ newlib/
│  ├─ llvmlibc/
├─ ...

Are you OK with me doing that in a follow up change as this one currently only affects the overlay package?

statham-arm · 2024-10-17T10:56:06Z

llvmlibc-samples/src/llvmlibc/baremetal-semihosting/hello.c

-/* Example that uses heap, string and math library */
+// Implementation of errno
+int *__llvm_libc_errno() {
+  static int errno;


Sorry to do another pedantic one, but I recommend calling that internal variable something other than errno. If a user starts from this code and adds #include <errno.h>, then the name errno will surely become a macro, making this function into nonsense. Any other name is fine.

Thanks for spotting. I've used a name that doesn't contain errno.

Not relevant for the example but could occur if #include <errno.h> is added.

statham-arm

Thanks for the fixes. I'm happy with this now, but of course other reviewers have been commenting on entirely different aspects of the patch.

statham-arm · 2024-10-17T12:54:52Z

llvmlibc-samples/src/llvmlibc/baremetal-semihosting/hello.c

+  strncat(out_s, world_s, world_s_len + 1);
+  // 2024-10-17 Embedded printf implementation does not currently
+  // support printing floating point numbers.
+  printf("%s %li\n", out_s, lround(400000 * atanf(1.0f)));


Because you wrote lround rather than lroundf, the float returned from atanf is being converted into a double, so this code is testing a mix of single and double precision operations.

I think that's actually a good thing – it makes it a better test! I point it out because you probably didn't do it on purpose, but I quite like it, and perhaps it's better not to "fix" it :-)

voltur01

LGTM too, thank you!

pratlucas

Are you OK with me doing that in a follow up change as this one currently only affects the overlay package?

Yes, that's ok. LGTM.

voltur01 reviewed Oct 16, 2024

View reviewed changes

Address review comments

aab9590

* Update copyright date on Makefile. * Add additional copyright message on linker script. * Add missing ) in half-open interval comment in linker script.

statham-arm reviewed Oct 17, 2024

View reviewed changes

rename errno to avoid clash if errno is a macro.

ffa667c

Not relevant for the example but could occur if #include <errno.h> is added.

statham-arm approved these changes Oct 17, 2024

View reviewed changes

voltur01 approved these changes Oct 18, 2024

View reviewed changes

pratlucas approved these changes Oct 18, 2024

View reviewed changes

smithp35 merged commit 8b733dd into ARM-software:main Oct 18, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add LLVM libc sample #538

Add LLVM libc sample #538

smithp35 commented Oct 16, 2024

voltur01 Oct 16, 2024

smithp35 Oct 16, 2024

voltur01 Oct 16, 2024 •

edited

Loading

smithp35 Oct 16, 2024

pratlucas commented Oct 16, 2024

smithp35 commented Oct 16, 2024 •

edited

Loading

statham-arm Oct 17, 2024

smithp35 Oct 17, 2024

statham-arm Oct 17, 2024

smithp35 Oct 17, 2024

pratlucas commented Oct 17, 2024

smithp35 commented Oct 17, 2024

statham-arm Oct 17, 2024

smithp35 Oct 17, 2024

statham-arm left a comment

statham-arm Oct 17, 2024

voltur01 left a comment

pratlucas left a comment

		@@ -0,0 +1,37 @@
		#
		# Copyright (c) 2020-2023, Arm Limited and affiliates.

	# Copyright (c) 2020-2023, Arm Limited and affiliates.
	# Copyright (c) 2020-2024, Arm Limited and affiliates.

Add LLVM libc sample #538

Add LLVM libc sample #538

Conversation

smithp35 commented Oct 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

voltur01 Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pratlucas commented Oct 16, 2024

smithp35 commented Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pratlucas commented Oct 17, 2024

smithp35 commented Oct 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

statham-arm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

voltur01 left a comment

Choose a reason for hiding this comment

pratlucas left a comment

Choose a reason for hiding this comment

voltur01 Oct 16, 2024 •

edited

Loading

smithp35 commented Oct 16, 2024 •

edited

Loading