Fixes case where terminator instruction is missing at end of function.

If the bitcode parser detects that the last block in the function
is missing a terminator, generate an error message and insert
a terminator instruction.

BUG= https://code.google.com/p/nativeclient/issues/detail?id=4214
R=stichnot@chromium.org

Review URL: https://codereview.chromium.org/1210013005.

diff --git a/pydir/run-pnacl-sz.py b/pydir/run-pnacl-sz.py
index e15b096..787910c 100755 (executable)
--- a/pydir/run-pnacl-sz.py
+++ b/pydir/run-pnacl-sz.py
@@ -78,6 +78,10 @@ def main():
     argparser.add_argument('--echo-cmd', required=False,
                            action='store_true',
                            help='Trace command that generates ICE instructions')
+    argparser.add_argument('--tbc', required=False, action='store_true',
+                           help='Input is textual bitcode (not .ll)')
+    argparser.add_argument('--expect-fail', required=False, action='store_true',
+                           help='Negate success of run by using LLVM not')
     argparser.add_argument('--args', '-a', nargs=argparse.REMAINDER,
                            default=[],
                            help='Remaining arguments are passed to pnacl-sz')
@@ -93,13 +97,24 @@ def main():
       raise RuntimeError("Can't specify both '--llvm-source' and " +
                          "'--no-local-syms'")
 
+    if args.llvm_source and args.tbc:
+      raise RuntimeError("Can't specify both '--tbc' and '--llvm-source'")
+
+    if args.llvm and args.tbc:
+      raise RuntimeError("Can't specify both '--tbc' and '--llvm'")
+
     cmd = []
-    if not args.llvm_source:
+    if args.tbc:
+      cmd = [os.path.join(pnacl_bin_path, 'pnacl-bcfuzz'), llfile,
+             '-bitcode-as-text', '-output', '-', '|']
+    elif not args.llvm_source:
       cmd = [os.path.join(pnacl_bin_path, 'llvm-as'), llfile, '-o', '-', '|',
              os.path.join(pnacl_bin_path, 'pnacl-freeze')]
       if not args.no_local_syms:
         cmd += ['--allow-local-symbol-tables']
       cmd += ['|']
+    if args.expect_fail:
+      cmd += [os.path.join(pnacl_bin_path, 'not')]
     cmd += [args.pnacl_sz]
     cmd += ['--target', args.target]
     if args.insts:

