"noreturn" follows virtual method implementation (opposite of g++)

[mitchblank]

Bugzilla Link	12200
Version	unspecified
OS	MacOS X
Attachments	test case
CC	@AaronBallman,@DougGregor,@eugenis,@mitchblank,@zygoloid,@rjmccall,@rnk,@seanm

Extended Description

Note this problem is with /usr/bin/c++ in the latest XCode "Version 4.3 (4E109)" Sorry if this isn't the best place to file xcode bugs or if it's already fixed in trunk. I looked around bugzilla and didn't see a duplicate.

I'm porting a large commercial C++ package to build on clang. Things mostly compiled OK, but I've had to track down a tricky crash. The issue turned out to be a difference in how clang treats the combination of "virtual" and "attribute((noreturn))" on methods.

For instance, on g++ you can do:

  class A {
    virtual void handleFoo() __attribute__((noreturn)) { throw FooNotImplmented(); }
  };
  class B : public A  {
    void handleFoo() { do_something(); }
  };

Here the "noreturn" only applies to A::handleFoo() (and you'll get a warning if you DON'T add the attribute), not any method which might overload it. So if someone does:

  B b;
  A *a = &b;
  a->handleFoo();   // will return since this isn't really A::handleFoo()
  puts("after handleFoo");

In clang++, the "noreturn" attribute from the virtual method seems to be interpreted below and the puts() will end up elided.

Attached is a simple test program which demonstrates this:

  $ g++ a.cpp
  $ ./a.out 
  in B::foo()
  at end of A::go()

  $ clang++ a.cpp
  $ ./a.out 
  in B::foo()
  in B::foo()
  Segmentation fault: 11 (core dumped)

  $ clang++ --version
  Apple clang version 3.1 (tags/Apple/clang-318.0.45) (based on LLVM 3.1svn)
  Target: x86_64-apple-darwin11.3.0
  Thread model: posix

NOTE: even if you think the llvm behavior here is the correct one (and there is a case to be made for it) then clang++ desperately needs to emit a warning in this case. clang is already very strict about "noreturn" being part of the function signature (as per Bug 10338) but it's silent if a non-noreturn method overrides a noreturn method in a base class. Instead it just emits broken code (it took about a day of reading assembly to figure out where the problem was :-( )

However, I think the best thing to do would be to just treat noreturn the same way as g++, i.e. never assume a virtual method dispatch won't return.

[seanm]

I'm pretty sure what you describe was exactly the cause of bug #​9395.

[mitchblank]

(I added a CC to John McCall who commented on that 9395 bug)

Ah, you're right that's the same issue... I spent some time searching through the bug database, but didn't find that one for some reason.

However, I don't think it should be closed "INVALID". To restate what I said above:

• Mixing "noreturn" and virtual method dispatch has different semantics like gcc, which is dubious. Do you really want to implement a gcc extension in an incompatible way? Seems like a bad idea to me but, hey, it's your compiler.

• If you do want to keep the different semantics, then there is definitely a diagnostics bug here. If you look at the 9395 test case, notice that at the time the compiler is parsing B::ThreadedGenerateData() the compiler knows that it's producing code that will crash when called through virtual method dispatch. That should be an error, but it doesn't even give a warning.

[rjmccall]

If -Wmissing-noreturn is suggesting putting 'noreturn' on a virtual function, that is a bug. And I agree that, given the semantics we're using, 'noreturn' should get inherited by (and checked in) overriders.

[mitchblank]

Well, the only recent clang++ I have to test with is what comes with Xcode; not sure how far from trunk that is.

"-Wmissing-noreturn" doesn't complain about if the base method isn't marked (although recent g++'s do complain in that case, consistent with their semantics) Not with the g++ 4.2.1 that comes with Xcode but I tried with 4.6.1 and got a warning.

However, it definitely doesn't give any indication that it's compiling a method that won't work. "clang++ -Wall -pedantic" is completely quiet.

[seanm]

I'll just add that in the case of ITK, there was no warning from clang about the noreturn being out of place. Worse, the noreturn was originally added to silence some gcc warning, but I don't know exactly what, must have been an old gcc because when the noreturn was removed, no new warnings appeared on the buildbots.

It sure would have been nice if clang did warn, would have saved me tens of man hours of work!

[AaronBallman]

An interesting note -- if you replace attribute((noreturn)) with [[noreturn]] (in the corrected location), the behavior between gcc and clang is identical, and matches gcc's behavior with attribute((noreturn)).

I think the difference here is that clang is treating attribute((noreturn)) as part of the function type in this example, instead of just the function declaration. This is different than the C++11 and C11 noreturn attributes. So what's eventually happening is that the IR generated for the call to foo() is looking at the function type, seeing that it's noreturn, and generating IR to say "this is noreturn" when that cannot be definitively stated. If the noreturn isn't wrapped into the function's type (in handleFunctionTypeAttr), then clang matches gcc's behavior.

[AaronBallman]

*** Bug llvm/llvm-bugzilla-archive#16649 has been marked as a duplicate of this bug. ***

[rnk]

We actually go to great trouble to keep noreturn on the function type. Do you think we can go ahead and remove that, or is it useful to have it?

[zygoloid]

We actually go to great trouble to keep noreturn on the function type. Do
you think we can go ahead and remove that, or is it useful to have it?

This is fundamental to how the attribute works. __attribute__((noreturn)) function types and non-__attribute__((noreturn)) function types are different types there. As an extreme example (that we don't quite get right today), this is valid:

  typedef void (*p)() __attribute__((noreturn));
  typedef void (*q)(); // different type
  void f(p) {}
  void f(q) {} // ok, overload

Instead, the problem seems to be that we're somehow inheriting bits of the type from the base class function to the derived class function. We shouldn't do that.

[zygoloid]

Instead, the problem seems to be that we're somehow inheriting bits of the
type from the base class function to the derived class function. We
shouldn't do that.

There's also an IRgen problem: it should ignore 'noreturn' when making a (non-devirtualized) virtual call -- that's a property of the function, not of the vtable slot.

[AaronBallman]

When I traced it, what I was seeing is CodeGenModule::ConstructAttributeList gets the function type, sees it's noreturn, and adds the attribute here:

// Don't use [[noreturn]] or _Noreturn for a call to a virtual function.
// These attributes are not inherited by overloads.
const CXXMethodDecl *MD = dyn_cast(Fn);
if (Fn->isNoReturn() && !(AttrOnCallSite && MD && MD->isVirtual()))
FuncAttrs.addAttribute(llvm::Attribute::NoReturn);

Since AttrOnCallSite is false for this call, the MD and MD->isVirtual are being skipped entirely, and the attribute is added.

[rjmccall]

There's also an IRgen problem: it should ignore 'noreturn' when making a
(non-devirtualized) virtual call -- that's a property of the function, not
of the vtable slot.

That's the GCC model. Our design has always been that attribute((noreturn)) --- when declared in the class definition --- is a property of the signature of the virtual function.

[eugenis]

It looks like we are not consistent in following that design.
In the following example,

struct A {
  [[noreturn]] virtual void f();
};

void g(A *a) {
  a->f();
}

We complain that
warning: function 'g' could be declared with attribute 'noreturn' [-Wmissing-noreturn]

but on the other hand the generated IR does not reflect this:

  %vtable = load void (%struct.A*)**, void (%struct.A*)*** %1, align 8
  %vfn = getelementptr inbounds void (%struct.A*)*, void (%struct.A*)** %vtable, i64 0
  %2 = load void (%struct.A*)*, void (%struct.A*)** %vfn, align 8
  call void %2(%struct.A* %0)
  ret void

I'm looking at this real-world example:
https://android.googlesource.com/platform/art/+/master/runtime/arch/context.h#82

This is a class hierarchy where a certain virtual method (DoLongJump) is always [[noreturn]]. An approach in #​10 leaves us no way to express this property and get virtual calls to DoLongJump marked as noreturn.

[rjmccall]

We support a mechanism for that: spell the attribute with the GCC syntax, which has different semantics from the C++11 syntax under our model.

It is arguable that -Wmissing-noreturn should not be warning in your example, because the C++11 attribute does not apply to overriders. On the other hand, if you figure that the intent of the code is almost certainly that all overriders will be noreturn, then the warning is probably reasonable despite the difference in semantics. That is, the writer of g should definitely consider putting noreturn on it, even if the language doesn't guarantee anything.

[AaronBallman]

mentioned in issue llvm/llvm-bugzilla-archive#16649