640300 - stack.js fails when a new call is the last statement in a block

Status: RESOLVED WONTFIX

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[Paul Biggar]

Attachment: Catch it and report an error

This happens a lot in elfhack.

Comment 1

[Benjamin Smedberg]

I don't understand the error... no "check"? The standard pattern was that GCC always assigns the result of operator new to a variable. What kind of code doesn't product that pattern here?

Comment 2

[Paul Biggar]

(In reply to comment #1)
> I don't understand the error... no "check"? The standard pattern was that GCC
> always assigns the result of operator new to a variable. What kind of code
> doesn't product that pattern here?

"no check" means that code like this:

  sometype* x () { return new sometype; }

that is, there is no check following the call to new.

I didn't check whether GCC assigned the result to new, is there a dump flag with which I can check the GCC IR just before dehydra gets it?

Comment 3

[Benjamin Smedberg]

Typically you just dump the interesting information from dehydra in your JS analysis. In this case, though, I'd expect there to be at least a STMT_RETURN after the call to operator new... how is it that operator new is really the *last* item in the block?

Comment 4

[Ehren Metcalfe]

Apologies for not helping with the other bugs but since I made the modifications that are throwing the error I could be of service.

If you look at the cfg pass using -fdump-tree-all you should see what the problem is.

With code like 

struct Blah {
  int x;
};

Blah* foo() { return new Blah; }

I see 

Blah* foo() ()
{
  void * D.2100;
  void * D.2098;
  struct Blah * D.2097;

<bb 2>:
  D.2100 = operator new (4);
  D.2098 = D.2100;
  D.2097 = (struct Blah *) D.2098;
  return D.2097;
}

the analysis always expects the cast except in the 'false positive' documented in the test suite.

If the error is right you're seeing

<bb X>
  D.2100 = operator new (4);
<bb N>
 something else

which seems weird. It could be differences in the version of 4.5 branch used to compile...

Comment 5

[Paul Biggar]

(In reply to comment #3)
> In this case, though, I'd expect there to be at least a STMT_RETURN
> after the call to operator new... how is it that operator new is really the
> *last* item in the block?

Actually, I'm wrong about the code sample above (I did it from memory, which failed me). Most of it is cases in a switch statement which do not have NULL checking. However, some of it is straightline code (a new followed by a branch, which should be fine to keep in the same basic block).

For the case statements, there really is no statement after the new:

  <L1>:
    D.41610 = operator new (4);
  
  <bb 10>:
    ...

The cast has been moved into the next block, presumably because the optimizer can see that all the other switch branches have the same cast.

I don't have a theory about why the straightline code get's split into two basic blocks, but it does.

Comment 6

[Ehren Metcalfe]

dumb question: have you disabled optimization?

Comment 7

[Paul Biggar]

(In reply to comment #6)
> dumb question: have you disabled optimization?

Why is that a dumb question? Is the assumption that this stuff all runs on non-optimized code?


As it turns out, I have. As confirmation, I didn't see any of the typical passes you'd see from optimization in the output of -ftree-dump-all.

Comment 8

[Ehren Metcalfe]

it's not a dumb question, you're right.

During some tests of other cfg treehydra scripts, I've had optimization completely mess things up.

Likely all of the dehydra scripts and process-pre-genericize-only treehydra scripts are fine at any level, but I wouldn't doubt that the other cfg scripts including ESP do incorrect things once GCC starts rearranging blocks.

Comment 9

[Ehren Metcalfe]

sorry I misread what you said, the error _does_ show up in non-optimized code?

Comment 10

[Ehren Metcalfe]

never mind. I've reproduced without optimization on elfhack elf.cpp

Comment 11

[Paul Biggar]

I think there's a small problem with this patch, which is that the actual check may be in the next block, and so the failure would be incorrect. However, I didnt come across that, so I think the best way to combat it is to put it in the error message.

How about:

"The result of operator new is not checked in the same basic block (possible false positive, please report if suspected)".

Comment 12

[Paul Biggar]

(In reply to comment #10)
> never mind. I've reproduced without optimization on elfhack elf.cpp

I reproduced both with |-Os -freorder-blocks|, and without.

Comment 13

[Ehren Metcalfe]

I think there might be an easy fix since I *think*, as you mentioned, the new block always falls through to the block with the cast.

Comment 14

[Benjamin Smedberg]

I'm just worried about this because the error is unrelated to the analysis being performed, which is a type analysis, and has nothing to do with checking on operator new calls. I guess it's fine if the error is basically an "analysis ICE": our analysis script is flawed (i.e. GCC is producing an IR which doesn't fit our expectations). But if we have testcases which produce the error, we really ought to fix the analysis script.

Comment 15

[Ehren Metcalfe]

I have an updated script + testcases reduced from elfhack. I'm just getting it together now.

Comment 16

[Paul Biggar]

Agreed, fixing beats ICEing.

Comment 17

[Ehren Metcalfe]

Attachment: mozilla-central patch

So far I've tested this on ElfHack, the xpcom static checking suite and partially on an older revision of mozilla-central. It works well for the IR patterns we know about (more general approach would be to use ESP but it's kinda overkill).

Comment 18

[Ehren Metcalfe]

Attachment: dehydra lib support

Comment 19

[(dormant account)]

Comment on attachment 518456 [details] [diff] [review]
mozilla-central patch

That's so unpolite of gcc to do this to us. I wonder if we can instead cheat in cfg_bb_iterator and bb_isn_iterator to "merge" pointless basic blocks in bb_isn_iterator & then skip em in  cfg_bb_iterator. Can do that in a followup

Comment 20

[Joshua Cranmer [:jcranmer]]

Dehydra and treehydra are no longer maintained by Mozilla.