Fix two issues with the SwiftGlibc module map by martinboehme · Pull Request #32404 · swiftlang/swift

martinboehme · 2020-06-16T10:30:54Z

The issues are:

Today, some submodules in SwiftGlibc fail to provide definitions that they should contain. As a consequence, Swift fails to import some code that compiles correctly with standalone Clang. As just one example, including signal.h should make the type pid_t available, but it currently does not.
SwiftGlibc is not compatible with the libc++ module map. Trying to include libc++ headers in a C++ module imported into Swift results in an error message about cyclic dependencies.

This change fixes both of these issues by making it so that SwiftGlibc no actually longer defines a module map for the glibc headers but merely makes all of the symbols from those headers available in a module that can be imported into Swift. C / Objective-C / C++ code, on the other hand, will now include the glibc headers texually.

For more context on the two issues and this fix, see this forum discussion:

https://forums.swift.org/t/problems-with-swiftglibc-and-proposed-fix/37594

This change only modifies glibc.modulemap.gyb for the time being but leaves bionic.modulemap.gyb and libc-openbsd.modulemap.gyb unchanged. The intent is to fix these in the same way, but it will be easier to do this in separate PRs that can be tested individually.

martinboehme · 2020-06-16T10:42:20Z

@swift-ci Please test

swift-ci · 2020-06-16T11:52:42Z

Build failed
Swift Test OS X Platform
Git Sha - a0ec0fd16b6350065ace857cdae8df9e2988762d

swift-ci · 2020-06-16T12:01:35Z

Build failed
Swift Test Linux Platform
Git Sha - a0ec0fd16b6350065ace857cdae8df9e2988762d

martinboehme · 2020-06-22T05:15:31Z

@swift-ci Please test OS X

martinboehme · 2020-06-22T05:16:15Z

A fix for the broken test on Linux is in progress at #32465.

swift-ci · 2020-06-22T07:07:07Z

Build failed
Swift Test OS X Platform
Git Sha - 7efdc17714aa0b70efcb195e444944c58e2c8c5b

martinboehme · 2020-06-22T07:26:58Z

Build failed
Swift Test OS X Platform
Git Sha - 7efdc17

This is failing with what looks like an unrelated error:

/Users/buildnode/jenkins/workspace/swift-PR-osx/branch-master/swift/test/Constraints/operator.swift:223:26: error: incorrect message found
09:06:03   // expected-error@-1 {{referencing operator function '==' on 'Array' requires that 'Dictionary<String, E>.Values' conform to 'Equatable'}}
09:06:03                          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
09:06:03                          operator function '==' requires that 'Dictionary<String, E>.Values' conform to 'Equatable'

shahmishal · 2020-10-01T06:55:12Z

Please update the base branch to main by Oct 5th otherwise the pull request will be closed automatically.

How to change the base branch: (Link)
More detail about the branch update: (Link)

test/Interop/C/stdlib/Inputs/include-signal.h

zoecarver · 2020-10-07T17:42:44Z

@swift-ci please smoke test.

zoecarver · 2020-10-07T17:46:44Z

@swift-ci please smoke test linux platform.

stdlib/public/Platform/glibc.modulemap.gyb

zoecarver · 2020-10-10T23:16:32Z

(This comment is just so I remember what needs to be done.) To get the Linux build green we need to remove the @_implementationOnly decorator from before import CoreFoundation in three files in swift-corelibs-foundation: Sources/Foundation/Data.swift, Sources/Foundation/NSLock.swift, and Sources/Foundation/Thread.swift.

scentini · 2020-10-22T09:50:41Z

@swift-ci please smoke test linux platform.

zoecarver · 2020-10-24T21:13:10Z

@scentini see my above comment. We're going to need to update those three files to get the Linux build working.

scentini · 2020-10-27T13:30:23Z

@zoecarver we are looking into fixing @_implementationOnly instead, see SR-13785.

zoecarver · 2020-10-27T17:50:43Z

@zoecarver we are looking into fixing @_implementationOnly instead, see SR-13785.

Oh, great. That's the better way to fix this :)

hlopko · 2020-12-01T09:12:32Z

@swift-ci please test

swift-ci · 2020-12-01T10:29:27Z

Build failed
Swift Test Linux Platform
Git Sha - 968f3350373858d0e167b52fc6a102eec8c48dfd

hlopko · 2021-01-07T11:37:53Z

@swift-ci please test

hlopko · 2021-01-08T19:29:53Z

We'd like to move forward with this PR. If nobody objects by Monday, we'll submit. We're happy to address all post-submit review comments. Thanks!

zoecarver

FWIW this LGTM. Ship it.

drodriguez · 2021-01-08T23:47:38Z

Not a blocker: Bionic (Android) has a different modulemap (stdlib/public/Platform/bionic.modulemap.gyb), which this PR doesn't change. From my understanding of forum post and the changes in this PR, it should be possible to do the same in Android and remove the specific module map, right?

Once this lands, another PR can be created to do that, no need to change this PR at all (specially because the Android CI cannot be requested for a PR, so checking that it works would be complicated).

finagolfin · 2021-01-21T17:06:11Z

@drodriguez, anybody working on the Android version of this pull? I can put it together if noone else is.

@3405691582, you may want to do the same for OpenBSD.

drodriguez · 2021-01-22T15:56:43Z

