[NFC] Source maps: Simplify the code and add comments (WebAssembly#5912)

Almost entirely trivial, except for this part:

-  if (nextDebugLocation.availablePos == 0 &&
-      nextDebugLocation.previousPos <= pos) {
+  if (nextDebugLocation.availablePos == 0) {
      return;

I believe removing the extra check has no effect. Removing it does not change
anything in the test suite, and logically, if we set availablePos to 0 then we
definitely want to return here - we set it to 0 to indicate there is nothing left
to read, which is what the code after it does.

As a result, we can remove the previousPos field entirely.

Author: kripken

src/wasm-binary.h

@@ -1526,34 +1526,48 @@ class WasmBinaryReader {
   MixedArena& allocator;
   const std::vector<char>& input;
 
+  // Source map debugging support.
+
   std::istream* sourceMap;
 
-  struct NextDebugLocation {
-    uint32_t availablePos;
-    uint32_t previousPos;
-    Function::DebugLocation next;
-  } nextDebugLocation;
-
-  // Whether debug info is present next or not in the next debug location. A
-  // debug location can contain debug info (file:line:col) or it might not. We
-  // need to track this boolean alongside |nextDebugLocation| - that is, we
-  // can't just do something like std::optional<DebugLocation> or such - as we
-  // still need to track the values in |next|, as later positions are relative
-  // to them. That is, if we have line number 100, then no debug info, and then
-  // line number 500, then when we get to 500 we will see "+400" which is
-  // relative to the last existing line number (we "skip" over the place without
-  // debug info).
+  // The binary position that the next debug location refers to. That is, this
+  // is the first item in a source map entry that we have read (the "column", in
+  // source map terms, which for wasm means the offset in the binary). We have
+  // read this entry, but have not used it yet (we use it when we read the
+  // expression at this binary offset).
+  //
+  // This is set to 0 as an invalid value if we reach the end of the source map
+  // and there is nothing left to read.
+  size_t nextDebugPos;
+
+  // The debug location (file:line:col) corresponding to |nextDebugPos|. That
+  // is, this is the next 3 fields in a source map entry that we have read, but
+  // not used yet.
+  //
+  // If that location has no debug info (it lacks those 3 fields), then this
+  // contains the info from the previous one, because in a source map, these
+  // fields are relative to their last appearance, so we cannot forget them (we
+  // can't just do something like std::optional<DebugLocation> or such); for
+  // example, if we have line number 100, then no debug info, and then line
+  // number 500, then when we get to 500 we will see "+400" which is relative to
+  // the last existing line number (we "skip" over a place without debug info).
+  Function::DebugLocation nextDebugLocation;
+
+  // Whether debug info is present on |nextDebugPos| (see comment there).
   bool nextDebugLocationHasDebugInfo;
 
+  // Settings.
+
   bool debugInfo = true;
   bool DWARF = false;
   bool skipFunctionBodies = false;
 
+  // Internal state.
+
   size_t pos = 0;
   Index startIndex = -1;
   std::set<Function::DebugLocation> debugLocation;
   size_t codeSectionLocation;
-
   std::set<BinaryConsts::Section> seenSections;
 
   // All types defined in the type section

src/wasm/wasm-binary.cpp

@@ -1638,8 +1638,8 @@ void WasmBinaryWriter::writeField(const Field& field) {
 WasmBinaryReader::WasmBinaryReader(Module& wasm,
                                    FeatureSet features,
                                    const std::vector<char>& input)
-  : wasm(wasm), allocator(wasm.allocator), input(input),
-    sourceMap(nullptr), nextDebugLocation{0, 0, {0, 0, 0}},
+  : wasm(wasm), allocator(wasm.allocator), input(input), sourceMap(nullptr),
+    nextDebugPos(0), nextDebugLocation{0, 0, 0},
     nextDebugLocationHasDebugInfo(false), debugLocation() {
   wasm.features = features;
 }
@@ -2796,7 +2796,7 @@ void WasmBinaryReader::readSourceMapHeader() {
 
   mustReadChar('\"');
   if (maybeReadChar('\"')) { // empty mappings
-    nextDebugLocation.availablePos = 0;
+    nextDebugPos = 0;
     return;
   }
   // read first debug location
@@ -2809,8 +2809,8 @@ void WasmBinaryReader::readSourceMapHeader() {
   uint32_t lineNumber =
     readBase64VLQ(*sourceMap) + 1; // adjust zero-based line number
   uint32_t columnNumber = readBase64VLQ(*sourceMap);
-  nextDebugLocation = {
-    position, position, {fileIndex, lineNumber, columnNumber}};
+  nextDebugPos = position;
+  nextDebugLocation = {fileIndex, lineNumber, columnNumber};
   nextDebugLocationHasDebugInfo = true;
 }
 
@@ -2819,21 +2819,18 @@ void WasmBinaryReader::readNextDebugLocation() {
     return;
   }
 
-  if (nextDebugLocation.availablePos == 0 &&
-      nextDebugLocation.previousPos <= pos) {
-    // if source map file had already reached the end and cache position also
-    // cannot cover the pos clear the debug location
+  if (nextDebugPos == 0) {
+    // We reached the end of the source map; nothing left to read.
     debugLocation.clear();
     return;
   }
 
-  while (nextDebugLocation.availablePos &&
-         nextDebugLocation.availablePos <= pos) {
+  while (nextDebugPos && nextDebugPos <= pos) {
     debugLocation.clear();
     // use debugLocation only for function expressions
     if (currFunction) {
       if (nextDebugLocationHasDebugInfo) {
-        debugLocation.insert(nextDebugLocation.next);
+        debugLocation.insert(nextDebugLocation);
       } else {
         debugLocation.clear();
       }
@@ -2842,18 +2839,17 @@ void WasmBinaryReader::readNextDebugLocation() {
     char ch;
     *sourceMap >> ch;
     if (ch == '\"') { // end of records
-      nextDebugLocation.availablePos = 0;
+      nextDebugPos = 0;
       break;
     }
     if (ch != ',') {
       throw MapParseException("Unexpected delimiter");
     }
 
     int32_t positionDelta = readBase64VLQ(*sourceMap);
-    uint32_t position = nextDebugLocation.availablePos + positionDelta;
+    uint32_t position = nextDebugPos + positionDelta;
 
-    nextDebugLocation.previousPos = nextDebugLocation.availablePos;
-    nextDebugLocation.availablePos = position;
+    nextDebugPos = position;
 
     auto peek = sourceMap->peek();
     if (peek == ',' || peek == '\"') {
@@ -2863,14 +2859,13 @@ void WasmBinaryReader::readNextDebugLocation() {
     }
 
     int32_t fileIndexDelta = readBase64VLQ(*sourceMap);
-    uint32_t fileIndex = nextDebugLocation.next.fileIndex + fileIndexDelta;
+    uint32_t fileIndex = nextDebugLocation.fileIndex + fileIndexDelta;
     int32_t lineNumberDelta = readBase64VLQ(*sourceMap);
-    uint32_t lineNumber = nextDebugLocation.next.lineNumber + lineNumberDelta;
+    uint32_t lineNumber = nextDebugLocation.lineNumber + lineNumberDelta;
     int32_t columnNumberDelta = readBase64VLQ(*sourceMap);
-    uint32_t columnNumber =
-      nextDebugLocation.next.columnNumber + columnNumberDelta;
+    uint32_t columnNumber = nextDebugLocation.columnNumber + columnNumberDelta;
 
-    nextDebugLocation.next = {fileIndex, lineNumber, columnNumber};
+    nextDebugLocation = {fileIndex, lineNumber, columnNumber};
     nextDebugLocationHasDebugInfo = true;
   }
 }