Skip to content

Conversation

@HugoSilvaSantos
Copy link
Contributor

  • Draft PR

if(NOT ENABLE_VARIANTS STREQUAL "all")
foreach(expected_variant ${ENABLE_VARIANTS})
if(NOT expected_variant IN_LIST enabled_variants)
message(FATAL_ERROR "Variant name ${expected_variant} not found in ${MULTILIB_JSON}")
Copy link
Contributor

Choose a reason for hiding this comment

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

Improving the error messaging is probably better done in a separate PR, since this is unrelated to the llvm-libc work which is already a large enough change,

Comment on lines +123 to +126
# Allow callers to provide URL-encoded semicolons (\"%3B\") so that lists can
# be passed through shell commands without extra escaping.
string(REPLACE "%3B" ";" LLVM_TOOLCHAIN_LIBRARY_VARIANTS "${LLVM_TOOLCHAIN_LIBRARY_VARIANTS}")
string(REPLACE "%3b" ";" LLVM_TOOLCHAIN_LIBRARY_VARIANTS "${LLVM_TOOLCHAIN_LIBRARY_VARIANTS}")
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure why this is necessary since it sounds like a problem with the shell commands rather than the CMake. I don't think it would be a good idea to go down this road if it leads to trying to make every CMake list option support multiple delimiters, instead of fixing the callers.