@buttaface I had the first step working (removing the complicated modulemap), but the test were failing. In the Android case, I think mostly because of the cross-compilation. These weekends have been quite busy and I haven't had time to work on anything, so if you want to have a try at this, feel free. Also, sorry for not being able to test and merge the other PR you had open. I will try to make time at some point.

finagolfin · 2021-01-23T02:32:07Z

@drodriguez, no problem, I figured you were busy. If we get this modulemap change in first, we may not need to modify that C++ test in the NDK pull, so I will try to get this working first.

This migrates OpenBSD to use the single-header Glibc modulemap proposed and implemented in swiftlang#32404, and necessitates introducing some missing headers for building Foundation added in swiftlang#38341. Additionally, incorporate nullability annotations in SwiftShims per

When SwiftGlibc.h was added in swiftlang#32404, no cmake code was added to copy it to the static resources. Copy it along with glibc.modulemap, which references it.

martinboehme force-pushed the swift-glibc-fix branch from c8628af to a0ec0fd Compare June 16, 2020 10:31

gribozavr requested a review from DougGregor June 16, 2020 12:45

gribozavr added the c++ interop Feature: Interoperability with C++ label Jun 16, 2020

MForster mentioned this pull request Jul 7, 2020

Link against the C++ standard library when C++ interop is enabled #30914

Merged

scentini changed the base branch from master to main October 1, 2020 08:56

zoecarver reviewed Oct 7, 2020

View reviewed changes

test/Interop/C/stdlib/Inputs/include-signal.h Outdated Show resolved Hide resolved

zoecarver force-pushed the swift-glibc-fix branch 2 times, most recently from 4f063e6 to 6754c9c Compare October 7, 2020 17:20

zoecarver force-pushed the swift-glibc-fix branch from e177654 to fb34283 Compare October 7, 2020 17:44

zoecarver reviewed Oct 8, 2020

View reviewed changes

stdlib/public/Platform/glibc.modulemap.gyb Outdated Show resolved Hide resolved

scentini mentioned this pull request Oct 28, 2020

[cxx-interop] Fix SR-13785 #34476

Merged

scentini mentioned this pull request Nov 10, 2020

Explicitly import Glibc in files that rely on it swiftlang/swift-corelibs-foundation#2921

Merged

hlopko force-pushed the swift-glibc-fix branch from fb34283 to 968f335 Compare December 1, 2020 09:11

hlopko added 5 commits January 7, 2021 12:37

Do not modify behavior of stdc-predef.h/features.h in glibc module

3312a62

Rename stdlib -> libc

1be4265

Add rationale to include-signal.swift

f66bae4

Only run include-signal test on linux

3ff3ab7

Fix cmake script

cd8760d

hlopko force-pushed the swift-glibc-fix branch from 2dd14c9 to cd8760d Compare January 7, 2021 11:37

zoecarver approved these changes Jan 8, 2021

View reviewed changes

scentini merged commit c5b7e7f into swiftlang:main Jan 11, 2021

finagolfin mentioned this pull request Feb 2, 2021

[android] Switch to new single-header modulemap for Bionic too #35707

Merged

3405691582 mentioned this pull request Dec 14, 2021

[stdlib] Use the single-header Glibc modulemap. #40557

Merged

finagolfin mentioned this pull request Jan 8, 2022

[build] Remove SWIFT_SDK_${OS}_ARCH_${ARCH}_LIBC_ARCHITECTURE_INCLUDE_DIRECTORY CMake variable #40774

Merged

finagolfin mentioned this pull request Feb 22, 2022

[5.6][stdlib] Switch to new single-header modulemap for Android and OpenBSD too #41510

Closed

mhjacobson mentioned this pull request Nov 20, 2022

stdlib: install SwiftGlibc.h in static resources directory #62209

Merged

finagolfin mentioned this pull request Jun 15, 2023

[stdlib] Get rid of the single bridging header for ELF platforms like linux #66665

Closed

swift-ci mentioned this pull request Sep 6, 2023

[SR-13032] Invalid .swiftinterface when textually including C struct in different modules #55477

Closed

Conversation

martinboehme commented Jun 16, 2020

Uh oh!

martinboehme commented Jun 16, 2020

Uh oh!

swift-ci commented Jun 16, 2020

Uh oh!

swift-ci commented Jun 16, 2020

Uh oh!

martinboehme commented Jun 22, 2020

Uh oh!

martinboehme commented Jun 22, 2020

Uh oh!

swift-ci commented Jun 22, 2020

Uh oh!

martinboehme commented Jun 22, 2020

Uh oh!

shahmishal commented Oct 1, 2020

Uh oh!

Uh oh!

zoecarver commented Oct 7, 2020

Uh oh!

zoecarver commented Oct 7, 2020

Uh oh!

Uh oh!

zoecarver commented Oct 10, 2020

Uh oh!

scentini commented Oct 22, 2020

Uh oh!

zoecarver commented Oct 24, 2020

Uh oh!

scentini commented Oct 27, 2020

Uh oh!

zoecarver commented Oct 27, 2020

Uh oh!

hlopko commented Dec 1, 2020

Uh oh!

swift-ci commented Dec 1, 2020

Uh oh!

hlopko commented Jan 7, 2021

Uh oh!

hlopko commented Jan 8, 2021

Uh oh!

zoecarver left a comment

Choose a reason for hiding this comment

Uh oh!

drodriguez commented Jan 8, 2021

Uh oh!

finagolfin commented Jan 21, 2021

Uh oh!

drodriguez commented Jan 22, 2021

Uh oh!

finagolfin commented Jan 23, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants