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

lib/Bitcode/NaCl/Analysis/NaClObjDump.cpp

 //===-- NaClObjDump.cpp - Dump PNaCl bitcode contents ---------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/ADT/STLExtras.h"
@@ skipping 21 matching lines @@
 namespace {
 
 using namespace llvm;
 
 static cl::opt<bool>
 ReportWarningsAsErrors(
     "Werror",
     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.
 /// 2) The representation allows -0 (which is different than 0).
 class SignRotatedInt {
 public:
   SignRotatedInt(uint64_t Value, Type* ValueType)
       : SignedValue(Value >> 1),
         IsNegated((Value & 0x1)
                   && !(ValueType->isIntegerTy()
@@ skipping 883 matching lines @@
   const char *VerifyIntArithmeticOp(const char *Op, Type *OpTy) {
     if (!PNaClABITypeChecker::isValidIntArithmeticType(OpTy)) {
       Errors() << Op << ": Invalid integer arithmetic type: " << *OpTy;
     }
     return Op;
   }
 
   // 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
                  << ". Expects: " << Expected << "\n";
       } else {
         Errors() << Op << ": Not allowed for type: " << *Ty << "\n";
       }
     }
   }
 
@@ skipping 1583 matching lines @@
   }
 
   /// Checks if block Value is a valid branch target for the current
   /// function block. If not, generates an appropriate error message.
   void VerifyBranchRange(uint64_t Value) {
     if (0 == Value || Value >= ExpectedNumBbs) {
       Errors() << "Branch " << BitcodeId('b', Value)
                << " 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(
     unsigned BlockID,
     NaClDisBlockParser *EnclosingParser)
     : NaClDisBlockParser(BlockID, EnclosingParser),
       CurrentBbIndex(-1),
       ExpectedNumBbs(0),
       InstIsTerminating(false) {
   Context->ResetLocalCounters();
@@ skipping 200 matching lines @@
     return "ult";
   case CmpInst::FCMP_ULE:
     return "ule";
   case CmpInst::FCMP_UNE:
     return "une";
   case CmpInst::FCMP_TRUE:
     return "true";
   }
 }
 
+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) {
   case naclbitc::CONSTANTS_BLOCK_ID: {
     NaClDisConstantsParser Parser(BlockID, this);
     return Parser.ParseThisBlock();
   }
   case naclbitc::VALUE_SYMTAB_BLOCK_ID: {
     if (!PNaClAllowLocalSymbolTables) break;
     NaClDisValueSymtabParser Parser(BlockID, this);
@@ skipping 209 matching lines @@
   case naclbitc::FUNC_CODE_INST_ALLOCA: {
     // ALLOCA:     [size, align]
     if (Values.size() != 2) {
       Errors() << "Function alloca record expects 2 arguments. Found: "
                << Values.size() << "\n";
       break;
     }
     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?
     Tokens() << NextInstId() << Space() << "=" << Space() << StartCluster()
              << "alloca" << Space() << "i8" << Comma() << FinishCluster()
              << Space() << StartCluster() << TokenizeType(SizeType) << Space()
              << SizeId << Comma() << FinishCluster() << Space()
              << StartCluster() <<"align" << Space() << Alignment << Semicolon()
              << FinishCluster();
     InstallInstType(GetPointerType());
     break;
   }
   case naclbitc::FUNC_CODE_INST_LOAD: {
     // LOAD: [op, align, ty]
     if (Values.size() != 3) {
       Errors() << "Function load record expects 3 arguments. Found: "
                << 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);
     Tokens() << NextInstId() << Space() << "=" << Space() << StartCluster()
              << "load" << Space() << TokenizeType(LoadType) << "*" << Space()
              << GetBitcodeId(RelativeToAbsId(Values[0])) << Comma()
              << FinishCluster() << Space() << StartCluster()
              << "align" << Space() << Alignment << Semicolon()
              << FinishCluster();
     InstallInstType(LoadType);
     break;
   }
   case naclbitc::FUNC_CODE_INST_STORE: {
     // STORE: [ptr, val, align]
     if (Values.size() != 3) {
       Errors() << "Function store record expects 3 arguments. Found: "
                << 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);
     Context->VerifyMemoryAccessAlignment("store", ValType, Alignment);
     Tokens() << StartCluster() << "store" << Space() << TokenizeType(ValType)
              << Space() << GetBitcodeId(Val) << Comma() << FinishCluster()
              << Space() << StartCluster() << TokenizeType(ValType) << "*"
              << Space() << GetBitcodeId(RelativeToAbsId(Values[0])) << Comma()
              << FinishCluster() << Space() << StartCluster() << "align"
              << Space() << Alignment << Semicolon() << FinishCluster();
@@ skipping 466 matching lines @@
     ObjDump.Error() << "Expected 1 top level block in bitcode: Found:"
                     << NumBlocksRead << "\n";
     ErrorsFound = true;
   }
 
   ObjDump.Flush();
   return ErrorsFound || Parser.GetNumErrors() > 0;
 }
 
 }