False positive when using virtual base class (could be clang bug)

[rianhunter]

with: stack 9bab2f0999d05a2c661db14a8ad95b20fe363030 clang version 3.5 (trunk 197014) Target: x86_64-apple-darwin13.0.0 Thread model: posix

Getting a false positive with this code:

include <iostream>

class Yo {
public:
  virtual ~Yo() {}
  virtual int cool() = 0;
};

class Inner : public virtual Yo {
public:
  int cool() {return 1;}
};

Yo *nice(Yo *a) {
  if (!a) return 0;
  return new Inner();
}

Yo *volatile foo;

int  main() {
  std::cout << nice(foo) << std::endl;
  return 0;
}

It happens due to an superfluous check for null after "new Inner()." This seems to be a compiler bug but wanted to double check here first. Two changes to this code can make this false positive go away, either change:

-class Inner : public virtual Yo {
+class Inner : public Yo {

or (no longer relevant)

-  if (!a) return 0;
-  return new Inner();
+  return a ? new Inner() : 0;

(Edit: second case isn't relevant for this bug report, it pushes the null check to a basic block where stack, or an optimizer, is unable to infer that the check is redundant)

If this is verified to be a Clang bug I will report there.

[xiw]

Interesting. This happens to virtual classes. The corresponding null-check generation part in clang is in CodeGenFunction::GetAddressOfBaseClass() in lib/CodeGen/CGClass.cpp. I'll commit a fix that works around this issue.

[xiw]

Can you try if commit f081d323694e212efc291367a8dc2a5d5e596a84 fixes the issue?

[rianhunter]

Yup, f081d323694e212efc291367a8dc2a5d5e596a84 does fix the issue.

Could you explain what is going on here? After looking over CodeGenFunction::GetAddressOfBaseClass() and ShouldNullCheckClassCastValue() in lib/CodeGen/CGExprScalar.cpp it still seems like there is a bug in one of those functions. Either there shouldn't be a null check at all or the null check should occur later in the cast.notnull block when searching for the base class pointer. If Clang is generating extra code in this case it'd be nice to get this fixed upstream as well.

[xiw]

I agree it looks suspicious. I don't understand that part either...

[rianhunter]

Okay cool, feel free to close. I'll file a clang bug report.

[rianhunter]

Seems like this null check exists to deal with the arbitrary case of the derived class pointer being null:

class Base {
};

class Derived : public virtual Base {
};

Derived *foo = nullptr;

Base *nice() {
  return foo;
}

This preceding code correctly generates a null check. It seems like the Clang codegen, probably for simplicity's sake, generates unnecessary code (intentionally assuming the optimizer will remove it). Is Stack's policy to add workarounds for these cases? I'm assuming there are other similar cases.

[xiw]

That makes sense.

Yeah, currently STACK recognizes such compiler-generated code via the names of basic blocks. See SimplifyDelete::visitDeleteBB() in src/SimplifyDelete.cc for other cases (e.g., new/delete).

[rianhunter]

Clang bug report here: http://llvm.org/bugs/show_bug.cgi?id=18243. Doesn't look like it's something they'll ever fix because I don't think they actually consider it broken

I think the incongruence here is that the clang codegen is designed to rely on the optimizer to kill the unnecessary code it generates, and stack is designed to catch redundant code that the optimizer would optimize away. So I guess it's inevitable that stack will need to have workarounds for clang (or it will have to work at the source/ast level).

It would be nice if there was a stricter contract on what code clang generated though. It feels wrong for stack to have workarounds.

[xiw]

I agree. Ideally we would need to implement our own code generation from the AST, or ask clang to mark compiler-generated code...