Skip to content

TSQR tests fail when compiled with -fcheck=bounds #481

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

Closed
thijssteel opened this issue Feb 2, 2021 · 14 comments · Fixed by #510
Closed

TSQR tests fail when compiled with -fcheck=bounds #481

thijssteel opened this issue Feb 2, 2021 · 14 comments · Fixed by #510
Assignees
Milestone

Comments

@thijssteel
Copy link
Collaborator

thijssteel commented Feb 2, 2021

On my machine, the tests all pass when using cmake and ctest.

However, with the following line in my make.inc:
FFLAGS = -O0 -frecursive -ggdb3 -fcheck=bounds
The TSQR tests fail.

Specifically, this line goes out of bounds. It seems like it could be an actual bug.

If KNB = N - KB + 1, then A( KB+KNB, KB) = A( N + 1, KB ) which might be out of bounds.

@weslleyspereira
Copy link
Collaborator

I observed the same error here.

Breakpoint 1, sorgtsqr_row (m=1, n=1, mb=3, nb=1, a=..., lda=1, t=..., ldt=1, work=..., lwork=1, info=0) at sorgtsqr_row.f:356
357	     $               A( KB+KNB, KB), LDA, WORK, KNB )
(gdb) whatis A
type = real(kind=4) (1,*)
(gdb) print KB+KNB
$15 = 2

@thijssteel
Copy link
Collaborator Author

Besides solving the failure of that specific test, i think it would be great to have bounds checks in the unit tests that run in the ci. Maybe a special testing target in cmake or something?

@langou
Copy link
Contributor

langou commented Feb 9, 2021

Hi @thijssteel, (1) yes, bound checks in the travis continuous integration would be terrific. (2) I am not sure how to enable this. Julien.

@christoph-conrads
Copy link
Contributor

(2) I am not sure how to enable this. Julien.

The most convenient method is probably via environment variables, e.g., env FFLAGS='-fcheck=bounds' cmake -DCMAKE_BUILD_TYPE=Debug -- ~/lapack. In the .travis-ci.yml, you could add the following lines then:

--- .travis.yml 2021-02-12 17:05:07.904837210 +0100                                                                                    
+++ my-travis.yml       2021-02-12 17:05:47.672775138 +0100                                                                            
@@ -14,6 +14,8 @@                                                                                                                      
 matrix:                                                                                                                               
   include:                                                                                                                            
   - os: linux                                                                                                                         
+    env: CMAKE_BUILD_TYPE=Debug FFLAGS='-fbounds-check'                                                                               
+  - os: linux                                                                                                                         
     env: CMAKE_BUILD_TYPE=Release                                                                                                     
   - os: linux                                                                                                                         
     env: CMAKE_BUILD_TYPE=Coverage                                                                                                    

@christoph-conrads
Copy link
Contributor

Failing tests due to this problem:

The following tests FAILED:
         13 - LAPACK-xlintsts_stest_in (Failed)
         35 - LAPACK-xlintstd_dtest_in (Failed)
         57 - LAPACK-xlintstc_ctest_in (Failed)
         79 - LAPACK-xlintstz_ztest_in (Failed)
Errors while running CTest

@langou
Copy link
Contributor

langou commented Feb 12, 2021

Just spoke off line with @weslleyspereira, here what we would like to do

(1) we want to add a target in Travis that will track the problem with "out of bounds" as suggested by @christoph-conrads

(2) but we want to add such a test only after fixing the actual "out of bounds" problems we already have in the code. I understand that @scr2016 is looking into the TSQR "out of bounds" problem. @weslleyspereira and I suggest that we wait on @scr2016 before editing our Travis' target.

@martin-frbg
Copy link
Collaborator

There is a bunch of trivial bounds errors in the tests that are already reported at the -Wextra level, typically inside a DO .. J=1,N an array element J-1 is set or otherwise referenced...e.g. in cbdt03.f

line  194 |             DO 20 J = 1, N
...                                              
line  202 |                   WORK( J-1 ) = WORK( J-1 ) + E( J-1 )

@thijssteel
Copy link
Collaborator Author

I don't want to offend anyone, but wouldn't it be better to just revert those couple of commits and make a PR? Its clear that they would have benefitted from a few reviews.

@martin-frbg
Copy link
Collaborator

Ah, those were the things that landed straight on master... guess I should open a new issue for "my" finds, it just looked convenient to put them here as "related".

@thijssteel
Copy link
Collaborator Author

thijssteel commented Feb 15, 2021

ah, those tests you just reported are not related to the Householder reconstruction routines.

@martin-frbg
Copy link
Collaborator

Indeed not, must be fairly old - I just chanced onto them with a gfortran10 -Wextra build

@weslleyspereira
Copy link
Collaborator

I don't want to offend anyone, but wouldn't it be better to just revert those couple of commits and make a PR? Its clear that they would have benefitted from a few reviews.

Hi @thijssteel. Could you please provide the hash codes for the commits? I would like to participate in this discussion. :)

@thijssteel
Copy link
Collaborator Author

@weslleyspereira

874b678 and 0db2865
They were committed to master without a pull request, see also #371

but i made that comment thinking the extra error martin reported was also related to that code, if its just the one error its probably fine.

@weslleyspereira
Copy link
Collaborator

Thanks @thijssteel! I saw the commits

scr2016 pushed a commit to scr2016/lapack that referenced this issue Mar 11, 2021
….f for bug Reference-LAPACK#481 (FFLAGS = -O0 -frecursive -ggdb3 -fcheck=bounds

The TSQR tests fail, array index out of bounds)
@scr2016 scr2016 linked a pull request Mar 11, 2021 that will close this issue
langou added a commit that referenced this issue Mar 11, 2021
@julielangou julielangou added this to the LAPACK 3.9.1 milestone Mar 25, 2021
christoph-conrads pushed a commit to christoph-conrads/lapack that referenced this issue May 23, 2021
….f for bug Reference-LAPACK#481 (FFLAGS = -O0 -frecursive -ggdb3 -fcheck=bounds

The TSQR tests fail, array index out of bounds)
christoph-conrads pushed a commit to christoph-conrads/lapack that referenced this issue May 23, 2021
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 a pull request may close this issue.

7 participants