Fix locking for printing error messages.
The method TopLevelParser::ErrorAt applies a lock to print the error
message. Unfortunately, it keeps the lock longer than necessary,
resulting in deadlock (on following fatal message) if error recovery
is not allowed.
Fixed by limiting scope of lock to only apply to the printing of the
error message.
Modified ClFlags to allow a "reset", and made ClFlags modifiable
by bitcode munge tests. This allowed us to test this problem as
a unit test.
BUG= https://code.google.com/p/nativeclient/issues/detail?id=4138
R=jvoung@chromium.org
Review URL: https://codereview.chromium.org/1091023002
diff --git a/src/IceClFlags.cpp b/src/IceClFlags.cpp
index 9e24665..fda4e06 100644
--- a/src/IceClFlags.cpp
+++ b/src/IceClFlags.cpp
@@ -245,6 +245,43 @@
AppName = IceString(argv[0]);
}
+void ClFlags::resetClFlags(ClFlags &OutFlags) {
+ // bool fields
+ OutFlags.AllowErrorRecovery = false;
+ OutFlags.AllowUninitializedGlobals = false;
+ OutFlags.DataSections = false;
+ OutFlags.DecorateAsm = false;
+ OutFlags.DisableInternal = false;
+ OutFlags.DisableIRGeneration = false;
+ OutFlags.DisableTranslation = false;
+ OutFlags.DumpStats = false;
+ OutFlags.FunctionSections = false;
+ OutFlags.GenerateUnitTestMessages = false;
+ OutFlags.PhiEdgeSplit = false;
+ OutFlags.RandomNopInsertion = false;
+ OutFlags.RandomRegAlloc = false;
+ OutFlags.SubzeroTimingEnabled = false;
+ OutFlags.TimeEachFunction = false;
+ OutFlags.UseSandboxing = false;
+ // Enum and integer fields.
+ OutFlags.Opt = Opt_m1;
+ OutFlags.OutFileType = FT_Iasm;
+ OutFlags.RandomMaxNopsPerInstruction = 0;
+ OutFlags.RandomNopProbabilityAsPercentage = 0;
+ OutFlags.TArch = TargetArch_NUM;
+ OutFlags.VMask = IceV_None;
+ // IceString fields.
+ OutFlags.DefaultFunctionPrefix = "";
+ OutFlags.DefaultGlobalPrefix = "";
+ OutFlags.TestPrefix = "";
+ OutFlags.TimingFocusOn = "";
+ OutFlags.TranslateOnly = "";
+ OutFlags.VerboseFocusOn = "";
+ // size_t and 64-bit fields.
+ OutFlags.NumTranslationThreads = 0;
+ OutFlags.RandomSeed = 0;
+}
+
void ClFlags::getParsedClFlags(ClFlags &OutFlags) {
if (::DisableIRGeneration)
::DisableTranslation = true;
diff --git a/src/IceClFlags.h b/src/IceClFlags.h
index 0762002..c89e695 100644
--- a/src/IceClFlags.h
+++ b/src/IceClFlags.h
@@ -26,27 +26,10 @@
ClFlags &operator=(const ClFlags &) = delete;
public:
- ClFlags()
- : // bool fields.
- AllowErrorRecovery(false),
- AllowUninitializedGlobals(false), DataSections(false),
- DecorateAsm(false), DisableInternal(false), DisableIRGeneration(false),
- DisableTranslation(false), DumpStats(false), FunctionSections(false),
- GenerateUnitTestMessages(false), PhiEdgeSplit(false),
- RandomNopInsertion(false), RandomRegAlloc(false),
- SubzeroTimingEnabled(false), TimeEachFunction(false),
- UseSandboxing(false),
- // Enum and integer fields.
- Opt(Opt_m1), OutFileType(FT_Iasm), RandomMaxNopsPerInstruction(0),
- RandomNopProbabilityAsPercentage(0), TArch(TargetArch_NUM),
- VMask(IceV_None),
- // IceString fields.
- DefaultFunctionPrefix(""), DefaultGlobalPrefix(""), TestPrefix(""),
- TimingFocusOn(""), TranslateOnly(""), VerboseFocusOn(""),
- // size_t and 64-bit fields.
- NumTranslationThreads(0), RandomSeed(0) {}
+ ClFlags() { resetClFlags(*this); }
static void parseFlags(int argc, char *argv[]);
+ static void resetClFlags(ClFlags &OutFlags);
static void getParsedClFlags(ClFlags &OutFlags);
static void getParsedClFlagsExtra(ClFlagsExtra &OutFlagsExtra);
diff --git a/src/PNaClTranslator.cpp b/src/PNaClTranslator.cpp
index 35cdcf7..ce46239 100644
--- a/src/PNaClTranslator.cpp
+++ b/src/PNaClTranslator.cpp
@@ -486,10 +486,12 @@
ErrorStatus.assign(Ice::EC_Bitcode);
++NumErrors;
Ice::GlobalContext *Context = Translator.getContext();
- Ice::OstreamLocker L(Context);
- raw_ostream &OldErrStream = setErrStream(Context->getStrDump());
- NaClBitcodeParser::ErrorAt(Level, Bit, Message);
- setErrStream(OldErrStream);
+ { // Lock while printing out error message.
+ Ice::OstreamLocker L(Context);
+ raw_ostream &OldErrStream = setErrStream(Context->getStrDump());
+ NaClBitcodeParser::ErrorAt(Level, Bit, Message);
+ setErrStream(OldErrStream);
+ }
if (Level >= naclbitc::Error &&
!Translator.getFlags().getAllowErrorRecovery())
Fatal();
diff --git a/unittest/BitcodeMunge.cpp b/unittest/BitcodeMunge.cpp
index fd7100c..689279e 100644
--- a/unittest/BitcodeMunge.cpp
+++ b/unittest/BitcodeMunge.cpp
@@ -19,19 +19,25 @@
namespace IceTest {
-bool IceTest::SubzeroBitcodeMunger::runTest(const char *TestName,
- const uint64_t Munges[],
- size_t MungeSize) {
- const bool AddHeader = true;
- setupTest(TestName, Munges, MungeSize, AddHeader);
+void IceTest::SubzeroBitcodeMunger::resetFlags() {
+ Ice::ClFlags::resetClFlags(Flags);
+ resetMungeFlags();
+}
- Ice::ClFlags Flags;
+void IceTest::SubzeroBitcodeMunger::resetMungeFlags() {
Flags.setAllowErrorRecovery(true);
Flags.setGenerateUnitTestMessages(true);
Flags.setOptLevel(Ice::Opt_m1);
Flags.setOutFileType(Ice::FT_Iasm);
Flags.setTargetArch(Ice::Target_X8632);
Flags.setVerbose(Ice::IceV_Instructions);
+}
+
+bool IceTest::SubzeroBitcodeMunger::runTest(const char *TestName,
+ const uint64_t Munges[],
+ size_t MungeSize) {
+ const bool AddHeader = true;
+ setupTest(TestName, Munges, MungeSize, AddHeader);
Ice::GlobalContext Ctx(DumpStream, DumpStream, nullptr, Flags);
Ice::PNaClTranslator Translator(&Ctx);
Translator.translateBuffer(TestName, MungedInput.get());
diff --git a/unittest/BitcodeMunge.h b/unittest/BitcodeMunge.h
index 6b639a7..0b99864 100644
--- a/unittest/BitcodeMunge.h
+++ b/unittest/BitcodeMunge.h
@@ -17,6 +17,8 @@
#include "llvm/Bitcode/NaCl/NaClBitcodeMunge.h"
+#include "IceClFlags.h"
+
namespace IceTest {
// Class to run tests on Subzero's bitcode parser. Runs a Subzero
@@ -27,7 +29,9 @@
public:
SubzeroBitcodeMunger(const uint64_t Records[], size_t RecordSize,
uint64_t RecordTerminator)
- : llvm::NaClBitcodeMunger(Records, RecordSize, RecordTerminator) {}
+ : llvm::NaClBitcodeMunger(Records, RecordSize, RecordTerminator) {
+ resetMungeFlags();
+ }
/// Runs PNaClTranslator to translate bitcode records (with defined
/// record Munges), and puts output into DumpResults. Returns true
@@ -39,6 +43,15 @@
uint64_t NoMunges[] = {0};
return runTest(TestName, NoMunges, 0);
}
+
+ /// Sets flags back to default assumptions for munging.
+ void resetFlags();
+
+ /// Flags to use to run tests. Use to change default assumptions.
+ Ice::ClFlags Flags;
+
+private:
+ void resetMungeFlags();
};
} // end of namespace IceTest
diff --git a/unittest/IceParseInstsTest.cpp b/unittest/IceParseInstsTest.cpp
index 5d45888..191eff9 100644
--- a/unittest/IceParseInstsTest.cpp
+++ b/unittest/IceParseInstsTest.cpp
@@ -63,6 +63,12 @@
EXPECT_FALSE(Munger.runTest("Nonexistent call arg"));
EXPECT_EQ("Error(66:4): Invalid function record: <34 0 4 2 100>\n",
Munger.getTestResults());
+
+ // Show that we generate a fatal error when not allowing error recovery.
+ Munger.Flags.setAllowErrorRecovery(false);
+ EXPECT_DEATH(
+ Munger.runTest("Nonexistent call arg"),
+ ".*ERROR: Unable to continue.*");
}
/// Test how we recognize alignments in alloca instructions.
