taskGNU Astronomy Utilities - Tasks: task #16114, Print warning where integer...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

task #16114: Print warning where integer overflow happens

Submitter:  Pedram Ashofteh-Ardakani <pedram>
Submitted:  Tue 01 Feb 2022 10:31:18 AM UTC
   
 
Should Start On:  Tue 01 Feb 2022 12:00:00 AM UTC Should be Finished on:  Tue 01 Feb 2022 12:00:00 AM UTC
Category:  Arithmetic Priority:  5 - Normal
Item Group:  Enhancement Status:  None
Privacy:  Public Assigned to:  None
Percent Complete:  80% Open/Closed:  Open
Effort:  0.00

Jump to the original submission

Mon 05 Jun 2023 12:04:26 AM UTC, comment #16: 

Thanks a lot for the nice work Siyang! I reviewed the commits and made some additional changes in the overflowcheck branch of my development fork. Have a look at the commits messages and changes (the good news is that the compile time is now almost identical to the 'master' branch).

To get a good statistical feeling of the effect of the overflow checks on the run-time, I wrote the following script after installing the Gnuastro of this branch (note that the script has to be run as root to allow cleaning the RAM). I chose 64-bit integer to best show the difference:


type=int64

astarithmetic 10000 10000 2 makenew 100 u8 + \
              20 mknoise-sigma $type -onoised-1.fits
astarithmetic 10000 10000 2 makenew 100 u8 + \
              20 mknoise-sigma $type -onoised-2.fits

for i in $(seq 25); do
    echo test $i
    sync; echo 3 > /proc/sys/vm/drop_caches
    { time astarithmetic noised-1.fits noised-2.fits x -ojunk.fits -g1 -q; } \
        2> junk.txt
    awk '/^real/{print $2}' junk.txt | sed -e's|0m||' -e's|s||' >> $type.txt
done


Here is the result as two histograms:



In short: the overflow checks almost double the run-time :-(. Can you have a look to see if you can reproduce this difference? Did I do a bad change somewhere in my commits?

Mohammad Akhlaghi <makhlaghi>
Group administrator
Tue 30 May 2023 01:29:14 AM UTC, comment #15: 

Hi Mohammad, I ran

astarithmetic noised.fits noised.fits + -g1

which took around 0.7 seconds with or without overflow checks.




I also ran

astarithmetic noised.fits noised.fits x noised.fits x -g1

which took about 1 second with or without overflow checks.


So far it looks like the overflow checks aren't having a noticeable effect on the runtime speed. Good to see!


I have also updated my code to print one overflow message per arithemtic operation. You can find it at https://codeberg.org/decaf/gnuastro/src/branch/overflow-check.

Siyang He <decaf>
Wed 24 May 2023 01:03:30 PM UTC, comment #14: 

For the speed check, disable the warning message when re-building Gnuastro with overflow check. Otherwise, with all these pixels, the warning messages will take the time.

In the end, we just want one warning message for the whole command (so if multiple pixels result in an overflow, only one warning is printed). We can do this by setting a flag variable (that overflow happened or not) and the check will just activate this flag.

After all the inputs are complete, the function will just print the warning once (no matter how many overflow happened).

Do you think you can implement this also?

Mohammad Akhlaghi <makhlaghi>
Group administrator
Wed 24 May 2023 12:56:28 PM UTC, comment #13: 

Thanks Siyang.

Now that the building time is done, let's test the effect of the over-flow check on the run-time of a program. This will help to decide if we want to have it as the default mode (so no-overflow check should be given an option), or not.

You can make an arbitrarily large (in this case 10000x10000 pixels) unsigned 8-bit integer image with the command below.


$ astarithmetic 10000 10000 2 makenew 100 u8 + \
                20 mknoise-sigma u8 -onoised.fits


Then try a basic '+' operator on the noised image two times: once when you build Arithmetic with over-flow check and once without it.  Just make sure to empty your RAM (as root) before every run (so the loading time is not included):


$ sudo su
# sync; echo 3 > /proc/sys/vm/drop_caches


Mohammad Akhlaghi <makhlaghi>
Group administrator
Wed 03 May 2023 03:06:33 AM UTC, comment #12: 

Hi Mohammad, I have updated the overflow check per the comments. The code remains at https://codeberg.org/decaf/gnuastro/src/branch/overflow-check

Overview of changes:
- Error reporting: I switched fprintf() for error().
- Compilation time: I replaced switch statements with the token pasting operator at three places:
      1. Determining the appropriate overflow check,
      2. Determining the output type when printing error messages,
      3. Determining whether to include overflow checking code at all. Many operators, such as ">", never cause overflows, and overflow checks would only add to compilation time without providing any benefit.
- I also updated doc/gnuastro.texi to include the GAL_ARITHMETIC_FLAG_OVERFLOW_CHECK flag.

On my machine it takes 5 minutes to compile the master branch, and about 5:30 for the overflow check.

Siyang He <decaf>
Mon 24 Apr 2023 11:14:53 PM UTC, comment #11: 

Thanks a lot for the nice commit Siyang, being able to read through all those layers of macros and editing/adding the necessary components was not trivial!

I was able to compile the branch successfully. To test it, I added the following line after the variable definitions of 'arithmetic_operator_run' in 'bin/arithmetic/arithmetic.c' (to activate overflow checking):


flags|=GAL_ARITHMETIC_FLAG_OVERFLOW_CHECK;


After running the following command (using 'tests/during-dev.sh'), I was able to see the warning message:


$ astarithmetic 200 100 u8 + --quiet
Overflow. Operation: +. Type: uint8_t. Left: 200. Right: 100. Max: 255. Min: 0.
44


Here are some comments:

  • For printing warnings, it is much better to use the 'error()' function (with 'EXIT_SUCCESS' as its first argument). If you search for 'EXIT_SUCCESS' in Gnuastro's source, you will see many instances. The reason is that 'error()' also prints the name of the calling program (which is necessary when the user sees the warning in the output of a script with many programs). It also has some other very useful features that follow basic standards. Don't worry about portability, since Gnuastro uses the GNU portability library, this function will work in Gnuastro's source even for those non-GNU operating systems that don't have 'error()'.
  • You can't break the error message with 'error()' into in multiple calls like you do with 'fprintf'. To fix this, all the separate 'COMPOSE_OVERFLOW_ERR_MSG' calls can write a string (using 'asprintf' for example) and you can put the call of 'error()' at the end of the function and print the string in the same call ;-).
  • The main thing that slightly worried me was the compilation time. Have you investigated how to speed up the compilation? For the user it doesn't really matter (since they only compile once), but during developing, it becomes important.
  • I have sent you the copyright form to fill and submit. Hopefully by the time that is done, we will also make progress on the points above and we can merge it into the 'master' branch ;-).
