Closed Bug 450392 Opened 16 years ago Closed 16 years ago

SM: fixing -Wstrict-aliasing=2 warnings in dtoa.c

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: igor, Assigned: igor)

References

Details

Attachments

(1 file)

Currently dtoa.c, when compiled with GCC 4.2 with -fstrict-aliasing -Wstrict-aliasing=2, generates over 100 warnings like:

dtoa.c:1114: warning: dereferencing type-punned pointer might break strict-aliasing rules

Since such warnings may indicate real bugs and since -fstrict-aliasing is enabled by default with GCC at -O2/-Os optimization levels, we should fix the code to avoid the warnings.
Attached patch fix v1Splinter Review
The patch changes word0, word1 and dval macros to assume that their argument has union type U, not double. This required to declare few variables to be of type U and update the places that use the variable in a double context to use dval() macro.

The patch also fixes aliasing warnings in jsapi.c|jscntxt.c|jsdbgapi.cpp so now -Wstrict-aliasing=2 generates the single warning in jsarray.c in:

static JSBool
array_lookupProperty(JSContext *cx, JSObject *obj, jsid id, JSObject **objp,
                     JSProperty **propp)
{
...
    *propp = (JSProperty *)&(obj->fslots[JSSLOT_ARRAY_LOOKUP_HOLDER]);
...
} 

Here the warning is a true false positive as the result is never dereferenced as JSProperty *.
Attachment #333542 - Flags: review?(crowder)
Comment on attachment 333542 [details] [diff] [review]
fix v1

Bigger patch than I hoped, but smaller than I feared.  I hope this is the last of the big changes we need to make to dtoa.c.  I'll try to contact dtoa.c's author again about upstreaming your work and some of the tweaks I made earlier.

Should we add -Wstrict-aliasing=2 to Makefile.ref ?  Makefile.in ?  Would be good not to regress here again.  Does this patch apply cleanly to the tracemonkey repo, or will it merge seamlessly?  How does that work?
Attachment #333542 - Flags: review?(crowder) → review+
Out of curiosity, did you try benchmarking this code against the last version?  Is this slower?  Faster?
(In reply to comment #3)
> Out of curiosity, did you try benchmarking this code against the last version? 

Do you have any particular benchmarks? 
I had a little microbenchmark I was using for the old dtoa bug, it was just a loop that converted a few different double values to strings, and recorded its time-delta.
With the patch I do see a slowdown of 1% for the best time of the test below that tries to exercise string->number conversion. This is on Linux-64. A similar test for double->string conversion shows no performance differences. I guess this is due to inevitable string allocation that puts too much noise to expose any slowdown or speedup.

function test(N) {

    var d1 = "0.2008";
    var d2 = "3e50";
    var d3 = "7e-50";
    var zero = 0.0;
    var sum = 0;
    var start_time = Date.now();
    for (var i = 0; i != N; ++i) {
        sum += zero * d1 * d2 * d3 * d1 * d2 * d3;
    }
    var time = Date.now() - start_time;
    if (sum !== 0.0)
        throw "Bad result: "+ sum;
    return time;
}

var array = [];
for (var i = 0; i != 20; ++i) {
    var time = test(1e5);
    if (i != 0)
        array.push(time);
}

// Print sorted array of the results
array.sort(function(a, b) a - b);
print(array);

Blocks: 450691
(In reply to comment #2)
> Should we add -Wstrict-aliasing=2 to Makefile.ref ?  Makefile.in ?

I filed the bug 450691 for that.

> Does this patch apply cleanly to the
> tracemonkey repo, or will it merge seamlessly?

The patch applies cleanly there.

landed - http://hg.mozilla.org/index.cgi/mozilla-central/rev/043ea4ef249c
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
I backed this out to try to fix failed mochitests:

Windows:
*** 67512 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/content/tests/widgets/test_tooltip.xul | step 1 clientX - got 11, expected 14
*** 67513 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/content/tests/widgets/test_tooltip.xul | step 1 clientY - got 20, expected 23
*** 67525 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/content/tests/widgets/test_tooltip.xul | step 3 clientX - got 93, expected 96
*** 67526 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/content/tests/widgets/test_tooltip.xul | step 3 clientY - got 20, expected 23

Linux:
*** 26070 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/ajax/scriptaculous/test_Scriptaculous.html | testElementMorph - 7 assertions, 1 failures, 0 errors
*** 67156 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/content/tests/widgets/test_tooltip.xul | step 1 clientX - got 11, expected 14
*** 67157 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/content/tests/widgets/test_tooltip.xul | step 1 clientY - got 24, expected 27
*** 67169 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/content/tests/widgets/test_tooltip.xul | step 3 clientX - got 108, expected 111
*** 67170 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/content/tests/widgets/test_tooltip.xul | step 3 clientY - got 24, expected 27
*** 67182 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/content/tests/widgets/test_tooltip.xul | step 4 clientX - got 200, expected 201
*** 67188 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/content/tests/widgets/test_tooltip.xul | hover tooltip attribute top position of tooltip - got 200, expected 201
*** 67200 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/content/tests/widgets/test_tooltip.xul | step 6 clientX - got 198, expected 199
*** 67206 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/content/tests/widgets/test_tooltip.xul | hover tooltip after size increased top position of tooltip - got 198, expected 199
*** 67219 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/content/tests/widgets/test_tooltip.xul | step 8 clientX - got 200, expected 201
*** 67225 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/content/tests/widgets/test_tooltip.xul | hover tooltip after size decreased top position of tooltip - got 200, expected 201

Mac (Seamonkey) seemed OK.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Sorry, it looks like this patch was not to blame. It can be relanded.
re-landed - http://hg.mozilla.org/index.cgi/mozilla-central/rev/85e2992d921b
Status: REOPENED → RESOLVED
Closed: 16 years ago16 years ago
Resolution: --- → FIXED
Flags: in-testsuite-
Flags: in-litmus-
The author of dtoa.c released a new version of dtoa.c on Monday (2009-03-16)
that fixes not only the strict aliasing issues but also the parentheses and some
(but not all) "may be used uninitialized" gcc warnings.  You can get the new
dtoa.c from http://www.netlib.org/fp/dtoa.c and read the change log at
http://www.netlib.org/fp/changes.

I'm going to submit NSPR's old dtoa.c patch in bug 362134 to the author of dtoa.c.
If you have patched your copy of dtoa.c, I encourage you to submit your patches
to the author.
The original dtoa bug (bug 384244) contains the original dtoa.c I worked against, so it shouldn't be -too- hard to extract the delta between that and our current dtoa.c, to figure out what we've changed.  I think we have not been very cautious about recording those changes.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: