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

lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp

Index: lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp
diff --git a/lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp b/lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp
index 2d0be943df7b9f0ab0fc2c1ce26d06b2f2000ff3..fd68d3185cc26e94c6cbfaf76c2cd6adff58974b 100644
--- a/lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp
+++ b/lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp
@@ -28,7 +28,6 @@
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/MathExtras.h"
 #include "llvm/Support/MemoryBuffer.h"
-#include "llvm/Support/raw_ostream.h"
 using namespace llvm;
 
 
@@ -184,6 +183,26 @@ std::error_code NaClBitcodeReader::Error(ErrorType E,
   return Error(E);
 }
 
+std::error_code NaClBitcodeReader::getAlignmentValue(
+    uint64_t Exponent, unsigned &Alignment) {
+  // Note: Alignement = 2 ** (Exponent - 1).

[jvoung (off chromium), 2014/12/16 23:24:39]

Alignement -> Alignment

[Karl, 2014/12/17 20:52:38]

Done.

+  if (Exponent == 0) {

[jvoung (off chromium), 2014/12/16 23:24:39]

Hmm, I don't think we ever said that "alloca" could not have an align of 0.

I think only for load/store was that ever checked by the ABI checker. I'm not
sure what C/C++ source would give you an alloca with align 0, but it might
better to continue to allow 0. Is the reason for this the "Exp - 1" vs the "(1
<< x) >> 1" ?

[Karl, 2014/12/17 20:52:38]

Good point. I forgot about the alloca default case. Putting back to use two
shifts.

+    Alignment = 1; // Just in case it is accessed.
+    return Error(InvalidValue, "Alignment must be greater than 0");
+  }
+  if (Exponent > 30) { // Note: Exponent is one larger than actual.

[jvoung (off chromium), 2014/12/16 23:24:39]

"than actual" -> "than the limit"?

[jvoung (off chromium), 2014/12/16 23:24:39]

include/llvm/IR/Value.h has "MaximumAlignment", which is "29", so you may be
able to use that as the constant.

The downside of using that is if it changes out from under us.

Maybe use that + 1, and static assert that it is 29? If upstream changes it, we
could then fork the limit or follow... WDYT?

[Karl, 2014/12/17 20:52:38]

Good point. Adding reference to the constant.

[Karl, 2014/12/17 20:52:38]

Done.

+    Alignment = 1; // Just in case it is accessed.
+    std::string Buffer;
+    raw_string_ostream StrBuf(Buffer);
+    StrBuf << "Alignment can't be greater than 2**29. Found: 2**"
+           << (Exponent - 1);
+    return Error(InvalidValue, StrBuf.str());
+  }
+  uint32_t FixedExponent = Exponent - 1;
+  Alignment = 1 << FixedExponent;
+  return std::error_code();
+}
+
 std::error_code NaClBitcodeReader::ParseTypeTable() {
   DEBUG(dbgs() << "-> ParseTypeTable\n");
   if (Stream.EnterSubBlock(naclbitc::TYPE_BLOCK_ID_NEW))
@@ -402,7 +421,9 @@ public:
           return Reader.Error(NaClBitcodeReader::InvalidRecord,
                               "Bad GLOBALVAR_VAR record");
         ProcessingGlobal = true;
-        VarAlignment = (1 << Record[0]) >> 1;
+        if (std::error_code EC =
+            Reader.getAlignmentValue(Record[0], VarAlignment))
+          return EC;
         VarIsConstant = Record[1] != 0;
         // Assume (by default) there is a single initializer.
         VarInitializersNeeded = 1;
@@ -1480,8 +1501,10 @@ std::error_code NaClBitcodeReader::ParseFunctionBody(Function *F) {
       unsigned OpNum = 0;
       if (popValue(Record, &OpNum, NextValueNo, &Size))
         return Error(InvalidRecord, "Invalid ALLOCA record");
-      unsigned Align = Record[1];
-      I = new AllocaInst(Type::getInt8Ty(Context), Size, (1 << Align) >> 1);
+      unsigned Alignment;
+      if (std::error_code EC = getAlignmentValue(Record[1], Alignment))
+        return EC;
+      I = new AllocaInst(Type::getInt8Ty(Context), Size, Alignment);
       break;
     }
     case naclbitc::FUNC_CODE_INST_LOAD: {
@@ -1499,7 +1522,13 @@ std::error_code NaClBitcodeReader::ParseFunctionBody(Function *F) {
       Op = ConvertOpToType(Op, T->getPointerTo(), CurBBNo);
       if (Op == nullptr)
         return Error(InvalidTypeForValue, "Can't convert cast to type");
-      I = new LoadInst(Op, "", false, (1 << Record[OpNum]) >> 1);
+      unsigned Alignment;
+      if (std::error_code EC =
+         getLoadStoreAlignmentValue(Record[OpNum], TheModule->getDataLayout(),
+                                    T, "load", Alignment)) {
+        return EC;
+      }
+      I = new LoadInst(Op, "", false, Alignment);
       break;
     }
     case naclbitc::FUNC_CODE_INST_STORE: {
@@ -1511,10 +1540,17 @@ std::error_code NaClBitcodeReader::ParseFunctionBody(Function *F) {
           OpNum+1 != Record.size())
         return Error(InvalidRecord, "Invalid STORE record");
       Val = ConvertOpToScalar(Val, CurBBNo);
-      Ptr = ConvertOpToType(Ptr, Val->getType()->getPointerTo(), CurBBNo);
+      Type *ValType = Val->getType();
+      Ptr = ConvertOpToType(Ptr, ValType->getPointerTo(), CurBBNo);
       if (Ptr == nullptr)
         return Error(InvalidTypeForValue, "Can't convert cast to type");
-      I = new StoreInst(Val, Ptr, false, (1 << Record[OpNum]) >> 1);
+      unsigned Alignment;
+      if (std::error_code EC =
+         getLoadStoreAlignmentValue(Record[OpNum], TheModule->getDataLayout(),

[jvoung (off chromium), 2014/12/16 23:24:39]

A little ambivalent about the extra checks -- I don't think it would be easy to
switch the check on here, but switch it off later when the abi checker runs, so
that there isn't new overhead. On the other hand, the overhead is *probably*
negligible.

[Karl, 2014/12/17 20:52:38]

I decided to remove this, since the bitcode reader accepts 0 (for non-finalized
pexes), but a finalized pexe applies the additional constraints. Hence, we must
wait for the ABI verifier to find the mistake.

+                                    ValType, "store", Alignment)) {
+        return EC;
+      }
+      I = new StoreInst(Val, Ptr, false, Alignment);
       break;
     }
     case naclbitc::FUNC_CODE_INST_CALL: