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

 
 

bug #41827: Fix dead assignment octave_rand::do_float_scalar

Submitted by:  None
Submitted on:  Tue 11 Mar 2014 03:39:05 AM UTC  
 
Category: LibrariesSeverity: 3 - Normal
Priority: 6Item Group: Incorrect Result
Status: FixedAssigned to: None
Originator Name: Branden ArcherOriginator Email: -unavailable-
Open/Closed: ClosedRelease: dev
Operating System: GNU/Linux

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

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>
Project 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

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

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>
Project 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 File(s):
   
   
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
  • -unavailable- added by rik5 (Posted a comment)
  • -unavailable- added by None (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only project members can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 6 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Thu 20 Mar 2014 04:01:30 PM UTCrik5StatusIn Progress=>Fixed
      Open/ClosedOpen=>Closed
    Thu 20 Mar 2014 12:54:08 AM UTCNoneAttached File-=>Added 18542.patch, #30953
    Thu 13 Mar 2014 10:43:37 PM UTCrik5Priority5 - Normal=>6
      StatusNone=>In Progress
    Tue 11 Mar 2014 03:39:05 AM UTCNoneAttached File-=>Added 18539.patch, #30853

    Back to the top


    Powered by Savane 3.1-cleanup1