Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

xt-clang: support bundled newlib #59291

Closed
wants to merge 10 commits into from

Conversation

dcpleung
Copy link
Member

This PR contains commits to support the (very old) newlib bundled with Xtensa LLVM toolchain (xt-clang). See individual commits for details.

@dcpleung
Copy link
Member Author

Compliance check failure is due to multi-line #if where the second line starts with spaces.

@dcpleung
Copy link
Member Author

The newlib in xt-clang is too broken to support Zephyr. So closing it for now...

@dcpleung dcpleung closed this Jul 19, 2023
* there are no corresponding *_DECLARED macros, so we need to define
* them manually to avoid them being re-declared.
*/
#define HAS__TIMESPEC_H 0
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Vague concerns about a name in the 'application' namespace, but prefixing it with '_' would risk conflicting with a C library define.

@@ -23,6 +23,8 @@
#include <zephyr/sys/mem_manage.h>
#include <sys/time.h>

#include <newlib.h>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Surely one of the above headers already includes this? Odd, if not. But, nbd in any case.

@@ -303,10 +305,8 @@ void *_sbrk(intptr_t count)
}
__weak FUNC_ALIAS(_sbrk, sbrk, void *);

#ifdef CONFIG_MULTITHREADING
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uh, this isn't good -- what does this old version of newlib do for locking? Seems like this will cause all kinds of trouble if multiple threads call into stdio or anything else in newlib that requires locking.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I have read, old newlib is not really thread safe. That's why they introduced the retargetable locks.

@@ -450,7 +450,21 @@ void __retarget_lock_release_recursive(_LOCK_T lock)
__ASSERT_NO_MSG(lock != NULL);
k_mutex_unlock((struct k_mutex *)lock);
}
#endif /* CONFIG_MULTITHREADING */
#else /* CONFIG_MULTITHREADING && _RETARGETABLE_LOCKING */
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want to look into sharing the common malloc implementation instead of using the newlib one? I realize it's not directly aligned with your goals here, but I don't know anyone else poking at newlib code...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cannot find a definite way to exclude a function from a static library during linking. IIRC, providing multiple implementation may result in the linker either complaining, or picking one depending on linking order. If it is latter, cmake scripts may need to be changed to ensure ordering all the time.

Copy link
Collaborator

@keith-packard keith-packard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've got a couple of questions, but no major blockers here. Thanks for figuring this out.

@dcpleung
Copy link
Member Author

dcpleung commented Jul 19, 2023

From what I can tell, the newlib in xt-clang is very old and that there are various issues... After given it some thoughs, I think we should not re-introduce the old behavior here. Given that we have picolibc, we should be using that instead, IMHO.

@keith-packard
Copy link
Collaborator

Yeah, iirc, you had some adventures with that too. Good luck!

Add a board config file for intel_ace15_mtpm which is simply
a copy from intel_adsp_cavs25.

Signed-off-by: Daniel Leung <[email protected]>
@dcpleung
Copy link
Member Author

dcpleung commented Aug 2, 2023

Need to re-open this... this is needed to use the profiling tools provided by Cadence.

Xtensa xt-clang has very old newlib (possibly 2.0.0) which
does not have sys/_timespec.h, and also various typedefs
done. So we need to avoid including non-existing header
file, and avoid re-declaring the typedefs.

Signed-off-by: Daniel Leung <[email protected]>
...for old newlib version.

This is needed to use the newlib bundled with Xtensa xt-clang
toolchain, which predates the introduction of retargetable
locking.

Signed-off-by: Daniel Leung <[email protected]>
The newlib bundled with Xtensa xt-clang is an old version
which predates the introduction of retargatable locking.
So we need to skip all the locking tests but retains
the stress tests if xt-clang is used.

Signed-off-by: Daniel Leung <[email protected]>
Typecast the arguments to both toupper() and tolower() to int.
Some ancient newlib has a way to generate warning with -Wall
so we need to be strict about the type of argument.

Signed-off-by: Daniel Leung <[email protected]>
xt-clang and its ancient newlib define uintptr_t as unsigned int
instead of unsigned long which results in compiler complaining
about any logging using %lx for uintptr_t of different type.
So use the toolchain provided PRIxPTR and PRIuPTR for proper
formatting.

Signed-off-by: Daniel Leung <[email protected]>
Clang would generate floating pointer instructions to process
floats as there usually is no soft float support from host or
vendor LLVM toolchain. Without CONFIG_FPU enabled, the platform
runs assuming no floating point instructions would be used, and
encountering such instructions would result in exception. So
skip this test if CONFIG_FPU is not enabled to avoid such
exceptions.

Signed-off-by: Daniel Leung <[email protected]>
Clang would generate floating pointer instructions to process
floats as there usually is no soft float support from host or
vendor LLVM toolchain. Without CONFIG_FPU enabled, the platform
runs assuming no floating point instructions would be used, and
encountering such instructions would result in exception. So
only print float if CONFIG_FPU is enabled to avoid such
exceptions.

Signed-off-by: Daniel Leung <[email protected]>
This introduces CONFIG_NEWLIB_LIBC_IN_TOOLCHAIN_IS_ANCIENT
to indicate if the newlib coming from the toolchain is of
very old verion (usually predates the retargetable locks).
There are various changes needed to support ancient newlib
with regard to function prototypes, data types and header
files.

Signed-off-by: Daniel Leung <[email protected]>
This sets the variable TOOLCHAIN_HAS_NEWLIB to true for
xt-clang so we can use the built-in newlib provided by
the Xtensa toolchain.

Note that C++ STL support with newlib is broken (same
as xcc), so exclude xt-clang from the libcxx tests
for now.

Signed-off-by: Daniel Leung <[email protected]>
@dcpleung dcpleung closed this Sep 6, 2023
@dcpleung dcpleung deleted the xcc/newlib branch September 6, 2023 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants