The March LLVM rebranch turned on ODR checking for C, broke compiling Foundation for Android

[finagolfin]

Description
My daily Android CI started failing when building Foundation for the trunk SDK since the rebranch last week:

/usr/local/lib/android/sdk/ndk/25.2.9519653/toolchains/llvm/prebuilt/linux-x86_64/sysroot/usr/include/bits/timespec.h:48:10: error: 'timespec' has different definitions in different modules; first difference is defined here found field 'tv_sec' with type 'time_t' (aka 'long')
  time_t tv_sec;
         ^
/usr/local/lib/android/sdk/ndk/25.2.9519653/toolchains/llvm/prebuilt/linux-x86_64/sysroot/usr/include/linux/time.h:26:23: note: but in 'CDispatch' found field 'tv_sec' with type '__kernel_old_time_t' (aka 'long')
  __kernel_old_time_t tv_sec;
                      ^
/home/runner/work/swift-android-sdk/swift-android-sdk/sdk-config/swift-corelibs-foundation/Sources/Foundation/Bridging.swift:13:29: error: could not build C module 'CoreFoundation'

This seems like an incorrect error, as it knows the underlying type is always long but has now started complaining about the intermediate C typedef names used.

There have been no changes in libdispatch and nothing relevant in Foundation in the meantime, so the problem is clearly in the compiler or more specifically, the underlying libclang code it calls. I don't see anything relevant that changed in lib/ClangImporter/ between the March 2 and March 6 snapshots, so most likely something in LLVM changed with the rebranch that is now affecting the Swift compiler, as this is an LLVM error message.

Steps to reproduce
Build CoreFoundation for Android, either using my CI shown or natively in the Termux app.

Expected behavior
Foundation builds as always.

Environment

• Swift 5.9 trunk since the Mar. 6 snapshot
• Deployment target: Android AArch64 and armv7

Additional context

The build still works for Android x86_64, probably because only the x86_64 asm/signal.h includes linux/time.h, so the same header definition is used for both these corelibs on Android x86_64.

@shahmishal, anyone else reporting a similar issue since the rebranch?

@compnerd, @hyp, @egorzhdan, let me know if one of you knows what's going on here.

[finagolfin]

@vsapsai, I see that you work on these ODR issues a lot in clang. This error was around in clang before the rebranch but wasn't triggered until now: is it possible it was inadvertently enabled for Swift at some point?

[finagolfin]

Looked into this further, the issue appears to be two definitions of timespec in Android's Bionic C library: one in linux/time.h that is normally included through time.h and a few other headers, and another in bits/timespec.h that is included in signal.h and two other headers.

Both should be identical but use different typedefs for tv_sec that always resolve to the same type. However, clang and now Swift's ClangImporter appear to have turned on some ODR checking since the recent rebranch that errors if the typedefs are different in different C modules, even if it knows the underlying types are the same.

This may make sense for user libraries, but causes problems like this for the rats nests' of C library headers. If time.h is included first in one module and signal.h in another, Swift's ClangImporter now complains that they are different definitions, even though it accepted them as identical definitions before the recent rebranch. If both headers are included in the same module, whichever comes first is the one used, because of the include guards in each header.

I was able to use that latter feature to work around the problem on my Android CI today, by including signal.h before wherever time.h or unistd.h was included, to make sure the same header definition is always used. That required modifying libdispatch when building the Android SDK and headers in llbuild, swift-tools-support-core, and swift-crypto to build those Swift repos without causing an ODR conflict.

This isn't sustainable though, as any random C module may include one or the other and then cause these clashes again. I think this should be loosened back to the way it was, ie compare the underlying types not the typedefs in a definition.

@vsapsai, @avl-llvm, @JDevlieghere, let me know what you think.

[vsapsai]

If a field has a different type name in different module, clang considered it as an ODR violation in C++ since 2017. My change is to bring the same kind of checking to C and Objective-C.

The guard _STRUCT_TIMESPEC implies both definitions shouldn't be used together at the same time. And it's not entirely clear to my why it should be OK from Swift.

I have to admit ODR is C++ thing and I don't know what C requires for using the same name for different [even if interchangeable] entities.

[finagolfin]

Thanks for the explanation and links, @vsapsai. I am not sure if this is the right approach for C, as it won't be a problem if there aren't multiple definitions with different typedefs, but maybe C sometimes requires this workaround. If this is the only multiply-defined C struct in Bionic, maybe this can be worked around.

I see that your pull made it into the just-released clang 16: I will ask the Android NDK devs what they think, as they will have to update the NDK to use this clang soon.

[finagolfin]

A fix has been merged in Bionic trunk, hopefully it will be the only one needed. Leaving this open until that makes it into a released Android NDK and no more problems are seen.

[finagolfin]

NDK 26 with the linked fix doesn't show any other problems like this, so closing.