This is an archive of the discontinued LLVM Phabricator instance.

[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?

350

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.

350

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
350

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
357
  • 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
357

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.
thakis added a comment.Nov 4 2019, 8:10 AM

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