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.

Sign up for GitHub

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

Jump to bottom

Fix many compiler warnings #8203

Merged
merged 1 commit into from
Nov 23, 2020
Merged

Fix many compiler warnings #8203

merged 1 commit into from
Nov 23, 2020

Conversation

jsquyres
Copy link
Member

  • Add some missing AC_CHECK_SIZEOF's in configure.ac
  • Remove some unused variables
  • Initialize some variables
  • Fix some parameter types
  • Cast where appropriate/safe to fix warnings
  • Move ompi/mca/common/monitoring Fortran bindings to a separate .c file so that they can use different #define's than the C bindings, and therefore compile properly / without warnings. (@bosilca please review this part)
  • Fix signedness discrepancies
  • Who knew? Separated these into multiple #if's, instead: 
    // This is undefined behavior
#define HAVE_FOO defined(FOO)
#define YOW (HAVE_FOO && defined(BAR))
  • Fix some typos in OMPI_BUILD_HOST logic
  • Don't "2>/dev/null" in OMPI_BUILD_HOST logic; it just hides errors

Signed-off-by: Jeff Squyres jsquyres@cisco.com

@jsquyres
Copy link
Member Author

@bosilca @hjelmn @devreal This PR contains the OSC signed change that we started discussing in email.

jsquyres added a commit to jsquyres/ompi that referenced this pull request Nov 12, 2020
@jsquyres
COMMIT MESSAGE TO BE AMENDED
50ed871 
This is *more-or-less* a cherry pick of the master PR
open-mpi#8203, but with a bunch of
v4.1.x-specific warnings fixed, too (and some dropped from open-mpi#8203 that
aren't relevant here in the v4.1.x branch).

This commit message will be amended after open-mpi#8203 is finalized+merged.

Signed-off-by: Jeff Squyres <jsquyres@cisco.com>
@jsquyres jsquyres mentioned this pull request Nov 12, 2020
Merged
@jsquyres
Copy link
Member Author

Pushed some changes so that I can git pull down to a different machine to fix the Fortran/monitoring issue.

@jsquyres
Fix many compiler warnings:
14aa5fa 
- Add some missing AC_CHECK_SIZEOF's in configure.ac
- Remove some unused variables
- Initialize some variables
- Fix some parameter types
- Cast where appropriate/safe to fix warnings
- Move ompi/mca/common/monitoring Fortran bindings to a separate .c
  file so that they can use different #define's than the C bindings,
  and therefore compile properly / without warnings.
- Fix signedness discrepancies
- Who knew?  Separated these into multiple #if's, instead:
  ```
  // This is undefined behavior
  #define HAVE_FOO defined(FOO)
  #define YOW (HAVE_FOO && defined(BAR))
  ```
- Fix some typos in OMPI_BUILD_HOST logic
- Don't "2>/dev/null" in OMPI_BUILD_HOST logic; it just hides errors

Signed-off-by: Jeff Squyres <jsquyres@cisco.com>
@jsquyres jsquyres marked this pull request as ready for review November 14, 2020 18:13
@jsquyres
Copy link
Member Author

@bosilca @ggouaillardet @devreal Made all the changes from the discussion here. This PR now passes CI and is ready for review.

jsquyres added a commit to jsquyres/ompi that referenced this pull request Nov 14, 2020
@jsquyres
COMMIT MESSAGE TO BE AMENDED
dbaa448 
This PR is not yet final. It is more-or-less a cherry-pick from open-mpi#8203,
but with some v4.1.x-specific fixes and some not-relevant-to-v4.1.x
fixes dropped.

--> Final commit from open-mpi#8203 may be: 14aa5fa

The commit message on the commit(s) on this PR will be updated after

Fixes open-mpi#8195. This PR doesn't fix all the warnings from open-mpi#8195, but
fixes many of them (e.g., I didn't get the "string might be truncated"
warnings on my Mac).

Signed-off-by: Jeff Squyres <jsquyres@cisco.com>
@ggouaillardet
Copy link
Contributor

FWIW, strncpy() and strncat() were designed to zero the remaining part of the string in order to prevent any potential data leak (this is a little known fact I did not know for more than a decade). They were not designed to protect against all buffer overflows.
strlcpy() and strlcat() were later added for that, but these are not available everywhere, especially on Linux.
PMIx uses the pmix_strncpy() subroutine (which is strlcpy() like to me).

One option would be to define opal_strlcpy() and opal_strlcat() (that use the library implementation if available, and a simple Open MPI implementation otherwise, since performance is unlikely to be critical here).

@jsquyres
Copy link
Member Author

@bosilca @ggouaillardet @devreal Can you please review?

@bosilca
Copy link
Member

bosilca commented Nov 22, 2020

There are still few comments not addressed.

@jsquyres
Copy link
Member Author

@bosilca Which ones? I didn't think @ggouaillardet's comments about strlcat() etc. applied here because a) we decided just to use strcat(), and b) in the code path addressed in this PR, we are guaranteed that the strcat() is going to go into a buffer that is long enough.

@bosilca
Copy link
Member

bosilca commented Nov 22, 2020

You don't see my review ?

@devreal
Copy link
Contributor

devreal commented Nov 22, 2020

@jsquyres See my comment here: #8203 (comment) I don't see how the respective warning has been fixed. I think we still need to fix the function signature of ompi_coll_base_retain_datatypes_w as proposed here: #8203 (comment)

Otherwise it looks good to me 👍

@ggouaillardet
Copy link
Contributor

@devreal in #8207 the const qualifier was added to the internal struct ompi_coll_base_nbc_request_t.

@jjhursey
Copy link
Member

bot:ibm:retest

@jsquyres
Copy link
Member Author

Ok, I think all comments have been addressed. I'll merge.

@jsquyres jsquyres merged commit 3edd62e into open-mpi:master Nov 23, 2020
@jsquyres jsquyres deleted the pr/fix-warnings branch November 23, 2020 15:15
@ibm-ompi
Copy link

The IBM CI (GNU/Scale) build failed! Please review the log, linked below.

Gist: https://gist.github.com/ibm-ompi/88b480fb98dfd2bdac939f023e1f8fe6

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

@ggouaillardet ggouaillardet ggouaillardet left review comments

@bosilca bosilca bosilca left review comments

@devreal devreal devreal approved these changes

@hjelmn hjelmn Awaiting requested review from hjelmn

Assignees
No one assigned
Labels
Target: main Target: v5.0.x
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jsquyres @ggouaillardet @bosilca @devreal @jjhursey @ibm-ompi