Skip to content

S
swiftshader
Commit 7a99327d by Karl Schimpf
Options

Restore function-local variables to use a vector.

CL 1282523002 changed the bitcode parser from using a vector, to using an unordered map. This was done because one could forward reference a local variable, and would freeze the computer trying to allocate a vector large enough to contain the index. This patch goes back to using vectors. To fix the forward variable reference, we use the number of bytes in the function to determine if the index is possible. This stops very large (probematic) vector resizes from happening. BUG=None R=stichnot@chromium.org Review URL: https://codereview.chromium.org/1293923002 .
parent 98ed4464
Showing with 67 additions and 42 deletions
src/PNaClTranslator.cpp
...@@ -1314,11 +1314,12 @@ public: ...@@ -1314,11 +1314,12 @@ public:
} }
private: private:
typedef std::unordered_map<NaClBcIndexSize_t, Ice::Operand *> OperandMap;
Ice::TimerMarker Timer; Ice::TimerMarker Timer;
// The number of words in the bitstream defining the function block. // The number of words in the bitstream defining the function block.
uint64_t NumBytesDefiningFunction = 0; uint64_t NumBytesDefiningFunction = 0;
// Maximum number of records that can appear in the function block, based on
// the number of bytes defining the function block.
uint64_t MaxRecordsInBlock = 0;
// The corresponding ICE function defined by the function block. // The corresponding ICE function defined by the function block.
std::unique_ptr<Ice::Cfg> Func; std::unique_ptr<Ice::Cfg> Func;
// The index to the current basic block being built. // The index to the current basic block being built.
...@@ -1335,7 +1336,7 @@ private: ...@@ -1335,7 +1336,7 @@ private:
size_t CachedNumGlobalValueIDs; size_t CachedNumGlobalValueIDs;
// Holds operands local to the function block, based on indices // Holds operands local to the function block, based on indices
// defined in the bitcode file. // defined in the bitcode file.
OperandMap LocalOperands; std::vector<Ice::Operand *> LocalOperands;
// Holds the index within LocalOperands corresponding to the next // Holds the index within LocalOperands corresponding to the next
// instruction that generates a value. // instruction that generates a value.
NaClBcIndexSize_t NextLocalInstIndex; NaClBcIndexSize_t NextLocalInstIndex;
...@@ -1370,12 +1371,12 @@ private: ...@@ -1370,12 +1371,12 @@ private:
void EnterBlock(unsigned NumWords) final { void EnterBlock(unsigned NumWords) final {
// Note: Bitstream defines words as 32-bit values. // Note: Bitstream defines words as 32-bit values.
NumBytesDefiningFunction = NumWords * sizeof(uint32_t); NumBytesDefiningFunction = NumWords * sizeof(uint32_t);
// We know that all records are minimally defined by a two-bit abreviation.
MaxRecordsInBlock = NumBytesDefiningFunction * (CHAR_BIT >> 1);
} }
void ExitBlock() override; void ExitBlock() override;
bool verifyAllForwardRefsDefined();
// Creates and appends a new basic block to the list of basic blocks. // Creates and appends a new basic block to the list of basic blocks.
Ice::CfgNode *installNextBasicBlock() { Ice::CfgNode *installNextBasicBlock() {
assert(!isIRGenerationDisabled()); assert(!isIRGenerationDisabled());
...@@ -1469,25 +1470,44 @@ private: ...@@ -1469,25 +1470,44 @@ private:
assert(Op || isIRGenerationDisabled()); assert(Op || isIRGenerationDisabled());
// Check if simple push works. // Check if simple push works.
NaClBcIndexSize_t LocalIndex = Index - CachedNumGlobalValueIDs; NaClBcIndexSize_t LocalIndex = Index - CachedNumGlobalValueIDs;
if (LocalIndex == LocalOperands.size()) {
LocalOperands.push_back(Op);
return;
}
// Must be forward reference, expand vector to accommodate.
if (LocalIndex >= LocalOperands.size()) {
if (LocalIndex > MaxRecordsInBlock) {
std::string Buffer;
raw_string_ostream StrBuf(Buffer);
StrBuf << "Forward reference @" << Index << " too big. Have "
<< CachedNumGlobalValueIDs << " globals and function contains "
<< NumBytesDefiningFunction << " bytes";
Fatal(StrBuf.str());
// Recover by using index one beyond the maximal allowed.
LocalIndex = MaxRecordsInBlock;
}
LocalOperands.resize(LocalIndex + 1);
}
// If element not defined, set it. // If element not defined, set it.
Ice::Operand *&IndexedOp = LocalOperands[LocalIndex]; Ice::Operand *OldOp = LocalOperands[LocalIndex];
if (IndexedOp == nullptr) { if (OldOp == nullptr) {
IndexedOp = Op; LocalOperands[LocalIndex] = Op;
return; return;
} }
// See if forward reference matchers. // See if forward reference matches.
if (IndexedOp == Op) if (OldOp == Op)
return; return;
// Error has occurred. // Error has occurred.
std::string Buffer; std::string Buffer;
raw_string_ostream StrBuf(Buffer); raw_string_ostream StrBuf(Buffer);
StrBuf << "Multiple definitions for index " << Index << ": " << *Op StrBuf << "Multiple definitions for index " << Index << ": " << *Op
<< " and " << *IndexedOp; << " and " << *OldOp;
Error(StrBuf.str()); Error(StrBuf.str());
IndexedOp = Op; LocalOperands[LocalIndex] = Op;
} }
// Returns the relative operand (wrt to BaseIndex) referenced by // Returns the relative operand (wrt to BaseIndex) referenced by
...@@ -1988,26 +2008,6 @@ private: ...@@ -1988,26 +2008,6 @@ private:
} }
}; };
bool FunctionParser::verifyAllForwardRefsDefined() {
NaClBcIndexSize_t NumInstructions =
NextLocalInstIndex - CachedNumGlobalValueIDs;
if (NumInstructions == LocalOperands.size())
return true;
// Find undefined forward references and report.
std::vector<NaClBcIndexSize_t> UndefinedFwdRefs;
for (const OperandMap::value_type &Elmt : LocalOperands)
if (Elmt.first >= NextLocalInstIndex)
UndefinedFwdRefs.push_back(Elmt.first);
std::sort(UndefinedFwdRefs.begin(), UndefinedFwdRefs.end());
for (const NaClBcIndexSize_t Index : UndefinedFwdRefs) {
std::string Buffer;
raw_string_ostream StrBuf(Buffer);
StrBuf << "Instruction forward reference not defined: " << Index;
Error(StrBuf.str());
}
return false;
}
void FunctionParser::ExitBlock() { void FunctionParser::ExitBlock() {
// Check if the last instruction in the function was terminating. // Check if the last instruction in the function was terminating.
if (!InstIsTerminating) { if (!InstIsTerminating) {
...@@ -2027,8 +2027,6 @@ void FunctionParser::ExitBlock() { ...@@ -2027,8 +2027,6 @@ void FunctionParser::ExitBlock() {
} }
if (isIRGenerationDisabled()) if (isIRGenerationDisabled())
return; return;
if (!verifyAllForwardRefsDefined())
return;
// Before translating, check for blocks without instructions, and // Before translating, check for blocks without instructions, and
// insert unreachable. This shouldn't happen, but be safe. // insert unreachable. This shouldn't happen, but be safe.
size_t Index = 0; size_t Index = 0;
...@@ -2078,21 +2076,17 @@ void FunctionParser::ProcessRecord() { ...@@ -2078,21 +2076,17 @@ void FunctionParser::ProcessRecord() {
return; return;
} }
uint64_t NumBbs = Values[0];
// Check for bad large sizes, since they can make ridiculous memory // Check for bad large sizes, since they can make ridiculous memory
// requests and hang the user for large amounts of time. Note: We know // requests and hang the user for large amounts of time.
// that each basic block must have a terminator instruction, and each uint64_t NumBbs = Values[0];
// instruction is minimally defined by a two-bit abreviation. if (NumBbs > MaxRecordsInBlock) {
uint64_t MaxBbs = NumBytesDefiningFunction * (CHAR_BIT >> 1);
if (NumBbs > MaxBbs) {
std::string Buffer; std::string Buffer;
raw_string_ostream StrBuf(Buffer); raw_string_ostream StrBuf(Buffer);
StrBuf << "Function defines " << NumBbs StrBuf << "Function defines " << NumBbs
<< " basic blocks, which is too big for a function containing " << " basic blocks, which is too big for a function containing "
<< NumBytesDefiningFunction << " bytes"; << NumBytesDefiningFunction << " bytes";
Error(StrBuf.str()); Error(StrBuf.str());
NumBbs = MaxBbs; NumBbs = MaxRecordsInBlock;
} }
if (NumBbs == 0) { if (NumBbs == 0) {
......
tests_lit/parse_errs/Inputs/bad-var-fwdref.tbc 0 → 100644
65535,8,2;
1,1;
65535,17,2;
1,3;
2;
7,32;
21,0,0,1;
65534;
8,2,0,0,0;
65535,19,2;
5,0;
65534;
65535,14,2;
1,0,102;
65534;
65535,12,2;
1,1;
43,3105555534,1;
10;
65534;
65534;
tests_lit/parse_errs/bad-var-fwdref.test 0 → 100644
; Test if we recognize a forward reference that can't be in function block.
; REQUIRES: no_minimal_build
; RUN: not %pnacl_sz -bitcode-as-text %p/Inputs/bad-var-fwdref.tbc \
; RUN: -bitcode-format=pnacl -notranslate -no-ir-gen -build-on-read 2>&1 \
; RUN: | FileCheck %s
; CHECK: Forward reference @3105555534 too big. Have 1 globals and function contains 16 bytes
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment