If I understand correctly, Operator is basically a trivial temporary
object that holds some common functionality for both Instruction and
ConstantExpr.
It looks to me like Operator is doing something illegal though. For
example, in this member function of class Operator (in
include/llvm/Operator.h):
/// getOpcode - Return the opcode for this Instruction or ConstantExpr.
///
unsigned getOpcode() const {
if (const Instruction *I = dyn_cast<Instruction>(this))
return I->getOpcode();
return cast<ConstantExpr>(this)->getOpcode();
}
In this function, the `this` pointer is not pointing at an Operator
object. That's undefined behavior, right?
The problem that this Operator is trying to solve is similar to the
situation with DeclContext's inside Clang. The primary difference is
that while Operator isn't actualy a parent, DeclContext is a proper
parent (using multiple inheritance), and so these casts become legit
cross-casts for DeclContext. I'm not sure if multiple inheritance is
the right solution here, but we definitely don't want UB. Maybe it can
just store a User* to the Instruction/ConstantExpr?
Thoughts? Is this UB?
CC'ing Dan Gohman since it looks like he's the one that initially put
Operator in place.
-- Sean Silva
On Sat, Oct 6, 2012 at 8:43 PM, Sean Silva <silvas at purdue.edu> wrote:> If I understand correctly, Operator is basically a trivial temporary > object that holds some common functionality for both Instruction and > ConstantExpr. > > It looks to me like Operator is doing something illegal though. For > example, in this member function of class Operator (in > include/llvm/Operator.h): > > /// getOpcode - Return the opcode for this Instruction or ConstantExpr. > /// > unsigned getOpcode() const { > if (const Instruction *I = dyn_cast<Instruction>(this)) > return I->getOpcode(); > return cast<ConstantExpr>(this)->getOpcode(); > } > > In this function, the `this` pointer is not pointing at an Operator > object. That's undefined behavior, right?Yes. -Eli
>> In this function, the `this` pointer is not pointing at an Operator >> object. That's undefined behavior, right? > > Yes.Well then I guess the next step is how to fix it. Maybe someone with a bit more experience with the Operator code and how it is used could make a suggestion? (I just stumbled upon this code yesterday while doing some refactoring, so I'm not intimately familiar with how it is generally used). I would really rather not use multiple inheritance for this. It looks like a lot of what it does could be maybe be done through traits? Can someone with more experience with the code weigh in? -- Sean Silva On Sun, Oct 7, 2012 at 12:28 AM, Eli Friedman <eli.friedman at gmail.com> wrote:> On Sat, Oct 6, 2012 at 8:43 PM, Sean Silva <silvas at purdue.edu> wrote: >> If I understand correctly, Operator is basically a trivial temporary >> object that holds some common functionality for both Instruction and >> ConstantExpr. >> >> It looks to me like Operator is doing something illegal though. For >> example, in this member function of class Operator (in >> include/llvm/Operator.h): >> >> /// getOpcode - Return the opcode for this Instruction or ConstantExpr. >> /// >> unsigned getOpcode() const { >> if (const Instruction *I = dyn_cast<Instruction>(this)) >> return I->getOpcode(); >> return cast<ConstantExpr>(this)->getOpcode(); >> } >> >> In this function, the `this` pointer is not pointing at an Operator >> object. That's undefined behavior, right? > > Yes. > > -Eli