[BUG] -fmodules usage broke with including <cstring> since NDK 29

[finagolfin]

Description

The Swift compiler testsuite generates a header for C++ interoperability and checks it using the NDK clang++ for Android, but it has been broken since NDK 29, though hidden behind the earlier #2230 issue till now.

Reducing it, I get the following simple header and command:

> cat free.h
#include <cstring>
#include <cstdint>
#include <stdlib.h>
void opaqueFree(void *_Nonnull p) noexcept {
    free(p);
}

# works fine with NDK 28
> /home/finagolfin/android-ndk-r28c/toolchains/llvm/prebuilt/linux-x86_64/bin/clang++ -target aarch64-unknown-linux-android23 -isystem /home/finagolfin/android-ndk-r28c/toolchains/llvm/prebuilt/linux-x86_64/sysroot -x c++-header -c free.h  -fmodules -fcxx-modules

# breaks with NDK 30 beta 2
> /home/finagolfin/android-ndk-r30-beta2/toolchains/llvm/prebuilt/linux-x86_64/bin/clang++ -target aarch64-unknown-linux-android23 -isystem /home/finagolfin/android-ndk-r30-beta2/toolchains/llvm/prebuilt/linux-x86_64/sysroot -x c++-header -c free.h  -fmodules -fcxx-modules
free.h:5:5: error: missing '#include <malloc.h>'; 'free' must be declared before it is used
    5 |     free(p);
      |     ^
/home/finagolfin/android-ndk-r30-beta2/toolchains/llvm/prebuilt/linux-x86_64/bin/../sysroot/usr/include/malloc.h:108:6: note: declaration here is not visible
  108 | void free(void* _Nullable __ptr);
      |      ^
1 error generated.

I applied the fix for #2230 to NDK 29 and see the same error with that NDK too. Removing the -fmodules flag or the first <cstring> include gets it to not error again.

Since this isn't reproducible with other libc++ platforms, it is most likely a C++ modularity problem caused by some incompatibility between libc++ changes after NDK 28 and the Bionic headers in the NDK.

I am using a supported NDK

• I have checked and the NDK I'm using is currently supported

Affected versions

r29, r30

[enh-google]

Removing the -fmodules flag or the first include gets it to not error again.

sounds like another libc++ problem then?

[finagolfin]

Sure, but like #2230, it is very likely an interaction between libc++ and Bionic, since eg it doesn't show up on macOS with libc++. Not as important as that one, as that one was breaking the build of a core library for Swift's C++ Interop, whereas this is just an isolated test from the Swift test suite.

Will I need to dig into this myself? 😉 I know the NDK doesn't officially support modules yet, #1161.

[enh-google]

Will I need to dig into this myself? 😉 I know the NDK doesn't officially support modules yet, #1161.

yeah, no change there :-)

[finagolfin]

@hnrklssn, is there any existing clang tooling that would help find such modularity problems? I had to randomly poke around in the NDK headers last time, not looking forward to doing that again.

[hnrklssn]

Xcode ships a module verifier. You may be able to use it with the NDK, not entirely sure. @egorzhdan and @cyndyishida would know more.

[cyndyishida]

I don't think there's good tooling that tests non-Darwin platforms. The Xcode module verifier is not likely to work for Android. It is mostly a wrapper over clang, though. One could craft a simple version of this as a regression test based on something like

clang -x objective-c++ -target arm64-apple-macos27.0 -std=gnu17 -Wsystem-headers
  -fmodules-cache-path=<tmp>/cache -fmodules -fcxx-modules
  -I <relevant search paths> -F  <relevant search paths>
  -fsyntax-only -fobjc-arc 
  -sysroot <relevant search path> 
  -ferror-limit=0
  -Wno-error -Wno-deprecated-declarations -Xclang -diagnostic-log-file -Xclang <tmp>/diags 
  -Wnon-modular-include-in-module -Wnon-modular-include-in-framework-module
  -Wincomplete-umbrella -Wquoted-include-in-framework-header -Watimport-in-framework-header -Wframework-include-private-from-public -Wincomplete-framework-module-declaration
  -Wmodule-import-in-extern-c <tmp>/Test.m

where Test.m is a source file that includes the modules you want to test, like @import Foo;. The *framework* warnings are likely less relevant here. You could have something similar with swiftc directly too.
CC:@ian-twilightcoder

[ian-twilightcoder]

Xcode ships a module verifier. You may be able to use it with the NDK, not entirely sure. @egorzhdan and @cyndyishida would know more.

Xcode's module verifier isn't usable outside of Xcode projects building frameworks, but what Cyndy has is a good representation of it.https://github.com/swiftlang/swift-build/blob/b6ae279e0a4bd9f4f19ef1d9afcd2bba662ce81b/Sources/SWBTaskConstruction/TaskProducers/OtherTaskProducers/ModuleVerifierTaskProducer.swift#L237 is the main part of the implementation.

[finagolfin]

Alright, thanks, I will try some of those clang flags and see what they do.

[finagolfin]

I spent an hour looking into this, using the simple method of dumping out cached modules with -fmodules-cache-path=/tmp/foo, then examining the pre-compiled modules. The culprit appears to be @ldionne's libc++ module map rewrite from a couple years ago, llvm/llvm-project#110501, which first made it into NDK 29, interacting badly with the libc++ or Bionic headers.

Specifically, the problem may be that the rewrite moved cstdlib into a massive std module, so that Bionic's stdlib.h and its functions now show up in the std_math_h module instead:

> strings cache-wrong/1U1QVBU445QF9/std_math_h-2ZTUF15SXOD4O.pcm |ag "stdlib\.h|free"
 stdlib.h
/home/finagolfin/android-ndk-r30-beta2/toolchains/llvm/prebuilt/linux-x86_64/bin/../sysroot/usr/include/stdlib.h
/home/finagolfin/android-ndk-r30-beta2/toolchains/llvm/prebuilt/linux-x86_64/bin/../sysroot/usr/include/stdlib.h
stdlib.h
free
free_aligned_sized
free_sized

clang++ then correctly sees that free() is in the wrong module and doesn't use it, but tells you that something is going wrong with modules:

> /home/finagolfin/android-ndk-r30-beta2/toolchains/llvm/prebuilt/linux-x86_64/bin/clang++ -target aarch64-unknown-linux-android23 -isystem /home/finagolfin/android-ndk-r30-beta2/toolchains/llvm/prebuilt/linux-x86_64/sysroot -x c++-header -c free.h  -fmodules -fcxx-modules -fmodules-cache-path=cache-wrong/
free.h:5:5: error: missing '#include <malloc.h>'; 'free' must be declared before it is used
    5 |     free(p);
      |     ^
/home/finagolfin/android-ndk-r30-beta2/toolchains/llvm/prebuilt/linux-x86_64/bin/../sysroot/usr/include/malloc.h:108:6: note: declaration here is not visible
  108 | void free(void* _Nullable __ptr);
      |      ^
1 error generated.

As might be expected, adding the line #include <math.h> to my above free.h header then fixes building with it!

@ldionne and @enh-google, any idea how we go about fixing this problem of the cstdlib functions incorrectly showing up in std_math_h?

[ian-twilightcoder]

Did you get any -Wnon-modular-include-in-module diagnostics? Those are usually the tell-tale ones.

[finagolfin]

I didn't try any of those extra clang module warning flags, as I wasn't sure if they'd apply here. Trying the one you just listed with my command above made no difference when I tried it now.

[ian-twilightcoder]

Hmm. The only explanation I can think of why std_math_h would contain stdlib functions is that it transitively includes a non-modular <stdlib.h> header (or whatever it is that declares free).

[finagolfin]

That stdlib.h is included directly by the C++ math.h in the NDK, a modification to upstream libc++ that has been there for years, going back to NDK 28 also. It didn't cause this problem back then, but apparently does now after the libc++ reorg since NDK 29.

Since this is clearly a consequence of the Android-modified libc++ in the NDK leaking out modules like this after the libc++ reorg upstream, hopefully the Android NDK devs have some solution in mind.

[enh-google]

That stdlib.h is included directly by the C++ math.h in the NDK, a modification to upstream libc++ that has been there for years, going back to NDK 28 also. It didn't cause this problem back then, but apparently does now after the libc++ reorg since NDK 29.

Since this is clearly a consequence of the Android-modified libc++ in the NDK leaking out modules like this after the libc++ reorg upstream, hopefully the Android NDK devs have some solution in mind.

huh. i wasn't aware that we still had any local changes like that. i'll ask around to see if anyone knows where that came from/what the point was supposed to be...

if you remove that line in your local copy of the ndk, does your module problem go away? does other stuff still work?