Fix re-declarations of builtin functions with clang 10 by omajid · Pull Request #32837 · dotnet/runtime

omajid · 2020-02-26T00:20:30Z

Clang commit 39aa8954a4846b317d3da2f0addfce8224b438de has moved exception handling mismatches under the -fms-compatibility flag. This breaks compilation of pal under clang 10 (and newer).

The compilation error looks like this:

In file included from .../pal/src/misc/tracepointprovider.cpp:19:
In file included from .../pal/src/include/pal/palinternal.h:620:
In file included from /usr/bin/../lib/gcc/x86_64-redhat-linux/10/../../../../include/c++/10/stdlib.h:30:
/usr/include/stdlib.h:112:36: error: exception specification in declaration does not match previous declaration
__extension__ extern long long int atoll (const char *__nptr)
                                   ^
.../pal/inc/pal.h:4227:33: note: previous declaration is here
PALIMPORT long long int __cdecl atoll(const char *) THROW_DECL;
                                ^

The simplest fix seems to be to make clang do the same thing as gcc and define THROW_DECL as throw().

Testing via https://godbolt.org shows that even clang 3.3 compiles this successfully without additional compiler options:

    extern "C" long long atoll(char const*) throw();
    #include <stdlib.h>

An alternative fix would be to use -fms-compatibility.

More details at https://bugzilla.redhat.com/show_bug.cgi?id=1807176

omajid · 2020-03-02T16:20:47Z

Thanks to @serge-sans-paille for tracking down the root cause!

Clang commit 39aa8954a4846b317d3da2f0addfce8224b438de has moved exception handling mismatches under the -fms-compatibility flag. This breaks compilation of pal under clang 10 (and newer). The compilation error looks like this: In file included from .../pal/src/misc/tracepointprovider.cpp:19: In file included from .../pal/src/include/pal/palinternal.h:620: In file included from /usr/bin/../lib/gcc/x86_64-redhat-linux/10/../../../../include/c++/10/stdlib.h:30: /usr/include/stdlib.h:112:36: error: exception specification in declaration does not match previous declaration __extension__ extern long long int atoll (const char *__nptr) ^ .../pal/inc/pal.h:4227:33: note: previous declaration is here PALIMPORT long long int __cdecl atoll(const char *) THROW_DECL; ^ The simplest fix seems to be to make clang do the same thing as gcc and define THROW_DECL as 'throw()'. Testing via https://godbolt.org shows that even clang 3.3 compiles this successfully without additional compiler options: extern "C" long long atoll(char const*) throw(); #include <stdlib.h> An alternative fix would be to use -fms-compatibility. More details at https://bugzilla.redhat.com/show_bug.cgi?id=1807176

omajid · 2020-03-06T13:09:18Z

cc'ing some folks who last touched this part of the code, hoping to get a review: @janvorli @franksinankaya

janvorli

LGTM, thank you!

omajid · 2020-03-09T22:44:43Z

I just noticed that src/coreclr/src/pal/inc/mbusafecrt.h includes a similar declaration. Is it worth it fixing that too just for consistency?

This fixes 3 different set of build issues are seen when compiling with clang 10. - Clang 10 fails to compile slist.h because the code contained is actually invalid. The assignment operator being used doesn't exist. This is a backport of dotnet/runtime#33096 - Clang 10 has moved exception-handling mismatches in function declarations under the -fms-compatibility flag (instead of the -fms-extensions flag). Our declarations of atoll and other similar functions are missing the exception declaration `throw()`. This mismatch in exception declarations makes clang 10 unable to build this code. Fix it by defining THROW_DECL as `throw()` which is supported at least as far back as clang 3.3. This is a backport of dotnet/runtime#32837 - Clang 10 has enabled (or made default?) the `misleading-indentation` warning which causes errors when trying to compile code in src/tools/metainfo/mdinfo.cpp. This is a backport of dotnet/runtime#33406

This fixes 3 different set of build issues are seen when compiling with clang 10. - Clang 10 fails to compile slist.h because the code contained is actually invalid. The assignment operator being used doesn't exist. This is a backport of dotnet/runtime#33096 - Clang 10 has moved exception-handling mismatches in function declarations under the -fms-compatibility flag (instead of the -fms-extensions flag). Our declarations of atoll and other similar functions are missing the exception declaration `throw()`. This mismatch in exception declarations makes clang 10 unable to build this code. Fix it by defining THROW_DECL as `throw()` which is supported at least as far back as clang 3.3. This is a backport of dotnet/runtime#32837 - Clang 10 has enabled additional warnings. Lets turn of -Werror globally in this release branch by making the `-ignorewarnings` switch to `./build.sh` be the default.

This fixes 3 different sets of build issues seen when compiling with Clang 10. - Clang 10 fails to compile slist.h because the code contained is actually invalid. The assignment operator being used doesn't exist. This is a backport of dotnet/runtime#33096 - Clang 10 has moved exception-handling mismatches in function declarations under the -fms-compatibility flag (instead of the -fms-extensions flag). Our declarations of atoll and other similar functions are missing the exception declaration `throw()`. This mismatch in exception declarations makes clang 10 unable to build this code. Fix it by defining THROW_DECL as `throw()` which is supported at least as far back as clang 3.3. This is a backport of dotnet/runtime#32837 - Clang 10 has enabled additional warnings. Lets turn of -Werror globally in this release branch by making the `-ignorewarnings` switch to `./build.sh` be the default.

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>

Dotnet-GitSync-Bot added the area-Infrastructure-libraries label Feb 26, 2020

omajid closed this Feb 26, 2020

omajid reopened this Feb 26, 2020

omajid changed the title ~~WIP: Fix build with clang 10~~ WIP: Fix re-declations of builtin functions with clang 10 Feb 26, 2020

omajid closed this Feb 26, 2020

omajid reopened this Feb 26, 2020

omajid force-pushed the clang-10-llvm-throwdecl branch 4 times, most recently from 69abe8d to 365529b Compare March 2, 2020 16:15

omajid changed the title ~~WIP: Fix re-declations of builtin functions with clang 10~~ Fix re-declations of builtin functions with clang 10 Mar 2, 2020

omajid force-pushed the clang-10-llvm-throwdecl branch from 365529b to dd5c8f0 Compare March 2, 2020 16:16

omajid marked this pull request as ready for review March 2, 2020 16:16

omajid force-pushed the clang-10-llvm-throwdecl branch from dd5c8f0 to 4ead130 Compare March 2, 2020 16:56

omajid force-pushed the clang-10-llvm-throwdecl branch from 4ead130 to 4c8f9b2 Compare March 2, 2020 16:59

omajid changed the title ~~Fix re-declations of builtin functions with clang 10~~ Fix re-declarations of builtin functions with clang 10 Mar 2, 2020

janvorli approved these changes Mar 6, 2020

View reviewed changes

janvorli merged commit 9dd7cb5 into dotnet:master Mar 6, 2020

omajid mentioned this pull request Mar 11, 2020

[release/3.1] Fix build with clang 10 dotnet/coreclr#28026

Merged

omajid mentioned this pull request Jun 2, 2020

Q: runtime pal.h declarations #37310

Closed

omajid mentioned this pull request Jun 2, 2020

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

Merged

ghost locked as resolved and limited conversation to collaborators Dec 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix re-declarations of builtin functions with clang 10#32837

Fix re-declarations of builtin functions with clang 10#32837
janvorli merged 1 commit intodotnet:masterfrom
omajid:clang-10-llvm-throwdecl

omajid commented Feb 26, 2020 •

edited

Loading

Uh oh!

omajid commented Mar 2, 2020

Uh oh!

omajid commented Mar 6, 2020

Uh oh!

janvorli left a comment

Uh oh!

omajid commented Mar 9, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

omajid commented Feb 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

omajid commented Mar 2, 2020

Uh oh!

omajid commented Mar 6, 2020

Uh oh!

janvorli left a comment

Choose a reason for hiding this comment

Uh oh!

omajid commented Mar 9, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

omajid commented Feb 26, 2020 •

edited

Loading