[SR-486] swiftc does not properly handle include of headers already imported from headers in other modules.

[phausler]

Previous ID	SR-486
Radar	None
Original Reporter	@phausler
Type	Bug

Additional Detail from JIRA

Votes	1
Component/s	Compiler
Labels	Bug, ClangImporter
Assignee	None
Priority	Medium

md5: cc5f2e28759eb92ea98f4446682f6e9e

Issue Description:

swiftc does not properly handle include of headers already imported from headers in other modules; this is a regression in that previously it would emit a warning and now is emitting an error

x86_64/Foundation/Foundation/NSLock.swift.o.swiftdeps -module-cache-path ../Ninja-ReleaseAssert/foundation-linux-x86_64
Foundation/NSLock.swift:147:52: error: ambiguous use of 'PTHREAD_MUTEX_RECURSIVE'
pthread_mutexattr_settype(attrs, Int32(PTHREAD_MUTEX_RECURSIVE))
^
SwiftGlibc.PTHREAD_MUTEX_RECURSIVE:1:12: note: found this candidate
public var PTHREAD_MUTEX_RECURSIVE: Int { get }
^
CoreFoundation.PTHREAD_MUTEX_RECURSIVE:1:12: note: found this candidate
public var PTHREAD_MUTEX_RECURSIVE: Int { get }

[phausler]

Swift version 2.2-dev (LLVM 3ebdbb2c7e, Clang f66c5bb67b, Swift faba6e5)
Target: x86_64-unknown-linux-gnu
this was a working version

[belkadan]

I can't think of anything that changed recently. @DougGregor?

(Regardless, it's not really Swift's fault. "Headers already imported from headers in other modules" means that the header belongs in its own module. A header can't belong to two modules.)

[swift-ci]

Comment by Mish Awadah (JIRA)

CI began hitting this at the following revisions:

Commit 1ea703c by nrotem
[Compression] Implement a new strategy for finding frequently used substrings.
The file was modified utils/name-compression/CBCGen.py

Commit f182d39 by nrotem
[Mangler] Move the body of the 'finalize' methods into the cpp files.
The file was modified include/swift/AST/Mangle.h
The file was modified lib/AST/Mangle.cpp

Commit c3c0f23 by lukeh
[SR-440] warnings cleanup

Define CFBool when MACTYPES and _OS_OSTYPES_H are defined
The file was modified CoreFoundation/RunLoop.subproj/CFRunLoop.h
The file was modified CoreFoundation/RunLoop.subproj/CFMachPort.c
The file was modified TestFoundation/TestNSString.swift
The file was modified CoreFoundation/Base.subproj/CFBase.h

[phausler]

The previous behavior was a warning

<unknown>:0: warning: ambiguous use of internal linkage declaration 'PTHREAD_MUTEX_RECURSIVE' defined in multiple modules
/usr/include/pthread.h:51:3: note: declared here in module 'SwiftGlibc.POSIX.pthread'
PTHREAD_MUTEX_RECURSIVE = PTHREAD_MUTEX_RECURSIVE_NP,
^
/usr/include/pthread.h:51:3: note: declared here in module 'CoreFoundation.CFBase'
PTHREAD_MUTEX_RECURSIVE = PTHREAD_MUTEX_RECURSIVE_NP,
^

[swift-ci]

Comment by Bryan Chan (JIRA)

With a recent version of the master branch, I am seeing a similar issue when building a Swift package that imports a C library whose header includes pthread.h. Is it the same bug?

~/src/ModuleBug/SwiftLibFoo$ swift build -v
/opt/swift/bin/swiftc --driver-mode=swift -I /opt/swift/lib/swift/pm -L /opt/swift/lib/swift/pm -lPackageDescription /home/bryanpkc/src/ModuleBug/SwiftLibFoo/Package.swift -fileno 3
/usr/bin/git clone --recursive --depth 10 /home/bryanpkc/src/ModuleBug/CLibFoo /home/bryanpkc/src/ModuleBug/SwiftLibFoo/Packages/CLibFoo
Cloning into '/home/bryanpkc/src/ModuleBug/SwiftLibFoo/Packages/CLibFoo'...
warning: --depth is ignored in local clones; use file:// instead.
done.
/usr/bin/git -C /home/bryanpkc/src/ModuleBug/SwiftLibFoo/Packages/CLibFoo fetch --tags origin
/usr/bin/git -C /home/bryanpkc/src/ModuleBug/SwiftLibFoo/Packages/CLibFoo tag -l
/usr/bin/git -C /home/bryanpkc/src/ModuleBug/SwiftLibFoo/Packages/CLibFoo tag -l
/usr/bin/git -C /home/bryanpkc/src/ModuleBug/SwiftLibFoo/Packages/CLibFoo tag -l
/usr/bin/git -C /home/bryanpkc/src/ModuleBug/SwiftLibFoo/Packages/CLibFoo reset --hard 1.0.0
HEAD is now at 3aa2da7 Update path to header
/usr/bin/git -C /home/bryanpkc/src/ModuleBug/SwiftLibFoo/Packages/CLibFoo branch -m 1.0.0
Resolved version: 1.0.0
/usr/bin/git -C /home/bryanpkc/src/ModuleBug/SwiftLibFoo/Packages/CLibFoo config --get remote.origin.url
/opt/swift/bin/swiftc --driver-mode=swift -I /opt/swift/lib/swift/pm -L /opt/swift/lib/swift/pm -lPackageDescription /home/bryanpkc/src/ModuleBug/SwiftLibFoo/Packages/CLibFoo/Package.swift -fileno 3
/usr/bin/git -C /home/bryanpkc/src/ModuleBug/SwiftLibFoo/Packages/CLibFoo rev-parse --abbrev-ref HEAD
/usr/bin/git -C /home/bryanpkc/src/ModuleBug/SwiftLibFoo/Packages/CLibFoo-1.0.0 config --get remote.origin.url
/opt/swift/bin/swiftc --driver-mode=swift -I /opt/swift/lib/swift/pm -L /opt/swift/lib/swift/pm -lPackageDescription /home/bryanpkc/src/ModuleBug/SwiftLibFoo/Packages/CLibFoo-1.0.0/Package.swift -fileno 3
/opt/swift/bin/swift-build-tool -f /home/bryanpkc/src/ModuleBug/SwiftLibFoo/.build/debug.yaml -v
/opt/swift/bin/swiftc -module-name SwiftLibFoo -incremental -emit-dependencies -emit-module -emit-module-path /home/bryanpkc/src/ModuleBug/SwiftLibFoo/.build/debug/SwiftLibFoo.swiftmodule -output-file-map /home/bryanpkc/src/ModuleBug/SwiftLibFoo/.build/debug/SwiftLibFoo.build/output-file-map.json -num-threads 8 -c /home/bryanpkc/src/ModuleBug/SwiftLibFoo/Sources/SwiftLibFoo/main.swift -I /home/bryanpkc/src/ModuleBug/SwiftLibFoo/.build/debug -j8 -D SWIFT_PACKAGE -Onone -g -enable-testing -Xcc -fmodule-map-file=/home/bryanpkc/src/ModuleBug/SwiftLibFoo/Packages/CLibFoo-1.0.0/module.modulemap -module-cache-path /home/bryanpkc/src/ModuleBug/SwiftLibFoo/.build/debug/ModuleCache
<module-includes>:1:10: note: in file included from <module-includes>:1:
#include "/home/bryanpkc/src/ModuleBug/libfoo/foo.h"
         ^
/home/bryanpkc/src/ModuleBug/libfoo/foo.h:5:1: error: declaration of 'pthread_mutex_t' must be imported from module 'SwiftGlibc.C.signal' before it is required
pthread_mutex_t  *getFooMutex();
^
/usr/include/x86_64-linux-gnu/bits/pthreadtypes.h:128:3: note: previous declaration is here
} pthread_mutex_t;
  ^
/home/bryanpkc/src/ModuleBug/SwiftLibFoo/Sources/SwiftLibFoo/main.swift:2:8: error: could not build Objective-C module 'CLibFoo'
import CLibFoo
       ^
