search

apple / swift-nio-ssl

TLS Support for SwiftNIO, based on BoringSSL.
https://swiftpackageindex.com/apple/swift-nio-ssl/main/documentation/niossl
Apache License 2.0
388 stars 139 forks source link

Bug: Missing '#include <stdlib.h>'; 'abort' must be declared before it is used #259

Closed mauliknshah closed 3 years ago

mauliknshah commented 3 years ago

/Users/Maulik/Library/Developer/Xcode/DerivedData/TestFluentPostgres-gjwkxowtrhpkuzdpxewtalgpeaqq/SourcePackages/checkouts/swift-nio-ssl/Sources/CNIOBoringSSL/include/CNIOBoringSSL_span.h:139:7: Missing '#include '; 'abort' must be declared before it is used

To Reproduce

Steps to reproduce the behavior:

  1. Create a new Project.
  2. Add Swift-Nio and Swift-nio-ssl dependency with "Up to next major" selection.
  3. Compile.

Expected behavior

The compiler would throw Error as stated in the first line.

Environment

Swift-Nio-SSL version: 2.10.0 Swift-Nio version: 2.24.0 XCode Version: 11.7 or higher(Same error with XCode 12.3 Beta)

macOS Version: 11.0.1 (Big Sur)

Additional context

Seems like before updating the OS, it was working fine.

SercanKaraoglu commented 3 years ago

the same issue as here https://github.com/apple/swift-nio-ssl/issues/254

SercanKaraoglu commented 3 years ago

@Lukasa my pr were solving this

Lukasa commented 3 years ago

This remains a clang bug: the header file is well formatted.

Lukasa commented 3 years ago

I just performed a fresh install and followed these steps. Big Sur 11.0.1, Xcode 12.2. I do not see this error, everything compiles cleanly.

At this stage I recommend filing a report with Apple via Feedback Assistant. Can you then paste the report number here?

SercanKaraoglu commented 3 years ago

here you go https://feedbackassistant.apple.com/feedback/8925072 @Lukasa

xanderdunn commented 3 years ago

This build failure reproduces on latest swift trunk 2020-12-05:

$ which swift
/Library/Developer/Toolchains/swift-DEVELOPMENT-SNAPSHOT-2020-12-05-a.xctoolchain/usr/bin/swift
$ swift --version
Apple Swift version 5.3-dev (LLVM 389c6ea22592d7c, Swift 65db86ba034403f)
Target: x86_64-apple-darwin20.1.0
$ swift build
Fetching https://github.com/apple/swift-nio.git
Cloning https://github.com/apple/swift-nio.git
Resolving https://github.com/apple/swift-nio.git at 2.25.0
In file included from /Users/xander/dev/swift-nio-ssl/Sources/CNIOBoringSSL/ssl/tls_record.cc:109:
In file included from /Users/xander/dev/swift-nio-ssl/Sources/CNIOBoringSSL/include/CNIOBoringSSL_ssl.h:149:
In file included from /Users/xander/dev/swift-nio-ssl/Sources/CNIOBoringSSL/include/CNIOBoringSSL_pem.h:67:
In file included from /Users/xander/dev/swift-nio-ssl/Sources/CNIOBoringSSL/include/CNIOBoringSSL_x509.h:76:
In file included from /Users/xander/dev/swift-nio-ssl/Sources/CNIOBoringSSL/include/CNIOBoringSSL_obj.h:62:
In file included from /Users/xander/dev/swift-nio-ssl/Sources/CNIOBoringSSL/include/CNIOBoringSSL_bytestring.h:20:
/Users/xander/dev/swift-nio-ssl/Sources/CNIOBoringSSL/include/CNIOBoringSSL_span.h:139:7: error: missing '#include <stdlib.h>'; 'abort' must be declared before it is used
      abort();
      ^
/Applications/Xcode-beta.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.0.sdk/usr/include/stdlib.h:131:7: note: declaration here is not visible
void     abort(void) __cold __dead2;
         ^
...

macOS 11.0.1. Xcode 12.2 beta 3.

Note that it's specific to macOS. This does not reproduce when building on Linux.

Including the workaround described here until the complier bug is fixed would be helpful to developers using NIO.

Lukasa commented 3 years ago

This seems increasingly like the Big Sur SDK is incompatible with the version of clang being used by S4T and currently present on Swift master. I'll continue to chase this up, we should really be pressing the Swift folks to resolve this. If it lasts too much longer we'll be forced to work around this, which would really be a shame.

ldionne commented 3 years ago

Can you attach the output of running the same clang command that's failing and adding -v to it? That will print the list of include directories that Clang is searching, and I believe it will make it easier to troubleshoot the problem. Looking at swift build --help, I believe running like this would do the trick:

swift build -v -Xcxx -v

(run swift build as verbose, and pass -v to the compiler)

xanderdunn commented 3 years ago

macOS 11.0.1:

