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

Fix type precision in dorbdb6 and zunbdb6 #702

Merged
merged 1 commit into from
Aug 10, 2022

Conversation

mjacobse
Copy link
Contributor

@mjacobse mjacobse commented Aug 9, 2022

Description

Building shared libraries on current master 984dcc0 with -flto -Wlto-type-mismatch generates the warnings

lapack/SRC/dorbdb6.f:252: warning: type of ‘dlassq’ does not match original declaration [-Wlto-type-mismatch]
  252 |       CALL DLASSQ( M1, X1, INCX1, SCL, SSQ )
      | 
lapack/SRC/dlassq.f90:136: note: ‘dlassq’ was previously declared here
  136 | subroutine DLASSQ( n, x, incx, scl, sumsq )
      | 
lapack/SRC/dlassq.f90:136: note: code may be misoptimized unless ‘-fno-strict-aliasing’ is used

and

lapack/SRC/zunbdb6.f:224: warning: type of ‘dlamch’ does not match original declaration [-Wlto-type-mismatch]
  224 |       EPS = DLAMCH( 'Precision' )
      | 
lapack/INSTALL/dlamch.f:68: note: return value type mismatch
   68 |       DOUBLE PRECISION FUNCTION DLAMCH( CMACH )
      | 
lapack/INSTALL/dlamch.f:68: note: type ‘double’ should match type ‘float’
lapack/INSTALL/dlamch.f:68: note: ‘dlamch’ was previously declared here
lapack/INSTALL/dlamch.f:68: note: code may be misoptimized unless ‘-fno-strict-aliasing’ is used

showing that these calls to DLASSQ and DLAMCH are not correct; they are using wrong argument and return types respectively.

To me it looks like a simple oversight of not replacing REAL with DOUBLE PRECISION in two places. And that indeed gets rid of the type mismatches. Hence this pull request.

If the choice of REAL is intentional in these places, then I believe the calls should still be fixed, but in a different way. Perhaps casting to DOUBLE PRECISION before passing to DLASSQ and using SLAMCH instead of DLAMCH?

Checklist

  • [N/A] The documentation has been updated.
  • [N/A] If the PR solves a specific issue, it is set to be closed on merge.

@weslleyspereira
Copy link
Collaborator

Thanks! Maybe this is related to #695.

@weslleyspereira weslleyspereira merged commit d073b03 into Reference-LAPACK:master Aug 10, 2022
@weslleyspereira weslleyspereira mentioned this pull request Aug 10, 2022
@mjacobse
Copy link
Contributor Author

mjacobse commented Aug 10, 2022

Maybe it could be worth to add a CI build with link time optimization and -Werror=lto-type-mismatch to avoid similar bugs in the future? Adding explicit interfaces instead of the EXTERNAL declarations for external functions to detect this would probably be even better, but also much more effort. For LTO you simply pass the flag and can start detecting (some of?) these errors, albeit only at link time.

@weslleyspereira
Copy link
Collaborator

Thanks for the suggestion, @mjacobse. I spend 1 hour to find all type mismatches and implicit type casts in the library. See #703.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants