[libFuzzer] Create single template for visiting Inline8bitCounters

[kevinwkt]

Summary

Created IterateInline8bitCounters, a single template for visiting Inline8bitCounters (nested for loop)
Made InitializeUnstableCounters and UpdateUnstableCounters both send a lambda to IterateInline8bitCounters.

Patch by Kyungtak Woo (@kevinwkt).

[Dor1s]

Left some minor comments, but I think it's fine to ask Matt or Kostya to take a look and address all the feedback altogether, just to avoid changing things back and forth if we have different opinions.

lib/fuzzer/FuzzerTracePC.cpp

68–69

I don't like getting an address of an array element, IMO Beg + j would be better, but let's see what the code owners think.

77

change beg to Beg

and actually it's not a "beginning" anymore, needs another name, maybe CounterPtr or just Counter.

86–89

beg -> Beg

lib/fuzzer/FuzzerTracePC.h

182

nit: not a language expert myself, but maybe this should be IterateOverInline8bitCounters. Current version LGTM as well.

[metzman]

LGTM with Max's suggestions

[kevinwkt]

Changed beg to Counter for both InitializeUnstableCounters and UpdateUnstableCounters.

[morehouse]

Maybe change the callback signature to take i, j, and UnstableIdx instead. Then we can also use IterateInline8bitCounters from UpdateObservedPCs.

lib/fuzzer/FuzzerTracePC.cpp

68–69

Can we just pass Beg[j] instead?

[kevinwkt]

PTAL
Changed callback signature from "UnstableIdx, &Beg[j]" to "i, j, UnstableIdx".
Modified UpdateObservedPCs to use the template function.

Did not use template function for case: NumGuards == NumPCsInPCTables because of initial check in template function.

[morehouse]

lib/fuzzer/FuzzerTracePC.cpp

62

Maybe this should be an assert.

63

Since we've generalized this function, we might want to change this variable to a more general name (e.g., CounterIdx).

[kevinwkt]

ptal
Changed UnstableIdx to CounterIdx.
Moved initial " NumInline8bitCounters && NumInline8bitCounters == NumPCsInPCTables" out of "IterateInline8bitCounters" to use it also in case: NumGuards == NumPCsInPCTables within UpdateObservedPCs().

[morehouse]

lib/fuzzer/FuzzerTracePC.cpp

209

I don't think this will work. IterateInline8bitCounters iterates over ModuleCounters, not Modules.

[kevinwkt]

ptal: fixed
Reverted for when Guards are used.
UnstableIdx is still changed to CounterIdx

[Dor1s]

Rebase and getting ready to land.

[kevinwkt]

Fixed syntax error causing CounterIdx error.

lib/fuzzer/FuzzerTracePC.h

private:
   size_t NumPCsInPCTables;
 
   uint8_t *Counters() const;
   uintptr_t *PCs() const;
 
   Set<uintptr_t> ObservedPCs;
   Set<uintptr_t> ObservedFuncs;
 
+  template <class Callback>
+  void IterateInline8bitCounters(Callback CB) const;
+
   std::pair<size_t, size_t> FocusFunction = {-1, -1};  // Module and PC IDs.
 
   ValueBitMap ValueProfileMap;
   uintptr_t InitialStack;
 };
 
 template <class Callback>
 // void Callback(size_t FirstFeature, size_t Idx, uint8_t Value);

lib/fuzzer/FuzzerTracePC.cpp

if (ObservedPCs.size())
     return ObservedPCs.size();
   size_t Res = 0;
   for (size_t i = 1, N = GetNumPCs(); i < N; i++)
     if (PCs()[i])
       Res++;
   return Res;
 }
 
