Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix a build issue on FreeBSD, correctly define the SwiftShims for stddef and stdint and add required module exports #14040

Closed
wants to merge 1 commit into from

Conversation

Rogiel
Copy link

@Rogiel Rogiel commented Jan 20, 2018

This fixes a issue with the CMake script under FreeBSD and possibly resolves some issues when compiling libdispatch/Foundation.

glibc.modulemap.gyb: stdarg, stdbool, stddef, stdint, sys/param.h

Required when compiling libdispatch/Foundation, otherwise we get a lot of bogus "must import Glibc module" errors
CFBase.h:88:13: error: declaration of 'uint64_t' must be imported from module 'SwiftGlibc.C.pty' before it is required

Should this be enabled only for FreeBSD?

glibc.modulemap.gyb: sys/signal.h

Resolves a Clang/Swift compiler crash when compiling libdispatch

In short, when building the module without sys/signal.h, a struct __sigset definition is not serialized within the module, causing the ASTWriter::getDeclID method to trigger "Declaration not emitted!" assertion (see SR-6034).

Was not sure if it should be included together with signal or under sys. Kept under signal because I think it would make more sense.

AddSwift.cmake

libclang_rt.builtins-${arch}.a is copied by Ninja itself, if we don't use a "-l" it will interpret as a dependency and try to resolve it. Since the CMake script never declares a target emitting this file and it does not exists initially, it will fail. This will only trigger on clean builds (only if the file was not copied already).

Alternatively a add_custom_command could be used declaring libclang_rt.builtins-${arch}.a as a output. This would enable CMake/Ninja to properly resolve the dependencies.

Resolves SR-6034.

Copy link
Member

@dcci dcci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the changes to the module map & the FreeBSD specific bits are fine, but I can't judge the cmake bits. Hope somebody else chimes in.

elseif("${sdk}" STREQUAL "FREEBSD")
list(APPEND swiftlib_private_link_libraries_targets
"${SWIFTLIB_DIR}/clang/lib/freebsd/libclang_rt.builtins-${arch}.a")
"-l${SWIFTLIB_DIR}/clang/lib/freebsd/libclang_rt.builtins-${arch}.a")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll defer this to somebody else, I'm not so sure why you need the -l

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CMake's target_link_libraries or *_LINK_LIBRARIES properties will interpret input as either a target or as a file. Since targets can't have slashes in them, it can only be a file. The stdlib will now have a dependency on this file.

When a Ninja build starts it will check for dependencies and see that there's a file missing and no target that will export it. This will trigger an error.

I believe there is a target somewhere else in the CMake script that will copy this file once the build starts. I couldn't find it, but could see that if this line was commented out the file would "magically" appear and subsequent builds were fine even without the -l. However the copy target never declared that this specific file will be its output causing Ninja to fail due to a missing dependency and not knowing how to create it.

Ideally this should be solved by fixing the copy target outputs but the link directive is a workaround anyway. Or (probably?) fixed in the compiler by properly linking the library whenever the compiler emits code using atomics.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh, there is a task open to have the driver add this, this is quite unfortunate.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also encountered the same problem.Linking it manually make it work temporarily.

@@ -126,27 +126,30 @@ module SwiftGlibc [system] {
export *
}
module signal {
% if CMAKE_SDK == "FREEBSD":
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The glibc module map changes look alright

@dcci
Copy link
Member

dcci commented Jan 21, 2018

@swift-ci please test

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - ba4df1f6aa6314905d42a4058e3a4db13286f74b

@dcci
Copy link
Member

dcci commented Jan 21, 2018

Your patch is breaking the linux build (failures in the module map).
https://ci.swift.org/job/swift-PR-Linux/2544/consoleFull#2135205203122a513-f36a-4c87-8ed7-cbc36a1ec144

I think you might just want to guard some of the lines in the gyb file to be included only when the host system is FreeBSD.

…def and stdint and add required module exports
@Rogiel
Copy link
Author

Rogiel commented Jan 23, 2018

@dcci this is exactly the behavior described in SwiftStddef.h:

// On Linux, the story is different. We get the error message
// "/usr/include/x86_64-linux-gnu/sys/types.h:146:10: error: 'stddef.h' file not
// found"
// This is a known Clang/Ubuntu bug.

Guarded the those includes in Glic module map and the build should now pass.

@dcci
Copy link
Member

dcci commented Jan 26, 2018

@swift-ci please test

1 similar comment
@compnerd
Copy link
Member

compnerd commented Feb 9, 2018

@swift-ci please test

@CodaFi
Copy link
Contributor

CodaFi commented Nov 18, 2019

We believe that this is fixed in Swift 5.0. I'm going to close this out, but if I'm wrong please feel free to comment on SR-6034.

@CodaFi CodaFi closed this Nov 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants