Revert of Enforce 32/64 bit compatibility for persistent objects. (pa…

…tchset chromium#7 id:260001 of https://codereview.chromium.org/2490303002/ )

Reason for revert:
I suspect this broke webkit_unit_tests on Nexus 4: https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Android%20%28Nexus4%29/builds/57305  (that CL landed for that build even though it doesn't show up crbug.com/667838)
https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.webkit%2FWebKit_Android__Nexus4_%2F57305%2F%2B%2Frecipes%2Fsteps%2Fwebkit_unit_tests%2F0%2Fstdout

Many histogram-related errors

Original issue's description:
> Enforce 32/64 bit compatibility for persistent objects.
>
> Architecture changes such as an upgrade from 32-bit to 64-bit can be problematic
> for binary structures held on disk because the "struct" representations of the data
> can change with differences in the natural word size which affects the size and
> alignment requirements of fields.
>
> The checks added here ensure that an "object" stored in the PMA will have the same
> binary layout across different compilers and word sizes.
>
> It's definitely a benefit as it already found two cases where the data was represented
> differently between 32-bit and 64-bit natural sizes.
>
> SHERIFFS: This could cause compile problems outside of the gamut of
> the try-bots.  Just revert it if so.
>
> BUG=546019
> TBR=siggi  (for browser_watcher change)
>
> Committed: https://crrev.com/3c30ee3e18065db69c15c2f6abf8d912f573bb4f
> Cr-Commit-Position: refs/heads/master@{#433849}

TBR=asvitkine@chromium.org,siggi@chromium.org,bcwhite@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=546019

Review-Url: https://codereview.chromium.org/2521513005
Cr-Commit-Position: refs/heads/master@{#433930}

base/debug/activity_analyzer.cc

@@ -25,10 +25,9 @@ ThreadActivityAnalyzer::ThreadActivityAnalyzer(void* base, size_t size)
 ThreadActivityAnalyzer::ThreadActivityAnalyzer(
     PersistentMemoryAllocator* allocator,
     PersistentMemoryAllocator::Reference reference)
-    : ThreadActivityAnalyzer(allocator->GetAsArray<char>(
+    : ThreadActivityAnalyzer(allocator->GetAsObject<char>(
                                  reference,
-                                 GlobalActivityTracker::kTypeIdActivityTracker,
-                                 1),
+                                 GlobalActivityTracker::kTypeIdActivityTracker),
                              allocator->GetAllocSize(reference)) {}
 
 ThreadActivityAnalyzer::~ThreadActivityAnalyzer() {}
@@ -110,9 +109,8 @@ void GlobalActivityAnalyzer::PrepareAllAnalyzers() {
   for (PersistentMemoryAllocator::Reference tracker_ref : tracker_references_) {
     // Get the actual data segment for the tracker. This can fail if the
     // record has been marked "free" since the type will not match.
-    void* base = allocator_->GetAsArray<char>(
-        tracker_ref, GlobalActivityTracker::kTypeIdActivityTracker,
-        PersistentMemoryAllocator::kSizeAny);
+    void* base = allocator_->GetAsObject<char>(
+        tracker_ref, GlobalActivityTracker::kTypeIdActivityTracker);
     if (!base)
       continue;
 

base/debug/activity_tracker.cc

@@ -150,8 +150,7 @@ ActivityTrackerMemoryAllocator::GetObjectReference() {
 
 void ActivityTrackerMemoryAllocator::ReleaseObjectReference(Reference ref) {
   // Zero the memory so that it is ready for immediate use if needed later.
-  char* mem_base = allocator_->GetAsArray<char>(
-      ref, object_type_, PersistentMemoryAllocator::kSizeAny);
+  char* mem_base = allocator_->GetAsObject<char>(ref, object_type_);
   DCHECK(mem_base);
   memset(mem_base, 0, object_size_);
 
@@ -307,15 +306,9 @@ void ActivityUserData::SetReference(StringPiece name,
 // the very first time the thread is seen. All fields must be of exact sizes
 // so there is no issue moving between 32 and 64-bit builds.
 struct ThreadActivityTracker::Header {
-  // Expected size for 32/64-bit check.
-  static constexpr size_t kExpectedInstanceSize = 80;
-
   // This unique number indicates a valid initialization of the memory.
   std::atomic<uint32_t> cookie;
-
-  // The number of Activity slots (spaces that can hold an Activity) that
-  // immediately follow this structure in memory.
-  uint32_t stack_slots;
+  uint32_t reserved;  // pad out to 64 bits
 
   // The process-id and thread-id (thread_ref.as_id) to which this data belongs.
   // These identifiers are not guaranteed to mean anything but are unique, in
@@ -337,6 +330,9 @@ struct ThreadActivityTracker::Header {
   int64_t start_time;
   int64_t start_ticks;
 
+  // The number of Activity slots in the data.
+  uint32_t stack_slots;
+
   // The current depth of the stack. This may be greater than the number of
   // slots. If the depth exceeds the number of slots, the newest entries
   // won't be recorded.
@@ -795,23 +791,9 @@ ThreadActivityTracker* GlobalActivityTracker::CreateTrackerForCurrentThread() {
   }
 
   // Convert the memory block found above into an actual memory address.
-  // Doing the conversion as a Header object enacts the 32/64-bit size
-  // consistency checks which would not otherwise be done. Unfortunately,
-  // some older compilers and MSVC don't have standard-conforming definitions
-  // of std::atomic which cause it not to be plain-old-data. Don't check on
-  // those platforms assuming that the checks on other platforms will be
-  // sufficient.
-  // TODO(bcwhite): Review this after major compiler releases.
   DCHECK(mem_reference);
-  void* mem_base;
-#if !defined(OS_WIN) && !defined(OS_ANDROID)
-  mem_base = allocator_->GetAsObject<ThreadActivityTracker::Header>(
-      mem_reference, kTypeIdActivityTracker);
-#else
-  mem_base = allocator_->GetAsArray<char>(mem_reference, kTypeIdActivityTracker,
-                                          PersistentMemoryAllocator::kSizeAny);
-#endif
-
+  void* mem_base =
+      allocator_->GetAsObject<char>(mem_reference, kTypeIdActivityTracker);
   DCHECK(mem_base);
   DCHECK_LE(stack_memory_size_, allocator_->GetAllocSize(mem_reference));
 
@@ -844,8 +826,8 @@ void* GlobalActivityTracker::GetUserDataMemory(
       return nullptr;
   }
 
-  void* memory = allocator_->GetAsArray<char>(
-      *reference, kTypeIdUserDataRecord, PersistentMemoryAllocator::kSizeAny);
+  void* memory =
+      allocator_->GetAsObject<char>(*reference, kTypeIdUserDataRecord);
   DCHECK(memory);
   return memory;
 }
@@ -878,10 +860,9 @@ GlobalActivityTracker::GlobalActivityTracker(
                            kCachedUserDataMemories,
                            /*make_iterable=*/false),
       user_data_(
-          allocator_->GetAsArray<char>(
+          allocator_->GetAsObject<char>(
               allocator_->Allocate(kGlobalDataSize, kTypeIdGlobalDataRecord),
-              kTypeIdGlobalDataRecord,
-              PersistentMemoryAllocator::kSizeAny),
+              kTypeIdGlobalDataRecord),
           kGlobalDataSize) {
   // Ensure the passed memory is valid and empty (iterator finds nothing).
   uint32_t type;

base/debug/activity_tracker.h

@@ -415,11 +415,6 @@ class BASE_EXPORT ActivityUserData {
 // objects.
 class BASE_EXPORT ThreadActivityTracker {
  public:
-  // This structure contains all the common information about the thread so
-  // it doesn't have to be repeated in every entry on the stack. It is defined
-  // and used completely within the .cc file.
-  struct Header;
-
   using ActivityId = uint32_t;
 
   // This is the base class for having the compiler manage an activity on the
@@ -515,6 +510,11 @@ class BASE_EXPORT ThreadActivityTracker {
  private:
   friend class ActivityTrackerTest;
 
+  // This structure contains all the common information about the thread so
+  // it doesn't have to be repeated in every entry on the stack. It is defined
+  // and used completely within the .cc file.
+  struct Header;
+
   Header* const header_;        // Pointer to the Header structure.
   Activity* const stack_;       // The stack of activities.
   const uint32_t stack_slots_;  // The total number of stack slots.
@@ -539,7 +539,7 @@ class BASE_EXPORT GlobalActivityTracker {
   // will be safely ignored. These are public so that an external process
   // can recognize records of this type within an allocator.
   enum : uint32_t {
-    kTypeIdActivityTracker = 0x5D7381AF + 3,   // SHA1(ActivityTracker) v3
+    kTypeIdActivityTracker = 0x5D7381AF + 2,   // SHA1(ActivityTracker) v2
     kTypeIdUserDataRecord = 0x615EDDD7 + 1,    // SHA1(UserDataRecord) v1
     kTypeIdGlobalDataRecord = 0xAFE61ABE + 1,  // SHA1(GlobalDataRecord) v1
 

base/metrics/field_trial.cc

@@ -61,9 +61,6 @@ const size_t kFieldTrialAllocationSize = 64 << 10;  // 64 KiB
 // object that we unpickle and read from. Any changes to this structure requires
 // a bump in kFieldTrialType id defined above.
 struct FieldTrialEntry {
-  // Expected size for 32/64-bit check.
-  static constexpr size_t kExpectedInstanceSize = 8;
-
   // Whether or not this field trial is activated. This is really just a boolean
   // but marked as a uint32_t for portability reasons.
   uint32_t activated;

base/metrics/histogram_samples.h

@@ -27,9 +27,6 @@ class SampleCountIterator;
 class BASE_EXPORT HistogramSamples {
  public:
   struct Metadata {
-    // Expected size for 32/64-bit check.
-    static constexpr size_t kExpectedInstanceSize = 24;
-
     // Initialized when the sample-set is first created with a value provided
     // by the caller. It is generally used to identify the sample-set across
     // threads and processes, though not necessarily uniquely as it is possible
@@ -58,21 +55,7 @@ class BASE_EXPORT HistogramSamples {
     // might mismatch even when no memory corruption has happened.
     HistogramBase::AtomicCount redundant_count;
 
-    // 4 bytes of padding to explicitly extend this structure to a multiple of
-    // 64-bits. This is required to ensure the structure is the same size on
-    // both 32-bit and 64-bit builds.
-    char padding[4];
-  };
-
-  // Because sturctures held in persistent memory must be POD, there can be no
-  // default constructor to clear the fields. This derived class exists just
-  // to clear them when being allocated on the heap.
-  struct LocalMetadata : Metadata {
-    LocalMetadata() {
-      id = 0;
-      sum = 0;
-      redundant_count = 0;
-    }
+    Metadata() : id(0), sum(0), redundant_count(0) {}
   };
 
   explicit HistogramSamples(uint64_t id);
@@ -119,7 +102,7 @@ class BASE_EXPORT HistogramSamples {
   // In order to support histograms shared through an external memory segment,
   // meta values may be the local storage or external storage depending on the
   // wishes of the derived class.
-  LocalMetadata local_meta_;
+  Metadata local_meta_;
   Metadata* meta_;
 
   DISALLOW_COPY_AND_ASSIGN(HistogramSamples);

base/metrics/persistent_histogram_allocator.cc

@@ -35,7 +35,7 @@ const char kResultHistogram[] = "UMA.CreatePersistentHistogram.Result";
 // so that, if the structure of that object changes, stored older versions
 // will be safely ignored.
 enum : uint32_t {
-  kTypeIdHistogram = 0xF1645910 + 3,    // SHA1(Histogram)   v3
+  kTypeIdHistogram   = 0xF1645910 + 2,  // SHA1(Histogram)   v2
   kTypeIdRangesArray = 0xBCEA225A + 1,  // SHA1(RangesArray) v1
   kTypeIdCountsArray = 0x53215530 + 1,  // SHA1(CountsArray) v1
 
@@ -226,10 +226,6 @@ PersistentMemoryAllocator::Reference PersistentSampleMapRecords::CreateNew(
 // This data will be held in persistent memory in order for processes to
 // locate and use histograms created elsewhere.
 struct PersistentHistogramAllocator::PersistentHistogramData {
-  // Expected size for 32/64-bit check.
-  static constexpr size_t kExpectedInstanceSize =
-      40 + 2 * HistogramSamples::Metadata::kExpectedInstanceSize;
-
   int32_t histogram_type;
   int32_t flags;
   int32_t minimum;
@@ -244,7 +240,7 @@ struct PersistentHistogramAllocator::PersistentHistogramData {
   // Space for the histogram name will be added during the actual allocation
   // request. This must be the last field of the structure. A zero-size array
   // or a "flexible" array would be preferred but is not (yet) valid C++.
-  char name[sizeof(uint64_t)];  // Force 64-bit alignment on 32-bit builds.
+  char name[1];
 };
 
 PersistentHistogramAllocator::Iterator::Iterator(
@@ -342,15 +338,14 @@ std::unique_ptr<HistogramBase> PersistentHistogramAllocator::AllocateHistogram(
       return nullptr;
     }
 
-    size_t ranges_count = bucket_count + 1;
-    size_t ranges_bytes = ranges_count * sizeof(HistogramBase::Sample);
+    size_t ranges_bytes = (bucket_count + 1) * sizeof(HistogramBase::Sample);
     PersistentMemoryAllocator::Reference counts_ref =
         memory_allocator_->Allocate(counts_bytes, kTypeIdCountsArray);
     PersistentMemoryAllocator::Reference ranges_ref =
         memory_allocator_->Allocate(ranges_bytes, kTypeIdRangesArray);
     HistogramBase::Sample* ranges_data =
-        memory_allocator_->GetAsArray<HistogramBase::Sample>(
-            ranges_ref, kTypeIdRangesArray, ranges_count);
+        memory_allocator_->GetAsObject<HistogramBase::Sample>(
+            ranges_ref, kTypeIdRangesArray);
 
     // Only continue here if all allocations were successful. If they weren't,
     // there is no way to free the space but that's not really a problem since
@@ -544,9 +539,8 @@ std::unique_ptr<HistogramBase> PersistentHistogramAllocator::CreateHistogram(
   PersistentHistogramData histogram_data = *histogram_data_ptr;
 
   HistogramBase::Sample* ranges_data =
-      memory_allocator_->GetAsArray<HistogramBase::Sample>(
-          histogram_data.ranges_ref, kTypeIdRangesArray,
-          PersistentMemoryAllocator::kSizeAny);
+      memory_allocator_->GetAsObject<HistogramBase::Sample>(
+          histogram_data.ranges_ref, kTypeIdRangesArray);
 
   const uint32_t max_buckets =
       std::numeric_limits<uint32_t>::max() / sizeof(HistogramBase::Sample);
@@ -575,9 +569,8 @@ std::unique_ptr<HistogramBase> PersistentHistogramAllocator::CreateHistogram(
           created_ranges.release());
 
   HistogramBase::AtomicCount* counts_data =
-      memory_allocator_->GetAsArray<HistogramBase::AtomicCount>(
-          histogram_data.counts_ref, kTypeIdCountsArray,
-          PersistentMemoryAllocator::kSizeAny);
+      memory_allocator_->GetAsObject<HistogramBase::AtomicCount>(
+          histogram_data.counts_ref, kTypeIdCountsArray);
   size_t counts_bytes =
       CalculateRequiredCountsBytes(histogram_data.bucket_count);
   if (!counts_data || counts_bytes == 0 ||

base/metrics/persistent_memory_allocator.cc

@@ -295,14 +295,6 @@ PersistentMemoryAllocator::PersistentMemoryAllocator(Memory memory,
       corrupt_(0),
       allocs_histogram_(nullptr),
       used_histogram_(nullptr) {
-  // These asserts ensure that the structures are 32/64-bit agnostic and meet
-  // all the requirements of use within the allocator. They access private
-  // definitions and so cannot be moved to the global scope.
-  static_assert(sizeof(PersistentMemoryAllocator::BlockHeader) == 16,
-                "struct is not portable across different natural word widths");
-  static_assert(sizeof(PersistentMemoryAllocator::SharedMetadata) == 56,
-                "struct is not portable across different natural word widths");
-
   static_assert(sizeof(BlockHeader) % kAllocAlignment == 0,
                 "BlockHeader is not a multiple of kAllocAlignment");
   static_assert(sizeof(SharedMetadata) % kAllocAlignment == 0,
@@ -368,7 +360,7 @@ PersistentMemoryAllocator::PersistentMemoryAllocator(Memory memory,
     if (!name.empty()) {
       const size_t name_length = name.length() + 1;
       shared_meta()->name = Allocate(name_length, 0);
-      char* name_cstr = GetAsArray<char>(shared_meta()->name, 0, name_length);
+      char* name_cstr = GetAsObject<char>(shared_meta()->name, 0);
       if (name_cstr)
         memcpy(name_cstr, name.data(), name.length());
     }
@@ -416,8 +408,7 @@ uint64_t PersistentMemoryAllocator::Id() const {
 
 const char* PersistentMemoryAllocator::Name() const {
   Reference name_ref = shared_meta()->name;
-  const char* name_cstr =
-      GetAsArray<char>(name_ref, 0, PersistentMemoryAllocator::kSizeAny);
+  const char* name_cstr = GetAsObject<char>(name_ref, 0);
   if (!name_cstr)
     return "";