Coreclr gnuport #22129

franksinankaya · 2019-01-22T17:55:48Z

This is an attempt to port PAL layer to GNU compiler. The rest will follow later.

dnfclas · 2019-01-22T17:55:59Z

All CLA requirements met.

am11 · 2019-01-22T19:36:54Z

Many thanks! If this progresses all the way to CoreFX and CoreSetup, it would really help Solaris/SmartOS ports, where the main blocker is lack of full LLVM toolchain support (case in point missing LLDB, while Clang6 and LLVM6 were recently ported via pkgsrc). However, GCC is readily available.

franksinankaya · 2019-01-22T19:43:12Z

We can start building on top once this goes in. Everybody has different interests for this project. GCC is the common denominator for me too.

filipnavara · 2019-01-22T20:40:13Z

@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test
@dotnet-bot test Windows_NT x64 full_opt ryujit CoreCLR Perf Tests Correctness
@dotnet-bot test Windows_NT x86 full_opt ryujit CoreCLR Perf Tests Correctness

src/pal/src/exception/seh.cpp

src/pal/src/include/pal/context.h

src/pal/src/locale/utf8.cpp

src/pal/inc/pal.h

src/pal/src/sync/cs.cpp

MSVC and GNU compilers use different attributes for noinline. Abstract away compiler differences.

__cdecl is not defined by default on GNU compilers.

__sync_swap doesn't exist on GNU. Replacing with __atomic_exchange_n which is universally available.

A pointer value is usually unsigned long on most platforms. Casting it to integer causes signedness issues. Use size_t to be efficient on all 32 and 64 bit architectures.

Correct error statement. GNU G++ is picky about the string following the error statement with ' character in it. It needs to be enclosed with double quotes.

Seeing these warnings with GNU G++ compiler src/pal/src/sync/cs.cpp: In function ‘void CorUnix::InternalInitializeCriticalSectionAndSpinCount(PCRITICAL_SECTION, DWORD, bool)’: src/pal/src/sync/cs.cpp:630:48: warning: converting to non-pointer type ‘SIZE_T {aka long unsigned int}’ from NULL [-Wconversion-null] pPalCriticalSection->OwningThread = NULL; ^ src/pal/src/sync/cs.cpp: In function ‘void CorUnix::InternalLeaveCriticalSection(CorUnix::CPalThread*, _CRITICAL_SECTION*)’: src/pal/src/sync/cs.cpp:880:43: warning: converting to non-pointer type ‘SIZE_T {aka long unsigned int}’ from NULL [-Wconversion-null] pPalCriticalSection->OwningThread = NULL; ^

franksinankaya · 2019-01-23T04:55:49Z

I'm confused. Are these two failures caused by my change or did they exist already?

I reduced the changeset to a single commit and it still happens.

filipnavara · 2019-01-23T06:56:22Z

They look unrelated to your commit. I have seen elevated error rates on these two builds for the past few days. In most cases they look like infrastructure errors when the process is aborted due to timeout. However in this case the OS X build had some error I have not seen before.

@dotnet-bot test CentOS7.1 x64 Debug Innerloop Build
@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test

janvorli · 2019-01-23T08:54:49Z

@dotnet-bot test coreclr-ci please

franksinankaya · 2019-01-23T16:25:03Z

ok, this looks better after retries. I'll push the rest now.

franksinankaya · 2019-01-24T17:39:25Z

let me see if this retry command works.

@dotnet-bot test Ubuntu x64 Checked Innerloop Build and Test (Jit - TieredCompilation=0)
@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test
@dotnet-bot test Ubuntu x64 Checked Innerloop Build and Test
@dotnet-bot test coreclr-ci

janvorli · 2019-01-24T17:42:29Z

Don't worry about the coreclr-ci, it currently seems to never succeed in any PR.

franksinankaya · 2019-01-25T15:02:41Z

Anything else on this PR?

franksinankaya · 2019-01-30T15:49:02Z

