Fix build of native components on FreeBSD. #17091

Merged
merged 2 commits into from Mar 16, 2017

Conversation

@AustinWise
Collaborator

AustinWise commented Mar 14, 2017

I tried building the native components on FreeBSD and got two build errors:

FreeBSD 11 started to define ENODATA, which was not defined in 10.3:

[  6%] Building CXX object System.Native/CMakeFiles/System.Native-Static.dir/pal_errno.cpp.o
/usr/home/austin/corefx/src/Native/Unix/System.Native/pal_errno.cpp:13:9: error: 'ENODATA' macro redefined [-Werror,-Wmacro-redefined]
#define ENODATA ENOATTR
        ^
/usr/include/c++/v1/errno.h:155:9: note: previous definition is here
#define ENODATA 9919
        ^
1 error generated.

The both clang3.5 and clang3.7 from pkg complains about the callback that OpenSSL calls to do locking:

/usr/home/austin/corefx/src/Native/Unix/System.Security.Cryptography.Native/openssl.cpp:1267:18: error: releasing mutex 'g_locks[n]' that was not held [-Werror,-Wthread-safety-analysis]
        result = pthread_mutex_unlock(&g_locks[n]);
                 ^
/usr/home/austin/corefx/src/Native/Unix/System.Security.Cryptography.Native/openssl.cpp:1270:9: error: mutex 'g_locks[n]' is not held on every path through here [-Werror,-Wthread-safety-analysis]
    if (result != 0)
        ^
/usr/home/austin/corefx/src/Native/Unix/System.Security.Cryptography.Native/openssl.cpp:1263:18: note: mutex acquired here
        result = pthread_mutex_lock(&g_locks[n]);
                 ^
2 errors generated.
-// ENODATA is not defined on FreeBSD.
-#if defined(__FreeBSD__)
+// ENODATA is not defined in FreeBSD 10.3 but is defined in 11.0
+#if defined(__FreeBSD__) & !defined(ENODATA)

This comment has been minimized.

@stephentoub

stephentoub Mar 14, 2017

Member

Does this need to be OS-specific? I'm wondering if it can just be:

#ifndef ENODATA
#define ENODATA ENOATTR
#endif

or is ENODATA only equivalent to ENOATTR specifically on FreeBSD?

@stephentoub

stephentoub Mar 14, 2017

Member

Does this need to be OS-specific? I'm wondering if it can just be:

#ifndef ENODATA
#define ENODATA ENOATTR
#endif

or is ENODATA only equivalent to ENOATTR specifically on FreeBSD?

@@ -1267,6 +1271,8 @@ static void LockingCallback(int mode, int n, const char* file, int line)
result = pthread_mutex_unlock(&g_locks[n]);
}
+#pragma clang diagnostic push

This comment has been minimized.

@nil4

nil4 Mar 14, 2017

Shouldn't this be clang diagnostic pop?

@nil4

nil4 Mar 14, 2017

Shouldn't this be clang diagnostic pop?

Fix push to pop.
You must not hop on pop.
@AustinWise

This comment has been minimized.

Show comment
Hide comment
@AustinWise

AustinWise Mar 15, 2017

Collaborator

Changes recently made to this pull request

I fixed the copy-paste error of changing push to pop.

Summary of Findings with on the idea of generally defining ENODATA as ENOATTR when the former does not exist

I did some research on ENODATA as @stephentoub suggested. In short, I don't think my change of defining ENODATA as ENOATTR is OS agnostic.

Specifically, the most recent version of POSIX (2008) defines ENODATA but does not define ENOATTR. And Linux, for example, does not use ENOATTR for much of anything.

Additional notes about ENODATA

Additionally, the current mapping between ENODATA and SocketError.NoData seems a little suspect.

POSIX defines ENODATA as:

No message is available on the STREAM head read queue.

So according to POSIX, ENODATA only has relevance to the old STREAMS system. However SocketError.NoData is defined as:

The requested name or IP address was not found on the name server.

WSANO_DATA, which SocketError.NoData is based on, says pretty much the same thing. So on Windows is has to do with DNS name lookup fails instead of STREAMS.

I have not had a change to check to see if the various libc libraries available for Linux do anything special with ENODATA, so I'm not entirely confident in recommending its removable from the PAL mapping. I may continue to research this as time permits.

Additional research on the usage of ENODATA

  • The Linux kernel uses ENODATA in a lot of places places as just a "invalid parameter" error code.
  • FreeBSD does not define ENODATA in their manual, but does define ENOATTR to indicate that an extended attribute does not exist. The use of these error codes in their source confirms this.
  • While NetBSD defines both in their manual, they mostly use ENODATA for extended attribute errors.
  • Illumos only defines ENODATA in their manual, and it is only used in a small number of libraries, drivers, and one place in STREAMS. ENOATTR is not defined.
  • Mac OS X, or rather at least Swift agrees with the definition of ENODATA as being related to STREAM.
Collaborator

AustinWise commented Mar 15, 2017

Changes recently made to this pull request

I fixed the copy-paste error of changing push to pop.

Summary of Findings with on the idea of generally defining ENODATA as ENOATTR when the former does not exist

I did some research on ENODATA as @stephentoub suggested. In short, I don't think my change of defining ENODATA as ENOATTR is OS agnostic.

Specifically, the most recent version of POSIX (2008) defines ENODATA but does not define ENOATTR. And Linux, for example, does not use ENOATTR for much of anything.

Additional notes about ENODATA

Additionally, the current mapping between ENODATA and SocketError.NoData seems a little suspect.

POSIX defines ENODATA as:

No message is available on the STREAM head read queue.

So according to POSIX, ENODATA only has relevance to the old STREAMS system. However SocketError.NoData is defined as:

The requested name or IP address was not found on the name server.

WSANO_DATA, which SocketError.NoData is based on, says pretty much the same thing. So on Windows is has to do with DNS name lookup fails instead of STREAMS.

I have not had a change to check to see if the various libc libraries available for Linux do anything special with ENODATA, so I'm not entirely confident in recommending its removable from the PAL mapping. I may continue to research this as time permits.

Additional research on the usage of ENODATA

  • The Linux kernel uses ENODATA in a lot of places places as just a "invalid parameter" error code.
  • FreeBSD does not define ENODATA in their manual, but does define ENOATTR to indicate that an extended attribute does not exist. The use of these error codes in their source confirms this.
  • While NetBSD defines both in their manual, they mostly use ENODATA for extended attribute errors.
  • Illumos only defines ENODATA in their manual, and it is only used in a small number of libraries, drivers, and one place in STREAMS. ENOATTR is not defined.
  • Mac OS X, or rather at least Swift agrees with the definition of ENODATA as being related to STREAM.
@mellinoe

This comment has been minimized.

Show comment
Hide comment
@mellinoe

mellinoe Mar 16, 2017

Contributor

Okay, based on that analysis, it sounds like ENOATTR is only equivalent to ENODATA on FreeBSD.

LGTM.

Contributor

mellinoe commented Mar 16, 2017

Okay, based on that analysis, it sounds like ENOATTR is only equivalent to ENODATA on FreeBSD.

LGTM.

@mellinoe mellinoe merged commit 821c66d into dotnet:master Mar 16, 2017

14 checks passed

Innerloop CentOS7.1 Debug x64 Build and Test Build finished.
Details
Innerloop CentOS7.1 Release x64 Build and Test Build finished.
Details
Innerloop OSX10.12 Debug x64 Build and Test Build finished.
Details
Innerloop OSX10.12 Release x64 Build and Test Build finished.
Details
Innerloop PortableLinux Debug x64 Build and Test Build finished.
Details
Innerloop PortableLinux Release x64 Build and Test Build finished.
Details
Innerloop Ubuntu14.04 Debug x64 Build and Test Build finished.
Details
Innerloop Ubuntu14.04 Release x64 Build and Test Build finished.
Details
Innerloop Windows_NT Debug x86 Build and Test Build finished.
Details
Innerloop Windows_NT Release x64 Build and Test Build finished.
Details
Vertical netfx Build Build finished.
Details
Vertical uap Build Build finished.
Details
Vertical uapaot Build Build finished.
Details
Windows_NT Debug AllConfigurations Build Build finished.
Details

@karelz karelz modified the milestone: 2.0.0 Mar 18, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment