Page MenuHomePhabricator

[clang-format] Remove the dependency on frontend
ClosedPublic

Authored by MyDeveloperDay on Oct 14 2019, 8:41 PM.

Details

Summary

Address review comments from D68554: [clang-format] Proposal for clang-format to give compiler style warnings by trying to drop the dependency again on Frontend whilst keeping the same format diagnostic messages

Not completely happy with having to do a split in order to get the StringRef for the Line the error occurred on, but could see a way to use SourceManager and SourceLocation to give me a single line?

But this removes the dependency on frontend which should keep the binary size down.

Diff Detail

Event Timeline

MyDeveloperDay created this revision.Oct 14 2019, 8:41 PM

My intuitive solution would have been to get the char* for the start and end-location and then search forward and backwards for \n.

My intuitive solution would have been to get the char* for the start and end-location and then search forward and backwards for \n.

I may need to, it feels slow for large files which I suspect is in the splitting, which will be mostly unnecessary.

My intuitive solution would have been to get the char* for the start and end-location and then search forward and backwards for \n.

I may need to, it feels slow for large files which I suspect is in the splitting, which will be mostly unnecessary.

It's a weird trade-off: for lots of lines, the inside-out algorithm will be better, but if we have a very very long line with lots of warnings, the inside-out algorithm will be quadratic.

MyDeveloperDay updated this revision to Diff 225320.EditedOct 16 2019, 3:36 PM

Remove need to split lines or search for \n

General Algorithm

  1. take Location of Replacement line and column
  2. make a new SourceLocation of line and column =0 (beginning of that replacement line)
  3. make a new SourceLocation of line+1 and column = 0 (beginning of the next line)
  4. use getCharacterData() to get pointers to beginning of each line
  5. make a String from those begin and end ptrs

Seems very much faster when run over clang source tree (removal of noticeable pauses on large files)

klimek added inline comments.Oct 17 2019, 1:54 AM
clang/tools/clang-format/ClangFormat.cpp
344

Looks unused?

349

This is unexpected: I'd have expected this to be PLoc.getLine() here and PLoc.getLine() + 1 below. I'm probably missing something?

MyDeveloperDay marked 2 inline comments as done.Oct 17 2019, 2:05 AM
clang/tools/clang-format/ClangFormat.cpp
344

correct, I changed my mind and now call ArrayRef<std::pair<unsigned, unsigned>>() as the last argument not sure if thay would be slower than me creating one empty one outside the loop, any thoughts? I'll go with a consensus and will remove the other.

349

This caught me out too, but it kept pulling the next line. So I think it must be a zero based line as opposed to PesumedLoc which uses line 1 to mean the first line..

but leet me double check.

MyDeveloperDay marked an inline comment as done.Oct 17 2019, 2:16 AM
MyDeveloperDay added inline comments.
clang/tools/clang-format/ClangFormat.cpp
349

OK this then perhaps is wrong, given the assert.. let me double check what is going on.

SourceLocation SourceManager::translateFileLineCol(const FileEntry *SourceFile,
                                                  unsigned Line,
                                                  unsigned Col) const {
  assert(SourceFile && "Null source file!");
  assert(Line && Col && "Line and column should start from 1!");

  FileID FirstFID = translateFile(SourceFile);
  return translateLineCol(FirstFID, Line, Col);
}

Update out by one errors and unused variables

MyDeveloperDay marked 2 inline comments as done.Oct 17 2019, 9:51 AM

klimek: ping :)

klimek accepted this revision.Oct 21 2019, 8:19 PM

lg

clang/tools/clang-format/ClangFormat.cpp
356
  • 1 is to exclude the \n I'd assume?
This revision is now accepted and ready to land.Oct 21 2019, 8:19 PM
MyDeveloperDay marked 2 inline comments as done.Oct 21 2019, 11:31 PM
MyDeveloperDay added inline comments.
clang/tools/clang-format/ClangFormat.cpp
356

correct, without the -1 I end up with an extra blank line in the output

This revision was automatically updated to reflect the committed changes.
MyDeveloperDay marked an inline comment as done.

Since this was reverted: are you looking into relanding this?

Since this was reverted: are you looking into relanding this?

I'm not totally sure how they reproduced the UBSAN issue

The revert message said:

Vlad Tsyrklevich via cfe-commits <cfe-commits@lists.llvm.org>
Tue, Oct 29, 1:51 PM (7 days ago)
to via, mydeveloperday

I've reverted this commit as it was causing UBSan failures on the ubsan bot. These failures looked like:
llvm/lib/Support/SourceMgr.cpp:440:48: runtime error: pointer index expression with base 0x000000000000 overflowed to 0xfffffffffffffffa

Looking at a backtrace, this line was reached from the `Diags.print(nullptr, llvm::errs(), (ShowColors && !NoShowColors));` call introduced in this change.

Is that enough to see what's wrong? If not, @vlad.tsyrklevich , can you give repro steps for that ubsan failure that made you revert ec66603 in efed314?

The revert message said:

Vlad Tsyrklevich via cfe-commits <cfe-commits@lists.llvm.org>
Tue, Oct 29, 1:51 PM (7 days ago)
to via, mydeveloperday

I've reverted this commit as it was causing UBSan failures on the ubsan bot. These failures looked like:
llvm/lib/Support/SourceMgr.cpp:440:48: runtime error: pointer index expression with base 0x000000000000 overflowed to 0xfffffffffffffffa

Looking at a backtrace, this line was reached from the `Diags.print(nullptr, llvm::errs(), (ShowColors && !NoShowColors));` call introduced in this change.

Is that enough to see what's wrong? If not, @vlad.tsyrklevich , can you give repro steps for that ubsan failure that made you revert ec66603 in efed314?

Note to self: Took me a bit to set up a docker container to build with clang-10 on linux (I'm normally a windows guy), but hopefully, I can debug from here...

test1.cpp:1:7: warning: code should be clang-formatted [-Wclang-format-violations]
/llvm-project/llvm/lib/Support/SourceMgr.cpp:440:48: runtime error: applying non-zero offset 1844674407
3709551610 to null pointer
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /buildareas/llvm2/llvm-project/llvm/lib/Support/SourceMgr.cpp:44
0:48 in