@@ -167,11 +171,28 @@ set(
set(FVP_CONFIG_DIR "${CMAKE_CURRENT_SOURCE_DIR}/fvp/config")
set(LLVM_TOOLCHAIN_C_LIBRARY
"picolibc" CACHE STRING
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is retained as a legacy setting, then it shouldn't have a default setting to always enable it. Though it might be easier to simply make it throw an error to tell people to stop using it.

# If you want to stop cmake updating the repos then run
# cmake . -DFETCHCONTENT_FULLY_DISCONNECTED=ON
if(LLVM_TOOLCHAIN_C_LIBRARY STREQUAL picolibc)
if("picolibc" IN_LIST LLVM_TOOLCHAIN_ENABLED_LIBCS)
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than check the list, LLVM_TOOLCHAIN_ENABLE_PICOLIBC should already conatin whether picolibc is used or not.

endif()
list(LENGTH LLVM_TOOLCHAIN_ENABLED_LIBCS _overlay_libc_count)
if(NOT _overlay_libc_count EQUAL 1)
message(FATAL_ERROR
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think these restrictions are still needed. Eventually, a picolibc overlay may be needed, and having an overlay contain multiple libcs would also be covenient to generate for the same reason as a standard toolchain package.

include(${CMAKE_CURRENT_SOURCE_DIR}/cmake/fetch_picolibc.cmake)
endif()
if(LLVM_TOOLCHAIN_C_LIBRARY MATCHES "^newlib")
if("newlib" IN_LIST LLVM_TOOLCHAIN_ENABLED_LIBCS
Copy link
Contributor

Choose a reason for hiding this comment

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

Likewise checking LLVM_TOOLCHAIN_ENABLE_NEWLIB (and/or the LLVM_TOOLCHAIN_ENABLE_NEWLIB_NANO suggested above)

if(libc MATCHES "^newlib")
install(
FILES
${CMAKE_CURRENT_SOURCE_DIR}/ATfE-SBOM-newlib-overlay.spdx.json
Copy link
Contributor

Choose a reason for hiding this comment

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

The current SBOMs are hardcoded, so the overlay files may not be correct for non-overlay packages.

Copy link
Contributor

Choose a reason for hiding this comment

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

Overlay SBOM should cover the library only, today if one unpacks all overlays they would get the main SBOM + library SBOMs that are expected to be consistent. I think the same logic will apply to the combined package.
In the future, we may want to split picolibc out, when it can be an overlay.

Copy link
Contributor

Choose a reason for hiding this comment

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

At the moment, ATfE-SBOM.spdx.json covers both LLVM toolchain components and picolibc, so yes the picolibc parts will need splitting out since it's possible to build a package without it, but the rest of the SBOM covering the toolchain is still needed.

It may help to drop the 'overlay' part of the name, and simply add the required SBOMs based on whether each library is enabled, regardless of whether the overlay layout is used.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, ATfE-SBOM-<lib name>.spdx.json should be good, however this is a separate improvement.

DESTINATION
${LIBC_CFG_DIR}
)
endif()
Copy link
Contributor

Choose a reason for hiding this comment

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

If no match is found, it would be better to throw an error so that files aren't accidentally missed later if the names change,

set(PACKAGE_FILE_NAME ${PACKAGE_NAME}-${PACKAGE_VERSION}-${CPACK_SYSTEM_NAME})
else()
set(PACKAGE_FILE_NAME ${PACKAGE_NAME}_${LLVM_TOOLCHAIN_C_LIBRARY}-${PACKAGE_VERSION}-${CPACK_SYSTEM_NAME})
set(PACKAGE_FILE_NAME ${PACKAGE_NAME}_${LLVM_TOOLCHAIN_PRIMARY_LIBC}-${PACKAGE_VERSION}-${CPACK_SYSTEM_NAME})
Copy link
Contributor

Choose a reason for hiding this comment

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

A new naming scheme is needed, otherwise a package with picolibc+llvmlibc will have the same name as a picolibc+newlib package, or one that only includes picolibc

add_custom_target(check-${LLVM_TOOLCHAIN_C_LIBRARY})
set(LLVM_TOOLCHAIN_LIBC_CHECK_TARGETS "")
foreach(libc IN LISTS LLVM_TOOLCHAIN_ENABLED_LIBCS)
set(_libc_check_target check-${libc})
Copy link
Contributor

Choose a reason for hiding this comment

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

Having a single check-libc target to run all the libc tests would make more sense to a user. Then individual libc tests could be invoked through e.g. check-multilib-picolibc-libc or check-multilib-llvmlibc-libc?

Again we may want to settle on a better naming scheme since using the name of the libc as the multilib and the test target may quickly get confusing.

foreach(check_target check-${LLVM_TOOLCHAIN_C_LIBRARY} check-compiler-rt check-cxx check-cxxabi check-unwind)
foreach(libc IN LISTS LLVM_TOOLCHAIN_ENABLED_LIBCS)
set(multilib_project multilib-${libc})
set(libc_install_dir ${CMAKE_CURRENT_BINARY_DIR}/llvm/${TARGET_LIBRARIES_DIR}/${libc})
Copy link
Contributor

Choose a reason for hiding this comment

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

Only the libcs that are in addition to the default/primary one need to be placed in a subdirectory (unless an overlay is being generated). Likewise a configuration file won't be needed for every libc, sice the configuration file exists to select the non-default directory.

foreach(libc IN LISTS LLVM_TOOLCHAIN_ENABLED_LIBCS)
if(libc STREQUAL picolibc)
set(_libc_display_name "Picolibc")
set(_libc_license_names COPYING.NEWLIB COPYING.picolibc)
Copy link
Contributor

Choose a reason for hiding this comment

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

COPYING.NEWLIB was removed from picolibc, so there's no reason to try in include it.

foreach(libc IN LISTS LLVM_TOOLCHAIN_ENABLED_LIBCS)
if(libc STREQUAL picolibc)
set(libc_license_sources "")
set(_picolibc_newlib_license ${picolibc_SOURCE_DIR}/COPYING.NEWLIB)
Copy link
Contributor

Choose a reason for hiding this comment

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

As above, picolibc no longer has COPYING.NEWLIB in its sources, so the file will never be found and there's no reason to always trigger the warning.

Similarly defining the license file names above and then their paths+names again here in two separate loops seems unnecessary and it should be possible to combine them - and not rely on a string matching loop (the files needed should be determined by the CMake options)

Hugo Silva and others added 4 commits January 27, 2026 16:09
- Add enable flags per libc
- Fix COPYING.NEWLIB error when creating the atfe package
- Fix samples related issue
@github-actions github-actions bot added the downstream-change Downstream change to LLVM tree label Jan 27, 2026
@github-actions
Copy link

This pull review modifies files outside of the arm-software directory, so please ensure it follows the Downstream Patch Policy.
An automated check will test if the tagging requirements have been met. Please wait for approving reviews from both Arm Toolchain for Embedded and Arm Toolchain for Linux teams before merging.

@HugoSilvaSantos HugoSilvaSantos force-pushed the arm-software-libc-atfe-package branch from 793356a to 37e947a Compare January 27, 2026 16:38
@dcandler dcandler removed the downstream-change Downstream change to LLVM tree label Jan 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants