Fix call instructions to check parameter types for consistency.
Checks each parameter of a call against the corresponding function
signature. It also checks that parameters are valid PNaCl parameter
types, unless the call is to an intrinsic.
BUG= https://code.google.com/p/nativeclient/issues/detail?id=4309
R=stichnot@chromium.org
Review URL: https://codereview.chromium.org/1354673002 .
diff --git a/src/IceTypes.cpp b/src/IceTypes.cpp
index 6cc79b7..3756a10 100644
--- a/src/IceTypes.cpp
+++ b/src/IceTypes.cpp
@@ -37,7 +37,8 @@
};
// Define a temporary set of enum values based on ICETYPE_PROPS_TABLE
enum {
-#define X(tag, IsVec, IsInt, IsFloat, IsIntArith, IsLoadStore, CompareResult) \
+#define X(tag, IsVec, IsInt, IsFloat, IsIntArith, IsLoadStore, IsParam, \
+ CompareResult) \
_props_table_tag_##tag,
ICETYPE_PROPS_TABLE
#undef X
@@ -51,7 +52,8 @@
ICETYPE_TABLE
#undef X
// Assert that tags in ICETYPE_PROPS_TABLE is in ICETYPE_TABLE.
-#define X(tag, IsVec, IsInt, IsFloat, IsIntArith, IsLoadStore, CompareResult) \
+#define X(tag, IsVec, IsInt, IsFloat, IsIntArith, IsLoadStore, IsParam, \
+ CompareResult) \
static_assert( \
(unsigned)_table_tag_##tag == (unsigned)_props_table_tag_##tag, \
"Inconsistency between ICETYPE_PROPS_TABLE and ICETYPE_TABLE");
@@ -69,13 +71,15 @@
};
// Define constants for boolean flag if vector in ICETYPE_PROPS_TABLE.
enum {
-#define X(tag, IsVec, IsInt, IsFloat, IsIntArith, IsLoadStore, CompareResult) \
+#define X(tag, IsVec, IsInt, IsFloat, IsIntArith, IsLoadStore, IsParam, \
+ CompareResult) \
_props_table_IsVec_##tag = IsVec,
ICETYPE_PROPS_TABLE
#undef X
};
// Verify that the number of vector elements is consistent with IsVec.
-#define X(tag, IsVec, IsInt, IsFloat, IsIntArith, IsLoadStore, CompareResult) \
+#define X(tag, IsVec, IsInt, IsFloat, IsIntArith, IsLoadStore, IsParam, \
+ CompareResult) \
static_assert((_table_elts_##tag > 1) == _props_table_IsVec_##tag, \
"Inconsistent vector specification in ICETYPE_PROPS_TABLE");
ICETYPE_PROPS_TABLE
@@ -107,14 +111,16 @@
bool TypeIsScalarFloatingType;
bool TypeIsVectorFloatingType;
bool TypeIsLoadStoreType;
+ bool TypeIsCallParameterType;
Type CompareResultType;
};
const TypePropertyFields TypePropertiesTable[] = {
-#define X(tag, IsVec, IsInt, IsFloat, IsIntArith, IsLoadStore, CompareResult) \
+#define X(tag, IsVec, IsInt, IsFloat, IsIntArith, IsLoadStore, IsParam, \
+ CompareResult) \
{ \
IsVec, IsInt, IsInt & !IsVec, IsInt & IsVec, IsIntArith, IsFloat, \
- IsFloat & !IsVec, IsFloat & IsVec, IsLoadStore, CompareResult \
+ IsFloat & !IsVec, IsFloat & IsVec, IsLoadStore, IsParam, CompareResult \
} \
,
ICETYPE_PROPS_TABLE
@@ -240,6 +246,14 @@
return false;
}
+bool isCallParameterType(Type Ty) {
+ size_t Index = static_cast<size_t>(Ty);
+ if (Index < IceType_NUM)
+ return TypePropertiesTable[Index].TypeIsCallParameterType;
+ llvm_unreachable("Invalid type for isCallParameterType()");
+ return false;
+}
+
Type getCompareResultType(Type Ty) {
size_t Index = static_cast<size_t>(Ty);
if (Index < IceType_NUM)
diff --git a/src/IceTypes.def b/src/IceTypes.def
index b86dba8..8db8c63 100644
--- a/src/IceTypes.def
+++ b/src/IceTypes.def
@@ -19,33 +19,33 @@
// sandboxes, but for now the 64-bit architectures use ELF64:
// https://code.google.com/p/nativeclient/issues/detail?id=349 TODO: Whoever
// adds AArch64 will need to set ABI e_flags.
-#define TARGETARCH_TABLE \
- /* enum value, printable string, is_elf64, e_machine, e_flags */ \
- X(Target_X8632, "x86-32", false, EM_386, 0) \
- X(Target_X8664, "x86-64", true, EM_X86_64, 0) \
- X(Target_ARM32, "arm32", false, EM_ARM, EF_ARM_EABI_VER5) \
- X(Target_ARM64, "arm64", true, EM_AARCH64, 0) \
- X(Target_MIPS32,"mips32", false, EM_MIPS, 0) \
- //#define X(tag, str, is_elf64, e_machine, e_flags)
+#define TARGETARCH_TABLE \
+ /* enum value, printable string, is_elf64, e_machine, e_flags */ \
+ X(Target_X8632, "x86-32", false, EM_386, 0) \
+ X(Target_X8664, "x86-64", true, EM_X86_64, 0) \
+ X(Target_ARM32, "arm32", false, EM_ARM, EF_ARM_EABI_VER5) \
+ X(Target_ARM64, "arm64", true, EM_AARCH64, 0) \
+ X(Target_MIPS32,"mips32", false, EM_MIPS, 0) \
+//#define X(tag, str, is_elf64, e_machine, e_flags)
-#define ICETYPE_TABLE \
- /* enum value, log_2(size), align, # elts, element type, printable */ \
- /* string (size and alignment in bytes) */ \
- X(IceType_void, -1, 0, 1, IceType_void, "void") \
- X(IceType_i1, 0, 1, 1, IceType_i1, "i1") \
- X(IceType_i8, 0, 1, 1, IceType_i8, "i8") \
- X(IceType_i16, 1, 1, 1, IceType_i16, "i16") \
- X(IceType_i32, 2, 1, 1, IceType_i32, "i32") \
- X(IceType_i64, 3, 1, 1, IceType_i64, "i64") \
- X(IceType_f32, 2, 4, 1, IceType_f32, "float") \
- X(IceType_f64, 3, 8, 1, IceType_f64, "double") \
- X(IceType_v4i1, 4, 1, 4, IceType_i1, "<4 x i1>") \
- X(IceType_v8i1, 4, 1, 8, IceType_i1, "<8 x i1>") \
- X(IceType_v16i1, 4, 1, 16, IceType_i1, "<16 x i1>") \
- X(IceType_v16i8, 4, 1, 16, IceType_i8, "<16 x i8>") \
- X(IceType_v8i16, 4, 2, 8, IceType_i16, "<8 x i16>") \
- X(IceType_v4i32, 4, 4, 4, IceType_i32, "<4 x i32>") \
- X(IceType_v4f32, 4, 4, 4, IceType_f32, "<4 x float>") \
+#define ICETYPE_TABLE \
+ /* enum value, log_2(size), align, # elts, element type, printable */ \
+ /* string (size and alignment in bytes) */ \
+ X(IceType_void, -1, 0, 1, IceType_void, "void") \
+ X(IceType_i1, 0, 1, 1, IceType_i1, "i1") \
+ X(IceType_i8, 0, 1, 1, IceType_i8, "i8") \
+ X(IceType_i16, 1, 1, 1, IceType_i16, "i16") \
+ X(IceType_i32, 2, 1, 1, IceType_i32, "i32") \
+ X(IceType_i64, 3, 1, 1, IceType_i64, "i64") \
+ X(IceType_f32, 2, 4, 1, IceType_f32, "float") \
+ X(IceType_f64, 3, 8, 1, IceType_f64, "double") \
+ X(IceType_v4i1, 4, 1, 4, IceType_i1, "<4 x i1>") \
+ X(IceType_v8i1, 4, 1, 8, IceType_i1, "<8 x i1>") \
+ X(IceType_v16i1, 4, 1, 16, IceType_i1, "<16 x i1>") \
+ X(IceType_v16i8, 4, 1, 16, IceType_i8, "<16 x i8>") \
+ X(IceType_v8i16, 4, 2, 8, IceType_i16, "<8 x i16>") \
+ X(IceType_v4i32, 4, 4, 4, IceType_i32, "<4 x i32>") \
+ X(IceType_v4f32, 4, 4, 4, IceType_f32, "<4 x float>") \
//#define X(tag, sizeLog2, align, elts, elty, str)
// Dictionary:
@@ -54,25 +54,27 @@
// F - Is floating point value (scalar or vector).
// IA - Is integer arithmetic type
// LS - true if load/store allowed on type.
+// P - true if can be used for parameter of call.
// CR - Result type of compare instruction for argument type
// (IceType_void if disallowed)
-#define ICETYPE_PROPS_TABLE \
- /* Enum Value V I F IA LS CR */ \
- X(IceType_void, 0, 0, 0, 0, 0, IceType_void) \
- X(IceType_i1, 0, 1, 0, 0, 0, IceType_i1) \
- X(IceType_i8, 0, 1, 0, 1, 1, IceType_i1) \
- X(IceType_i16, 0, 1, 0, 1, 1, IceType_i1) \
- X(IceType_i32, 0, 1, 0, 1, 1, IceType_i1) \
- X(IceType_i64, 0, 1, 0, 1, 1, IceType_i1) \
- X(IceType_f32, 0, 0, 1, 0, 1, IceType_i1) \
- X(IceType_f64, 0, 0, 1, 0, 1, IceType_i1) \
- X(IceType_v4i1, 1, 1, 0, 0, 0, IceType_v4i1) \
- X(IceType_v8i1, 1, 1, 0, 0, 0, IceType_v8i1) \
- X(IceType_v16i1, 1, 1, 0, 0, 0, IceType_v16i1) \
- X(IceType_v16i8, 1, 1, 0, 1, 1, IceType_v16i1) \
- X(IceType_v8i16, 1, 1, 0, 1, 1, IceType_v8i1) \
- X(IceType_v4i32, 1, 1, 0, 1, 1, IceType_v4i1) \
- X(IceType_v4f32, 1, 0, 1, 0, 1, IceType_v4i1) \
-//#define X(tag, IsVec, IsInt, IsFloat, IsIntArith, IsLoadStore, CompareResult)
+#define ICETYPE_PROPS_TABLE \
+ /* Enum Value V I F IA LS P CR */ \
+ X(IceType_void, 0, 0, 0, 0, 0, 0, IceType_void) \
+ X(IceType_i1, 0, 1, 0, 0, 0, 0, IceType_i1) \
+ X(IceType_i8, 0, 1, 0, 1, 1, 0, IceType_i1) \
+ X(IceType_i16, 0, 1, 0, 1, 1, 0, IceType_i1) \
+ X(IceType_i32, 0, 1, 0, 1, 1, 1, IceType_i1) \
+ X(IceType_i64, 0, 1, 0, 1, 1, 1, IceType_i1) \
+ X(IceType_f32, 0, 0, 1, 0, 1, 1, IceType_i1) \
+ X(IceType_f64, 0, 0, 1, 0, 1, 1, IceType_i1) \
+ X(IceType_v4i1, 1, 1, 0, 0, 0, 1, IceType_v4i1) \
+ X(IceType_v8i1, 1, 1, 0, 0, 0, 1, IceType_v8i1) \
+ X(IceType_v16i1, 1, 1, 0, 0, 0, 1, IceType_v16i1) \
+ X(IceType_v16i8, 1, 1, 0, 1, 1, 1, IceType_v16i1) \
+ X(IceType_v8i16, 1, 1, 0, 1, 1, 1, IceType_v8i1) \
+ X(IceType_v4i32, 1, 1, 0, 1, 1, 1, IceType_v4i1) \
+ X(IceType_v4f32, 1, 0, 1, 0, 1, 1, IceType_v4i1) \
+//#define X(tag, IsVec, IsInt, IsFloat, IsIntArith, IsLoadStore, IsParam, \
+// CompareResult)
#endif // SUBZERO_SRC_ICETYPES_DEF
diff --git a/src/IceTypes.h b/src/IceTypes.h
index f176e9b..f6705e9 100644
--- a/src/IceTypes.h
+++ b/src/IceTypes.h
@@ -81,6 +81,14 @@
/// Returns true if the given type can be used in a load instruction.
bool isLoadStoreType(Type Ty);
+/// Returns true if the given type can be used as a parameter type in a call.
+bool isCallParameterType(Type Ty);
+
+/// Returns true if the given type can be used as the return type of a call.
+inline bool isCallReturnType(Type Ty) {
+ return Ty == IceType_void || isCallParameterType(Ty);
+}
+
/// Returns type generated by applying the compare instructions (icmp and fcmp)
/// to arguments of the given type. Returns IceType_void if compare is not
/// allowed.
diff --git a/src/PNaClTranslator.cpp b/src/PNaClTranslator.cpp
index d7678f3..ac9698e 100644
--- a/src/PNaClTranslator.cpp
+++ b/src/PNaClTranslator.cpp
@@ -2644,12 +2644,13 @@
// Extract out the called function and its return type.
uint32_t CalleeIndex = convertRelativeToAbsIndex(Values[1], BaseIndex);
Ice::Operand *Callee = getOperand(CalleeIndex);
+ const Ice::FuncSigType *Signature = nullptr;
Ice::Type ReturnType = Ice::IceType_void;
const Ice::Intrinsics::FullIntrinsicInfo *IntrinsicInfo = nullptr;
if (Record.GetCode() == naclbitc::FUNC_CODE_INST_CALL) {
Ice::FunctionDeclaration *Fcn = Context->getFunctionByID(CalleeIndex);
- const Ice::FuncSigType &Signature = Fcn->getSignature();
- ReturnType = Signature.getReturnType();
+ Signature = &Fcn->getSignature();
+ ReturnType = Signature->getReturnType();
// Check if this direct call is to an Intrinsic (starts with "llvm.")
bool BadIntrinsic;
@@ -2661,13 +2662,23 @@
raw_string_ostream StrBuf(Buffer);
StrBuf << "Invalid PNaCl intrinsic call to " << Name;
Error(StrBuf.str());
- appendErrorInstruction(ReturnType);
+ if (ReturnType != Ice::IceType_void)
+ appendErrorInstruction(ReturnType);
return;
}
} else {
ReturnType = Context->getSimpleTypeByID(Values[2]);
}
+ // Check return type.
+ if (IntrinsicInfo == nullptr && !isCallReturnType(ReturnType)) {
+ std::string Buffer;
+ raw_string_ostream StrBuf(Buffer);
+ StrBuf << "Return type of called function is invalid: " << ReturnType;
+ Error(StrBuf.str());
+ ReturnType = Ice::IceType_i32;
+ }
+
// Extract call information.
uint64_t CCInfo = Values[0];
CallingConv::ID CallingConv;
@@ -2677,12 +2688,21 @@
StrBuf << "Function call calling convention value " << (CCInfo >> 1)
<< " not understood.";
Error(StrBuf.str());
- appendErrorInstruction(ReturnType);
+ if (ReturnType != Ice::IceType_void)
+ appendErrorInstruction(ReturnType);
return;
}
bool IsTailCall = static_cast<bool>(CCInfo & 1);
Ice::SizeT NumParams = Values.size() - ParamsStartIndex;
-
+ if (Signature && NumParams != Signature->getNumArgs()) {
+ std::string Buffer;
+ raw_string_ostream StrBuf(Buffer);
+ StrBuf << "Call has " << NumParams
+ << " parameters. Signature expects: " << Signature->getNumArgs();
+ Error(StrBuf.str());
+ // Error recover by only checking parameters in both signature and call.
+ NumParams = std::min(NumParams, Signature->getNumArgs());
+ }
if (isIRGenerationDisabled()) {
assert(Callee == nullptr);
// Check that parameters are defined.
@@ -2695,30 +2715,67 @@
setNextLocalInstIndex(nullptr);
return;
}
-
// Create the call instruction.
Ice::Variable *Dest = (ReturnType == Ice::IceType_void)
? nullptr
: getNextInstVar(ReturnType);
- Ice::InstCall *Inst = nullptr;
+ std::unique_ptr<Ice::InstCall> Inst;
if (IntrinsicInfo) {
- Inst = Ice::InstIntrinsicCall::create(Func.get(), NumParams, Dest, Callee,
- IntrinsicInfo->Info);
+ Inst.reset(Ice::InstIntrinsicCall::create(Func.get(), NumParams, Dest,
+ Callee, IntrinsicInfo->Info));
} else {
- Inst = Ice::InstCall::create(Func.get(), NumParams, Dest, Callee,
- IsTailCall);
+ Inst.reset(Ice::InstCall::create(Func.get(), NumParams, Dest, Callee,
+ IsTailCall));
}
// Add parameters.
for (Ice::SizeT ParamIndex = 0; ParamIndex < NumParams; ++ParamIndex) {
- Inst->addArg(
- getRelativeOperand(Values[ParamsStartIndex + ParamIndex], BaseIndex));
+ Ice::Operand *Op =
+ getRelativeOperand(Values[ParamsStartIndex + ParamIndex], BaseIndex);
+ if (Op == nullptr) {
+ std::string Buffer;
+ raw_string_ostream StrBuf(Buffer);
+ StrBuf << "Parameter " << ParamIndex << " of call not defined";
+ Error(StrBuf.str());
+ if (ReturnType != Ice::IceType_void)
+ appendErrorInstruction(ReturnType);
+ return;
+ }
+
+ // Check that parameter type is valid.
+ if (Signature) {
+ if (Op->getType() != Signature->getArgType(ParamIndex)) {
+ std::string Buffer;
+ raw_string_ostream StrBuf(Buffer);
+ StrBuf << "Call argument " << *Op << " expects "
+ << Signature->getArgType(ParamIndex)
+ << ". Found: " << Op->getType();
+ Error(StrBuf.str());
+ } else if (IntrinsicInfo == nullptr &&
+ !isCallParameterType(Op->getType())) {
+ // TODO(kschimpf): Move this check to the function declaration, so
+ // that it only needs to be checked once.
+ std::string Buffer;
+ raw_string_ostream StrBuf(Buffer);
+ StrBuf << "Call argument " << *Op
+ << " matches declaration but has invalid type: "
+ << Op->getType();
+ Error(StrBuf.str());
+ }
+ } else if (!isCallParameterType(Op->getType())) {
+ std::string Buffer;
+ raw_string_ostream StrBuf(Buffer);
+ StrBuf << "Call argument " << *Op
+ << " has invalid type: " << Op->getType();
+ Error(StrBuf.str());
+ }
+ Inst->addArg(Op);
}
// If intrinsic call, validate call signature.
if (IntrinsicInfo) {
Ice::SizeT ArgIndex = 0;
- switch (IntrinsicInfo->validateCall(Inst, ArgIndex)) {
+ switch (IntrinsicInfo->validateCall(Inst.get(), ArgIndex)) {
case Ice::Intrinsics::IsValidCall:
break;
case Ice::Intrinsics::BadReturnType: {
@@ -2750,7 +2807,7 @@
}
}
- CurrentNode->appendInst(Inst);
+ CurrentNode->appendInst(Inst.release());
return;
}
case naclbitc::FUNC_CODE_INST_FORWARDTYPEREF: {
diff --git a/tests_lit/parse_errs/call-fcn-bad-param-type.ll b/tests_lit/parse_errs/call-fcn-bad-param-type.ll
new file mode 100644
index 0000000..2314732
--- /dev/null
+++ b/tests_lit/parse_errs/call-fcn-bad-param-type.ll
@@ -0,0 +1,15 @@
+; Test that even if a call parameter matches its declaration, it must still
+; be a legal call parameter type (unless declaration is intrinsic).
+
+; REQUIRES: no_minimal_build
+
+; RUN: %p2i --expect-fail -i %s --insts | FileCheck %s
+
+declare void @f(i8);
+
+define void @Test() {
+entry:
+ call void @f(i8 1)
+; CHECK: Call argument 1 matches declaration but has invalid type: i8
+ ret void
+}
diff --git a/tests_lit/parse_errs/call-fcn-bad-return-type.ll b/tests_lit/parse_errs/call-fcn-bad-return-type.ll
new file mode 100644
index 0000000..b1c8cd8
--- /dev/null
+++ b/tests_lit/parse_errs/call-fcn-bad-return-type.ll
@@ -0,0 +1,16 @@
+; Test that even if a call return type matches its declaration, it must still be
+; a legal call return type (unless declaration is intrinsic).
+
+; REQUIRES: no_minimal_build
+
+; RUN: %p2i --expect-fail -i %s --insts | FileCheck %s
+
+declare i1 @f();
+
+define void @Test() {
+entry:
+ %v = call i1 @f()
+; CHECK: Return type of called function is invalid: i1
+ ret void
+}
+
diff --git a/tests_lit/parse_errs/call-indirect-i8.ll b/tests_lit/parse_errs/call-indirect-i8.ll
new file mode 100644
index 0000000..0a3b2bc
--- /dev/null
+++ b/tests_lit/parse_errs/call-indirect-i8.ll
@@ -0,0 +1,13 @@
+; Tests that we don't allow illegal sized parameters on indirect calls.
+
+; REQUIRES: no_minimal_build
+
+; RUN: %p2i --expect-fail -i %s --insts | FileCheck %s
+
+define void @CallIndirectI32(i32 %f_addr) {
+entry:
+ %f = inttoptr i32 %f_addr to i32(i8)*
+ %r = call i32 %f(i8 1)
+; CHECK: Call argument 1 has invalid type: i8
+ ret void
+}
diff --git a/tests_lit/reader_tests/call-indirect.ll b/tests_lit/reader_tests/call-indirect.ll
index 3da87dd..d3d7e1d 100644
--- a/tests_lit/reader_tests/call-indirect.ll
+++ b/tests_lit/reader_tests/call-indirect.ll
@@ -1,10 +1,7 @@
; Test parsing indirect calls in Subzero.
; RUN: %p2i -i %s --insts | FileCheck %s
-; RUN: %if --need=allow_disable_ir_gen --command \
-; RUN: %p2i -i %s --args -notranslate -timing -no-ir-gen \
-; RUN: | %if --need=allow_disable_ir_gen --command \
-; RUN: FileCheck --check-prefix=NOIR %s
+; RUN: %p2i -i %s --args -notranslate -timing | FileCheck --check-prefix=NOIR %s
define internal void @CallIndirectVoid(i32 %f_addr) {
entry:
@@ -21,14 +18,14 @@
define internal i32 @CallIndirectI32(i32 %f_addr) {
entry:
- %f = inttoptr i32 %f_addr to i32(i8, i1)*
- %r = call i32 %f(i8 1, i1 false)
+ %f = inttoptr i32 %f_addr to i32(i64, i32)*
+ %r = call i32 %f(i64 1, i32 %f_addr)
ret i32 %r
}
; CHECK-NEXT: define internal i32 @CallIndirectI32(i32 %f_addr) {
; CHECK-NEXT: entry:
-; CHECK-NEXT: %r = call i32 %f_addr(i8 1, i1 false)
+; CHECK-NEXT: %r = call i32 %f_addr(i64 1, i32 %f_addr)
; CHECK-NEXT: ret i32 %r
; CHECK-NEXT: }
