Page MenuHomePhabricator

[Preprocessor] -E -P: Ensure newline after 8 skipped lines.
ClosedPublic

Authored by Meinersbur on Jul 27 2021, 5:02 PM.

Details

Summary

The implementation of -fminimize-whitespace (D104601) revised the logic when to emit newlines. There was no case to handle when more than 8 lines were skippped in -P (DisableLineMarkers) mode and instead fell through the case intended for -fminimize-whitespace, i.e. emit nothing. This patch will emit one newline in this case.

The newline logic is slightly reorganized. The -P -fminimize-whitespace case is handled explicitly and emitting at least one newline is the new fallback case. The choice between emitting a line marker or up to 8 empty lines is now a choice only with enabled line markers. The up to 8 newlines likely are fewer characters than a line directive, but in -P mode this had the paradoxic effect that it would print up to 7 empty lines, but none at all if more than 8 lines had to be skipped. Now with DisableLineMarkers, we don't consider printing empty lines (just start a new line) which matches gcc's behavior.

The line-directive-output-mincol.c test is replaced with a more comprehensive test skip-empty-lines.c also testing the more than 8 skipped lines behaviour with all flag combinations.

Diff Detail

Event Timeline

Meinersbur requested review of this revision.Jul 27 2021, 5:02 PM
Meinersbur created this revision.
Meinersbur edited the summary of this revision. (Show Details)Jul 27 2021, 10:39 PM

Thanks! I can confirm that this fixes the issue I ran into.

This revision is now accepted and ready to land.Jul 28 2021, 9:52 AM

@Meinersbur After landing this, can you coordinate with @tstellar to get an ack for backporting this to the 13.x release branch, which also suffers from the regression?

  • Fix embrassing mistakes in skip-empty-lines test (misspelled "LINEMARKES", MINWS<->MINCOL, absolute path)
  • Skip-empty-lines now also checks leading whitespace behavior
  • Fix typos in comments
Meinersbur edited the summary of this revision. (Show Details)Jul 28 2021, 7:53 PM
  • Use correct base commit
This revision was landed with ongoing or failed builds.Jul 28 2021, 8:51 PM
This revision was automatically updated to reflect the committed changes.

@Meinersbur After landing this, can you coordinate with @tstellar to get an ack for backporting this to the 13.x release branch, which also suffers from the regression?

Release blocker has been created: http://llvm.org/PR51261

sberg added a subscriber: sberg.Aug 2 2021, 3:05 AM

Bisecting shows that this commit causes

$ echo foo | clang -E -P -

foo

to emit a leading newline now.

That breaks e.g. LibreOffice's configure, which uses echo __clang_version__ | clang -E -P - to obtain the value of that macro, and expects a single line of output.

sberg added a comment.Aug 25 2021, 1:31 AM

Bisecting shows that this commit causes

$ echo foo | clang -E -P -

foo

to emit a leading newline now.

That breaks e.g. LibreOffice's configure, which uses echo __clang_version__ | clang -E -P - to obtain the value of that macro, and expects a single line of output.

I now filed https://bugs.llvm.org/show_bug.cgi?id=51616 "Bad newlines in -E -P output after https://reviews.llvm.org/D106924 '[Preprocessor] -E -P: Ensure newline after 8 skipped lines.'"