This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by Meinersbur on Aug 15 2017, 2:17 PM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

Meinersbur created this revision.Aug 15 2017, 2:17 PM
  • Do not fast-return with -i option
MatzeB edited edge metadata.Aug 17 2017, 11:43 AM
  • 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?

Fix some out-of-bounds accesses

MatzeB accepted this revision.Aug 17 2017, 4:33 PM

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?

This revision is now accepted and ready to land.Aug 17 2017, 4:33 PM
Meinersbur marked an inline comment as done.Aug 17 2017, 4:56 PM
Meinersbur added inline comments.
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 marked an inline comment as done.Aug 17 2017, 5:33 PM
Meinersbur added inline comments.
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.

fpcmp: Fix endless loop in non-whitespace case

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 added inline comments.
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 accepted this revision.Aug 21 2017, 9:35 AM

LGTM

Meinersbur added inline comments.Aug 21 2017, 10:09 AM
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 added inline comments.Aug 21 2017, 12:18 PM
tools/fpcmp.c
283 ↗(On Diff #111654)

Okay. The code is fine in that case.

This revision was automatically updated to reflect the committed changes.