Mohammad Akhlaghi <makhlaghi>
Group administrator
Wed 29 Mar 2023 08:21:19 PM UTC, comment #10: 

Excellent Mohammad!

distcheck passed and you can find my changes at https://codeberg.org/decaf/gnuastro/src/branch/overflow-check

Overview of changes:
- lib/gnuastro/arithmetic.h: I define the flag GAL_ARITHMETIC_FLAG_OVERFLOW_CHECK.
- lib: I add "overflow_check" versions of the plus, minus, multiply, and divide operations.
- lib/arithmetic.c: I call the appropriate version (check for overflow vs. don't check) of each arithmetic function from arithmetic_binary().
- lib/gnuastro-internal/arithmetic-binary.h: I define a few more levels of macros for overflow checks.

I would appreciate your feedback. Thank you!

Siyang He <decaf>
Tue 28 Mar 2023 01:41:53 PM UTC, comment #9: 

Thanks a lot for informing us about this problem Siyang! Nice catch! Fortunately the fix was pretty simple (it must have been inserted in a debugging effort, but we forgot to remove that extra option!).

I fixed it as part of bug #63969 (that I just defined). Just one tip: in terms of project management, it is always important to keep things separate/modular: even though you found this problem while working on this task, it is not related to this task, so you should define a "bug" for it (generally, all errors are bugs) ;-).

Thanks again!

Mohammad Akhlaghi <makhlaghi>
Group administrator
Tue 28 Mar 2023 04:25:30 AM UTC, comment #8: 

I was running "make distcheck" when I encountered the following errors:

ERROR: files left in build directory after distclean:
./tests/tmpdir-psf-select-stars/axisratio-match.fits
./tests/tmpdir-psf-select-stars/axisratio-raw.fits
./tests/tmpdir-psf-select-stars/catalog-gaia--17--10.fits
./tests/tmpdir-psf-select-stars/catalog-main--17--6.000000e+00.fits
./tests/tmpdir-psf-select-stars/catalog_contaminated_objects.txt
./tests/tmpdir-psf-select-stars/circular-objects.fits
./tests/tmpdir-psf-select-stars/less-than-2-stars.txt
./tests/tmpdir-psf-select-stars/more-than-2-stars.txt
./tests/tmpdir-psf-select-stars/parallax-good.fits
./tests/tmpdir-psf-select-stars/skycoverage.fits


For comparison, I ran "make distcheck" on the unmodified source files on the master branch too, and got the same errors. Any idea how I can fix this?

Siyang He <decaf>
Wed 01 Mar 2023 03:06:00 PM UTC, comment #7: 

Good to know Mohammad. I can take this one.

Siyang He <decaf>
Tue 21 Feb 2023 04:57:24 PM UTC, comment #6: 

Thanks for looking into this problem. In this case, it does indeed seem necessary to do manual checks on each operator! But having it active all the time will force a significant overhead; we should add a flag for this so it is only activated when the user wants.

Flags are passed to the Arithmetic library through bits in the 'flags' argument. You can see their definition macros in the macros starting with 'GAL_ARITHMETIC_FLAG_' of 'lib/gnuastro/arithmetic.h' (searching for their names in the Arithmetic library's source files will hopefully give you some demos of how to use them).

For the time being, you can just add this flag manually in the top-level 'gal_arithmetic' function (and not take too much time for bringing it from the command-line). Later, I can connect it to a command-line option easily ;-).

About the final problem with '-128'. The reason is that "blank" values for signed integers are defined to be the minimum value of that type. In floating points, "blank" values are "NaN" or "Not a Number". Similar to NaNs in a floating point type, in Gnuastro any operation on an integer blank value will return false or the blank value itself.

Mohammad Akhlaghi <makhlaghi>
Group administrator
Tue 21 Feb 2023 03:55:33 AM UTC, comment #5: 

I took a look at this.

Initially I tried to check errno's value against ERANGE, but never found a match. From googling I get the sense that it's on the C function's part to set errno (as opposed to having the OS set it automatically).

If this is true then I'd have to write overflow checks for each operator.

...so I wrote an overflow checker for addition. Right now I am calling the checker from arithmetic_binary() within lib/arithmetic.c. Specifically, arithmetic_binary() calls the checker before calling arithmetic_plus(). If this is a good place to call the checker then I can go ahead and write checkers for other operators.

As an aside, while testing my checker I found the following :


>$ astarithmetic 4 -128 int8 +
-128


I don't think I see this on the task list.

Siyang He <decaf>
Sun 29 Jan 2023 12:44:22 PM UTC, comment #4: 

This task is open for anyone who is interested.

Mohammad Akhlaghi <makhlaghi>
Group administrator
Sun 22 May 2022 01:07:55 PM UTC, comment #3: 

Have you made any progress on this Tushar?

Mohammad Akhlaghi <makhlaghi>
Group administrator
Fri 11 Mar 2022 10:20:37 AM UTC, comment #2: 

This is a good bug you chose Tashar! Its all yours ;-).

