Paul Biggar Reporter
	Description • 14 years ago

This happens a lot in elfhack.

	Benjamin Smedberg
	Comment 1 • 14 years ago

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?

	Paul Biggar Reporter
	Comment 2 • 14 years ago

(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?

	Benjamin Smedberg
	Comment 3 • 14 years ago

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?

	Ehren Metcalfe
	Comment 4 • 14 years ago

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

	Paul Biggar Reporter
	Comment 5 • 14 years ago

(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.

	Ehren Metcalfe
	Comment 6 • 14 years ago

dumb question: have you disabled optimization?

	Paul Biggar Reporter
	Comment 7 • 14 years ago

(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.

	Ehren Metcalfe
	Comment 8 • 14 years ago

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.

	Ehren Metcalfe
	Comment 9 • 14 years ago

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

	Ehren Metcalfe
	Comment 10 • 14 years ago

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

	Paul Biggar Reporter
	Comment 11 • 14 years ago

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)".

	Paul Biggar Reporter
	Comment 12 • 14 years ago

(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.

	Ehren Metcalfe
	Comment 13 • 14 years ago

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.

	Benjamin Smedberg
	Comment 14 • 14 years ago

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.

	Ehren Metcalfe
	Comment 15 • 14 years ago

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

	Paul Biggar Reporter
	Comment 16 • 14 years ago

Agreed, fixing beats ICEing.

	Ehren Metcalfe
	Comment 17 • 14 years ago

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).

	(dormant account)
	Comment 19 • 14 years ago

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

	Joshua Cranmer [:jcranmer]
	Comment 20 • 12 years ago

Dehydra and treehydra are no longer maintained by Mozilla.