Closed Bug 880208 Opened 7 years ago Closed 5 years ago

[PJS] Add UnsafeGet instrinsics

Categories

(Core :: JavaScript Engine, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED WONTFIX

People

(Reporter: nmatsakis, Assigned: nmatsakis)

References

Details

(Whiteboard: [leave open])

Attachments

(2 files, 1 obsolete file)

When implementing array-like types in self-hosted code, and in particular multi-dimensional arrays, it is helpful to be able to disable redundant bounds checking. It is also helpful to be able to give hints as to what data is fully encapsulated and will not change. To this end I have added two intrinsics:

    UnsafeGetElement(arr, idx)
    UnsafeGetImmutableElement(arr, idx)

Both are equivalent to arr[idx], but (in ion-compiled code) the former skips bounds checks, and the latter skips bounds checks and also assumes that the data cannot be mutated by stores.
Attachment #762075 - Flags: review?(kvijayan)
Comment on attachment 762075 [details] [diff] [review]
Add UnsafeGet and UnsafeGetImmutable.

Review of attachment 762075 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/ion/MCallOptimize.cpp
@@ +1060,5 @@
>  
>  IonBuilder::InliningStatus
> +IonBuilder::inlineUnsafeGetElement(CallInfo &callInfo,
> +                                   GetElemSafety safety)
> +{

Seems like |safety| can only be one of |GetElem_Unsafe| or |GetElem_UnsafeImmutable|, and never |GetElem_Normal|.

Would be good to assert that here.
Attachment #762075 - Flags: review?(kvijayan) → review+
Assignee: general → nmatsakis
Address djvj's comments.
Attachment #762075 - Attachment is obsolete: true
Attachment #762449 - Flags: review+
Depends on: 883395
Unfortunately, I had to back this out in:

https://hg.mozilla.org/integration/mozilla-inbound/rev/a8e3d80187d1

This caused breakage in jsfunfuzz itself (see bug 883395), another sample testcase is also coming up. There was no response on IRC when I pinged for some time, and I also spoke to djvj and Luke (in-person).

The testcase in bug 883395 also looks like it could cause real-world breakage, so backing out to avoid having to deal with nightly crashes. Please feel free to request for fuzzing of patches from :decoder or me (:gkw).
Attached file Debug stack
function exploreDeeper(a, n) {
    var hns = Object.getOwnPropertyNames(a);
    for (var j = 0; j < hns.length; ++j) {
        var hn = hns[j];
        try {
            va = a[hn];
        } catch (e) {}
    }
}
gns = Object.getOwnPropertyNames(this);
for (var i = 0; gns.length; ++i) {
    var gn = gns[i];
    if (gn.charCodeAt(0) < 0x60) {
        g = this[gn];
        if (g.toString().indexOf("[native code]") != -1) {
            exploreDeeper(g.prototype, +".e");
        }
    }
}

asserts rev 7fc2d9ea82b6 at Assertion failure: args[arri].isObject(), at vm/SelfHosting.cpp - tested on a threadsafe 64-bit debug shell on Mac 10.8 (--enable-more-deterministic is not required)
https://hg.mozilla.org/mozilla-central/rev/18c1fd169792
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
This was backed out, see above.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [leave open]
Target Milestone: mozilla24 → ---
Depends on: 883524
No longer depends on: 883524
The problem is that the ParallelArray code doesn't include guards to check that `this` is always a parallel array object (and, presumably, to take a slow path in the event that it's not, so as to be "proxy-proof").
(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #9)
> This was backed out, see above.

Backout made mozilla-central:

http://hg.mozilla.org/mozilla-central/rev/a8e3d80187d1
Blocks: PJS
Status: REOPENED → RESOLVED
Closed: 7 years ago5 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.