diff --git a/src/PNaClTranslator.cpp b/src/PNaClTranslator.cpp
index 5382bcc..fea11d0 100644 (file)
--- a/src/PNaClTranslator.cpp
+++ b/src/PNaClTranslator.cpp
@@ -1961,9 +1961,16 @@ private:
 };
 
 void FunctionParser::ExitBlock() {
-  if (isIRGenerationDisabled()) {
-    return;
+  // Check if the last instruction in the function was terminating.
+  if (!InstIsTerminating) {
+    Error("Last instruction in function not terminator");
+    if (isIRGenerationDisabled())
+      return;
+    // Recover by inserting an unreachable instruction.
+    CurrentNode->appendInst(Ice::InstUnreachable::create(Func.get()));
   }
+  if (isIRGenerationDisabled())
+    return;
   // Before translating, check for blocks without instructions, and
   // insert unreachable. This shouldn't happen, but be safe.
   size_t Index = 0;
@@ -2256,6 +2263,7 @@ void FunctionParser::ProcessRecord() {
   }
   case naclbitc::FUNC_CODE_INST_RET: {
     // RET: [opval?]
+    InstIsTerminating = true;
     if (!isValidRecordSizeInRange(0, 1, "return"))
       return;
     if (Values.empty()) {
@@ -2270,10 +2278,10 @@ void FunctionParser::ProcessRecord() {
       }
       CurrentNode->appendInst(Ice::InstRet::create(Func.get(), RetVal));
     }
-    InstIsTerminating = true;
     return;
   }
   case naclbitc::FUNC_CODE_INST_BR: {
+    InstIsTerminating = true;
     if (Values.size() == 1) {
       // BR: [bb#]
       if (isIRGenerationDisabled())
@@ -2306,7 +2314,6 @@ void FunctionParser::ProcessRecord() {
       CurrentNode->appendInst(
           Ice::InstBr::create(Func.get(), Cond, ThenBlock, ElseBlock));
     }
-    InstIsTerminating = true;
     return;
   }
   case naclbitc::FUNC_CODE_INST_SWITCH: {
@@ -2318,6 +2325,7 @@ void FunctionParser::ProcessRecord() {
     // unnecesary data fields (i.e. constants 1).  These were not
     // cleaned up in PNaCl bitcode because the bitcode format was
     // already frozen when the problem was noticed.
+    InstIsTerminating = true;
     if (!isValidRecordSizeAtLeast(4, "switch"))
       return;
 
@@ -2383,17 +2391,16 @@ void FunctionParser::ProcessRecord() {
     if (isIRGenDisabled)
       return;
     CurrentNode->appendInst(Switch);
-    InstIsTerminating = true;
     return;
   }
   case naclbitc::FUNC_CODE_INST_UNREACHABLE: {
     // UNREACHABLE: []
+    InstIsTerminating = true;
     if (!isValidRecordSize(0, "unreachable"))
       return;
     if (isIRGenerationDisabled())
       return;
     CurrentNode->appendInst(Ice::InstUnreachable::create(Func.get()));
-    InstIsTerminating = true;
     return;
   }
   case naclbitc::FUNC_CODE_INST_PHI: {

diff --git a/tests_lit/lit.cfg b/tests_lit/lit.cfg
index 10d1df1..3090e60 100644 (file)
--- a/tests_lit/lit.cfg
+++ b/tests_lit/lit.cfg
@@ -48,7 +48,7 @@ config.name = 'subzero'
 config.test_format = lit.formats.ShTest()
 
 # suffixes: A list of file extensions to treat as test files.
-config.suffixes = ['.ll']
+config.suffixes = ['.ll', '.test']
 
 # test_source_root: The root path where tests are located.
 config.test_source_root = os.path.dirname(__file__)
@@ -106,14 +106,16 @@ config.substitutions.append(('%lc2i', ' '.join(iflc2i_atts_cmd + pnacl_sz_cmd
 
 config.substitutions.append(('%pnacl_sz', pnacl_sz_tool))
 
-pnaclbintools = [r"\bFileCheck\b",
-                 r"\ble32-nacl-objdump\b",
-                 r"\bllvm-as\b",
-                 r"\bllvm-mc\b",
-                 r"\bllvm-readobj\b",
-                 r"\bnot\b",
-                 r"\bpnacl-freeze\b",
-                 r"\bpnacl-bcdis\b"]
+pnaclbintools = [r'\b' + x + r'\b' for x in
+                 ['FileCheck',
+                  'le32-nacl-objdump',
+                  'llvm-as',
+                  'llvm-mc',
+                  'llvm-readobj',
+                  'not',
+                  'pnacl-bcdis',
+                  'pnacl-bcfuzz',
+                  'pnacl-freeze']]
 
 for tool in pnaclbintools:
   # The re.sub() line is adapted from one of LLVM's lit.cfg files.

diff --git a/tests_lit/llvm2ice_tests/Input/no-terminator-inst.tbc b/tests_lit/llvm2ice_tests/Input/no-terminator-inst.tbc
new file mode 100644 (file)
index 0000000..1f89762
--- /dev/null
+++ b/tests_lit/llvm2ice_tests/Input/no-terminator-inst.tbc
@@ -0,0 +1,31 @@
+65535,8,2;
+1,1;
+65535,17,2;
+1,4;
+7,32;
+2;
+21,0,0,0;
+7,1;
+65534;
+8,2,0,0,0;
+65535,19,2;
+5,0;
+65534;
+65535,14,2;
+1,0,102,105,98;
+65534;
+65535,12,2;
+1,3;
+65535,11,2;
+1,0;
+4,2;
+65534;
+28,2,1,36;
+11,1,2,1;
+10,2;
+2,3,2,1;
+34,0,5,0;
+2,1,5,0;
+65534;
+5534;
+65534;

diff --git a/tests_lit/llvm2ice_tests/invalid.test b/tests_lit/llvm2ice_tests/invalid.test
new file mode 100644 (file)
index 0000000..e6ec2ed
--- /dev/null
+++ b/tests_lit/llvm2ice_tests/invalid.test
@@ -0,0 +1,7 @@
+; Test that we handle functions that don't end with a terminator instruction.
+; See issue: https://code.google.com/p/nativeclient/issues/detail?id=4214
+
+RUN: %p2i --expect-fail --tbc -i %p/Input/no-terminator-inst.tbc --insts \
+RUN:        | FileCheck --check-prefix=NO-TERM-INST %s
+
+; NO-TERM-INST: Last instruction in function not terminator