What is the recommended review policy for this change? My initial plan was to do this piecemeal meaning get the PAL layer in and then do follow up with incremental patches. It looks like this patchset is growing as I started to compile other code outside of PAL now.

src/pal/src/exception/seh.cpp

src/pal/inc/pal.h

CMakeLists.txt

src/pal/inc/mbusafecrt.h

janvorli · 2019-01-30T16:18:32Z

What is the recommended review policy for this change?

It depends on how much you'll end up changing. If you expect just a few remaining things to fix, then it would make sense to have it all in one change. But if you think there is still a lot changes needed, then let's do it in smaller chunks.

GNU compiler doesn't support optnone attribute. pal/src/exception/seh-unwind.cpp:449:77: warning: ‘optnone’ attribute directive ignored [-Wattributes]

franksinankaya · 2019-01-30T16:44:04Z

What is the recommended review policy for this change?

It depends on how much you'll end up changing. If you expect just a few remaining things to fix, then it would make sense to have it all in one change. But if you think there is still a lot changes needed, then let's do it in smaller chunks.

I see the current state as coherent enough to be checked in. It allows PAL layered to be compiled with GCC without a single line of change assuming all builds pass. I'm sure there will be other changes in other directories but I think we have to handle them separately in order not to lose the context.

GNU compiler doesn't have an intrinsic for these. Open code them using the provided implementation.

/usr/include/string.h:43:28: error: declaration of ‘void* memcpy(void*, const void*, size_t) throw ()’ has a different exception specifier size_t __n) __THROW __nonnull ((1, 2));

franksinankaya · 2019-01-31T15:45:21Z

Time to check-in? Any other review comments?

janvorli

LGTM, thank you!

janvorli · 2019-01-31T15:48:44Z

After the Linux-musl CI leg completes, we can merge it in

franksinankaya · 2019-02-01T14:52:20Z

Is this something I need to run?
I'm not familiar with the Coreclr CI.

janvorli · 2019-02-01T15:27:13Z

No, these were the ci legs that were running automatically for your changes. Now all the legs have passed, so I am merging it in.

franksinankaya · 2019-02-01T15:29:23Z

Thank you!

franksinankaya · 2019-02-02T19:44:22Z

1.st follow up posted here: #22369

This contains a grab bag of fixes to fix the build with clang 10. - dotnet#23075 Fix missing includes in coreclr/src/debug/createdump/ - dotnet/runtime#33096 SList::Init: add missing constructor - A subset of dotnet#22129 Just the parts that introduce the THROW_DECL macro in pal.h - dotnet/runtime#32837 This fixes THROW_DECL introduce in the previous PR to work with clang, which is required in clang 10. - One new change: In a significant divergance, this commits adds more THROW_DECL macros to all the math functions to address a ton of errors pointed out when building SOS: In file included from /home/omajid/devel/dotnet/coreclr/src/ToolBox/SOS/Strike/strike.cpp:116: In file included from /home/omajid/devel/dotnet/coreclr/src/vm/hillclimbing.h:19: In file included from /home/omajid/devel/dotnet/coreclr/src/inc/complex.h:16: In file included from /usr/bin/../lib/gcc/x86_64-redhat-linux/10/../../../../include/c++/10/math.h:36: In file included from /usr/bin/../lib/gcc/x86_64-redhat-linux/10/../../../../include/c++/10/cmath:45: In file included from /usr/include/math.h:290: /usr/include/bits/mathcalls.h:53:13: error: exception specification in declaration does not match previous declaration __MATHCALL (acos,, (_Mdouble_ __x)); ^ /home/omajid/devel/dotnet/coreclr/src/pal/inc/pal.h:4395:26: note: previous declaration is here PALIMPORT double __cdecl acos(double); ^ Then, to make sure the declarations and implementations match, it adds THROW_DECL to the definitions in src/pal/src/cruntime/math.cpp

This contains a grab bag of fixes to fix the build with clang 10. - dotnet#23075 Fix missing includes in coreclr/src/debug/createdump/ - dotnet/runtime#33096 SList::Init: add missing constructor - A subset of dotnet#22129 Just the parts that introduce the THROW_DECL macro in pal.h - dotnet/runtime#32837 This fixes THROW_DECL introduce in the previous PR to work with clang, which is required in clang 10. - One new change: In a significant divergance, this commits adds more THROW_DECL macros to all the math functions to address a ton of errors pointed out when building SOS: In file included from /home/omajid/devel/dotnet/coreclr/src/ToolBox/SOS/Strike/strike.cpp:116: In file included from /home/omajid/devel/dotnet/coreclr/src/vm/hillclimbing.h:19: In file included from /home/omajid/devel/dotnet/coreclr/src/inc/complex.h:16: In file included from /usr/bin/../lib/gcc/x86_64-redhat-linux/10/../../../../include/c++/10/math.h:36: In file included from /usr/bin/../lib/gcc/x86_64-redhat-linux/10/../../../../include/c++/10/cmath:45: In file included from /usr/include/math.h:290: /usr/include/bits/mathcalls.h:53:13: error: exception specification in declaration does not match previous declaration __MATHCALL (acos,, (_Mdouble_ __x)); ^ /home/omajid/devel/dotnet/coreclr/src/pal/inc/pal.h:4395:26: note: previous declaration is here PALIMPORT double __cdecl acos(double); ^ Then, to make sure the declarations and implementations match, it adds THROW_DECL to the definitions in src/pal/src/cruntime/math.cpp Co-authored-by: Mike McLaughlin <mikem@microsoft.com> Co-authored-by: Sinan Kaya <sinan.kaya@microsoft.com> Co-authored-by: Tom Deseyn <tom.deseyn@gmail.com>

This contains a grab bag of fixes to fix the build with clang 10. - #23075 Fix missing includes in coreclr/src/debug/createdump/ - dotnet/runtime#33096 SList::Init: add missing constructor - A subset of #22129 Just the parts that introduce the THROW_DECL macro in pal.h - dotnet/runtime#32837 This fixes THROW_DECL introduce in the previous PR to work with clang, which is required in clang 10. - One new change: In a significant divergance, this commits adds more THROW_DECL macros to all the math functions to address a ton of errors pointed out when building SOS: In file included from /home/omajid/devel/dotnet/coreclr/src/ToolBox/SOS/Strike/strike.cpp:116: In file included from /home/omajid/devel/dotnet/coreclr/src/vm/hillclimbing.h:19: In file included from /home/omajid/devel/dotnet/coreclr/src/inc/complex.h:16: In file included from /usr/bin/../lib/gcc/x86_64-redhat-linux/10/../../../../include/c++/10/math.h:36: In file included from /usr/bin/../lib/gcc/x86_64-redhat-linux/10/../../../../include/c++/10/cmath:45: In file included from /usr/include/math.h:290: /usr/include/bits/mathcalls.h:53:13: error: exception specification in declaration does not match previous declaration __MATHCALL (acos,, (_Mdouble_ __x)); ^ /home/omajid/devel/dotnet/coreclr/src/pal/inc/pal.h:4395:26: note: previous declaration is here PALIMPORT double __cdecl acos(double); ^ Then, to make sure the declarations and implementations match, it adds THROW_DECL to the definitions in src/pal/src/cruntime/math.cpp Co-authored-by: Mike McLaughlin <mikem@microsoft.com> Co-authored-by: Sinan Kaya <sinan.kaya@microsoft.com> Co-authored-by: Tom Deseyn <tom.deseyn@gmail.com> Co-authored-by: Mike McLaughlin <mikem@microsoft.com> Co-authored-by: Sinan Kaya <sinan.kaya@microsoft.com> Co-authored-by: Tom Deseyn <tom.deseyn@gmail.com>

