Fix more MSVC warnings, courgette/ edition.

courgette/encoded_program.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "courgette/encoded_program.h"
 
 #include <algorithm>
 #include <map>
 #include <string>
 #include <vector>
@@ skipping 28 matching lines @@
 // field constructors.  Ditto for the destructor.
 EncodedProgram::EncodedProgram() : image_base_(0) {}
 EncodedProgram::~EncodedProgram() {}
 
 // Serializes a vector of integral values using Varint32 coding.
 template<typename V>
 CheckBool WriteVector(const V& items, SinkStream* buffer) {
   size_t count = items.size();
   bool ok = buffer->WriteSizeVarint32(count);
   for (size_t i = 0; ok && i < count;  ++i) {
-    COMPILE_ASSERT(sizeof(items[0]) <= sizeof(uint32),  // NOLINT
-                   T_must_fit_in_uint32);

[Peter Kasting, 2014/09/30 00:08:01]

This assertion isn't correct.  It's legal to pass in types larger than uint32 as
long as they hold values that can be expressed in 32 bits -- something which
WriteSizeVarint32() LOG_ASSERT()s already.

     ok = buffer->WriteSizeVarint32(items[i]);
   }
   return ok;
 }
 
 template<typename V>
 bool ReadVector(V* items, SourceStream* buffer) {
   uint32 count;
   if (!buffer->ReadVarint32(&count))
     return false;
@@ skipping 128 matching lines @@
       (*rvas)[i] = previous;
     else
       previous = (*rvas)[i];
   }
 }
 
 CheckBool EncodedProgram::AddOrigin(RVA origin) {
   return ops_.push_back(ORIGIN) && origins_.push_back(origin);
 }
 
-CheckBool EncodedProgram::AddCopy(uint32 count, const void* bytes) {
+CheckBool EncodedProgram::AddCopy(size_t count, const void* bytes) {
   const uint8* source = static_cast<const uint8*>(bytes);
 
   bool ok = true;
 
   // Fold adjacent COPY instructions into one.  This nearly halves the size of
   // an EncodedProgram with only COPY1 instructions since there are approx plain
   // 16 bytes per reloc.  This has a working-set benefit during decompression.
   // For compression of files with large differences this makes a small (4%)
   // improvement in size.  For files with small differences this degrades the
   // compressed size by 1.3%
   if (!ops_.empty()) {
     if (ops_.back() == COPY1) {
       ops_.back() = COPY;
       ok = copy_counts_.push_back(1);
     }
     if (ok && ops_.back() == COPY) {
       copy_counts_.back() += count;
-      for (uint32 i = 0; ok && i < count; ++i) {
+      for (size_t i = 0; ok && i < count; ++i) {
         ok = copy_bytes_.push_back(source[i]);
       }
       return ok;
     }
   }
 
   if (ok) {
     if (count == 1) {
       ok = ops_.push_back(COPY1) && copy_bytes_.push_back(source[0]);
     } else {
       ok = ops_.push_back(COPY) && copy_counts_.push_back(count);
-      for (uint32 i = 0; ok && i < count; ++i) {
+      for (size_t i = 0; ok && i < count; ++i) {
         ok = copy_bytes_.push_back(source[i]);
       }
     }
   }
 
   return ok;
 }
 
 CheckBool EncodedProgram::AddAbs32(int label_index) {
   return ops_.push_back(ABS32) && abs32_ix_.push_back(label_index);
@@ skipping 180 matching lines @@
       if (!VectorAt(rel32_rva_, index, &rva))
         return false;
       uint32 decompressed_op;
       if (!DisassemblerElf32ARM::Decompress(ARM_OFF8,
                                             static_cast<uint16>(op),
                                             static_cast<uint32>(rva -
                                                                 current_rva),
                                             &decompressed_op)) {
         return false;
       }
-      uint16 op16 = decompressed_op;
+      uint16 op16 = static_cast<uint16>(decompressed_op);
       if (!output->Write(&op16, 2))
         return false;
       current_rva += 2;
       break;
     }
     case REL32ARM11: {
       uint32 index;
       if (!VectorAt(rel32_ix_, ix_rel32_ix, &index))
         return false;
       ++ix_rel32_ix;
       RVA rva;
       if (!VectorAt(rel32_rva_, index, &rva))
         return false;
       uint32 decompressed_op;
       if (!DisassemblerElf32ARM::Decompress(ARM_OFF11, (uint16) op,
                                             (uint32) (rva - current_rva),
                                             &decompressed_op)) {
         return false;
       }
-      uint16 op16 = decompressed_op;
+      uint16 op16 = static_cast<uint16>(decompressed_op);
       if (!output->Write(&op16, 2))
         return false;
       current_rva += 2;
       break;
     }
     case REL32ARM24: {
       uint32 index;
       if (!VectorAt(rel32_ix_, ix_rel32_ix, &index))
         return false;
       ++ix_rel32_ix;
@@ skipping 88 matching lines @@
       case ORIGIN: {
         RVA section_rva;
         if (!VectorAt(origins_, ix_origins, &section_rva))
           return false;
         ++ix_origins;
         current_rva = section_rva;
         break;
       }
 
       case COPY: {
-        uint32 count;
+        size_t count;
         if (!VectorAt(copy_counts_, ix_copy_counts, &count))
           return false;
         ++ix_copy_counts;
-        for (uint32 i = 0;  i < count;  ++i) {
+        for (size_t i = 0;  i < count;  ++i) {
           uint8 b;
           if (!VectorAt(copy_bytes_, ix_copy_bytes, &b))
             return false;
           ++ix_copy_bytes;
           if (!output->Write(&b, 1))
             return false;
         }
-        current_rva += count;
+        current_rva += static_cast<RVA>(count);

[Peter Kasting, 2014/09/30 00:08:01]

It's somewhat inconsistent for me to complain earlier about it not being obvious
that casting a size_t to a uint32 at the callsite of EmitBytesInstruction() is
safe, but then allow that same cast to happen here.  I imagine changing RVA from
a uint32 to a size_t is out of the question, so another possibility besides
doing this is to use RVA (instead of size_t) for the type in the places I
modified in this CL.  Or perhaps the code here should do some sort of
checked_cast or something.

[dgarrett, 2014/10/01 20:49:37]

I suspect that count should remain a uint32 here. That IS what's being read out
of the encoded format.

[Peter Kasting, 2014/10/01 22:12:05]

? |copy_counts_| is now a SizeTVector, so |count| really is a size_t.  This goes
hand-in-hand with changing AddCopy() to take a size_t, etc.

As I said previously, it would also be possible to undo all the size_t changes
(they're all connected) and use RVA everywhere, in which case the cast here
would basically move to DisassemblerElf32::ParseSimpleRegion().  We could then
decide to upgrade that cast to something like a checked_cast.

(Or we could just upgrade the cast here.)

         break;
       }
 
       case COPY1: {
         uint8 b;
         if (!VectorAt(copy_bytes_, ix_copy_bytes, &b))
           return false;
         ++ix_copy_bytes;
         if (!output->Write(&b, 1))
           return false;
@@ skipping 211 matching lines @@
   if (assembled)
     return C_OK;
   return C_ASSEMBLY_FAILED;
 }
 
 void DeleteEncodedProgram(EncodedProgram* encoded) {
   delete encoded;
 }
 
 }  // end namespace