This is an archive of the discontinued LLVM Phabricator instance.

[clang-format] Fix bug with ENAS_DontAlign and empty lines
ClosedPublic

Authored by jtbandes on Jul 28 2017, 2:15 PM.

Details

Summary

This fixes a bug in ENAS_DontAlign (introduced in D32733) where blank lines had an EscapedNewlineColumn of 0, causing a subtraction to overflow when converted back to unsigned and leading to runaway memory allocation.

This restores the original approach of a separate loop as originally proposed in https://reviews.llvm.org/D32733?vs=97397&id=97404, now with a proper justification :)

Diff Detail

Repository
rL LLVM

Event Timeline

jtbandes created this revision.Jul 28 2017, 2:15 PM
djasper edited edge metadata.Jul 30 2017, 10:40 PM

Could you explain this in more detail? Which subtraction is underflowing? Why can't we just add a ternary expression there to handle the case?

I can add some clarity but I can't claim to fully understand the whole program flow here yet, so my explanation is probably insufficient.

The overflow (underflow? but I think that means something specific to FP) is on line formerly-650: EscapedNewlineColumn - Offset - 1. This expression computes number of spaces required to place the \ in the correct position. When EscapedNewlineColumn was 0, the Offset would end up being large enough for the computation on line 650 to roll over to a large value.

Here is where my understanding is a bit fuzzier, and perhaps I should spend more time reading the code. When the alignment style is not ENAS_DontAlign, the function alignEscapedNewlines() adjusts the EscapedNewlineColumn of each line, including blank lines, which seems to result in EscapedNewlineColumn being always sufficiently large to do this subtraction without wrapping. (The approach of immediately return;ing for ENAS_DontAlign, committed in rL302428, was causing the EscapedNewlineColumn to be 0 here.)

It is a possibility to simply check the EscapedNewlineColumn and do something different inside of appendNewlineText(). I'd be interested to hear your thoughts on which approach is better. Perhaps if I study the code some more there will a nice simplification will reveal itself to me...

jtbandes updated this revision to Diff 108860.EditedJul 30 2017, 11:19 PM

Okay, I think this approach is better:

  • Rename the version of appendNewlineText used for escaped newlines to appendEscapedNewlineText to reduce confusability.
  • If ENAS_DontAlign, skip all of the offset calculation logic. Just append space-backslash-newline.
  • Restore the offset calculation to use EscapedNewlineColumn - 1, which it was before D32733. This was only changed to support the early-return approach.
  • Leave in the assert.
jtbandes updated this revision to Diff 108863.Jul 30 2017, 11:29 PM
  • Undo change in argument list
djasper added inline comments.Aug 4 2017, 1:19 AM
lib/Format/WhitespaceManager.cpp
650 ↗(On Diff #108863)

Note that when you have an empty line, this would turn into:

#define A \
  int i; \
 \                <-- Note the 1-space indent here.
  int j; \
  int k;

With my alternative below, that "\" will just be put at column 0, which probably isn't better or worse.

656 ↗(On Diff #108863)

You could change this to:

unsigned Spaces =
    std::max<int>(1, EscapedNewlineColumn - PreviousEndOfTokenColumn - 1);
for (unsigned i = 0; i < Newlines; ++i) {
  Text.append(Spaces, ' ');
  Text.append(UseCRLF ? "\\\r\n" : "\\\n");
  Spaces = std::max<int>(0, EscapedNewlineColumn - 1);
}

And it should work without problems and without special code path.

jtbandes added inline comments.Aug 5 2017, 1:57 PM
lib/Format/WhitespaceManager.cpp
650 ↗(On Diff #108863)

I suppose that can be changed back to 1 by using std::max<int>(1, EscapedNewlineColumn - 1); instead, right? I don't have strong feelings about whether it should be 0 or 1.

djasper accepted this revision.Aug 6 2017, 10:18 PM

Thanks you.

lib/Format/WhitespaceManager.cpp
650 ↗(On Diff #108863)

I don't either. Leave as is for now.

This revision is now accepted and ready to land.Aug 6 2017, 10:18 PM

Thanks. Can you commit this when you get a chance? I don't have permissions.

This revision was automatically updated to reflect the committed changes.