devirtualisation appears to crash clang on covariant functions on ARM

1. Does the devirtualiser work in the presence of covariant returns? If it
doesn't then it's unlikely the devirtualiser will work with destructors when
using the ARM ABI.

Not in general, since that can require pointer updates if one type is
not at offset 0 of the other.

2. If it does work (even if covariance prevents devirtualisation), how can
it be stopped from crashing in the verifier in cases like this?

Is the return visible at the C++ level at all? If so, I don't think
you have an option other than implementing support for covariant
return or detecting that case in destructors and avoiding
devirtualizing. If the return is not visible in C++, can you just
create a new llvm variable that is being created to hold it and add a
bitcast to the original one?

Many thanks for any assistance,
Dave

Cheers,
Rafael

1. Does the devirtualiser work in the presence of covariant returns? If it
doesn't then it's unlikely the devirtualiser will work with destructors when
using the ARM ABI.

Not in general, since that can require pointer updates if one type is
not at offset 0 of the other.

Right, talking to people who implement other compilers that issue had been mentioned.

2. If it does work (even if covariance prevents devirtualisation), how can
it be stopped from crashing in the verifier in cases like this?

Is the return visible at the C++ level at all?

I don't believe that there's any way to observe this at the actual C++ level. (Presumably you could use some inline some assembly that new about this, but that's presumably beyond any standard.)

If the return is not visible in C++, can you just
create a new llvm variable that is being created to hold it and add a
bitcast to the original one?

That sounds like quite an involved patch, since (if I understand things correctly) you'd have to do this during the ABI-fixing stage, which would require determining that the devirtualisation pass did devirtualise the call. As an intermediate step to avoid the crash and get the regression test completing, albeit failing, how about a patch to avoid devirtualising destructors on ARM: would that be ok?

Cheers,
Dave

That sounds like quite an involved patch, since (if I understand things correctly) you'd have to do this during the ABI-fixing stage, which would require determining that the devirtualisation pass did devirtualise the call. As an intermediate step to avoid the crash and get the regression test completing, albeit failing, how about a patch to avoid devirtualising destructors on ARM: would that be ok?

I was under the impression that you could build the cast at the end of
CodeGenFunction::EmitCXXMemberCallExpr. Instead of

return EmitCXXMemberCall...

you would have

foo = return EmitCXXMemberCall
if (needs cast)
  foo = cast foo;
return foo;

wouldn't that work? I will let John comment about the option of
disabling devirtualization of destructorns on ARM.

Cheers,
Dave

Cheers,
Rafael

I was under the impression that you could build the cast at the end of
CodeGenFunction::EmitCXXMemberCallExpr. Instead of

return EmitCXXMemberCall...

you would have

foo = return EmitCXXMemberCall
if (needs cast)
foo = cast foo;
return foo;

wouldn't that work? I will let John comment about the option of
disabling devirtualization of destructorns on ARM.

Ah, this looks like a slightly smaller task than I thought. I'll see if I can figure out some more of the steps in implementing this.

I'm actually picking up bits and pieces about the clang implementation starting from a base of zero knowledge; I'm basically an LLVM person who has the goal of fixing the historically failing regression tests (both LLVM and clang) on the ARM platform.

Thanks for the insight about the code,
Cheers,
Dave

Killing all frontend devirtualization of destructors is a bit over-the-top.

Note that we shouldn't actually need a conversion here because, while
destructors do generally return 'this', the correctness of that return value
is not guaranteed for *virtual* destructor calls, basically so that we don't
have to thunk them just to maintain this optimization. So I'd like to know
what's actually introducing a dependence on the return value.

John.

Hi,

Killing all frontend devirtualization of destructors is a bit

over-the-top.

Note that we shouldn't actually need a conversion here because, while
destructors do generally return 'this', the correctness of that return

value

is not guaranteed for *virtual* destructor calls, basically so that we

don't

have to thunk them just to maintain this optimization. So I'd like to

know

what's actually introducing a dependence on the return value.

To recap what's happening: on a platform using the ARM ABI (at least a linux
version) that test is crashing clang due to an assertion in the module
verifier. I don't understand exactly what's happening, but the error message
from the verifier (which can be obtained running the test on x86 with the
-triple armv7l-unknown-linux-gnueabihf)

Function return type does not match operand type of return inst!
  ret %"struct.Test6::D"* %this1
%"struct.Test6::A"*Broken module found, compilation aborted!

is to do with matching up return instruction types and type signature return
types is seeing a D* (derived class) returned where it wants an A* (parent
class). What I don't know is "which" signature this is: there'd be other
breakage everywhere if the actual D's destructor had a wrong type, so I'm
thinking it's either the signature of the method apparently being called
(A's specific destructor which returns an A*), or some sort of
combined-signature for the virtual function as a whole. Single stepping
through, _at the point the devirtualisation decision is taken_ both
functions have the return type code for void (as they would in "generic
C++") so the devirtualisation decision gets taken.

In terms of what to do: I'm of the opinion that having code that can crash
the compiler is not a good behaviour. However, I lack the expertise to
resolve the underlying problem here properly in the time I've got allocated.
Suppressing devirtualisation of destructors _only when using the ARM CXXABI_
(which I've got a patch to do) at least stops the compiler crashing, and so
would be my preferred intermediate solution. But I'm very open to other
ideas or ways to improve the situation on the ARM platform for this case.

Many thanks,
Dave

r166651.

-Eli

Thank you.

John.

Just finished a an ARM native rebuild and retested and can confirm the test
now passes. Thanks for fixing.

Hi David,