$ git log
commit c585b79ac062b3a41a51335722500734aa6c9eb9 (HEAD -> main, origin/main, origin/HEAD)
Author: Sercan Karaoglu <sercan_karaoglu@yahoo.com>
Date:   Wed Dec 2 15:28:37 2020 +0100

    change cxx language standard (#260)
$ which swift
/Library/Developer/Toolchains/swift-DEVELOPMENT-SNAPSHOT-2020-12-05-a.xctoolchain/usr/bin/swift
$ swift --version
Apple Swift version 5.3-dev (LLVM 389c6ea22592d7c, Swift 65db86ba034403f)
Target: x86_64-apple-darwin20.1.0
$ rm -rf .build
$ swift build -v -Xcxx -v

Full output here

ldionne commented 3 years ago

Thanks a lot. Here's a one line reproducer:

cat <<EOF | /Library/Developer/Toolchains/swift-DEVELOPMENT-SNAPSHOT-2020-12-12-a.xctoolchain/usr/bin/clang -xc++ - -isysroot $(xcrun --show-sdk-path) -fmodules -fmodule-name=CNIOBoringSSL -fsyntax-only
#include <cstdlib>
int main() { abort(); }
EOF

Gives:

<stdin>:2:14: error: missing '#include <stdlib.h>'; 'abort' must be declared before it is used
int main() { abort(); }
             ^
<SDKROOT>/usr/include/stdlib.h:131:7: note: declaration here is not visible
void     abort(void) __cold __dead2;
         ^
1 error generated.

If I change to std::abort(), it works. Basically, you're using <cstdlib> and then using ::abort(). However, the <cstdlib> module defines std::abort(), not ::abort(). Those are different declarations because they are in different namespaces. Changing to std::abort() should fix your problem -- I believe Clang is correct (albeit perhaps a bit pedantic) here.

mauliknshah commented 3 years ago

I think, I didn't add one crucial piece of information, which is I am working on the "Swift for Tenserflow 0.12 Release" toolchain. I am sorry for the late response.

xanderdunn commented 3 years ago

@mauliknshah Swift for Tensorflow forks each release off of Swift dev trunk. Hence it's the same issue on both toolchains.

@Lukasa are you able to make the minor change that @ldionne suggested?

Lukasa commented 3 years ago

The code in question is BoringSSL and it has been compiling correctly for months. The only change is clang’s enforcement. If you believe clang is right and Boring is wrong, we should report this bug upstream.

ldionne commented 3 years ago

The code in question is BoringSSL and it has been compiling correctly for months.

Compiling for months doesn't mean much. Sometimes the compiler changes or improves diagnostics and code that was seemingly valid forever starts breaking. It's just the way it is, and it's impossible to make things better without sometimes breaking a few things.

The only change is clang’s enforcement. If you believe clang is right and Boring is wrong, we should report this bug upstream.

Yes, I believe that would be the best way forward.

Lukasa commented 3 years ago

Out of curiosity, trunk of both clang and GCC do not consider this a problem, at least according to godbolt: https://godbolt.org/z/Mz5G8a. This seems like to be a clang and macOS specific issue?

ldionne commented 3 years ago

You're not compiling with modules enabled.

Lukasa commented 3 years ago

Sure, but there's no error even if I enable modules: https://godbolt.org/z/5jPsbo.

Irregardless I'm fine with not litigating it, happy to push this upstream to Boring.

SercanKaraoglu commented 3 years ago

Why do you use BoringSSL btw, especially looking at here, it doesn't sound good:

Although BoringSSL is an open source project, it is not intended for general use, as OpenSSL is. We don't recommend that third parties depend upon it. Doing so is likely to be frustrating because there are no guarantees of API or ABI stability.
Lukasa commented 3 years ago

We discussed this in 2018 on the Swift forums: https://forums.swift.org/t/rfc-moving-swiftnio-ssl-to-boringssl/18280

SercanKaraoglu commented 3 years ago

Okay I see it thanks, looks like pure Swift crypto implementation would have been more future proof but wasn't possible due to the effort required at that time. Self contained SSL also sounds good

Lukasa commented 3 years ago

Should be fixed by #261, if you get a moment I'd love to have you test with the relevant branch to confirm it fixes your problem.

xanderdunn commented 3 years ago

Huge thanks @Lukasa! I'm traveling at the moment and will be able to test it tomorrow.

xanderdunn commented 3 years ago

Inflight wifi managed to do a git pull! I successfully built swift-nio-ssl latest main commit 62bf5083df970e67c886210fa5b857eacf044b7c using both Swift trunk snapshot 2020-12-05 and Swift For TensorFlow 0.12. The issue appears to be resolved! Thank you! Could you roll a new release so that downstream projects can peg their swift-nio-ssl dependency to this fix?

Lukasa commented 3 years ago

Yup, we're doing our release validation at the moment. Hopefully should be in the next 24 hours or so.

Lukasa commented 3 years ago

Should be resolved by 2.10.2. Thanks for your patience here folks!