[llvm-commits] [llvm] r165764 - /llvm/trunk/lib/ExecutionEngine/JIT/JITEmitter.cpp

[Sean Silva]

Author: silvas
Date: Thu Oct 11 18:30:38 2012
New Revision: 165764

URL: http://llvm.org/viewvc/llvm-project?rev=165764&view=rev
Log:
Remove buggy classof().

This classof() is effectively saying that a MachineCodeEmitter "is-a"
JITEmitter, but JITEmitter is in fact a descendant of
MachineCodeEmitter, so this is not semantically correct. Consequently,
none of the assertions that rely on these classof() actualy check
anything.

Remove the RTTI (which didn't actually check anything) and use
static_cast<> instead.

Post-Mortem Bug Analysis
========================

Cause of the bug
----------------

r55022 appears to be the source of the classof() and assertions removed
by this commit. It aimed at removing some dynamic_cast<> that were
solely in the assertions. A typical diff hunk from that commit looked
like:

  -  assert(dynamic_cast<JITEmitter*>(MCE) && "Unexpected MCE?");
  -  JITEmitter *JE = static_cast<JITEmitter*>(getCodeEmitter());
  +  assert(isa<JITEmitter>(MCE) && "Unexpected MCE?");
  +  JITEmitter *JE = cast<JITEmitter>(getCodeEmitter());

Hence, the source of the bug then seems to be an attempt to replace
dynamic_cast<> with LLVM-style RTTI without properly setting up the
class hierarchy for LLVM-style RTTI. The bug therefore appears to be
simply a "thinko".

What initially indicated the presence of the bug
------------------------------------------------

After implementing automatic upcasting for isa<>, classof() functions of
the form

  static bool classof(const Foo *) { return true; }

were removed, since they only serve the purpose of optimizing
statically-OK upcasts. A subsequent recompilation triggered a build
failure on the isa<> tests within the removed asserts, since the
automatic upcasting (correctly) failed to substitute this classof().

Key to pinning down the root cause of the bug
---------------------------------------------

After being alerted to the presence of the bug, some thought about the
semantics which were being asserted by the buggy classof() revealed that
it was incorrect.

How the bug could have been prevented
-------------------------------------

This bug could have been prevented by better documentation for how to
set up LLVM-style RTTI. This should be solved by the recently added
documentation HowToSetUpLLVMStyleRTTI. However, this bug suggests that
the documentation should clearly explain the contract that classof()
must fulfill. The HowToSetUpLLVMStyleRTTI already explains this
contract, but it is a little tucked away. A future patch will expand
that explanation and make it more prominent.

There does not appear to be a simple way to have the compiler prevent
this bug, since fundamentally it boiled down to a spurious classof()
where the programmer made an erroneous statement about the conversion.
This suggests that perhaps the interface to LLVM-style RTTI of classof()
is not the best. There is already some evidence for this, since in a
number of places Clang has classof() forward to classofKind(Kind K)
which evaluates the cast in terms of just the Kind. This could probably
be generalized to simply a `static const Kind MyKind;` field in leaf
classes and `static const Kind firstMyKind, lastMyKind;` for non-leaf
classes, and have the rest of the work be done inside Casting.h,
assuming that the Kind enum is laid out in a preorder traversal of the
inheritance tree.

Modified:
    llvm/trunk/lib/ExecutionEngine/JIT/JITEmitter.cpp

Modified: llvm/trunk/lib/ExecutionEngine/JIT/JITEmitter.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/ExecutionEngine/JIT/JITEmitter.cpp?rev=165764&r1=165763&r2=165764&view=diff
==============================================================================
--- llvm/trunk/lib/ExecutionEngine/JIT/JITEmitter.cpp (original)
+++ llvm/trunk/lib/ExecutionEngine/JIT/JITEmitter.cpp Thu Oct 11 18:30:38 2012
@@ -384,11 +384,6 @@
       delete MemMgr;
     }
 
-    /// classof - Methods for support type inquiry through isa, cast, and
-    /// dyn_cast:
-    ///
-    static inline bool classof(const MachineCodeEmitter*) { return true; }
-
     JITResolver &getJITResolver() { return Resolver; }
 
     virtual void startFunction(MachineFunction &F);
@@ -1265,15 +1260,13 @@
     return Addr;
 
   // Get a stub if the target supports it.
-  assert(isa<JITEmitter>(JCE) && "Unexpected MCE?");
-  JITEmitter *JE = cast<JITEmitter>(getCodeEmitter());
+  JITEmitter *JE = static_cast<JITEmitter*>(getCodeEmitter());
   return JE->getJITResolver().getLazyFunctionStub(F);
 }
 
 void JIT::updateFunctionStub(Function *F) {
   // Get the empty stub we generated earlier.
-  assert(isa<JITEmitter>(JCE) && "Unexpected MCE?");
-  JITEmitter *JE = cast<JITEmitter>(getCodeEmitter());
+  JITEmitter *JE = static_cast<JITEmitter*>(getCodeEmitter());
   void *Stub = JE->getJITResolver().getLazyFunctionStub(F);
   void *Addr = getPointerToGlobalIfAvailable(F);
   assert(Addr != Stub && "Function must have non-stub address to be updated.");
@@ -1294,6 +1287,5 @@
   updateGlobalMapping(F, 0);
 
   // Free the actual memory for the function body and related stuff.
-  assert(isa<JITEmitter>(JCE) && "Unexpected MCE?");
-  cast<JITEmitter>(JCE)->deallocateMemForFunction(F);
+  static_cast<JITEmitter*>(JCE)->deallocateMemForFunction(F);
 }