499019 - Analysis thinks reachable code is unreachable in array_sort

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[Benjamin Smedberg]

When I take the full set of patches for bug 491988 and bug 498398 (do JS_REQUIRES_STACK analysis on function pointers), the analysis is doing something... strange. In array_sort it claims that a block of code is unreachable which is definitely actually reachable:

http://hg.mozilla.org/users/bsmedberg_mozilla.com/redgreen-pointer-analysis/file/011783a8cf4c/js/src/jsarray.cpp#l2275

dmandelin, other than running the ESP tracing to find out ESP thinks this code is unreachable, I don't understand the trace output enough to know what to do next. Could you have a look? You should be able to reproduce by pulling the repo I linked above and building with static analysis... let me know if you can't!

Comment 1

[David Mandelin [:dmandelin]]

AFAICT, the analysis is finding that line to be reachable. But it doesn't find that out until its second pass analyzing that block, which is entirely normal and impossible to avoid in general. Here are the log notes from those two passes:

# Pass 1
/home/dmandelin/sources/redgreen-pointer-analysis/js/src/jsarray.cpp:2275:26: upd\
ate bb: BB61
merge BB37 -> BB61
      | Not Reached
  in state:
      | Not Reached
    loc: /home/dmandelin/sources/redgreen-pointer-analysis/js/src/jsarray.cpp:227\
5:26
    CALL_EXPR               void     :=  js_LeaveTrace(cx)
      | Not Reached

# Pass 2
/home/dmandelin/sources/redgreen-pointer-analysis/js/src/jsarray.cpp:2275:26: upd\
ate bb: BB61
merge BB37 -> BB61
      | T array_sort:T, vec:!0, newlen:!0, fval:!0
  in state:
      | T array_sort:T, vec:!0, newlen:!0, fval:!0
    loc: /home/dmandelin/sources/redgreen-pointer-analysis/js/src/jsarray.cpp:227\
5:26
    CALL_EXPR               void     :=  js_LeaveTrace(cx)
      | 1 array_sort:1, vec:!0, newlen:!0, fval:!0

I assume the reason you noticed this was the spurious errors in this block:

/home/dmandelin/sources/redgreen-pointer-analysis/js/src/jsarray.cpp: In function ‘JSBool array_sort(JSContext*, uintN, jsval*)’:
/home/dmandelin/sources/redgreen-pointer-analysis/js/src/jsarray.cpp:2279: error: cannot call JS_REQUIRES_STACK function js_AllocStack(JSContext*, uintN, void**)
/home/dmandelin/sources/redgreen-pointer-analysis/js/src/jsarray.cpp:2287: error: cannot call JS_REQUIRES_STACK function js_FreeStack(JSContext*, void*)

The problem is that the checker is trying to check each site as the analysis proceeds, so it checks it both on the first pass, where the block is 'currently' not reached, and the second pass, where it is. In itself, checking right away is not necessarily a problem: when the ESP algorithm is partway done, the current states represent the possible program states for some subset of the actual paths. Thus, if there are no errors for all paths, there will be no errors for the subset, and no errors are reported, exactly as desired.

But the second problem is that the code the checker uses to get the current state returns a state of 'any value' (TOP) and so returns an error. The current state as a summary should be 'no value/not reached' (BOTTOM), which represents no program paths, and so no errors should be reported.

I think the correct fix matching jorendorff's original design intent is not to report errors when the current state is 'not reached'. I'll make a patch on that idea in a bit.

Comment 2

[David Mandelin [:dmandelin]]

Attachment: Patch for checker bug

Here's a patch for the problem identified above.

Comment 3

[Jason Orendorff [:jorendorff]]

Comment on attachment 384522 [details] [diff] [review]
Patch for checker bug

I think jsstack.js is the buggiest code I have ever written.

Comment 4

[David Mandelin [:dmandelin]]

Pushed to TM as 56748b2c1911. Is this bug fixed now?

Comment 5

[Benjamin Smedberg]

Yes.

Comment 6

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/56748b2c1911