Don't allow instructions/globals to use alignment > 2**29.

lib/Bitcode/NaCl/Analysis/NaClObjDump.cpp

Index: lib/Bitcode/NaCl/Analysis/NaClObjDump.cpp
diff --git a/lib/Bitcode/NaCl/Analysis/NaClObjDump.cpp b/lib/Bitcode/NaCl/Analysis/NaClObjDump.cpp
index 3f84857feae7e2e8337affef7f6fffabdc09d02b..e3cd46ebbe46af69bd4d87930654d2f2c13276a4 100644
--- a/lib/Bitcode/NaCl/Analysis/NaClObjDump.cpp
+++ b/lib/Bitcode/NaCl/Analysis/NaClObjDump.cpp
@@ -39,6 +39,12 @@ ReportWarningsAsErrors(
     cl::desc("Report warnings as errors."),
     cl::init(false));
 
+static cl::opt<bool>
+NoDumpFinalizeRules(

[jvoung (off chromium), 2014/12/17 23:36:38]

I'm a bit confused about where this is used.

[Karl, 2014/12/17 23:53:00]

One would use this as a command-line argument on a non-finalized pexe. In such
cases, the alignment can be zero. However, for finalized pexes, zero is not
allowed on load/store.

There are actual examples of this where pnacl-freeze allows load/store alignment
of 0. It would be nice to be able to run pnacl-bcdis on such files (to see there
contents) without getting flooded with error messages. Hence, the point of this
flag.

[jvoung (off chromium), 2014/12/18 00:05:40]

Hmm but does tool work with non-finalized pexes?

For example, LLVMBitCodes.h and the plain BitcodeReader.cpp use stuff like
bitc::MODULE_CODE_GLOBALVAR, but the NaCl readers do not?

[Karl, 2014/12/18 16:56:05]

You are correct that we require the overall structure to match PNaCl rules. What
I am talking about is that we have many test pexe's (mostly hand written) for
which pnacl-freeze works. Note that pnacl-freeze doesn't check that the PNaCl
ABI has been fully met. These frozen pexes are then fed into various other tools
(using the PNaCl bitcode reader). These tests pass because they don't call the
PNaCl ABi verifier.

In such cases, since the file is in pexe format, we may want to look at their
contents. That is the purpose of this.

Note that when I tested the reader restriction on load/store alignments, I
discovered this problem. It is one of the reasons I backed off and removed this
from the PNaCl bitcode reader.

The alternative is to fix (about 15 LLVM tests). I decided not to do this.
Rather, I backed out the reader check. Rather, I added a flag to pnacl-bcdis so
that they could be read and looked at. An example case of this
llvm/test/NaCl/Bitcode/fast.ll. Most of the breakage is the use of 0 alignment
for a load/store.

[jvoung (off chromium), 2014/12/18 17:10:22]

Okay, I see. Yes it's very easy to do a hand-written test that mistakenly drops
the alignment (I've done it several times ;-)). I was confused because the flag
isn't used by any test and the default is "false", but it's okay if you know
you'll be using it.

It feels like for tools that disassemble (like llvm-dis, and pnacl-bcdis) the
goal is to see the contents so unless it's not possible it should be able to at
least dump the part that has the error.

A bit of bikeshedding the name of the flag though. Most of the other flags have
"pnaclabi" in them. E.g., "-verify-pnaclabi-functions" or
"-pnaclabi-allow-debug-metadata=0", or "-pnaclabi-verify-fatal-errors=0", so it
seems better to have something more consistent with those names.

[jvoung (off chromium), 2014/12/18 17:18:56]

Another way to look at it is the load/store align values being set to the
"right" thing is done by the -pnacl-abi-simplify-{pre/post}opt transformations,
not "pnacl-finalize" (which basically does a strip-metadata pass plus
pnacl-freeze), so it seems better to not mention "finalize" in this flag name.

[Karl, 2014/12/18 18:56:19]

Renamed to IgnorePNaClABIChecks (cl flag "-ignore-pnaclabi-checks").

Also added checks on this flag for appropriate calls to "PNaClABI..." methods.

+    "no-pnacl-dump-finalize-rules",
+    cl::desc("Don't dump finalized-only PNaCl bitcode violations"),
+    cl::init(false));
+
 /// Class to handle sign rotations in a human readable form. That is,
 /// the sign is in the low bit. The two special cases are:
 /// 1) -1 is true for i1.
@@ -942,7 +948,8 @@ public:
   // Checks the Alignment for loading/storing a value of type Ty. If
   // invalid, generates an appropriate error message.
   void VerifyMemoryAccessAlignment(const char *Op, Type *Ty,
-                                   uint64_t Alignment) {
+                                   unsigned Alignment) {
+    if (NoDumpFinalizeRules) return;
     if (!PNaClABIProps::isAllowedAlignment(&DL, Alignment, Ty)) {
       if (unsigned Expected = NaClGetExpectedLoadStoreAlignment(DL, Ty)) {
         Errors() << Op << ": Illegal alignment for " << *Ty
@@ -2546,6 +2553,11 @@ private:
                << " out of range. Not in [1," << ExpectedNumBbs << "]\n";
     }
   }
+
+  /// Convert alignment exponent (i.e. power of two (or zero)) to the
+  /// corresponding alignment to use. If alignment is too large, it generates
+  /// an error message and returns 0.
+  unsigned getAlignmentValue(uint64_t Exponent);
 };
 
 NaClDisFunctionParser::NaClDisFunctionParser(
@@ -2766,6 +2778,23 @@ const char *NaClDisFunctionParser::GetFcmpPredicate(uint32_t Opcode) {
   }
 }
 
+namespace {
+
+static const unsigned MaxAlignmentExponent = 29;
+static_assert(
+    (1u << MaxAlignmentExponent) == Value::MaximumAlignment,
+    "Inconsistency between Value.MaxAlignment and PNaCl alignment limit");
+}
+
+unsigned NaClDisFunctionParser::getAlignmentValue(uint64_t Exponent) {
+  if (Exponent > MaxAlignmentExponent + 1) {
+    Errors() << "Alignment can't be greater than 2**" << MaxAlignmentExponent
+             << ". Found: 2**" << (Exponent - 1) << "\n";
+    return 0;
+  }
+  return (1 << static_cast<unsigned>(Exponent)) >> 1;
+}
+
 bool NaClDisFunctionParser::ParseBlock(unsigned BlockID) {
   ObjDumpSetRecordBitAddress(GetBlock().GetStartBit());
   switch (BlockID) {
@@ -2995,7 +3024,7 @@ void NaClDisFunctionParser::ProcessRecord() {
     uint32_t SizeOp = RelativeToAbsId(Values[0]);
     Type* SizeType = GetValueType(SizeOp);
     BitcodeId SizeId(GetBitcodeId(SizeOp));
-    uint64_t Alignment = (1 << Values[1]) >> 1;
+    unsigned Alignment = getAlignmentValue(Values[1]);
     if (!PNaClABIProps::isAllocaSizeType(SizeType))
       Errors() << PNaClABIProps::ExpectedAllocaSizeType() << "\n";
     // TODO(kschimpf) Are there any constraints on alignment?
@@ -3015,7 +3044,7 @@ void NaClDisFunctionParser::ProcessRecord() {
                << Values.size() << "\n";
       break;
     }
-    uint64_t Alignment = (1 << Values[1]) >> 1;
+    unsigned Alignment = getAlignmentValue(Values[1]);
     Type *LoadType = GetType(Values[2]);
     VerifyScalarOrVectorOp("load", LoadType);
     Context->VerifyMemoryAccessAlignment("load", LoadType, Alignment);
@@ -3035,7 +3064,7 @@ void NaClDisFunctionParser::ProcessRecord() {
                << Values.size() << "\n";
       break;
     }
-    uint64_t Alignment = (1 << Values[2]) >> 1;
+    unsigned Alignment = getAlignmentValue(Values[2]);
     uint32_t Val = RelativeToAbsId(Values[1]);
     Type *ValType = GetValueType(Val);
     VerifyScalarOrVectorOp("store", ValType);