* Abstract away NOINLINE statement MSVC and GNU compilers use different attributes for noinline. Abstract away compiler differences. * Replace __sync_swap with __atomic_exchange_n __sync_swap doesn't exist on GNU. Replacing with __atomic_exchange_n which is universally available. * Define CDECL for GNUC __cdecl is not defined by default on GNU compilers. * Define gcc version of __declspec(thread) * Correct pointer casting A pointer value is usually unsigned long on most platforms. Casting it to integer causes signedness issues. Use size_t to be efficient on all 32 and 64 bit architectures. * Put quotes around the error string Correct error statement. GNU G++ is picky about the string following the error statement with ' character in it. It needs to be enclosed with double quotes. * Fix casting problem Seeing these warnings with GNU G++ compiler src/pal/src/sync/cs.cpp: In function ‘void CorUnix::InternalInitializeCriticalSectionAndSpinCount(PCRITICAL_SECTION, DWORD, bool)’: src/pal/src/sync/cs.cpp:630:48: warning: converting to non-pointer type ‘SIZE_T {aka long unsigned int}’ from NULL [-Wconversion-null] pPalCriticalSection->OwningThread = NULL; ^ src/pal/src/sync/cs.cpp: In function ‘void CorUnix::InternalLeaveCriticalSection(CorUnix::CPalThread*, _CRITICAL_SECTION*)’: src/pal/src/sync/cs.cpp:880:43: warning: converting to non-pointer type ‘SIZE_T {aka long unsigned int}’ from NULL [-Wconversion-null] pPalCriticalSection->OwningThread = NULL; ^ * Abstract optnone compiler attribute GNU compiler doesn't support optnone attribute. pal/src/exception/seh-unwind.cpp:449:77: warning: ‘optnone’ attribute directive ignored [-Wattributes] * Set the aligned attribute for GNU compiler * Make __rotl and __rotr functions portable GNU compiler doesn't have an intrinsic for these. Open code them using the provided implementation. * Define deprecated attribute for gcc * Add throw specifier for GCC /usr/include/string.h:43:28: error: declaration of ‘void* memcpy(void*, const void*, size_t) throw ()’ has a different exception specifier size_t __n) __THROW __nonnull ((1, 2)); Commit migrated from dotnet/coreclr@fc7a8fb

franksinankaya force-pushed the coreclr_gnuport branch 4 times, most recently from b856355 to 01be68f Compare January 22, 2019 19:26

janvorli reviewed Jan 22, 2019

View reviewed changes

src/pal/src/exception/seh.cpp Outdated Show resolved Hide resolved

src/pal/src/include/pal/context.h Show resolved Hide resolved

src/pal/src/locale/utf8.cpp Outdated Show resolved Hide resolved

janvorli reviewed Jan 22, 2019

View reviewed changes

src/pal/inc/pal.h Outdated Show resolved Hide resolved

jkotas reviewed Jan 22, 2019

View reviewed changes

src/pal/src/sync/cs.cpp Outdated Show resolved Hide resolved

franksinankaya added 7 commits January 22, 2019 23:56

Abstract away NOINLINE statement

6befc40

MSVC and GNU compilers use different attributes for noinline. Abstract away compiler differences.

Define CDECL for GNUC

12582d3

__cdecl is not defined by default on GNU compilers.

Replace __sync_swap with __atomic_exchange_n

1c9f923

__sync_swap doesn't exist on GNU. Replacing with __atomic_exchange_n which is universally available.

Define gcc version of __declspec(thread)

df20217

Correct pointer casting

88129ee

A pointer value is usually unsigned long on most platforms. Casting it to integer causes signedness issues. Use size_t to be efficient on all 32 and 64 bit architectures.

Put quotes around the error string

6a5e36c

Correct error statement. GNU G++ is picky about the string following the error statement with ' character in it. It needs to be enclosed with double quotes.

franksinankaya force-pushed the coreclr_gnuport branch 3 times, most recently from b216d7b to 6befc40 Compare January 23, 2019 03:39

franksinankaya force-pushed the coreclr_gnuport branch 2 times, most recently from 857d178 to e1a34ee Compare January 30, 2019 15:29

