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

openmp_wrapper.h stubs signature use __GOMP_NOTHROW #3923

Merged
merged 4 commits into from
Feb 13, 2021

Conversation

bsergean
Copy link
Contributor

@bsergean bsergean commented Feb 8, 2021

Fix #3915. OpenMP stubs do not use the noexcept attibute which is present in the gcc version of openmp, and which trigger compilation errors as seen below. dmlc-core uses the same technique and macro.

/usr/lib/gcc/x86_64-linux-gnu/9/include/omp.h:114:12: error: declaration of ‘int omp_get_thread_num() noexcept’ has a different exception specifier
114 | extern int omp_get_thread_num (void) __GOMP_NOTHROW;
| ^~~~~~~~~~~~~~~~~~
...
xxx/include/LightGBM/utils/openmp_wrapper.h:81:14: note: from previous declaration ‘int omp_get_thread_num()’
81 | inline int omp_get_thread_num() {return 0;}
| ^~~~~~~~~~~~~~~~~~

Fix microsoft#3915. OpenMP stubs do not use the noexcept attibute which is present in the gcc version of openmp, and which trigger compilation errors as seen below. dmlc-core uses the same technique and macro.

/usr/lib/gcc/x86_64-linux-gnu/9/include/omp.h:114:12: error: declaration of ‘int omp_get_thread_num() noexcept’ has a different exception specifier
  114 | extern int omp_get_thread_num (void) __GOMP_NOTHROW;
      |            ^~~~~~~~~~~~~~~~~~
...
xxx/include/LightGBM/utils/openmp_wrapper.h:81:14: note: from previous declaration ‘int omp_get_thread_num()’
   81 |   inline int omp_get_thread_num() {return 0;}
      |              ^~~~~~~~~~~~~~~~~~
@bsergean
Copy link
Contributor Author

bsergean commented Feb 8, 2021

The lint plan fails with cpplint not being found. Sounds like a setup issue, and that is ran on travis it seems. All the other checks look happy though.

include/LightGBM/utils/openmp_wrapper.h Outdated Show resolved Hide resolved
Yes make sense, just changed it

Co-authored-by: James Lamb <[email protected]>
@StrikerRUS
Copy link
Collaborator

cpplint is working now and yields the following errror:

include/LightGBM/utils/openmp_wrapper.h:96:  Using C-style cast.  Use static_cast<int>(...) instead  [readability/casting] [4]

@bsergean Could you please fix it?

@bsergean
Copy link
Contributor Author

bsergean commented Feb 9, 2021

include/LightGBM/utils/openmp_wrapper.h:96:  Using C-style cast.  Use static_cast<int>(...) instead  [readability/casting] [4]

On line 96 there isn't any cast that I can see.

Do you know what's going on ? Is it in the inlined code that a cast exists ?

@StrikerRUS
Copy link
Collaborator

@bsergean

Do you know what's going on ?

Sorry, I'm not a cpp coder. If you are sure this error is false positive, feel free to mark that line as // NOLINT or // NOLINTNEXTLINE.

@StrikerRUS
Copy link
Collaborator

To cpp reviewers.
As we do not test threadless version of LightGBM on our CIs, I've run some tasks manually with -DUSE_OPENMP=OFF flag.
All builds with gcc 10.2.0 on macOS, gcc 4.8.4 on Linux, Clang 10.0.0 on Linux, AppleClang 9.1.0.9020039 on macOS were passed.

Copy link
Collaborator

@guolinke guolinke left a comment

Choose a reason for hiding this comment

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

Thank you!

@StrikerRUS
Copy link
Collaborator

@jameslamb Your review comment was addressed I believe, but your review blocks merging. Could you please unblock?

@jameslamb
Copy link
Collaborator

sorry, didn't realize my review was blocking

@jameslamb jameslamb merged commit 89da990 into microsoft:master Feb 13, 2021
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity since it was closed. To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues including a reference to this.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

openmp/gcc compatibility problem with nothrow attribute -> lead compile error
4 participants