[libc] Fix printf config not working

[michaelrj-google]

The list of printf copts available in config.json wasn't working because
the printf_core subdirectory was included before the printf_copts
variable was defined, making it effectively nothing for the printf
internals. Additionally, the tests weren't respecting the flags so they
would cause the tests to fail. This patch reorders the cmake in src and
adds flag handling in test.

[llvmbot]

@llvm/pr-subscribers-libc

Changes

The list of printf copts available in config.json wasn't working because
the printf_core subdirectory was included before the printf_copts
variable was defined, making it effectively nothing for the printf
internals. Additionally, the tests weren't respecting the flags so they
would cause the tests to fail. This patch reorders the cmake in src and
adds flag handling in test.

---

Full diff: https://github.com/llvm/llvm-project/pull/66834.diff

2 Files Affected:

• (modified) libc/src/stdio/CMakeLists.txt (+3-3)
• (modified) libc/test/src/stdio/CMakeLists.txt (+12)

diff --git a/libc/src/stdio/CMakeLists.txt b/libc/src/stdio/CMakeLists.txt
index a13321d13722953..f3a75fb965c6e16 100644
--- a/libc/src/stdio/CMakeLists.txt
+++ b/libc/src/stdio/CMakeLists.txt
@@ -26,9 +26,6 @@ if(NOT LIBC_TARGET_ARCHITECTURE_IS_GPU)
   add_subdirectory(${CMAKE_CURRENT_SOURCE_DIR}/generic)
 endif()
 
-add_subdirectory(printf_core)
-add_subdirectory(scanf_core)
-
 add_entrypoint_object(
   fflush
   SRCS
@@ -286,6 +283,9 @@ add_entrypoint_object(
     ${printf_copts}
 )
 
+add_subdirectory(printf_core)
+add_subdirectory(scanf_core)
+
 add_entrypoint_object(
   ftell
   SRCS
diff --git a/libc/test/src/stdio/CMakeLists.txt b/libc/test/src/stdio/CMakeLists.txt
index e042a8bd8be68f2..98fa2deb8b0e258 100644
--- a/libc/test/src/stdio/CMakeLists.txt
+++ b/libc/test/src/stdio/CMakeLists.txt
@@ -112,6 +112,16 @@ add_libc_unittest(
     LibcMemoryHelpers
 )
 
+if(LIBC_CONF_PRINTF_DISABLE_FLOAT)
+  list(APPEND sprintf_test_copts "-DLIBC_COPT_PRINTF_DISABLE_FLOAT")
+endif()
+if(LIBC_CONF_PRINTF_DISABLE_INDEX_MODE)
+  list(APPEND sprintf_test_copts "-DLIBC_COPT_PRINTF_DISABLE_INDEX_MODE")
+endif()
+if(LIBC_CONF_PRINTF_DISABLE_WRITE_INT)
+  list(APPEND sprintf_test_copts "-DLIBC_COPT_PRINTF_DISABLE_WRITE_INT")
+endif()
+
 add_fp_unittest(
   sprintf_test
   UNIT_TEST_ONLY
@@ -123,6 +133,8 @@ add_fp_unittest(
     libc.src.stdio.sprintf
     libc.src.__support.FPUtil.fp_bits
     libc.src.__support.FPUtil.platform_defs
+  COMPILE_OPTIONS
+    ${sprintf_test_copts}
 )
 
 add_libc_unittest(

[sivachandra]

I do not think this pattern is how the copts are to be used. In this particular case, I think the right way would be to split sprintf_test into sprintf_basic_test, sprintf_float_test, sprintf_index_mode_test, sprintf_write_int_test etc. Then, test targets should be added as follows:

add_libc_test(
  sprintf_basic_test
  ...
)

if(NOT LIBC_CONF_PRINTF_DISABLE_FLOAT)
  add_fp_unittest(
    sprintf_float_test
  )
endif()

# Other conditionals on config options

There could be some compile opts that need to be inherited by the tests [1]. Even in such a case, the test source code should not directly use those internal copts - they are internal implementation details unrelated to the tests.

[1] - We do not have such a mechanism set up currently. We can add it when required. It will be required if the tests are calling internal implementation functions and not the entrypoints. We do have such tests but they are typically testing common infrastructure components not affected by config options.