Mohammad Akhlaghi <makhlaghi>
Group administrator
Fri 11 Mar 2022 08:05:32 AM UTC, comment #1: 

I went to this error and want to fix it . Could you please assign this task to me.
Thank you

Tushar Gupta <tushar_gupta>
Tue 01 Feb 2022 10:31:18 AM UTC, original submission:  

As explained in bug #61969 , arithmetic reads inputs and uses the smallest numerical data type possible to compute more efficiently with the least amount of memory. However, the user might not be aware of the integer overflow that might happen when calculations result in values that need larger storage types. For example:


$ astarithmetic 254 2 +
0


This happens because 254 and 2 are both of type uint8 that can store a maximum value of 255, so instead of getting the expected result 256, we'd get 0 as the result of an integer overflow. One way to work around this for now is to use floating points in the inputs, for instance:


$ astarithmetic 254.0 2 +
256


Since this is happening silently, we need to at least print a warning to notify the user of the circumstances. So this task is defined here as Mohammad pointed out in bug #61969:

> C library's 'errno.h' has a special error code for such range overflows: 'ERANGE'. We should add a check for this error code within arithmetic and if it happens, a warning should be printed that an overflow has happened (to standard-error; so its even printed with '--quiet'!).

Pedram Ashofteh-Ardakani <pedram>
Group Member

 

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

Attached Files
file #54813:  int64-both.jpg added by makhlaghi (27KiB - image/jpeg)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by decaf (Posted a comment)
  • -email is unavailable- added by makhlaghi (Posted a comment)
  • -email is unavailable- added by tushar_gupta (Posted a comment)
  • -email is unavailable- added by pedram (Submitted the item)
  • -email is unavailable- added by pedram
  •  

    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.

     

    Follow 5 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2023-06-04 makhlaghi Attached File- Added int64-both.jpg, #54813
    2023-04-24 makhlaghi Percent Complete0% 80%
    2023-01-29 makhlaghi Assigned totushar_gupta None
    2022-03-11 makhlaghi Assigned toNone tushar_gupta
    2022-02-01 pedram Carbon-Copy- Added makhlaghi

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code