janvorli reviewed Jan 30, 2019

View reviewed changes

src/pal/src/exception/seh.cpp Show resolved Hide resolved

src/pal/inc/pal.h Outdated Show resolved Hide resolved

CMakeLists.txt Outdated Show resolved Hide resolved

src/pal/inc/mbusafecrt.h Show resolved Hide resolved

franksinankaya and others added 2 commits January 30, 2019 16:34

Abstract optnone compiler attribute

ddf89fa

GNU compiler doesn't support optnone attribute. pal/src/exception/seh-unwind.cpp:449:77: warning: ‘optnone’ attribute directive ignored [-Wattributes]

Set the aligned attribute for GNU compiler

42a8d7c

franksinankaya force-pushed the coreclr_gnuport branch from e1a34ee to 313d74a Compare January 30, 2019 16:49

franksinankaya added 3 commits January 30, 2019 16:55

Make __rotl and __rotr functions portable

aa6c7ef

GNU compiler doesn't have an intrinsic for these. Open code them using the provided implementation.

Define deprecated attribute for gcc

0f324d6

Add throw specifier for GCC

657f85b

/usr/include/string.h:43:28: error: declaration of ‘void* memcpy(void*, const void*, size_t) throw ()’ has a different exception specifier size_t __n) __THROW __nonnull ((1, 2));

franksinankaya force-pushed the coreclr_gnuport branch from 313d74a to 657f85b Compare January 30, 2019 16:55

janvorli approved these changes Jan 31, 2019

View reviewed changes

janvorli merged commit fc7a8fb into dotnet:master Feb 1, 2019

franksinankaya deleted the coreclr_gnuport branch February 1, 2019 15:29

omajid mentioned this pull request Jun 2, 2020

[release/2.1] Fix build with clang 10 #28045

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Coreclr gnuport #22129

Coreclr gnuport #22129

franksinankaya commented Jan 22, 2019

dnfclas commented Jan 22, 2019 •

edited

Loading

am11 commented Jan 22, 2019

franksinankaya commented Jan 22, 2019

filipnavara commented Jan 22, 2019

franksinankaya commented Jan 23, 2019

filipnavara commented Jan 23, 2019

janvorli commented Jan 23, 2019

franksinankaya commented Jan 23, 2019

franksinankaya commented Jan 24, 2019

janvorli commented Jan 24, 2019

franksinankaya commented Jan 25, 2019

franksinankaya commented Jan 30, 2019

janvorli commented Jan 30, 2019

franksinankaya commented Jan 30, 2019

franksinankaya commented Jan 31, 2019

janvorli left a comment

janvorli commented Jan 31, 2019

franksinankaya commented Feb 1, 2019

janvorli commented Feb 1, 2019

franksinankaya commented Feb 1, 2019

franksinankaya commented Feb 2, 2019

Coreclr gnuport #22129

Coreclr gnuport #22129

Conversation

franksinankaya commented Jan 22, 2019

dnfclas commented Jan 22, 2019 • edited Loading

am11 commented Jan 22, 2019

franksinankaya commented Jan 22, 2019

filipnavara commented Jan 22, 2019

franksinankaya commented Jan 23, 2019

filipnavara commented Jan 23, 2019

janvorli commented Jan 23, 2019

franksinankaya commented Jan 23, 2019

franksinankaya commented Jan 24, 2019

janvorli commented Jan 24, 2019

franksinankaya commented Jan 25, 2019

franksinankaya commented Jan 30, 2019

janvorli commented Jan 30, 2019

franksinankaya commented Jan 30, 2019

franksinankaya commented Jan 31, 2019

janvorli left a comment

Choose a reason for hiding this comment

janvorli commented Jan 31, 2019

franksinankaya commented Feb 1, 2019

janvorli commented Feb 1, 2019

franksinankaya commented Feb 1, 2019

franksinankaya commented Feb 2, 2019

dnfclas commented Jan 22, 2019 •

edited

Loading