<unknown>:0: error: build had 1 command failures
error: exit(1): /opt/swift/bin/swift-build-tool -f /home/bryanpkc/src/ModuleBug/SwiftLibFoo/.build/debug.yaml -v

The module map in CLibFoo looks like this:

module CLibFoo [system] {
  header "/home/bryanpkc/src/ModuleBug/libfoo/foo.h"
  link "foo"
  export *
}

The header file foo.h contains only this:

#include <pthread.h>

pthread_mutex_t  *getFooMutex();

[swift-ci]

Comment by Bryan Chan (JIRA)

Apparently the issue I encountered has been discussed on the mailing list. Replacing <pthread.h> with <bits/pthreadtypes.h> works around the problem. However, it is not always possible to modify the header files of the C library one wishes to import into Swift.

[finagolfin]

I think I've hit this long-standing issue when natively building Swift on Android now, looks to me the FreeBSD issue detailed in SR-6034 is likely the same. After drodriguez's recent pull to add more Bionic headers to the stdlib modulemap, I started getting errors about multiple declarations of fcntl:

While building module 'SwiftOverlayShims':
In file included from <module-includes>:1:
/data/data/com.termux/files/home/src/swift/build/Ninja-Release/swift-android-aarch64/./lib/swift/shims/LibcOverlayShims.h:43:10: error: conflicting types for 'fcntl'
  return fcntl(fd, cmd, value);
         ^
<module-includes>:1:10: note: in file included from <module-includes>:1:
#include "LibcOverlayShims.h"
         ^
/data/data/com.termux/files/home/src/swift/build/Ninja-Release/swift-android-aarch64/./lib/swift/shims/LibcOverlayShims.h:43:10: error: conflicting types for 'fcntl'
  return fcntl(fd, cmd, value);
         ^
/data/data/com.termux/files/usr/include/bits/fcntl.h:36:5: note: previous declaration is here
int fcntl(int __fd, int __cmd, ...);
    ^
/data/data/com.termux/files/usr/include/bits/fcntl.h:36:5: note: previous declaration is here
int fcntl(int __fd, int __cmd, ...);
    ^

Note that the Bionic fcntl is declared in the nested header bits/fcntl.h, a nested declaration similar to the FreeBSD issue mentioned above, not directly in fcntl.h as in glibc.

Looking into it further, the issue appears to be that the Termux environment modifies Bionic's syslog.h and ifaddrs.h to include unistd.h, which also includes bits/fcntl.h just like fcntl.h, and so runs into this nested header problem with the Swift clang setup.

My best guess is that the ClangImporter in the Swift compiler doesn't keep preprocessor defines from nested headers like bits/fcntl.h, so include guards in those headers are ignored and cause havoc with modulemaps, when Swift code tries to use declarations from nested headers that might be included by multiple headers listed in the modulemap. This seems to be confirmed by this patch that worked around the problem for me:

diff --git a/stdlib/public/Platform/glibc.modulemap.gyb b/stdlib/public/Platform/glibc.modulemap.gyb                                                                                              index 92a0948505..ad2f2c7b78 100644                                                              --- a/stdlib/public/Platform/glibc.modulemap.gyb                                                 +++ b/stdlib/public/Platform/glibc.modulemap.gyb
@@ -248,10 +250,12 @@ module SwiftGlibc [system] {                                                      header "${GLIBC_INCLUDE_PATH}/netdb.h"                                                           export *                                                                                       }                                                                                           +% if CMAKE_SDK != "ANDROID":                                                                         module ifaddrs {                                                                                   header "${GLIBC_INCLUDE_PATH}/ifaddrs.h"                                                         export *                                                                                       }                                                                                           +% end
module search {                                                                                    header "${GLIBC_INCLUDE_PATH}/search.h"                                                          export *
@@ -260,10 +264,12 @@ module SwiftGlibc [system] {                                                      header "${GLIBC_INCLUDE_PATH}/spawn.h"                                                           export *                                                                                       }                                                                                           +% if CMAKE_SDK != "ANDROID":                                                                         module syslog {                                                                                    header "${GLIBC_INCLUDE_PATH}/syslog.h"                                                          export *                                                                                       }                                                                                           +% end
module tar {                                                                                       header "${GLIBC_INCLUDE_PATH}/tar.h"                                                             export *                                                                                  @@ -521,6 +531,14 @@ module SwiftGlibc [system] {                                                       header "${GLIBC_INCLUDE_PATH}/unistd.h"                                                          export *                                                                                       }                                                                                           +    module ifaddrs {                                                                            +      header "${GLIBC_INCLUDE_PATH}/ifaddrs.h"                                                  +      export *                                                                                  +    }                                                                                           +    module syslog {                                                                             +      header "${GLIBC_INCLUDE_PATH}/syslog.h"                                                   +      export *                                                                                  +    }                                                                                                module utime {                                                                                     header "${GLIBC_INCLUDE_PATH}/utime.h"                                                           export *

All this patch does is reorder the headers in the libc modulemap so that both ifaddrs.h and syslog.h come after unistd.h, so that its include guard is defined by then and unistd.h is presumably ignored for those two headers after that.

I came up with that workaround before finding this Jira issue, I just tried this workaround to directly list the nested header in the modulemap and it also seems to work:

@@ -341,6 +341,14 @@ module SwiftGlibc [system] {                                                       header "${GLIBC_INCLUDE_PATH}/fcntl.h"                                                           export *                                                                                       }                                                                                           +    module bits {                                                                               +      export *                                                                                  +                                                                                                +    module fcntl {                                                                              +      header "${GLIBC_INCLUDE_PATH}/bits/fcntl.h"                                               +      export *                                                                                  +    }                                                                                           +   }                                                                                                 module fnmatch {                                                                                   header "${GLIBC_INCLUDE_PATH}/fnmatch.h"                                                         export *

I'm also able to reproduce this nested header problem on linux, say if I try to invoke a type from bits/types.h in LibcOverlayShims.h:

diff --git a/stdlib/public/SwiftShims/LibcOverlayShims.h b/stdlib/public/SwiftShims/LibcOverlayShims.h                                                                                            index a4ec14402a..fbdb498ffb 100644                                                              --- a/stdlib/public/SwiftShims/LibcOverlayShims.h                                                +++ b/stdlib/public/SwiftShims/LibcOverlayShims.h                                                @@ -40,7 +40,8 @@ typedef int mode_t;                                                             // File control <fcntl.h>                                                                        #if !defined(_WIN32) || defined(__CYGWIN__)                                                      static inline int _swift_stdlib_fcntl(int fd, int cmd, int value) {                             -  return fcntl(fd, cmd, value);                                                                 + __uint8_t foo;                                                                                 + return fcntl(fd, cmd, value);                                                                   }                                                                                                                                                                                                 static inline int _swift_stdlib_fcntlPtr(int fd, int cmd, void* ptr) {

I then get a similar error since bits/types.h is included all over the place in glibc:

<module-includes>:1:10: note: in file included from <module-includes>:1:                         #include "LibcOverlayShims.h"                                                                             ^                                                                                       /home/butta/swift/build/Ninja-Release/swift-linux-x86_64/./lib/swift/shims/LibcOverlayShims.h:43:2: error: declaration of '__uint8_t' must be imported from module 'SwiftGlibc.POSIX.termios' before it is required                                                                                   __uint8_t foo;                                                                                   ^                                                                                               //usr/include/bits/types.h:38:23: note: previous declaration is here                             typedef unsigned char __uint8_t;                                                                                       ^                                                                          /home/butta/swift/swift/stdlib/public/Platform/Platform.swift:14:8: error: could not build C module 'SwiftOverlayShims'                                                                             import SwiftOverlayShims                                                                                ^

Of course, nobody would actually directly invoke __uint8_t: this is just to demonstrate the nested header problem is there on linux too, not just Android and FreeBSD.

I was unable to follow Bryan Chan's gmane link above to a swift-devel discussion about this issue, as that gmane site seems to be down, or find that thread elsewhere: maybe this problem is well-known. If it's not going to be fixed in the compiler, it should be documented prominently when using modulemaps.