[test-suite] Add -i option to fpcmp to ignore whitespace changes.

[Meinersbur]

Summary

Before this patch, fpcmp could loop forever when comparing two whitespace characters (e.g. '\n' vs. '\r' which typically happens when comparing files that only differ in LF vs. CRLF line endings).

Add a switch '-i' to fpcmp that allows ignoring whitespace changes. This is required for SPEC CPU 2017 511.povray_r which has its reference output stored with CRLF line endings, but the actual output on UNIX system has LF line endings.

[Meinersbur]

• Do not fast-return with -i option

[MatzeB]

• Adding a mode to ignore whitespace seems reasonable.
• I remember seeing fpcmp loop endlessly and never got around figuring out why, good to see this fixed.

tools/fpcmp.c

260 ↗

(On Diff #111261)

You mustn't pass char to libc character test functions but must cast characters to unsigned char (this is so intuitive, isn't it?). I guess you could also fix the existing isspace uses in the file while on it...

I wonder if this isn't a case that should rather be handled in CompareNumber.

297–298 ↗

(On Diff #111261)

Wouldn't we abort here when comparing an empty file with a file with just spaces even in -i mode?

[Meinersbur]

Fix some out-of-bounds accesses

[MatzeB]

This is looking good! I am wondering if one of the if blocks is still necessary (see below).

tools/fpcmp.c

260 ↗

(On Diff #111261)

Is the if (!ignore_whitespace) { ... } still necessary after your changes to CompareNumbers?

[Meinersbur]

tools/fpcmp.c

52 ↗

(On Diff #111582)

Pos[-1] is an invalid access if backing-up from the first character in the file.

101 ↗

(On Diff #111582)

F1P and F2P might have reached the end of file already, after skipping the whitespace. In that case these accesses are out-of-bounds. (Thy would read the terminating '\0', arguably not a number char, but they get printed in the fprintf below)

114–117 ↗

(On Diff #111582)

Possible stack overflow? Just have a long sequence of zeros (> 200 chars) in the file.

260 ↗

(On Diff #111261)

I wonder if this isn't a case that should rather be handled in CompareNumber.

I'd avoid making CompareNumbers have side-effects if no number was found. There are already too many bugs in there. (whitespace changes before numbers have always been ignored; intentional?)

297–298 ↗

(On Diff #111261)

fixed.

[Meinersbur]

tools/fpcmp.c

56 ↗

(On Diff #111582)

This is probably the line causing the infinite loop.

If we are in both, F1 and F2, just one char past an equal number (and the char-wise comparison does not advance because in my case, the following is LF and CR in the other). BackupNumber hence rewinds the entire number in F1 and F2, which is compared equal again, and the loop repeats at the CR/LF again. Two files containing "2a" and "2b" (equal numbers but different non-numeric characters following) trigger this already.

This means that this patch is not a complete fix for this phenomenon (I initially thought this happens only with whitespace, the aforementioned example loops as well). It is probably intended to match 0.2 and 0.20 as equal, but the assumption ("should") in the comment is definitely wrong.

I don't know a fix for this at the moment without breaking the use case.

260 ↗

(On Diff #111261)

No behaviour in CompareNumbers was changed. I just extracted the while loops into the skip_whitespace function which then can be called in multiple places.

[Meinersbur]

fpcmp: Fix endless loop in non-whitespace case

[Meinersbur]

Can somebody give their ok for the most recent diff update? Then I'll commit to trunk (and SPEC CPU 2017 which depends on this as well)

[hfinkel]

tools/fpcmp.c

283 ↗

(On Diff #111654)

Any particular reason you're using a bitwise instead of logical or. If not, I'd prefer || here.

[MatzeB]

LGTM

[Meinersbur]

tools/fpcmp.c

283 ↗

(On Diff #111654)

Yes, skip_whitespace has side-effects in that it advances the F1P/F2P pointers. When using the shortcut operators, F2P would no advance whitespace. However, the semantics are the same because in the next iteration skip_whitespace for F1P will return false and F2P will advance.

I know that using | for a logical operation is uncommon. The alternative is to store the return value of skip_whitespace into temporaries.

[hfinkel]

tools/fpcmp.c

283 ↗

(On Diff #111654)

Okay. The code is fine in that case.

test-suite/trunk/tools/fpcmp.c

static bool isNumberChar(char C) {
   switch (C) {
   case '0': case '1': case '2': case '3': case '4':
   case '5': case '6': case '7': case '8': case '9':
   case '.': return true;
   default: return isSignedChar(C) || isExponentChar(C);
   }
 }
 
-static const char *BackupNumber(const char *Pos, const char *FirstChar) {
+static const char *BackupNumber(const char *Pos, const char *FirstChar,
+                                const char *End) {
+  // If already at the very beginning, we cannot go back further.
+  // It ensures that the access Pos[-1] is valid.
+  // This also handles the case where the file is empty (FirstChar == End).
+  if (Pos == FirstChar)
+    return Pos;
+
   // If we didn't stop in the middle of a number, don't backup.
-  if (!isNumberChar(*Pos) && !isNumberChar(Pos[-1])) return Pos;
+  if (!(Pos == End || isNumberChar(*Pos)) && !isNumberChar(Pos[-1]))
+    return Pos;
+
   // If we're one past the number, the two numbers can have different number
   // of digits. Even if we're wrong, and we get the previous number, the
   // comparison should have failed anyway.
-  if (!isNumberChar(*Pos)) Pos--;
+  if (Pos == End || !isNumberChar(*Pos))
+    Pos--;
 
   // Otherwise, return to the start of the number.
   bool HasPeriod = false;
   while (Pos > FirstChar && isNumberChar(Pos[-1])) {
     // Backup over at most one period.
     if (Pos[-1] == '.') {
       if (HasPeriod)
         break;

 /// number.  This assumes that the buffer is null terminated, so it won't fall
 /// off the end.
 static const char *EndOfNumber(const char *Pos) {
   while (isNumberChar(*Pos))
     ++Pos;
   return Pos;
 }
 
+static bool skip_whitespace(const char **FP, const char *FEnd) {
+  bool skipped_some = false;
+  while (*FP < FEnd && isspace((unsigned char)**FP)) {
+    skipped_some = true;
+    ++*FP;
+  }
+  return skipped_some;
+}
+
 /// CompareNumbers - compare two numbers, returning true if they are different.
 static bool CompareNumbers(const char **F1PP, const char **F2PP,
                            const char *F1End, const char *F2End,
                            double AbsTolerance, double RelTolerance) {
   const char *F1P = *F1PP;
   const char *F2P = *F2PP;
   const char *F1NumEnd, *F2NumEnd;
   double V1 = 0.0, V2 = 0.0;
 
   // If one of the positions is at a space and the other isn't, chomp up 'til
   // the end of the space.
-  while (isspace(*F1P) && F1P != F1End)
-    ++F1P;
-  while (isspace(*F2P) && F2P != F2End)
-    ++F2P;
+  skip_whitespace(&F1P, F1End);
+  skip_whitespace(&F2P, F2End);
+
+  if (F1P == F1End || F2P == F2End) {
+    fprintf(stderr, ("%s: FP Comparison failed, reached end of file\n"),
+            g_program);
+    return true;
+  }
 
   // If we stop on numbers, compare their difference.
   if (!isNumberChar(*F1P) || !isNumberChar(*F2P)) {
     // The diff failed.
     F1NumEnd = F1P;
     F2NumEnd = F2P;
   } else {
     // Note that some ugliness is built into this to permit support for numbers
char *load_file(const char *path, long *size_out) {
   /* Close the file and return the data. */
   fclose(fp);
   *size_out = size;
   return data;
 }
 
 int diff_files_with_tolerance(const char *path_a, const char *path_b,
                               double absolute_tolerance,
-                              double relative_tolerance) {
+                              double relative_tolerance,
+                              bool ignore_whitespace) {
   /* First, load the file buffers completely into memory. */
   long A_size, B_size;
   char *data_a = load_file(path_a, &A_size);
   char *data_b = load_file(path_b, &B_size);
 
   /* Fast path equivalent buffers. */
   if (A_size == B_size && memcmp(data_a, data_b, A_size) == 0)
     return 0;
 
   /* Otherwise, if our tolerances are 0 then we are done. */
-  if (relative_tolerance == 0.0 && absolute_tolerance == 0.0) {
+  if (relative_tolerance == 0.0 && absolute_tolerance == 0.0 &&
+      !ignore_whitespace) {
     fprintf(stderr, "%s: files differ without tolerance allowance\n",
             g_program);
     return 1;
   }
 
   /* *** */
 
   // Okay, now that we opened the files, scan them for the first difference.
   const char *File1Start = data_a;
   const char *File2Start = data_b;
   const char *File1End = data_a + A_size;
   const char *File2End = data_b + B_size;
   const char *F1P = File1Start;
   const char *F2P = File2Start;
 
   while (1) {
     // Scan for the end of file or next difference.
-    while (F1P < File1End && F2P < File2End && *F1P == *F2P)
+    while (F1P < File1End && F2P < File2End) {
+      if (*F1P == *F2P) {
         ++F1P, ++F2P;
+        continue;
+      }
+
+      // With whitespace ignored, skip whitespace chars (if any) and recheck for
+      // the next difference.
+      if (ignore_whitespace) {
+        if (skip_whitespace(&F1P, File1End) | skip_whitespace(&F2P, File2End))
+          continue;
+      }
+
+      break;
+    }
 
     if (F1P >= File1End || F2P >= File2End) break;
 
+    // BackupNumber will also backup when at the first char after a number. This
+    // is to catch when one of the number has more trailing floating-point zeros
+    // than the other. However, when this is the case in both buffers, we'd
+    // repeat the most recent number forever.
+    // Therefore, we fast-stop if both buffers have no number at the current
+    // position.
+    if (!isNumberChar(*F1P) && !isNumberChar(*F2P)) {
+      fprintf(stderr, "%s: FP Comparison failed, not a numeric difference "
+                      "between '%c' and '%c'\n",
+              g_program, F1P[0], F2P[0]);
+      return 1;
+    }
+
     // Okay, we must have found a difference.  Backup to the start of the
     // current number each stream is at so that we can compare from the
     // beginning.
-    F1P = BackupNumber(F1P, File1Start);
-    F2P = BackupNumber(F2P, File2Start);
+    F1P = BackupNumber(F1P, File1Start, File1End);
+    F2P = BackupNumber(F2P, File2Start, File2End);
 
     // Now that we are at the start of the numbers, compare them, exiting if
     // they don't match.
     if (CompareNumbers(&F1P, &F2P, File1End, File2End,
                        absolute_tolerance, relative_tolerance))
       return 1;
   }
 
-  // Avoid reading before byte 0 in the loop below.
-  if (A_size == 0 || B_size == 0) {
-    fprintf(stderr, "%s File sizes differ\n", g_program);
-    return 1;
+  // Skip any whitespace at the end of the file.
+  if (ignore_whitespace) {
+    skip_whitespace(&F1P, File1End);
+    skip_whitespace(&F2P, File2End);
   }
 
   // Okay, we reached the end of file.  If both files are at the end, we
   // succeeded.
   bool F1AtEnd = F1P >= File1End;
   bool F2AtEnd = F2P >= File2End;
   if (!F1AtEnd || !F2AtEnd) {
     // Else, we might have run off the end due to a number: backup and retry.
-    if (F1AtEnd && isNumberChar(F1P[-1])) --F1P;
-    if (F2AtEnd && isNumberChar(F2P[-1])) --F2P;
-    F1P = BackupNumber(F1P, File1Start);
-    F2P = BackupNumber(F2P, File2Start);
+    F1P = BackupNumber(F1P, File1Start, File1End);
+    F2P = BackupNumber(F2P, File2Start, File2End);
 
     // Now that we are at the start of the numbers, compare them, exiting if
     // they don't match.
     if (CompareNumbers(&F1P, &F2P, File1End, File2End,
                        absolute_tolerance, relative_tolerance))
       return 1;
 
+    // There might be more whitespace after the number.
+    if (ignore_whitespace) {
+      skip_whitespace(&F1P, File1End);
+      skip_whitespace(&F2P, File2End);
+    }
+
     // If we found the end, we succeeded.
     if (F1P < File1End || F2P < File2End)
       return 1;
   }
 
   return 0;
 }
 
 void usage() {
-  fprintf(stderr, "usage: %s [-a VALUE] [-r VALUE] <path-A> <path-B>\n\n",
+  fprintf(stderr, "usage: %s [-a VALUE] [-r VALUE] [-i] <path-A> <path-B>\n\n",
           g_program);
   fprintf(stderr, "Compare two files using absolute and relative tolerances\n");
   fprintf(stderr, "when comparing differences between two character\n");
   fprintf(stderr, "which could be real numbers\n");
+  fprintf(stderr, "The -i switch ignores whitespace differences\n");
   exit(2);
 }
 
 int main(int argc, char * const argv[]) {
   double relative_tolerance = 0.0;
   double absolute_tolerance = 0.0;
+  bool ignore_whitespace = false;
   int i;
 
   g_program = argv[0];
   for (i = 1; i != argc; ++i) {
     const char *arg = argv[i];
     if (arg[0] != '-')
         break;
 
case 'r':
         }
         if (arg[1] == 'a')
           absolute_tolerance = value;
         else
           relative_tolerance = value;
       }
       break;
 
+    case 'i':
+      ignore_whitespace = true;
+      break;
+
     default:
       fprintf(stderr, "error: invalid argument '%s'\n\n", arg);
       usage();
     }
   }
 
   if (i + 2 != argc) {
     fprintf(stderr, "error: invalid number of arguments\n\n");
     usage();
   }
 
-  return diff_files_with_tolerance(argv[i], argv[i + 1],
-                                   absolute_tolerance, relative_tolerance);
+  return diff_files_with_tolerance(argv[i], argv[i + 1], absolute_tolerance,
+                                   relative_tolerance, ignore_whitespace);
 }