bugGNU Octave - Bugs: bug #63643, [octave-forge] (image) warnings...

 
 

bug #63643: [octave-forge] (image) warnings about "abs" when compiling with clang

Submitter:  Dmitri A. Sergatskov <dasergatskov>
Submitted:  Wed 11 Jan 2023 06:53:01 PM UTC
   
 
Category:  Octave Package Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Build Failure
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * other Operating System:  * GNU/Linux
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Tue 31 Jan 2023 09:22:03 PM UTC, comment #23: 

Closing as fixed, according to comment #22

Avinoam Kalma <avinoam>
Group Member
Fri 27 Jan 2023 02:25:32 PM UTC, comment #22: 

I have tested the tar.gz file from comment #21 (only under linux this this) and it still seems fine to me. I think we are now done with this issue and could CLOSE it.

Hartmut <hardy>
Fri 27 Jan 2023 06:23:03 AM UTC, comment #21: 

Yes, of course.
Thanks.

(file #54270)

Avinoam Kalma <avinoam>
Group Member
Fri 27 Jan 2023 06:08:55 AM UTC, comment #20: 

Could you upload a (hopefully last) tar.gz file to easily check, Avinoam?

Hartmut <hardy>
Thu 26 Jan 2023 05:27:59 PM UTC, comment #19: 

I have added eps in some of the cases
see:
http://hg.code.sf.net/p/octave/image/rev/849afbab4913
If you think that I should add in more places, please let me know.

Avinoam Kalma <avinoam>
Group Member
Thu 26 Jan 2023 01:17:46 PM UTC, comment #18: 

I agree with Dmitri, that in general all asserts that compare floats should have a tolerance of at least eps. Maybe we really should obey this general rule here...

Hartmut <hardy>
Thu 26 Jan 2023 12:54:50 PM UTC, comment #17: 

I just recompiled against the latest snapshot and I have another
FAIL (:


octave:1> pkg load image
octave:2> test stretchlim
***** test
 assert (stretchlim ([(.05:.05:1) (2:4)], 0.2), [0.25; 0.95], eps)
 assert (stretchlim ([(.05:.05:1) (2:5)], 0.2), [0.25; 1])
 assert (stretchlim ([(.05:.05:1) (2:6)], 0.2), [0.3; 1], eps)
 assert (stretchlim ([(.05:.05:1) (2:7)], 0.2), [0.3; 1])
!!!!! test failed
ASSERT errors for:  assert (stretchlim ([(.05:.05:1), (2:7)], 0.2),[0.3; 1])

  Location  |  Observed  |  Expected  |  Reason
    (1)          0.3          0.3        Abs err 5.5511e-17 exceeds tol 0 by 6e-17



I think all asserts that compare double should have non-zero tolerance.

Dmitri.
--


Dmitri A. Sergatskov <dasergatskov>
Thu 26 Jan 2023 10:31:30 AM UTC, comment #16: 

With a default configuration it builds and passes all tests for me. But when I tried a build with ASAN I discover some
problems with its configure/build system. I will make a separate bug report about it.

Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Thu 26 Jan 2023 08:35:55 AM UTC, comment #15: 

I have tested the .tar.gz file in comment #14. It still seems fine to me (this time only tested under linux). If Dmitri does not see any more test failures we should be done now.

Hartmut <hardy>
Thu 26 Jan 2023 05:21:44 AM UTC, comment #14: 

I have also checked the package, both on Linux (Ubuntu) and on Win-10,
but did not see any failure.
I have added tolerance of eps to one of the tests in stretchlim.m, see
http://hg.code.sf.net/p/octave/image/rev/ca0c90300beb

The new version is attached.
Please check.

(file #54267)

Avinoam Kalma <avinoam>
Group Member
Wed 25 Jan 2023 07:46:49 PM UTC, comment #13: 

I have also tested the latest .tar.gz file from comment #11. It installs fine (pkg install) and all tests pass (pkg test) for me, on Win10 as well as on Linux. So I would consider the original issue to be SOLVED now.

Nevertheless it seem reasonable to me to add a 1*eps tolerance to the unit tests mentioned in comment #12, since Dmitri sees them failing on a different machine.

Hartmut <hardy>
Wed 25 Jan 2023 03:42:36 PM UTC, comment #12: 

I do not see any warning during compile of the latest snaposhot.
"pkg test image" has one failure at stretchlim.m


octave:3> pkg load image
octave:4> test stretchlim
***** test
 assert (stretchlim ([(.05:.05:1) (2:4)], 0.2), [0.25; 0.95], eps)
 assert (stretchlim ([(.05:.05:1) (2:5)], 0.2), [0.25; 1])
 assert (stretchlim ([(.05:.05:1) (2:6)], 0.2), [0.3; 1])
 assert (stretchlim ([(.05:.05:1) (2:7)], 0.2), [0.3; 1])
!!!!! test failed
ASSERT errors for:  assert (stretchlim ([(.05:.05:1), (2:6)], 0.2),[0.3; 1])

  Location  |  Observed  |  Expected  |  Reason
    (1)          0.3          0.3        Abs err 5.5511e-17 exceeds tol 0 by 6e-17


Perhaps relax tolerance to "eps" in those asserts?

Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Wed 25 Jan 2023 05:38:27 AM UTC, comment #11: 

Thanks for the comments.
I have replaced "int" with "octave_idx_type".
see
http://hg.code.sf.net/p/octave/image/rev/7c52a54947b5

Please check


(file #54259)

Avinoam Kalma <avinoam>
Group Member
Tue 24 Jan 2023 08:00:23 PM UTC, comment #10: 

I have tested the .tar.gz file linked in comment #8. It does look fine to me (it installs fine, and all tests still pass, both under linux as under Win10). But if there is risk involved with using an int type here (as Markus suggests in comment #9) I would rather like to stick to the octave_idx_type here as well.

Hartmut <hardy>
Tue 24 Jan 2023 09:13:10 AM UTC, comment #9: 

`int` might be too small for really large arrays. It might be better to use `octave_idx_type` like Hartmut suggested.

Markus Mützel <mmuetzel>
Group administrator
Tue 24 Jan 2023 05:42:36 AM UTC, comment #8: 

Fixed in http://hg.code.sf.net/p/octave/image/rev/b52ba21bf824
adding declaration of found as int.
The new version is attached.

Please check.

(file #54257)

Avinoam Kalma <avinoam>
Group Member
Mon 23 Jan 2023 09:23:28 PM UTC, comment #7: 

Sorry, Mea Culpa.
Thanks, @Hartmut for checking this.
I will fix it.

Avinoam Kalma <avinoam>
Group Member
Mon 23 Jan 2023 06:53:51 PM UTC, comment #6: 

I have downloaded the tar.gz file from comment #5. Here is what I see when installing it under linux:


>> pkg install image-2.15.0+.tar.gz
__spatial_filtering__.cc: In function ‘octave_value do_filtering(const MT&, const boolNDArray&, ETout (*)(MT&, octave_idx_type, int), const MT&, int)’:
__spatial_filtering__.cc:266:8: error: ‘found’ was not declared in this scope; did you mean ‘round’?
  266 |   for (found = 0; found < se_nnz; )
      |        ^~~~~
      |        round
make: *** [Makefile:33: __spatial_filtering__.oct] Fehler 1
make: *** Auf noch nicht beendete Prozesse wird gewartet …
make: Verzeichnis „/tmp/oct-UoODTc/image-2.15.0+/src“ wird betreten
/opt/octave-7.3.0/bin/mkoctfile-7.3.0  -Wall -g -O2 __spatial_filtering__.cc
/opt/octave-7.3.0/bin/mkoctfile-7.3.0  -Wall -g -O2 __bilateral__.cc
/opt/octave-7.3.0/bin/mkoctfile-7.3.0  -Wall -g -O2 __eps__.cc
/opt/octave-7.3.0/bin/mkoctfile-7.3.0  -Wall -g -O2 __custom_gaussian_smoothing__.cc
/opt/octave-7.3.0/bin/mkoctfile-7.3.0  -Wall -g -O2 __boundary__.cc
/opt/octave-7.3.0/bin/mkoctfile-7.3.0  -Wall -g -O2 bwfill.cc
/opt/octave-7.3.0/bin/mkoctfile-7.3.0  -Wall -g -O2 rotate_scale.cc
/opt/octave-7.3.0/bin/mkoctfile-7.3.0  -Wall -g -O2 hough_line.cc
make: Verzeichnis „/tmp/oct-UoODTc/image-2.15.0+/src“ wird verlassen

error: pkg: error running 'make' for the image package
error: called from
    configure_make at line 117 column 9
    install at line 202 column 7
    pkg at line 612 column 9


Maybe we should re-add the "octave_idx_type" again between "for (" and "found = 0 ...".

Hartmut <hardy>
Sun 22 Jan 2023 08:25:00 PM UTC, comment #5: 

Thanks,
I have pushed another fix.
http://hg.code.sf.net/p/octave/image/rev/091badc82091
and uploaded a new version of the image package.
Please check.

(file #54252)

Avinoam Kalma <avinoam>
Group Member
Tue 17 Jan 2023 10:17:48 PM UTC, comment #4: 


Thanks for testing. I will check this warning.

Avinoam Kalma <avinoam>
Group Member
Tue 17 Jan 2023 10:09:32 PM UTC, comment #3: 

I do not see any "abs" warnings any more. The only warning I get is:

clang++ -I/usr/local/include/octave-8.0.1/octave/.. -I/usr/local/include/octave-8.0.1/octave -I/usr/local/include  -pthread -fopenmp -g -O2 -mavx -mavx2 -march=native -mtune=native -flto=thin -g  -Wall -O2 -o __bilateral__.oct  /tmp/oct-vU8e4a.o   -shared -Wl,-Bsymbolic  -L/usr/local/lib -shared -Wl,-Bsymbolic  -L/usr/local/lib -shared -Wl,-Bsymbolic
__spatial_filtering__.cc:266:24: warning: variable 'i' set but not used [-Wunused-but-set-variable]
  for (octave_idx_type i = 0, found = 0; found < se_nnz; i++)
                       ^
1 warning generated.


Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Tue 17 Jan 2023 06:51:31 PM UTC, comment #2: 

I have pushed a fix
http://hg.code.sf.net/p/octave/image/rev/7b80a4e6f14e
and uploaded a new version of image pkg, I will be glad if you can check the fix.

(file #54241)

Avinoam Kalma <avinoam>
Group Member
Sat 14 Jan 2023 09:13:16 AM UTC, comment #1: 

Thanks for reporting this.

If you replace abs/fabs by std::abs/std::fabs, as the log suggests,
does it solve the problem?

Avinoam Kalma <avinoam>
Group Member
Wed 11 Jan 2023 06:53:01 PM UTC, original submission:  

Lots of warnings compiling bwdist.cc (4 warnings) and
_custom_gaussian_smoothing_.cc (80 warnings)


bwdist.cc:424:27: warning: using floating point absolute value function 'fabs' when argument is of integer type [-Wabsolute-value]
  return abs(x)>abs(y) ? (fabs(x) + sqrt2_1 * fabs(y)) :
                          ^
bwdist.cc:424:27: note: use function 'std::abs' instead
  return abs(x)>abs(y) ? (fabs(x) + sqrt2_1 * fabs(y)) :
                          ^~~~
                          std::abs

...

clang++ -std=gnu++14 -I/usr/local/include/octave-8.0.1/octave/.. -I/usr/local/include/octave-8.0.1/octave -I/usr/local/include  -pthread -fopenmp -O2 -mavx -mavx2 -march=native -mtune=native -flto=thin -g  -Wall -O2 -o bwdist.oct  /tmp/oct-FvWdqw.o   -shared -Wl,-Bsymbolic  -L/usr/local/lib -shared -Wl,-Bsymbolic  -L/usr/local/lib -shared -Wl,-Bsymbolic
__custom_gaussian_smoothing__.cc:58:43: warning: using integer absolute value function 'abs' when argument is of floating point type [-Wabsolute-value]
              const octave_idx_type rur = abs (k*(ct*sqrtv2 - st*sqrtv1)); // 'rur' means 'row-upper-right'
                                          ^
__custom_gaussian_smoothing__.cc:58:43: note: use function 'std::abs' instead
              const octave_idx_type rur = abs (k*(ct*sqrtv2 - st*sqrtv1)); // 'rur' means 'row-upper-right'
                                          ^~~
                                          std::abs

...


Full log is attached.

image 2.14.0; octave 8.0.1; clang 15.0.1 on CentOs Stream 9.

Dmitri.
--


Dmitri A. Sergatskov <dasergatskov>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #54270:  image-2.15.0+.tar.gz added by avinoam (458KiB - application/x-gzip)
file #54267:  image-2.15.0+.tar.gz added by avinoam (458KiB - application/x-gzip)
file #54259:  image-2.15.0+.tar.gz added by avinoam (458KiB - application/x-gzip)
file #54257:  image-2.15.0+.tar.gz added by avinoam (458KiB - application/x-gzip)
file #54252:  image-2.15.0+.tar.gz added by avinoam (458KiB - application/x-gzip)
file #54241:  image-2.15.0+.tar.gz added by avinoam (458KiB - application/x-gzip)
file #54215:  image_abs_warnings.txt.gz added by dasergatskov (1KiB - application/gzip)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by mmuetzel (Posted a comment)
  • -email is unavailable- added by hardy (Posted a comment)
  • -email is unavailable- added by avinoam (Posted a comment)
  • -email is unavailable- added by avinoam
  • -email is unavailable- added by avinoam
  • -email is unavailable- added by dasergatskov (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only group members can vote.

     

    Follow 18 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2023-01-31 avinoam Summary[octave-forge](image) warnings about &quot;abs&quot; when compiling with clang [octave-forge] (image) warnings about "abs" when compiling with clang
    2023-01-31 avinoam StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2023-01-27 avinoam Attached File- Added image-2.15.0+.tar.gz, #54270
    2023-01-26 avinoam Attached File- Added image-2.15.0+.tar.gz, #54267
    2023-01-25 avinoam Attached File- Added image-2.15.0+.tar.gz, #54259
    2023-01-24 avinoam Attached File- Added image-2.15.0+.tar.gz, #54257
        StatusIn Progress Ready For Test
    2023-01-23 avinoam StatusReady For Test In Progress
    2023-01-22 avinoam Attached File- Added image-2.15.0+.tar.gz, #54252
        StatusIn Progress Ready For Test
        Release8.0.90 other
    2023-01-19 avinoam StatusReady For Test In Progress
    2023-01-17 avinoam Attached File- Added image-2.15.0+.tar.gz, #54241
        StatusNone Ready For Test
    2023-01-14 avinoam Carbon-Copy- Added carandraug
        Carbon-Copy- Added hardy
    2023-01-11 dasergatskov Attached File- Added image_abs_warnings.txt.gz, #54215

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code