-// Initializes unstable counters by copying Inline8bitCounters to unstable
-// counters.
-void TracePC::InitializeUnstableCounters() {
+template<class CallBack>
+void TracePC::IterateInline8bitCounters(CallBack CB) const {
   if (NumInline8bitCounters && NumInline8bitCounters == NumPCsInPCTables) {
-    size_t UnstableIdx = 0;
+    size_t CounterIdx = 0;
     for (size_t i = 0; i < NumModulesWithInline8bitCounters; i++) {
       uint8_t *Beg = ModuleCounters[i].Start;
       size_t Size = ModuleCounters[i].Stop - Beg;
       assert(Size == (size_t)(ModulePCTable[i].Stop - ModulePCTable[i].Start));
-      for (size_t j = 0; j < Size; j++, UnstableIdx++)
-        if (UnstableCounters[UnstableIdx] != kUnstableCounter)
-          UnstableCounters[UnstableIdx] = Beg[j];
-    }
-  }
+      for (size_t j = 0; j < Size; j++, CounterIdx++)
+        CB(i, j, CounterIdx);
+    }
+
+// Initializes unstable counters by copying Inline8bitCounters to unstable
+// counters.
+void TracePC::InitializeUnstableCounters() {
+  IterateInline8bitCounters([&](int i, int j, int UnstableIdx) {
+    if (UnstableCounters[UnstableIdx] != kUnstableCounter)
+      UnstableCounters[UnstableIdx] = ModuleCounters[i].Start[j];
+  });
 }
 
 // Compares the current counters with counters from previous runs
 // and records differences as unstable edges.
 void TracePC::UpdateUnstableCounters() {
-  if (NumInline8bitCounters && NumInline8bitCounters == NumPCsInPCTables) {
-    size_t UnstableIdx = 0;
-    for (size_t i = 0; i < NumModulesWithInline8bitCounters; i++) {
-      uint8_t *Beg = ModuleCounters[i].Start;
-      size_t Size = ModuleCounters[i].Stop - Beg;
-      assert(Size == (size_t)(ModulePCTable[i].Stop - ModulePCTable[i].Start));
-      for (size_t j = 0; j < Size; j++, UnstableIdx++)
-        if (Beg[j] != UnstableCounters[UnstableIdx])
-          UnstableCounters[UnstableIdx] = kUnstableCounter;
-    }
-  }
+  IterateInline8bitCounters([&](int i, int j, int UnstableIdx) {
+    if (ModuleCounters[i].Start[j] != UnstableCounters[UnstableIdx])
+  });
 }
 
 void TracePC::HandleInline8bitCountersInit(uint8_t *Start, uint8_t *Stop) {
   if (Start == Stop) return;
   if (NumModulesWithInline8bitCounters &&
       ModuleCounters[NumModulesWithInline8bitCounters-1].Start == Start) return;
   assert(NumModulesWithInline8bitCounters <
          sizeof(ModuleCounters) / sizeof(ModuleCounters[0]));
auto Observe = [&](const PCTableEntry &TE) {
     if (TE.PCFlags & 1)
       if (ObservedFuncs.insert(TE.PC).second && NumPrintNewFuncs)
         CoveredFuncs.push_back(TE.PC);
     ObservePC(TE.PC);
   };
 
   if (NumPCsInPCTables) {
     if (NumInline8bitCounters == NumPCsInPCTables) {
-      for (size_t i = 0; i < NumModulesWithInline8bitCounters; i++) {
-        uint8_t *Beg = ModuleCounters[i].Start;
-        size_t Size = ModuleCounters[i].Stop - Beg;
-        assert(Size ==
-               (size_t)(ModulePCTable[i].Stop - ModulePCTable[i].Start));
-        for (size_t j = 0; j < Size; j++)
-          if (Beg[j])
-            Observe(ModulePCTable[i].Start[j]);
-      }
+      IterateInline8bitCounters([&](int i, int j, int CounterIdx) {
+        if (ModuleCounters[i].Start[j])
+      });
     } else if (NumGuards == NumPCsInPCTables) {
       size_t GuardIdx = 1;
       for (size_t i = 0; i < NumModules; i++) {
         uint32_t *Beg = Modules[i].Start;
         size_t Size = Modules[i].Stop - Beg;
         assert(Size ==
                (size_t)(ModulePCTable[i].Stop - ModulePCTable[i].Start));
         for (size_t j = 0; j < Size; j++, GuardIdx++)
           if (Counters()[GuardIdx])
             Observe(ModulePCTable[i].Start[j]);
       }
     }
   }
 
   for (size_t i = 0, N = Min(CoveredFuncs.size(), NumPrintNewFuncs); i < N; i++) {
     Printf("\tNEW_FUNC[%zd/%zd]: ", i + 1, CoveredFuncs.size());
     PrintPC("%p %F %L", "%p", CoveredFuncs[i] + 1);
     Printf("\n");
   }
 }