bugGNU Octave - Bugs: bug #41827, Fix dead assignment...

 
 

bug #41827: Fix dead assignment octave_rand::do_float_scalar

Submitter:  None
Submitted:  Tue 11 Mar 2014 03:39:05 AM UTC
   
 
Category:  Libraries Severity:  3 - Normal
Priority:  6 Item Group:  Incorrect Result
Status:  Fixed Assigned to:  None
Originator Name:  Branden Archer Originator Email:  -email is unavailable-
Open/Closed:  * Closed Release:  * dev
Operating System:  * GNU/Linux Fixed Release:  None
Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Thu 20 Mar 2014 04:01:30 PM UTC, comment #3: 

The first patch is always the hardest trying to understand all of our coding conventions and styles.  I made a few tweaks to your patch.

1) I added an initial line that summarizes the changeset and mentions the bug

"Fix dead assignment in oct-rand.cc do_float_scalar (bug #41827)."

2) In the test section, I added a line '%!test' just ahead of your new code so that your test will be counted by itself, rather than being a part of the previous %!test block.

3) I also added you to the list of contributors to Octave which is in the preface of the manual.  We like to acknowledge everyone who helps out.

The changeset is here (http://hg.savannah.gnu.org/hgweb/octave/rev/53ba52143af7).

Rik <rik5>
Group administrator
Thu 20 Mar 2014 12:54:08 AM UTC, comment #2: 

Thanks for the pointers. Attached please find an update patch which includes both the original fix along with a test.

Kindly let me know if you find any issue with the patch, or request any additional changes.


(file #30953)

Anonymous
Thu 13 Mar 2014 10:43:37 PM UTC, comment #1: 

Okay, I ran Octave under gdb to trace this down and the error is real.  Sample code to reproduce the problem is


randg ("seed", 1);  # Use old-style generators
randg ([-2 Inf], "single")  # Return "float" values
ans =

   0   0


To add a test case for this, take a look at Appendix B of the manual (Test and Demo Functions).  Basically, you just need to add the Octave code example I posted along with an assert statement that the return results are NaN, rather than 0.

The place to add this is with the rest of the built-in randg tests.  The file in question is oct-rand.cc in libinterp/corefcn.  Search for DEFUN.*randg to find the function definition for randg.  At the end of the function, in a long C-style comment block, are the built-in tests.  You will find tests that use a fixed "seed".  Add the test in there somewhere.

For the changset comment format, take a look at Appendix D (Contributing guidelines).  You are almost there.  We like to append the bug number that is being fixed in the first line like so


Comment comment comment comment (bug #XXXXX).


Further on, we name the file and function and what was modified.  We report what was done, rather than why it was done.  The justification for why is usually in the very first line, like fix bug XXXXX.  So instead of explaining what Clang found, we would probably write

  • oct-rand.cc (do_float_scalar): Correctly assign temporary output to dretval

rather than retval.

Rik <rik5>
Group administrator
Tue 11 Mar 2014 03:39:05 AM UTC, original submission:  

From running Clang's scan-build tool, which performs some static
source code analysis, on the current head from the repository,
it was found that the octave_rand::do_float_scalar() method in

   liboctave/numberic/oct-rand.cc

has a 'dead assignment' on line 447. That is, under a specific
scenario (gamma distribution, use_old_generators==true,
and certain values of the passed in float to the function)
the result would be 0.0 (default value) instead of octave_NaN
(assigned value). From the context, it appears that the result
should be octave_NaN but is not due to a typo.

Attached is a patch which resolves the issue.

Note that this was found with static source code analysis,
and not by producing the issue in octave proper. Instead
of just submitting a fix, I am also interested in writing
a test case that will actually produce the error and
show that it is resolved. Can you point me in the right direction
toward causing this issue to occur in octave, and how one
would include it as a test case for automated testing? If it
is not too bad, I would like to then resubmit the patch
and include the related automated test with the change.

Thanks!

Anonymous

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #30953:  18542.patch added by None (1KiB - text/x-diff - Patch with change + test)
file #30853:  18539.patch added by None (938B - text/x-diff)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by None (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 6 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2014-03-20 rik5 StatusIn Progress Fixed
        Open/ClosedOpen Closed
    2014-03-20 None Attached File- Added 18542.patch, #30953
    2014-03-13 rik5 Priority5 - Normal 6
        StatusNone In Progress
    2014-03-11 None Attached File- Added 18539.patch, #30853

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code